[dns_check] change to network check #2924
Conversation
| def __init__(self, name, init_config, agentConfig, instances=None): | ||
| AgentCheck.__init__(self, name, init_config, agentConfig, instances) | ||
| self.default_timeout = init_config.get('default_timeout', self.DEFAULT_TIMEOUT) | ||
| def __init__(self, name, init_config, agentConfig, instances): |
There was a problem hiding this comment.
You can remove the constructor entirely if you are just calling the parent constructor
There was a problem hiding this comment.
I did this to set the default_timeout when configured in init_config. network.py only passes instance to _check(), not init_config.
https://github.com/DataDog/dd-agent/blob/master/checks/network_checks.py#L138
Is there a better way to set default_timeout when configured in the yaml file?
| # Fetches the conf | ||
| try: | ||
| hostname = instance.get('hostname') | ||
| except Exception: |
There was a problem hiding this comment.
instance.get('hostname')cannot return an exception.
Better to do something like
def _load_conf(self, instance):
hostname = instance.get('hostname')
if not hostname:
raise BadConfException('A valid "hostname" must be specified')
...| hostname, timeout, nameserver, record_type, resolver = self._load_conf(instance) | ||
|
|
||
| # Perform the DNS query, and report its duration as a gauge | ||
| start_time = end_time = 0.0 |
There was a problem hiding this comment.
You don't need to declare them, as you are declaring them a few lines later
| if end_time - start_time > 0: | ||
| self.gauge('dns.response_time', end_time - start_time, tags=tags) | ||
| self.service_check(self.SERVICE_CHECK_NAME, AgentCheck.OK, tags=self._get_tags(instance)) | ||
| self.gauge('dns.response_time', end_time - start_time, tags) |
There was a problem hiding this comment.
better to keep the keyword argument tags=tags
|
|
||
| def _get_tags(self, instance): | ||
| instance_name = instance.get('name') |
There was a problem hiding this comment.
I think people upgrading from a previous version won't have a name so we should default to the hostname
| def report_as_service_check(self, sc_name, status, instance, msg=None): | ||
| tags = self._get_tags(instance) | ||
|
|
||
| # FIXME: 5.3, Setting skip_event True here to hide deprecated option in yaml. |
|
When a |
|
Moving this PR to Triage since it's not a blocker for 5.10 but feel free to merge it whenever it's ready, I'll update the milestone accordingly |
|
@sjenriquez Then this will break compatibility for current users of the DNS check, if they upgrade their agent, the check will break. Can you fix that please ? |
What does this PR do?
Changes the DNS check to a network type check that allows instances to run in separate threads.
Motivation
Per customer request.
Additional Notes
Will affect this open PR: #2799