-
Notifications
You must be signed in to change notification settings - Fork 464
Add ability to filter out metric by id name #5616
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
base: main
Are you sure you want to change the base?
Conversation
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
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>
... |
…lyzer configuration.
…lyzer configuration.
core/src/main/java/org/apache/accumulo/core/spi/metrics/MeterRegistryFactory.java
Outdated
Show resolved
Hide resolved
…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.
server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsInfoImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsInfoImpl.java
Outdated
Show resolved
Hide resolved
server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsInfoImpl.java
Outdated
Show resolved
Hide resolved
server/base/src/main/java/org/apache/accumulo/server/metrics/MetricsInfoImpl.java
Outdated
Show resolved
Hide resolved
…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.
There was a problem hiding this 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.
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