Skip to content
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

Use correct config nodes for metrics settings; when using global meter registry set metrics config correctly #8008

Merged
merged 1 commit into from Nov 17, 2023

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Nov 15, 2023

Description

Resolves #7911
Resolves #7980

(I am using a single PR to resolve both bugs because fixing 7911 alone causes microprofile/metrics unit tests to fail; the fix to 7980 takes care of that.)

Changes


Fix: MetricsFeature incorrectly accessed global meter registry

The constructor uses the meter registry from the config if one has been set (that is correct). If none was set in the config, the code uses the global meter registry. That is also correct except that the code should pass the metrics config when retrieving the global meter registry so that the global registry's behavior conforms to the config.

As originally written, the code did not pass the metrics config so the global registry was set up using defaults, not the specified config.


Fix MetricsObserver#type() method

It incorrectly returned "log" instead of "metrics", causing the MP server to incorrectly add the metrics observer again via its observer provider instead of yielding to the one already added by the metrics MP CDI extension.

I expect this was an oversight after copying and pasting from LogObserver.


Optimize and fix: MetricsCdiExtension unnecessarily created a metrics config builder and initialized the MP RegistryFactory

The configure() method correctly initializes the MetricsFactory using MetricsFactory.getInstance(config). Doing so automatically sets the metricsConfigkept internally in the factory, so the code did not need to create its own config builder and build it to get themetricsConfig` to use in retrieving the global meter registry.

Also, the RegistryFactory is already initialized automatically by RegistryFactoryManager when the globe meter registry is set up, so there is no need to do so again in the configure() method.


Optimize: RegistryFactory#getInstance(MeterRegistry)

The static method now returns the existing registry factory instance if it exists and if the specified MeterRegistry is the same as the one that was used to initialized the existing one.


Clarification: MetricsFactoryManager did not itself refer to both metrics and server.features.observe.observers.metrics config nodes

Things worked because later code filled in the config using the top-level metrics config if the server.features... settings for metrics were absent. This change, while not really changing behavior, puts in one place in the code the logic to fetch from either config node.


The PR also adds a test to make sure the configuration is processed correctly and the metrics behavior reflects the config.

Documentation

These are bug fixes: no doc impact.

…r registry set metrics config correctly

Signed-off-by: Tim Quinn <tim.quinn@oracle.com>
@tjquinno tjquinno self-assigned this Nov 15, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Nov 15, 2023
@tjquinno tjquinno merged commit e923b0a into helidon-io:main Nov 17, 2023
12 checks passed
@tjquinno tjquinno deleted the 4.x-metrics-selective branch November 17, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
2 participants