Update CloudHandler.DispatchMetricMap to use MetricMap all the way through #341
Conversation
|
|
||
| All configuration is in a stanza named after the backend, and takes simple key value pairs. | ||
|
|
||
| **Cloud providers should be disabled on the aggregation server when using http forwarding, as the source IP isn't |
There was a problem hiding this comment.
I believe technically this became safe with #330
metric_map.go
Outdated
| } | ||
|
|
||
| // AsMetrics will synthesize Metrics from the MetricMap and return them as a slice | ||
| // TODO: Remove this function, it only supports tests now |
There was a problem hiding this comment.
I'll raise an issue after this is merged.
There was a problem hiding this comment.
Is it worth adding a
// deprecated: Used only for testing
To help with IDE highlighters?
There was a problem hiding this comment.
Sure. Does your IDE use deprecated or Deprecated? Goland seems to want the D or it won't highlight it.
There was a problem hiding this comment.
Goland has a "deprecate harder", for when the symbol is actually intended to be removed, but damned if I can find the comment syntax for that.
| func (ch *CloudHandler) handleInstanceInfo(ctx context.Context, info gostatsd.InstanceInfo) { | ||
| metrics := ch.awaitingMetrics[info.IP] | ||
| if len(metrics) > 0 { | ||
| mm := ch.awaitingMetrics[info.IP] |
There was a problem hiding this comment.
Someone plz double check my logic here:
- Metric comes in, goes to
handleIncomingMetrics - statsMetricHostsQueued is incremented because the host isn't in
awaitingMetricsorawaitingEvents - Event comes in, goes to
handleIncomingEvent - statsEventHostsQueued is not incremented because the host is in
awaitingMetrics - Lookup completes
handleInstanceInfosees the IP inawaitingMetrics, decrementsstatsMetricHostsQueuedhandleInstanceInfosees the IP inawaitingEvents, decrementsstatsEventHostsQueued, it's now-1
I think the situation occurred in both the old logic, and this new code? I'm inclined to think we should just drop both and use a single hostsQueued (or even just a len(toLookupIPs))?
There was a problem hiding this comment.
This potentially problematic logic can be ignored, there's a refactor queued up which addresses it.
There was a problem hiding this comment.
I look forward to the update.
There was a problem hiding this comment.
I look forward to merging my third tweaking of this branch in to this once this branch is merged to master :okay:
| } | ||
|
|
||
| // DispatchMetricMap re-dispatches a MetricMap through CloudHandler.processMetrics | ||
| // TODO: This is inefficient, and should be handled first class, however that is a major re-factor of |
There was a problem hiding this comment.
that is a major re-factor
so that was a lie
|
@tiedotguy I'll try and find some time to upgrade the clusters. A bit swamped right now, but I'll get to it asap. |
| return Counter{Value: value, Timestamp: timestamp, Source: source, Tags: tags.Copy()} | ||
| } | ||
|
|
||
| func (c *Counter) AddTagsSetSource(additionalTags Tags, newSource Source) { |
There was a problem hiding this comment.
Does this operation need to be threadsafe?
tiedotguy
force-pushed
the
tdg/update-cloudhandler-dispatchmetricmap
branch
from
29c83f8 to
ef0db6a
Compare
tiedotguy
force-pushed
the
tdg/update-cloudhandler-dispatchmetricmap
branch
from
ef0db6a to
a1f1353
Compare
|
:bueller: |
metric_map.go
Outdated
| } | ||
|
|
||
| // AsMetrics will synthesize Metrics from the MetricMap and return them as a slice | ||
| // TODO: Remove this function, it only supports tests now |
There was a problem hiding this comment.
Is it worth adding a
// deprecated: Used only for testing
To help with IDE highlighters?
| if ch.updateTagsAndHostname(m, m.Source) { | ||
| mmToDispatch.Receive(m) | ||
| mmToHandle := gostatsd.NewMetricMap() | ||
| mm.Counters.Each(func(metricName string, tagsKey string, c gostatsd.Counter) { |
There was a problem hiding this comment.
Should the pointer of the values be passed in here?
There was a problem hiding this comment.
You mean like:
mm.Counters.Each(func(metricName string, tagsKey string, c *gostatsd.Counter) {
...?
If so, no - a MetricMap is all structs, not pointers to structs.
| func (ch *CloudHandler) handleInstanceInfo(ctx context.Context, info gostatsd.InstanceInfo) { | ||
| metrics := ch.awaitingMetrics[info.IP] | ||
| if len(metrics) > 0 { | ||
| mm := ch.awaitingMetrics[info.IP] |
There was a problem hiding this comment.
I look forward to the update.
Instead of having a per source for metrics queue and a per source for events queue, this unifies it in to a single per host for anything queue. This simplifies the logic, and removes what appears to be a logic bug: #341 (comment)
This is the current "big" thing that I'm aware of blocking clustering, 'cause it interferes with loop detection.
As usual, commits are relatively stand-alone.
@mwhittington21 - if you could get a chance to test this at some point (I'll release it as 28.3.0-rc1), that would be much appreciated. I generally soak changes locally, but I can only validate this one with tests.