-
Notifications
You must be signed in to change notification settings - Fork 979
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 when a MeterFilter is configured after a meter is registered #4917
Conversation
d1e60fd
to
17acc78
Compare
micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java
Outdated
Show resolved
Hide resolved
micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java
Outdated
Show resolved
Hide resolved
micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java
Outdated
Show resolved
Hide resolved
micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java
Outdated
Show resolved
Hide resolved
micrometer-core/src/test/java/io/micrometer/core/instrument/MeterRegistryLoggingTest.java
Show resolved
Hide resolved
16c934b
to
3b82355
Compare
3b82355
to
7bdc5a1
Compare
FFT:
|
Thanks for the review. I didn't particularly think about composite registries. What kind of scenarios are you thinking might require special attention? |
micrometer-core/src/test/java/io/micrometer/core/instrument/MeterRegistryLoggingTest.java
Show resolved
Hide resolved
One particular case I am aware of is with Why we add filters to global registry? |
Thanks for the feedback. That's good to be aware of.
If whatever is configuring everything (e.g. Spring Boot auto-config) can be made to not do what will result in registering meters until after filters have been configured, this should be solvable. It's just made harder by there being a "hidden" dependency on the global MeterRegistry that e.g. Spring Boot doesn't and can't automatically know about. It's not clear what we can/should change about this pull request for this yet, so let's get this into RC1 and collect feedback. Let's try to figure out as many of these scenarios as we can in the upcoming month before GA based on testing with the RC1 version. Hopefully we can find a good solution for these. |
In normal usage we would expect this should not happen, but there is nothing enforcing or encouraging it to not happen. This warns users via logging that it is happening and advises to correct it, if possible. Otherwise, we want to learn about why it is not fixable. The problem with configuring MeterFilters after a Meter has been registered is that such filters will not be applied to previously registered meters. This can result in a mix of metrics with all filters applied and only some filters applied, which is potentially hard to notice.
7bdc5a1
to
84d1de1
Compare
…en DEBUG level is enabled See micrometer-metricsgh-4917
…en DEBUG level is enabled See micrometer-metricsgh-4917
In normal usage we would expect this should not happen, but there is nothing enforcing or encouraging it to not happen. This warns users via logging that it is happening and advises to correct it, if possible. Otherwise, we want to learn about why it is not fixable. The problem with configuring MeterFilters after a Meter has been registered is that such filters will not be applied to previously registered meters. This can result in a mix of metrics with all filters applied and only some filters applied, which is potentially hard to notice.