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

[powerdns_recursor] add integration #2490

Merged
merged 4 commits into from
May 13, 2016
Merged

Conversation

degemer
Copy link
Member

@degemer degemer commented May 11, 2016

This PR supersedes #2108.

It adds a service check, restrict metrics being collected, and adds integration tests.

@degemer degemer self-assigned this May 11, 2016
@degemer degemer added this to the 5.8.0 milestone May 11, 2016
@degemer degemer force-pushed the quentin/antocard-powerdns-recursor branch 10 times, most recently from 5d25218 to 25c0263 Compare May 11, 2016 17:43
@degemer
Copy link
Member Author

degemer commented May 11, 2016

@AntoCard and @janeczku I restricted the metrics collected to the ones you advised, and used rates to collect the counter metrics: I think it's more useful to know the questions rate than the number of questions, please let me know if I'm wrong.

@janeczku I think the metrics exposed by PowerDNS are too incomplete to be useful if converted to a histogram, so I also submitted them as rates.

@AntoCard Thanks again for your initial PR !

@degemer degemer mentioned this pull request May 11, 2016
@yannmh yannmh assigned gmmeyer and unassigned degemer May 13, 2016
except Exception:
self.service_check(self.SERVICE_CHECK_NAME, AgentCheck.CRITICAL,
tags=service_check_tags)
raise
Copy link
Contributor

@gmmeyer gmmeyer May 13, 2016

Choose a reason for hiding this comment

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

Would we want to raise a custom error here rather than just the exception that was thrown by requests?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be nice, but since I have no idea what a non-2xx status means for PowerDNS, it wouldn't provide much more information than the default requests error.

So for now I'm going to keep it this way, if someone has an issue with the check, we'll update it to precise the error message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reasonable.

@gmmeyer
Copy link
Contributor

gmmeyer commented May 13, 2016

I added a single note, but otherwise I think this looks fine.

@degemer degemer force-pushed the quentin/antocard-powerdns-recursor branch from 25c0263 to 0781d38 Compare May 13, 2016 18:35
degemer added 2 commits May 13, 2016 14:44
This check now only collects metrics deemed useful.
It also sends a service check.

The configuration file is cleaned, and a few comments are added for
documentation purposes.
Build from source (:broken_heart:) and then test.
@degemer degemer force-pushed the quentin/antocard-powerdns-recursor branch from 0781d38 to 2298a51 Compare May 13, 2016 18:45
@degemer
Copy link
Member Author

degemer commented May 13, 2016

Thanks for the review @gmmeyer ! I'll wait for the 💚 CI before merging.

@degemer degemer merged commit 73ced6e into master May 13, 2016
@degemer degemer deleted the quentin/antocard-powerdns-recursor branch May 13, 2016 21:21
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.

2 participants