wavefrontHQ / wavefront-sdk-go

Wavefront Core Go SDK
Apache License 2.0
4 stars 21 forks source link

chore: Sender refactor #174

Closed LukeWinikates closed 1 year ago

LukeWinikates commented 1 year ago

The NewSender function currently uses a pattern like this:

sender.pointHandler = newLineHandler(metricsReporter, cfg, internal.MetricFormat, "points", sender.internalRegistry)
    sender.histoHandler = newLineHandler(metricsReporter, cfg, internal.HistogramFormat, "histograms", sender.internalRegistry)
    sender.spanHandler = newLineHandler(tracesReporter, cfg, internal.TraceFormat, "spans", sender.internalRegistry)
    sender.spanLogHandler = newLineHandler(tracesReporter, cfg, internal.SpanLogsFormat, "span_logs", sender.internalRegistry)

In this pattern, the caller must know that you send points with the metricsReporter, you use the points prefix, and you set the MetricFormat.

This PR moves this knowledge into dedicated factory functions for each line type, so that the factory knows the information, and the caller only needs to identify the right kind of handler to create - the factory is responsible for knowing how to create valid handlers.

idea: keep pushing type-specific logic into the Handler abstraction

right now the Sender has methods that call a particular formatter and then pass its output to the correct handler. For example, SendMetric calls metric.Line and then tries to send that with the sender.pointHandler. It seems as though we could maybe package the line formatter and the line handler together somehow - why have the Sender know about so many handlers and have to know which one to use which format with, when we could instead have the handler itself know the format transformation it needs to do, in the same way that it knows what prefixes and headers to put on its HTTP requests?

idea: the Handler is actually a FormatSpecificBatchSender or something like that.

The Handler name seems to hide the fact that this abstraction is where formatted lines get batched together and eventually sent out.

jbooherl commented 1 year ago

A further enhancement could be to expand the setters used in NewLineHandler for the command line parameters but that could be another pr too. Just feels like it might help with the readability of the handlers

jbooherl commented 1 year ago

I do like the idea trying to move the line formatting into the Factories in a way. Then the handler could have like a .send that would do all the formatting and send to the correct handler. Is this what your thinking?

LukeWinikates commented 1 year ago

I do like the idea trying to move the line formatting into the Factories in a way. Then the handler could have like a .send that would do all the formatting and send to the correct handler. Is this what your thinking?

Yep, I think that direction might work. I'm hoping to try incrementally refactoring towards that through a series of small PRs like this one.

LukeWinikates commented 1 year ago

@jbooherl thanks for looking at this PR and approving it! Since I'm confident these changes are safe, I'm going to go ahead and merge it and then in the next couple of days I might open another that keeps building on this.