Skip to content

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

Open
dlmarion wants to merge 3 commits into
apache:mainfrom
dlmarion:enable-metrics-by-default
Open

Enable metrics by default to enable correct function of Monitor#6364
dlmarion wants to merge 3 commits 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.

@dlmarion
Copy link
Copy Markdown
Contributor Author

Commit 412d9d2 resolves some of the issues from the prior comment. Specifically, it does the following:

  • Creates a MeterRegistryFactory for the MonitorMeterRegistry so it can be configured independently.
  • Modifies MiniAccumuloConfigImpl to disable metrics.
  • Modifies ITs that use metrics to fully set them up.
  • Fixes an issue where cache and fate metrics were being emitted when metrics were disabled.
  • Modifies AbstractServer.getMetrics to only emit metrics if the MonitorMeterRegistry is configured.
  • Removed metrics configuration from some ITs that were not using them.

@ctubbsii and I had a discussion about the behavior of enabling / disabling metrics totally using the property GENERAL_MICROMETER_ENABLED. If this property is set to false, then no metrics are set up at all. There was a question as to whether this property should only apply to Accumulo related metrics. If so, then the properties GENERAL_MICROMETER_JVM_METRICS_ENABLED and GENERAL_MICROMETER_LOG_METRICS would act independently. I think it's fine to leave things as they are. If someone wants to set up the global registry using a Java Agent or Spring Boot, they can add the JVM and log metrics at that time and disable ours.

@dlmarion dlmarion requested a review from ctubbsii May 13, 2026 12:33
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