-
Notifications
You must be signed in to change notification settings - Fork 990
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 Prometheus meter registration failure #5228
Conversation
I don't think we should do this by default. #2068 has some history. I understand the usefulness of being able to identify these issues with a log message, but it's important to balance this with the principle that observability impact to your application should be minimal - I think this includes avoiding logging too much. It's better to lose some observability info than cause issues for the application. Logging this is far more reasonable if we're only talking about meter registration failures caused by instrumentation issues in code that users control in their application code, but it's a problem when we're talking about instrumentation issues in code that users don't control (third-party libraries). I believe that's essentially why we changed the default to doing nothing in Prometheus on meter registration failure due to the known limitation but we added a way ( |
@shakuzen Thanks for the explanation! Using a |
I like Also, I think if you have a composite registry, and all the registries will report meter registration failures by default, you will get this message multiple times, e.g.: composite(prometheus, simple) will report this 3 times. With an extra config option you can control which registries are ok to report such an issue and if the feature is off by default it won't disrupt anything, then you can turn it on for the registry of your choice if you want. What do you think? |
I don't want to keep this PR in a state of limbo. I think given the above discussion, it maybe is best to not do something in MeterRegistry by default that can't be overridden due to duplicate logging with multiple registries. I know this could be a generic issue and the reasoning behind wanting to do this potentially applies to any registry - it's hard to know you have meter registration failures in the first place if nothing is logged. But since the original issue is about the known registration failure that happens with the Prometheus registry, maybe a compromise is to use a WarnThenDebugLogger in the Prometheus registry only for now. Thoughts? |
@jonatan-ivanov @shakuzen Thanks for the feedback!
I updated this PR in this direction. |
Thanks for quickly updating the PR. I've polished things a bit in c799962 |
A meter registration failure seems to be an important event for users, but it's not easy for the users to be aware of the failure unless they register a callback for it.
It would be better to make the users easily aware of the failures by default.
See gh-5192