Remove dispatch metrics #331
Conversation
…icHandler interface
…TestMetricMapDispatch
…ith associated dead code cleanup. This also removes a pair of benchmarks, one for hot metrics, the other for splitting metrics up. These are not relevant anymore. Hot metrics will be handled at an earlier point before there is contention, and splitting MetricMaps is not a high volume act. If it is benchmarked it should be a MetricMap benchmark, not a BackendHandler benchmark.
| @@ -35,7 +36,7 @@ func (fn *flushNotifier) RegisterFlush() (ch <-chan time.Duration, unregister fu | |||
|
|
|||
| // NotifyFlush will notify any registered channels that a flush has completed. | |||
| // Non-blocking, thread-safe. | |||
| func (fn *flushNotifier) NotifyFlush(d time.Duration) { | |||
| func (fn *flushNotifier) NotifyFlush(ctx context.Context, d time.Duration) { | |||
There was a problem hiding this comment.
ctx isn't used within this function
There was a problem hiding this comment.
It was added to the Statser interface so that InternalStatser had a context, which it needs to drain the MetricConsolidator.
There was a problem hiding this comment.
Could you not add it to the select block done bellow?
There was a problem hiding this comment.
Ahh, I see what you mean. Technically yes, but there's really no point. The loop is basically non-blocking because of the default: clause.
Remember the primary purpose of cancelling a context is to signal a long-running call to finish - this isn't long running.
| func (is *InternalStatser) NotifyFlush(ctx context.Context, d time.Duration) { | ||
| mms := is.consolidator.Drain(ctx) | ||
| if mms == nil { | ||
| // context is canceled |
There was a problem hiding this comment.
You would only know that for sure if you checked ctx.Error() right?
There was a problem hiding this comment.
I'm breaking the abstraction and saying "yeah, I know why this failed". It's part of my DRAEASWH system.
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/ash2k/stager" |
| @@ -110,9 +110,13 @@ func (dp *DatagramParser) Run(ctx context.Context) { | |||
| accumE += eventCount | |||
| accumB += badLineCount | |||
| } | |||
| // TODO: Refactor this to use a MetricConsolidator | |||
There was a problem hiding this comment.
:stare: is there an issue for this?
There's some juggling with the benchmarks, which is primarily because they're now creating a MetricMap in the loop, which is not really valid. But they're in there so they're staying.
As always, commits are generally isolated.