Improve gorouter Route Registry Message Metrics#456
Improve gorouter Route Registry Message Metrics#456hoffmaen wants to merge 8 commits intocloudfoundry:mainfrom
Conversation
hoffmaen
force-pushed
the
route-registry-metrics
branch
from
efa7e82 to
8c017fb
Compare
peanball
left a comment
peanball
left a comment
There was a problem hiding this comment.
most important comment: route registration logging for any result is now Info level and will cause significant increase in logs in production. Please change it back to debug level output for "refreshes" and "no-ops".
Some other smaller suggestions are there but not vital.
metrics/metricsreporter.go
Outdated
| m.Logger.Debug("failed-sending-metric", log.ErrAttr(err), slog.String("metric", componentName)) | ||
| componentName = "unregistry_message." + action + "." + msg.Component() | ||
| } | ||
| m.Batcher.BatchIncrementCounter(componentName) |
There was a problem hiding this comment.
is this equivalent to the previously used m.Sender.IncrementCounter(componentName)?
| func (m *MetricsReporter) CaptureUnregistryMessage(msg ComponentTagged) { | ||
| var componentName string | ||
| if msg.Component() == "" { | ||
| componentName = "unregistry_message" | ||
| } else { | ||
| componentName = "unregistry_message." + msg.Component() | ||
| } | ||
| err := m.Sender.IncrementCounter(componentName) | ||
| if err != nil { | ||
| m.Logger.Debug("failed-sending-metric", log.ErrAttr(err), slog.String("metric", componentName)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This would change existing metrics, I don't think we should do this.
There was a problem hiding this comment.
just for my understanding:
Would you then want to have the "old" total metric and new breakdown metrics in addition?
Something like this?
registry_message: 100
registry_message.endpoint-registered: 5
registry_message.endpoint-updated: 94
registry_message.endpoint-unregistered: 1
There was a problem hiding this comment.
What I mean is that this change:
will change the emitted metrics for unregister messages from unregistry_message.* to registry_message.*. This would be a breaking change which doesn't seem to provide much value and could be avoided.
There was a problem hiding this comment.
You are right. We changed it back.
b1tamara
force-pushed
the
route-registry-metrics
branch
from
fae675a to
ab6772f
Compare
b1tamara
force-pushed
the
route-registry-metrics
branch
from
ab6772f to
ef4bebb
Compare
| componentName = "registry_message." + action | ||
| } else { | ||
| componentName = "registry_message." + msg.Component() | ||
| componentName = "registry_message." + action + "." + msg.Component() |
There was a problem hiding this comment.
This will probably also change the metric name right? I was hoping that we could preserve the metric name but only add an additional tag to it.
There was a problem hiding this comment.
This is what I had in mind. But it seems like the API is a bit cumbersome to use...
4 participants
Summary
Solution for cloudfoundry/routing-release#445.
Backward Compatibility
Breaking Change? Yes - metrics and log messages for route registry are renamed (needs to be fixed)