add support for powerdns authoritative metrics #2850
Conversation
There was a problem hiding this comment.
Hey @joewilliams! Thanks a lot for the Pull Request.
First, can you add a few tests to this? Even if they're just mocks, we'd really appreciate it.
I've added a few more comments inline, some are just questions, some are things that I think need to be fixed.
One last thing, just mentioning it for your information: we're working on a new project for checks, the Integrations SDK. As part of that, we'll be moving all the checks out of this repo and into new ones: integrations-core and integrations-extras.
| elif 'latency' in stat['name']: | ||
| self.gauge('powerdns.authoritative.{}'.format(stat['name']), float(stat['value']), tags=tags) | ||
| else: | ||
| self.rate('powerdns.authoritative.{}'.format(stat['name']), float(stat['value']), tags=tags) |
There was a problem hiding this comment.
Can you simplify the logic in this conditional? We usually like to list the metrics we gather within the check within a constant at the top of the class, like RATE_METRICS and GAUGE_METRICS.
This adds to the clarity of the check, because you can tell what is being gathered, but it also ensures that a single check isn't sending a potentially unlimited number of different metrics.
| config, tags = self._get_config(instance) | ||
| stats = self._get_pdns_stats(config) | ||
| for stat in stats: | ||
| self.log.debug('powerdns.authoritative.{}:{}'.format(stat['name'], stat['value'])) |
There was a problem hiding this comment.
Can you remove this logging statement? While debug logging is always more verbose, I don't think we should be logging everything that's collected from this check.
|
|
||
| def _get_pdns_stats(self, config): | ||
| url = "http://{}:{}/api/v1/servers/localhost/statistics".format(config.host, config.port) | ||
| service_check_tags = ['authoritative_host:{}'.format(config.host), 'authoritative_port:{}'.format(config.port)] |
There was a problem hiding this comment.
Should these tags be added to the other metrics as well?
|
Other than the comments I posted, I think this is pretty good! Thank you very much for your contribution! 😄 |
|
Hi @joewilliams, since the transition to the SDK has begun I have taken the liberty to open a PR with your work here: DataDog/integrations-extras#46. You authorship should be preserved. This should hopefully ease the pain of transitioning to the SDK. Let me know if you have any questions. |
|
Hi @joewilliams, Would it be OK if we close this PR and track further efforts in DataDog/integrations-extras#46 ? We're currently in the process migrating agent checks out this repo and into new https://github.com/DataDog/integrations-core and https://github.com/DataDog/integrations-extras repos. This change would fit into the https://github.com/DataDog/integrations-extras repo where @masci and @truthbk have migrated your PR. We'd appreciate your help in addressing the minor comments on this PR so that we can merge and release your contribution. To offer some background the reason we're moving checks into seperate repos is to allow us to release new checks such as yours here independently of the agent. This will allow us to merge/test them more quickly and offer you quicker feedback. Some background on this is available at: If you have any questions, we are happy to discuss here in this PR or on Slack in #integrations. Also I realize it is short notice but our next community office hours and code review is today at 10:30am PT and we'd be happy to review a PR with you there if you'd like to join us. The next one will be in 2 weeks. |
|
@irabinovitch sounds good. |
What does this PR do?
This PR adds support for PowerDNS Authoritative server metrics.
Motivation
We need the metrics.