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

[3.x] Fix improper handling of metrics global tags #5812

Merged
merged 2 commits into from
Jan 10, 2023

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Jan 9, 2023

Resolves #5791

MP Metrics requires that the config settingmp.metrics.tags (format tag1=value1,tag2=value2") should cause MP metrics to assign the specified "global tags" to decorate every MetricID during output.

In our SE implementation, we follow the same approach except with metrics.tags as the config key.

Investigating the issue revealed one error and one not-great practice.

  1. Error: MetricsSettings, (in SE), when using the default config to create an instance, used Config.create() without also using .get("metrics") to obtain the metrics-specific assignments. As a result, a top-level config key of "tags" (the issue described using an environment variable but a setting at the top level of any config source would show the problem) caused MetricsSettings to try to use that as the global metrics tag settings. The value was not formatted as the code expected so it threw an exception.
  2. Not-great practice: In MetricsCdiExtension, we have to merge settings from mp.metrics and metrics if both are present to create the metrics settings we use in preparing the MetricsSupport object. The code does this by creating a temporary Config object with config sources from the two config nodes at mp.metrics and metrics. But it did so by using Config.builder()... without disabling the built-in env. var. and system property sources. This should not have affected anything, but it was confusing because the mp.metrics and metrics sections of config used to create the temporary Config object would already have accounted for env. vars and properties.

The code contains fixes in these two spots plus new tests that run in separate JVMs to avoid cross-contaminating the test configs with the tag settings. (I probably could have used custom Config objects in the tests to simplify the test executions but I wanted to reproduce the conditions reported in the issue, which used an env. var.)

@tjquinno tjquinno added this to the 3.1.1 milestone Jan 9, 2023
@tjquinno tjquinno requested review from spericas and barchetta January 9, 2023 23:23
@tjquinno tjquinno self-assigned this Jan 9, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jan 9, 2023
@tjquinno tjquinno changed the title Fix improper handling of metrics global tags [3.x] Fix improper handling of metrics global tags Jan 9, 2023
@tjquinno tjquinno added the 3.x Issues for 3.x version branch label Jan 9, 2023
*/
static MetricsSettings create() {
return create(Config.create());
Copy link
Member

Choose a reason for hiding this comment

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

How incompatible is this change? I'm concerned that users will have defined a top-level config key of "tags" to get metrics global tags to work -- or maybe that is unlikely.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this would be very unlikely, and if they did it should be an easy change to begin using the correct key.

@tjquinno tjquinno merged commit c4e1028 into helidon-io:helidon-3.x Jan 10, 2023
@tjquinno tjquinno deleted the metrics-config-prefix-3.x branch January 10, 2023 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues for 3.x version branch OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.x] Metrics incorrectly uses top-level config, not metrics-level, in setting global tags
3 participants