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

Fix possible memory leak in ObjectNameAttributeFilter #973

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

kw4s
Copy link
Contributor

@kw4s kw4s commented Jun 13, 2024

If:

  • autoExcludeObjectNameAttributes is set to true (and it is by default)
  • application registers and unregisters a lot of MBeans with unique names
  • those MBeans contain attribute types that cannot be exported

Then the ObjectNameAttributeFilter will cause a memory leak where excludeObjectNameAttributesMap will keep adding information about new beans to the exclude map but will never remove it, keeping information about beans no longer registered in MBeanServer:

Screenshot from 2024-06-11 12-09-20

I've added similar approach to what is already implemented in JmxMBeanPropertyCache - that is, once we get a new list of beans that we're interested in, remove any old ones from the cache and dynamic attribute filter.
I've also split excludeObjectNameAttributesMap into two maps, one for attributes from config (as we don't want to remove them automatically) and the other one for dynamically added attributes (those should be considered for removal).
This approach will also be relevant when resolving conflicts with #897 (as there the property names from config should be treated as patterns but property names added dynamically should be treated as "literal" - compared with equals).

@kw4s
Copy link
Contributor Author

kw4s commented Jun 13, 2024

the minimal reproducible example (after adding necessary imports): https://gist.github.com/kw4s/8b4b3f9e076bec01f2a29f3651ce1d06

@dhoard
Copy link
Collaborator

dhoard commented Jun 21, 2024

@kw4s thanks for the PR!!!

Can you sign your PR (DCO requirement)?

Have you ran the integration tests?

@dhoard dhoard self-assigned this Jun 21, 2024
@kw4s kw4s force-pushed the fix-memleak-objectnamefilter branch 2 times, most recently from cd6c926 to 16e1345 Compare June 21, 2024 16:35
Signed-off-by: Pawel Kwasny <pwkwasny@gmail.com>
@kw4s kw4s force-pushed the fix-memleak-objectnamefilter branch from 16e1345 to 8af30cf Compare June 21, 2024 16:36
@kw4s
Copy link
Contributor Author

kw4s commented Jun 21, 2024

@dhoard
Hi, I've amended the commit to add sign-off, I've also applied spotless required changes (did not run full build before, and I did not notice there is style enforced, sorry about that) and checked all integration tests, all PASS.

@dhoard
Copy link
Collaborator

dhoard commented Aug 19, 2024

@kw4s I wanted to let you know that I haven't forgotten about the PR. We are on a critical path for OpenTelemetry integration. Once addressed, I'll review your PR.

@batk0
Copy link

batk0 commented Sep 20, 2024

Hello! I've stumbled on the same problem. What's the status of this? Can it be merged in near future?

@kw4s
Copy link
Contributor Author

kw4s commented Sep 20, 2024

For us, as a workaround, we just disabled autoExcludeObjectNameAttributes

@dhoard dhoard merged commit aedf4de into prometheus:main Oct 9, 2024
1 check passed
@dhoard
Copy link
Collaborator

dhoard commented Oct 9, 2024

@kw4s Thanks for the PR!!!

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.

3 participants