Enable metrics by default to enable correct function of Monitor#6364
Enable metrics by default to enable correct function of Monitor#6364dlmarion wants to merge 1 commit into
Conversation
This change removes the LoggingMeterRegistryFactory from the default value of the general.micrometer.factory property and changes the default value of general.micrometer.enabled from false to true.
ctubbsii
left a comment
There was a problem hiding this comment.
I was thinking that an alternative would be to trivially wrap the MonitorMeterRegistry with a MonitorMeterRegistryFactory, so that the monitor meter registry could be disabled independently, and because it provides a reasonable non-empty default value for the list of factories.
I was initially thinking we could get rid of the boolean enable property, since I thought we could infer disabled when the list was made to be empty. However, we'd still need the boolean because we should still have the ability to turn off Accumulo metrics when the global registry is set up with -javaagent or Spring Boot, or similar methods of populating the global registry, even if our list of factories is empty. In other words, we can't assume that an empty list means disable metrics and that a non-empty list means enable metrics, so we still need the explicit property to turn on and off Accumulo's own metrics. (Only mentioning it here, to record the reason for rejecting that idea.)
I do wonder what the performance impact is, and the build time impact for having the metrics on by default for all the ITs, when most ITs don't use them.
This change removes the LoggingMeterRegistryFactory from the default value of the general.micrometer.factory property and changes the default value of general.micrometer.enabled from false to true.