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 support for collectorNamePattern in config and JavaAgent #760

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Selikoff
Copy link

@Selikoff Selikoff commented Dec 17, 2022

Per Issue 759, there is no way to prevent certain metrics from being processed, such as jvm_threads_state, as this is run inside CollectionRegistry before the jmx_exporter code can apply any rules. For applications with a lot of threads, this can temporarily cause the application to hang even if the metrics are later filtered out by a rule.

The solution is to add a new pattern value to the config that can be sent into JavaAgent and then passed to the HTTPServer as a SampleNameFilter. This allows metrics to be skipped from processing and resolves issue #759 by adding a config such as:

collectorNamePattern : "^(?!jvm_threads_state$).*$"            
rules:
  - pattern: ".*"

Adding per contributors read me: @fstab @tomwilkie

Copy link
Member

@fstab fstab left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot for your PR. I added two minor comments, apart from that it's looking good.

…s optional collectorNamePattern to set from Config file to filter metrics.

Signed-off-by: scottselikoff <[email protected]>
@Selikoff Selikoff force-pushed the AddCollectorNamePattern branch from bef32c1 to b425a01 Compare December 23, 2022 17:46
@Selikoff Selikoff requested a review from fstab December 23, 2022 17:51
@Selikoff
Copy link
Author

Selikoff commented Jan 4, 2023

@fstab Can we merge this CL? I believe I addressed your concerns.

@dhoard
Copy link
Collaborator

dhoard commented Feb 9, 2023

@Selikoff looking at the PR... I think the changes should also be made to the standalone HTTP JMX exporter as well.

https://github.com/prometheus/jmx_exporter/blob/main/jmx_prometheus_httpserver_common/src/main/java/io/prometheus/jmx/WebServer.java

@ltning
Copy link

ltning commented Jul 23, 2023

I'd also would very much like to see this merged, currently I'm not sure I can deploy this on high-volume systems without risk.

@dhoard
Copy link
Collaborator

dhoard commented Jul 26, 2023

@Selikoff are you still interested in completing this PR?

@dhoard
Copy link
Collaborator

dhoard commented Oct 9, 2024

@Selikoff following up on the PR. Are you still interested in completing this PR?

@dhoard dhoard self-assigned this Oct 9, 2024
@dhoard
Copy link
Collaborator

dhoard commented Dec 19, 2024

@Selikoff following up on the PR. Are you still interested in completing this PR?

@dhoard dhoard requested review from dhoard and removed request for fstab December 19, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants