Skip to content

Enable metrics by default to enable correct function of Monitor#6364

Open
dlmarion wants to merge 1 commit into
apache:mainfrom
dlmarion:enable-metrics-by-default
Open

Enable metrics by default to enable correct function of Monitor#6364
dlmarion wants to merge 1 commit into
apache:mainfrom
dlmarion:enable-metrics-by-default

Conversation

@dlmarion
Copy link
Copy Markdown
Contributor

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.

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.
@dlmarion dlmarion added this to the 4.0.0 milestone May 11, 2026
@dlmarion dlmarion self-assigned this May 11, 2026
Copy link
Copy Markdown
Member

@ctubbsii ctubbsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants