micrometer-metrics / micrometer

An application observability facade for the most popular observability tools. Think SLF4J, but for observability.
https://micrometer.io
Apache License 2.0
4.45k stars 981 forks source link

Log meter registration failure events #5228

Open izeye opened 3 months ago

izeye commented 3 months ago

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

shakuzen commented 2 months ago

I don't think we should do this by default. https://github.com/micrometer-metrics/micrometer/issues/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 (MeterRegistry#onMeterRegistrationFailed) to configure logging if that's what users want (possibly locally or in a test environment rather than production). That's still configurable for users without this change, but I can appreciate the point that it's hard for users to know they should configure this because it's hard to realize the problem without logging. Perhaps as a compromise, we could use a WarnThenDebugLogger. Though this will result in double logging if a user also configures logging via onMeterRegistrationFailed. @jonatan-ivanov thoughts? You mentioned a concern elsewhere about duplicate logging when a composite meter registry is used.

izeye commented 2 months ago

@shakuzen Thanks for the explanation!

Using a WarnThenDebugLogger seems to be a good compromise for both.

jonatan-ivanov commented 2 months ago

I like WarnThenDebugLogger, I would still keep the option open of making the feature opt-in (off by default). Similar to Prometheus' registry.throwExceptionOnRegistrationFailure() (maybe by adding a listener that does this instead of modifying the meterRegistrationFailed method).

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?