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

Default JVM metrics for GC pools, class load count, and descriptors #198

Merged
merged 1 commit into from
Nov 30, 2018

Conversation

realark
Copy link
Contributor

@realark realark commented Nov 26, 2018

For our APM JMX graphs we're interested in adding charts for specific GC pools (eden, old gen, etc), class load counts, and open file descriptors.

Couple of questions:

  • Does it make sense to put these changes here (in core JMX)?
  • Process-wise, does it work to propose built-in metric changes like this (through a PR)?

@realark realark force-pushed the ark/gc-heap-pool-metrics branch from 23f6d2b to e688bc0 Compare November 29, 2018 17:36
Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

We have to make sure the beans you're matching aren't already matched by existing includes (JMXFetch can only match a JMX attribute with at most one include conf, so if an attribute matches multiple includes, the first one that matches will determine the metric name and type that JMXFetch will emit for this JMX attribute). Had a cursory look at the existing default metrics and it looks OK 👍

To answer your questions:

  • Adding these metrics in core JMXFetch means that all future versions of JMXFetch will collect these new metrics by default . If this was your intent then that makes perfect sense.
  • Process-wise a PR is fine, any explanation you may have on why these metrics are important and should be collected by default would be interesting. Also, if these beans/attributes only exist on certain environments, that'd be interesting to document as well.

@realark realark force-pushed the ark/gc-heap-pool-metrics branch from e688bc0 to b582e76 Compare November 29, 2018 21:56
@realark
Copy link
Contributor Author

realark commented Nov 29, 2018

Excellent, thanks! Yes it was my intent to add these to the default metrics.

These metrics will be used by APM to generate interesting JVM graphs and it seems like they are interesting to agent jmxfetch users as well.

@olivielpeau olivielpeau added this to the 0.23.0 milestone Nov 30, 2018
@olivielpeau
Copy link
Member

@realark we can merge this if you've confirmed it works well :)

@realark
Copy link
Contributor Author

realark commented Nov 30, 2018

@olivielpeau Thanks! Yep it's working well! Just did some more end-to-end testing to verify:

image

@realark realark merged commit 88f283f into DataDog:master Nov 30, 2018
@realark realark deleted the ark/gc-heap-pool-metrics branch November 30, 2018 18:10
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.

2 participants