-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Warn about gauge re-registration #5688
Warn about gauge re-registration #5688
Conversation
I added benchmarks to see the overhead if the registered I ran the tests quite a few times but on my machine it seems the "noise" is bigger than the difference between the two scenarios I tested. Before
|
micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java
Outdated
Show resolved
Hide resolved
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.
I'm fine merging since we didn't detect a consistent, significant increase in time taken via benchmarks.
See #5616 and #5617
I added the warning log entries and benchmarks since the extra logging is on the hot path.
The benchmarks don't test the case where the
Gauge
re-registration happens indirectly (as the result of aMeterFilter
transformation) but since the direct and the indirect cases call the same method, I think testing only the direct case is ok.I ran the benchmarks a couple of times but they were not as stable on my laptop as I would have liked, the difference between them was sometimes bigger between two identical executions (e.g. without logging) than between the two different ones (with vs. without logging).
So based on this either the benchmarks are wrong or the "noise" is bigger than the difference between the two scenarios I tested.
Before logging
After logging
/cc: @keith-turner