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

Limit MBeans query to certain scopes #63

Merged
merged 2 commits into from
Jul 29, 2015
Merged

Conversation

yannmh
Copy link
Member

@yannmh yannmh commented Jul 22, 2015

JMXFetch is currently fetching all available beans regardless of the
user configuration. This might cause a high memory load when the
application is exposing a lot of metrics (thus OOM issues).

This change is the first step towards a bigger JMXFetch memory
consumption improvement: the idea is to find, among the user defined
beans, some common patterns to limit the scopes to query.

For instance,

- org.datadog.jmxfetch.test:scope=sameScope,param=sameParam,type=sameType
- org.datadog.jmxfetch.test:scope=sameScope,param=notTheSameParam,type=sameType

would induce a query on org.datadog.jmxfetch.test:scope=sameScope,type=sameType,*
instead of *:*.

At the moment, only one scope per domain name is computed: it's kind of
the greatest common divisor/pattern of the beans.

Ideally, this should be improved to recognize multiple scopes among the
same domain name.

JMXFetch is currently fetching all available beans regardless of the
user configuration. This might cause a high memory load when the
application is exposing a lot of metrics (thus OOM issues).

This change is the first step towards a bigger JMXFetch memory
consumption improvement: the idea is to find, among the user defined
beans, some common patterns to limit the scopes to query.

For instance,
```
- org.datadog.jmxfetch.test:scope=sameScope,param=sameParam,type=sameType
- org.datadog.jmxfetch.test:scope=sameScope,param=notTheSameParam,type=sameType
```
would induce a query on "org.datadog.jmxfetch.test:scope=sameScope,type=sameType,*"
instead of "*:*".

At the moment, only one scope per domain name is computed: it's kind of
the greatest common divisor/pattern of the beans.

Ideally, this should be improved to recognize multiple scopes among the
same domain name.
@yannmh yannmh self-assigned this Jul 22, 2015
@yannmh yannmh added this to the 5.5.0 milestone Jul 22, 2015
}
}
catch (Exception e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Bit of a nitpick, but could we also log an error so that the exception is not completely silent when JMXFetch's stderr is not redirected to a log?
Something like LOGGER.error("Unable to compute common bean scope, querying all beans as a fallback")

Copy link
Contributor

Choose a reason for hiding this comment

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

(not a nitpick and it would be better to catch a more specific exception)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

Otherwise I can't think of a specific exception that i want to catch. I added this very generic try {} catch(){} statement in order to fallback in the previous behavior in case anything was turning wrong. I should maybe remove it completely.

@olivielpeau
Copy link
Member

Looks great! 👍

Nothing really important in my comments except for the exception thing.

Log common scope computing exceptions.
Improve the code readability.
@yannmh
Copy link
Member Author

yannmh commented Jul 29, 2015

Thanks for the feedback.
I adressed your comments. It should be ready to merge now 🚀.

This closes the JMXFetch v0.8.0 release: I'll write the CHANGELOG and submit the new artifact to dd-agent.

yannmh added a commit that referenced this pull request Jul 29, 2015
Limit MBeans query to certain scopes
@yannmh yannmh merged commit 98d0873 into master Jul 29, 2015
@yannmh yannmh deleted the yann/scope-beans-query branch July 29, 2015 14:59
yannmh added a commit that referenced this pull request Jul 29, 2015
* [FEATURE] Wildcard support on domains and bean names. See [#57][]
* [IMPROVEMENT] Memory saving by limiting MBeans queries to certain
* scopes. See [#63][]

[#57]: #57
[#63]: #63
yannmh added a commit that referenced this pull request Jul 29, 2015
* [FEATURE] Wildcard support on domains and bean names. See [#57][]
* [IMPROVEMENT] Memory saving by limiting MBeans queries to certain
* scopes. See [#63][]

[#57]: #57
[#63]: #63
[skip ci]
@yannmh yannmh mentioned this pull request Jul 29, 2015
yannmh added a commit that referenced this pull request Aug 25, 2015
Do not scope MBeans queries on `list_everything` or
`list_not_matching_attributes`.
Fix regression introduced with #63.
yannmh added a commit that referenced this pull request Aug 26, 2015
Do not scope MBeans queries on `list_everything` or
`list_not_matching_attributes`.
Fix regression introduced with #63.
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.

None yet

3 participants