Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: type-specific 'handlers' #176

Closed
wants to merge 1 commit into from
Closed

Conversation

LukeWinikates
Copy link
Contributor

@LukeWinikates LukeWinikates commented Nov 9, 2023

This PR consolidates a repetitive trySendWith pattern in senders.realSender into a simpler pattern.

This PR assumes that #175 will be merged first

Old Pattern

func (sender *realSender) SendMetric(name string, value float64, ts int64, source string, tags map[string]string) error {
	line, err := metric.Line(name, value, ts, source, tags, sender.defaultSource)
	return trySendWith(
		line,
		err,
		sender.pointHandler,
		sender.internalRegistry.PointsTracker(),
	)
}

New Pattern

func (sender *realSender) SendMetric(name string, value float64, ts int64, source string, tags map[string]string) error {
	return sender.pointSender.TrySend(metric.Line(name, value, ts, source, tags, sender.defaultSource))
}

In the new pattern, the caller of trySendWith only needs to tie together two related things (metric.Line and pointSender), whereas before they needed to chose correctly 3 times (metric.Line, pointHandler, PointsTracker).

The new pattern also mostly allows us to avoid line, err as local variables, so that the Send* methods can be as short as 1 line.

Other opportunistic changes

  • rename 'LineHandler' interface to 'BatchBuilder' - the main responsibility of this type seems to be collecting lines for a particular point type into batches of the right size and periodically flushing them. Maybe it should actually be called 'BatchBuffer'?
  • moves some line-building logic from SendDelta into a new delta.Line func
  • renames reporter to batchingHTTPReporter (or maybe it should be HTTPBatchReporter)

Issues

  • need to backfill tests for delta.Line
  • wavefront_sender_test.go has gotten a little weird - it mocks a 2nd-degree dependency instead of a direct one.
  • finalize or revert renames of LineHandler and reporter
  • maybe TypedSender is not a great name

SetRegistry(registry),
},
}
}

func (f *HandlerFactory) NewPointHandler(batchSize int) *RealLineHandler {
Copy link
Contributor

@jbooherl jbooherl Nov 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could the batchSize be added to the sender factory? Are there plans to make it configurable by sender?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants