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

put new gc metrics behind a flag #197

Merged
merged 3 commits into from
Nov 29, 2018
Merged

put new gc metrics behind a flag #197

merged 3 commits into from
Nov 29, 2018

Conversation

arbll
Copy link
Member

@arbll arbll commented Nov 22, 2018

In 6.6 we introduced new names for gc metrics :

  • jvm.gc.minor_collection_count
  • jvm.gc.minor_collection_time
  • jvm.gc.major_collection_count
  • jvm.gc.major_collection_time

This names make more sense but unfortunately due to some gotchas in the way jmxfetch collect metrics this also had the effect of not sending the metrics using the old names.

This PR fixes this by putting the new names behind a flag.

@olivielpeau olivielpeau added this to the 0.23.0 milestone Nov 28, 2018
@olivielpeau olivielpeau requested a review from truthbk November 28, 2018 19:34
@olivielpeau
Copy link
Member

(tracking this flag as something we could want to enable by default in version 7 of the Agent)

Copy link
Member

@truthbk truthbk left a comment

Choose a reason for hiding this comment

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

CircleCI failure is unrelated and fixed in alternative PR.

This looks good! Thank you! 🏗

@truthbk truthbk merged commit 6e70f8e into master Nov 29, 2018
@truthbk truthbk deleted the arbll/new-gc-metrics-flag branch November 29, 2018 17:08
@olivielpeau
Copy link
Member

Had a couple of questions/notes:

  • how would this PR affect JMXFetch in the APM java agent?
  • (not urgent): we should document this new option in the jmx.d/conf.yaml.example file that's shipped with the agent, and also in all the example conf files of integrations

@truthbk
Copy link
Member

truthbk commented Nov 30, 2018

Synced with APM, they think it's fine, but they will schedule some work on their side to make sure they have full compatibility with either collection mode.

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