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

Add kafka to the list of jmx checks #3304

Merged
merged 1 commit into from
Apr 20, 2017
Merged

Add kafka to the list of jmx checks #3304

merged 1 commit into from
Apr 20, 2017

Conversation

masci
Copy link
Contributor

@masci masci commented Apr 12, 2017

What does this PR do?

When SD uses auto_conf to init JMX-fetch related checks, it verifies that the check name is listed in the list JMX_CHECKS. Kafka was missing, making impossible to use SD with JMX an the autoconf config storage.

@masci masci added this to the 5.12.4 milestone Apr 12, 2017
@masci masci requested a review from truthbk April 12, 2017 05:59
@masci masci added the bugfix label Apr 12, 2017
@olivielpeau
Copy link
Member

Just a question: adding is_jmx: yes to the init_config section of the kafka auto conf yaml should work no?

That's currently the way we detect that a regular kafka.yaml file is a jmxfetch yaml config, and I don't see a reason why this detection would stop working with auto conf.

That said, even if this detection works, I don't see any reason not to merge this PR, kafka.yaml always refers to a jmxfetch config AFAIK.

@masci masci modified the milestones: 5.12.4, 5.13.0 Apr 12, 2017
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.

I think the problem is that the template generator does indeed check if the check_name is a JMX check before generating the template, if it's not found in the list, it's skipped. So this seems consistent. It's kind of a chicken and egg problem, the template might have the is_jmx set, but until we generate the template we simply consider it that, a template, not a valid configuration. We could maybe revisit, but this makes sense for now.

@truthbk truthbk merged commit 84f8be7 into master Apr 20, 2017
@olivielpeau olivielpeau deleted the massi/kafka-jmx branch May 8, 2017 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants