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

Add default factoryType tag in CommonsObjectPool2Metrics #5316

Conversation

HYEONSEOK1
Copy link
Contributor

@HYEONSEOK1 HYEONSEOK1 commented Jul 28, 2024

Overview

This PR addresses the issue of Prometheus tag collision by introducing distinct metric name prefixes for different pool types in the CommonsObjectPool2Metrics class. This change ensures that metrics from GenericObjectPool and GenericKeyedObjectPool have unique names, preventing conflicts caused by differing tag sets.

This PR addresses the issue of Prometheus tag collision by ensuring that all metrics have a consistent set of tags. For GenericObjectPool, a factoryType tag is added, and for other pool types, a default value of factoryType=none is used. This approach resolves the conflict caused by differing tag sets between GenericObjectPool and other pool types.

Changes

- Introduced distinct metric name prefixes: objectpool. for GenericObjectPool and keyedobjectpool. for GenericKeyedObjectPool.
- Updated the registerMetricsEventually method to apply the appropriate prefix based on the pool type.
- Adjusted metric registration methods (registerGaugeForObject, registerFunctionCounterForObject, registerTimeGaugeForObject) to use the new prefixes.

  • Added a default factoryType=none tag for all pool types except GenericObjectPool.

Reasoning

Prometheus requires that all meters with the same name have the same set of tag keys. The previous implementation violated this rule by adding a factoryType tag for GenericObjectPool, which was not present in other pool types. This caused registration failures in PrometheusMeterRegistry at this line. By ensuring that all metrics include a consistent set of tags, we eliminate tag collisions and comply with Prometheus' requirements.

@HYEONSEOK1 HYEONSEOK1 force-pushed the feature/objectpool-prometheus-tag-collision-fix branch from cac597d to b1c4c01 Compare July 28, 2024 07:39
@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Jul 30, 2024

Thank you for the PR!
Since in Micrometer we consider changing existing meter names and tags a breaking change, I don't think we should do this outside of a new major version.
What we can do though is assigning a value to factoryType if it is not available, e.g.: none. What do you think about doing that instead?

@jonatan-ivanov
Copy link
Member

Also, for everyone who might bump into this issue: you can work this around by adding a MeterFilter that adds a factoryType=none tag if it does not exists for these metrics.

@jonatan-ivanov jonatan-ivanov added the bug A general bug label Jul 30, 2024
@jonatan-ivanov jonatan-ivanov added this to the 1.9.18 milestone Jul 30, 2024
@HYEONSEOK1
Copy link
Contributor Author

Thank you for your feedback and suggestions!

I’ve reverted the previous changes and updated the code as suggested. The nameTag method now defaults to factoryType=none for all pool types except GenericObjectPool, which gets its specific factoryType tag.

HYEONSEOK1 and others added 2 commits July 31, 2024 14:42
There are cases where the factoryType tag is missing
which leads to issues in certain registries (i.e.: Prometheus).
This change adds a default "none" value to the factoryType tag so that
it will be always present.

See micrometer-metricsgh-5316
@jonatan-ivanov jonatan-ivanov force-pushed the feature/objectpool-prometheus-tag-collision-fix branch from b9f7ca9 to 90f3695 Compare July 31, 2024 22:12
@jonatan-ivanov jonatan-ivanov changed the base branch from main to 1.9.x July 31, 2024 22:12
@jonatan-ivanov
Copy link
Member

I polished your changes a bit and added tests so that we verify that all the tags are there all the time and rebased it on 1.9.x so that older versions of Micrometer will also get the fix.

@jonatan-ivanov jonatan-ivanov changed the title Separate Metric Names for Different Pool Types to Avoid Prometheus Tag Collision Add default factoryType tag in CommonsObjectPool2Metrics Jul 31, 2024
@jonatan-ivanov jonatan-ivanov merged commit a13915d into micrometer-metrics:1.9.x Jul 31, 2024
6 checks passed
@jonatan-ivanov jonatan-ivanov added the instrumentation An issue that is related to instrumenting a component label Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A general bug instrumentation An issue that is related to instrumenting a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants