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

Document new collect_default_jvm_metrics flag for JMXFetch integrations #8153

Merged
merged 3 commits into from
Dec 11, 2020

Conversation

gh123man
Copy link
Member

@gh123man gh123man commented Dec 7, 2020

What does this PR do?

Added docs for collect_default_jvm_metrics. This flag allows users to disable default jvm metrics in JMXFetch

Related PRs:
DataDog/datadog-agent#6923
DataDog/jmxfetch#345

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

apigirl
apigirl previously approved these changes Dec 7, 2020
Copy link
Contributor

@apigirl apigirl left a comment

Choose a reason for hiding this comment

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

docs look good

coignetp
coignetp previously approved these changes Dec 8, 2020
Copy link
Contributor

@coignetp coignetp left a comment

Choose a reason for hiding this comment

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

LGTM!

type: number

- name: collect_default_jvm_metrics
description: Allows disabling the collection of default JVM metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: Allows disabling the collection of default JVM metrics
description: Enables the collection of default JVM metrics

Small nit but feel like this wording would be more clear since it is disabled by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry if this was not clear! Actually this defaults to true. Perhaps I should call that out in the doc?
For context - the existing behavior is to collect default JVM metrics but I wanted to add a flag to explicitly disable that.

I thought it may be misleading to have a disable_collect_default_jvm_metrics: true because that logic feels backwards to me.

@ChristineTChen
Copy link
Contributor

Is this option related to collect_default_metrics in the init_config?

- name: collect_default_metrics
required: true
description: Whether or not the check should collect all default metrics.
value:
example: true
type: boolean

@gh123man
Copy link
Member Author

gh123man commented Dec 8, 2020

Is this option related to collect_default_metrics in the init_config?

- name: collect_default_metrics
required: true
description: Whether or not the check should collect all default metrics.
value:
example: true
type: boolean

Nope! This is a different behavior and it is specific to the instance.

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.

Just a comment regarding defaults and clarity.

@gh123man gh123man dismissed stale reviews from coignetp and apigirl via 1b6bba0 December 10, 2020 20:43
@gh123man gh123man requested review from apigirl and truthbk December 11, 2020 14:02
@gh123man gh123man merged commit 4711963 into master Dec 11, 2020
@gh123man gh123man deleted the brian/update-jmx-config-default-metrics branch December 11, 2020 14:20
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.

6 participants