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 jmx service checks #174

Merged
merged 2 commits into from
Jun 26, 2018
Merged

fix jmx service checks #174

merged 2 commits into from
Jun 26, 2018

Conversation

arbll
Copy link
Member

@arbll arbll commented Jun 25, 2018

What does this PR do

JMXfetch used the name of the configuration instead of the check name in the configuration to send service checks.
The recent refactor on datadog-agent that allowed having multiple configs for jmx by giving name different name exposed the issue and broke service checks for jmx. This fixes it.

@arbll arbll requested a review from truthbk June 25, 2018 18:27
@arbll arbll added this to the 0.21.0 milestone Jun 25, 2018
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.

LGTM, a couple of questions though:

  • I understand this code path is only active when JMXFetch is used with the Agent 6 (and not with the Agent 5)?
  • Is the check name here only used for the service check name, or should we look out for other uses of it in tags/JMXFetch's logic?

@arbll
Copy link
Member Author

arbll commented Jun 25, 2018

  • Yes, since they both have their own way of fetching configs this isn't too surprising. Agent5 allows for multiple config with the same name since it uses an Iterator. Agent6 needs to to have different names for configuration since we map the JSON to a dictionary.
  • I'll double check everything but this PR changes the check name at the instantiation of the check. It should replicate over everything using the check name.

[travis] remove debug flag
@truthbk truthbk force-pushed the arbll/fix-jmx-service-check branch from 508c620 to 322c92c Compare June 26, 2018 04:51
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.

This looks good to me, we're now using the right check name as submitted by the agent. This doesn't just affect service checks, but all metrics too.

The instance: tag had been broken in the previous agent release as we use the check name to build the instance tag. Unfortunately, this fix will cause churn to customers on 6.3.0. The fix will naturally also affect the service check as intended.

@truthbk truthbk modified the milestones: 0.21.0, 0.20.1 Jun 26, 2018
@truthbk truthbk merged commit f657b0b into master Jun 26, 2018
@truthbk truthbk deleted the arbll/fix-jmx-service-check branch June 26, 2018 16:45
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