-
Notifications
You must be signed in to change notification settings - Fork 223
Conversation
efa7e82 to
8c017fb
Compare
@peanball
peanball
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this equivalent to the previously used m.Sender.IncrementCounter(componentName)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would change existing metrics, I don't think we should do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. We changed it back.
fae675a to
ab6772f
Compare
ab6772f to
ef4bebb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I had in mind. But it seems like the API is a bit cumbersome to use...
Uh oh!
There was an error while loading. Please reload this page.
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)