Improve gorouter Route Registry Message Metrics - #456
Conversation
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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)?
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
You are right. We changed it back.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
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...
Sorry, something went wrong.
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)