Skip to content

Conversation

@ibilley7
Copy link
Contributor

@ibilley7 ibilley7 commented Jun 4, 2025

Users might want to the ability to filter out metrics that are not necessary for them to view. Created a static getMeterFilter function in MeterRegistryFactory to filter out metrics user does not want to see. Created new Property GENERAL_MICROMETER_ID_FILTERS, and filters specified via this property will be added to meter registries created in MeterInfoImpl

Closes issue #4599

Users might want to the ability to filter out metrics that are not necessary for them to view. Created a static getMeterFilter function in MeterRegistryFactory to filter out metrics user does not want to see. Created new Property GENERAL_MICROMETER_ID_FILTERS, and filters specified via this property will be added to meter registries created in MeterInfoImpl

Closes issue apache#4599
@DomGarguilo DomGarguilo added this to the 4.0.0 milestone Jun 4, 2025
@lbschanno
Copy link
Contributor

This PR is failing checks because we need to specifically allow the MeterFilter class within the apilyzer configuration. You need to insert the following:

<allow>io[.]micrometer[.]core[.]instrument[.]config[.]MeterFilter</allow>

to core/pom.xml, directly below where there is a similar statement for MeterRegistry, line 301, so that we then have the following:

 <allows>
                <allow>io[.]micrometer[.]core[.]instrument[.]MeterRegistry</allow>
                <allow>io[.]micrometer[.]core[.]instrument[.]config[.]MeterFilter</allow>
                <allow>io[.]opentelemetry[.]api[.]OpenTelemetry</allow>
                ...

ibilley7 and others added 3 commits June 12, 2025 14:05
…csInfoImpl.java. Moved MeterRegistryFactoryTest.java test cases into the MetricsInfoImplTest.java file and changed variable types to reflect the new location of the code.
…csInfoImpl.java. Moved MeterRegistryFactoryTest.java test cases into the MetricsInfoImplTest.java file and changed variable types to reflect the new location of the code.
Copy link
Member

@DomGarguilo DomGarguilo left a comment

Choose a reason for hiding this comment

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

This looks pretty good. I am wondering if we should switch the filtering from a "deny list" to an "accept list" i.e. instead of defining which meters we want to filter out, we define a filter for only the meters we want to see. I don't think it makes a huge difference but might make things more intuitive or safer but I'm open to considering others opinions on this.

ibilley7 added 2 commits June 23, 2025 11:15
…erFilter function in MetricsInfoImpl, trimmed whitespace from patternList, set finalPredicate to id->false instead of null to OR the predicates together without having to set the first predicate equal to finalPredicate and so that the return statement doesn't have to include reuireNonNullElseGet
…n, which results in no filters being applied. Changed JUnit tests to test this case, and deleted the previous JUnit test that tests if an empty pattern throws an exception (that no longer applies). Changed code formatting.
Copy link
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.

Rather than provide a way to specify patterns that we turn into a MeterFilter class that we maintain, I think it would be better to be able to simply specify a MeterFilter class that the user provides, whose implementation is up to the user. We could provide a simple regex-based one as a reference.

However, the user already has the ability to construct a meter registry, with any filters they want, because they can specify a MeterRegistryFactory class that adds any MeterFilter they want on their MeterRegistry at the time their specified factory constructs it. So, I'm not sure we need to add this extra configuration at all.

Instead, since we have a LoggingMeterRegistryFactory as a reference implementation for a MeterRegistryFactory, you could just add these MeterFilter options to that factory's implementation only. I don't think this should be done as an extra feature in MetricsInfoImpl for all MeterRegistryFactory implementations the user might specify, because I think it's the responsibility of the Factory that the user provides to produce a MeterRegistry that is configured the way they want it to be.

Comment on lines +61 to +67
* <pre>
* general.custom.metrics.opts.meter.id.filters = prefix1,prefix2,prefix3
*
* </pre>
*
* To assign a custom delimiter to filters, set
* {@code general.custom.metrics.opts.meter.id.delimiter} in the Accumulo configuration.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making the delimiter configurable, a different way of doing this, one that is more consistent with how we have set other properties, would be to support something like:

general.custom.metrics.loggingfactory.filter.exclude.1=prefix1
general.custom.metrics.loggingfactory.filter.exclude.2=prefix2
general.custom.metrics.loggingfactory.filter..exclude.3=prefix3

That way you don't have to worry about a delimiter at all.

Alternatively, make it a single regex filter:

general.custom.metrics.loggingfactory.filter.deny.regex=^(prefix1|prefix2|prefix3).*$

@ibilley7
Copy link
Contributor Author

Today is the last day of my internship and I am returning back to school, so I would need someone else to take over this ticket if its a good enhancement to move forward with, as I won't have time to focus on it.

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.

5 participants