Skip to content

MultiGauge doesn't work with MeterFilter.map() #6146

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 1, 2025
Merged

MultiGauge doesn't work with MeterFilter.map() #6146

merged 2 commits into from
May 1, 2025

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Apr 19, 2025

The MultiGauge doesn't seem to work with the MeterFilter.ignoreTags().

See https://github.com/micrometer-metrics/micrometer/pull/6070/files#r2051508975

@izeye izeye mentioned this pull request Apr 19, 2025
Merged
@jonatan-ivanov jonatan-ivanov added bug A general bug module: micrometer-core An issue that is related to our core module labels Apr 22, 2025
@jonatan-ivanov jonatan-ivanov added this to the 1.13.14 milestone Apr 22, 2025
@jonatan-ivanov
Copy link
Member

Thank you for finding this bug and proving it with a test.
I pushed a hack that I don't particularly like because:

  1. It extends the visibility of getMappedId from private to package-private (less of an issue I think)
  2. It adds an extra execution of MeterFilters, also see the commit message

I'm continue working on it to see if I can find a better solution (how to integrate this with the MeterFilter caching mechanism of the MeterRegistry).

@jonatan-ivanov
Copy link
Member

I added caching and an extra test to check the behavior of map from the angle when the tag is modified and different tag values are mapped to the same new value.

@jonatan-ivanov jonatan-ivanov requested a review from shakuzen April 22, 2025 22:26
@jonatan-ivanov jonatan-ivanov changed the title MultiGauge doesn't work with MeterFilter.ignoreTags() MultiGauge doesn't work with MeterFilter.map() Apr 22, 2025
shakuzen
shakuzen approved these changes May 1, 2025
View reviewed changes
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I may follow up post merge with some general polish to the MultiGauge code to try to name things in a way that hopefully makes the logic easier to read. It took a bit of re-reading and taking notes to make sure I understood what was happening before and what's happening now.

izeye and others added 2 commits May 1, 2025 11:56
@izeye @jonatan-ivanov
Test that MultiGauge doesn't work with MeterFilter
7bafe9f 
See https://github.com/micrometer-metrics/micrometer/pull/6070/files#r2051508975

Signed-off-by: Johnny Lim <[email protected]>
@jonatan-ivanov
Track mapped Meter.Id in MultiGauge
cf3e204 
MultiGauge maintains the set of ids of the Gauges it created. These ids
are "pre-filtered" which means it stores the ids before the possible
transformations done by MeterFilters.
This changes the behavior and makes MultiGauge tracking the ids after
the MeterFilter transformations. This also means that after this change
MeterFilters can run twice.
@jonatan-ivanov jonatan-ivanov merged commit 6154d4c into micrometer-metrics:1.13.x May 1, 2025
8 checks passed
@jonatan-ivanov
Copy link
Member

Thank you @izeye!

@izeye izeye deleted the gh-6070 branch May 11, 2025 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonatan-ivanov jonatan-ivanov jonatan-ivanov left review comments

@shakuzen shakuzen shakuzen approved these changes

Assignees
No one assigned
Labels
bug A general bug module: micrometer-core An issue that is related to our core module
Projects
None yet
Milestone
1.13.14
Development

Successfully merging this pull request may close these issues.
3 participants
@izeye @jonatan-ivanov @shakuzen