Add CoreDNS integration #2091

shraykay · 2018-08-22T04:37:47Z

What does this PR do?

Adding an integration for CoreDNS -- CoreDNS emits metrics via Prometheus.
See sample events from plugins: here, here, and here.

Motivation

Kubernetes is moving from KubeDNS -> CoreDNS starting v1.11

Review checklist

PR has a meaningful title or PR has the no-changelog label attached
Feature or bugfix has tests
Git history is clean
If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

Additional Notes

First time making a contribution! Please let me know what I may have missed, I borrowed heavily from KubeDNS's integration.

shraykay · 2018-08-22T04:39:48Z

@gmmeyer should I make changes to support OpenMetricsBaseCheck in this PR or in a separate one?

l0k0ms

Hello, Many thanks for your contribution 🙇 I left some comment regarding the documentation of your new integration.

l0k0ms · 2018-08-22T06:40:43Z

coredns/metadata.csv

+coredns.response_code_count.count,count,,response code,,instant number of responses per zone and rcode,1,coredns,response code
+coredns.proxy_request_count,gauge,,request,,query count per upstream.,1,coredns,proxy request count
+coredns.proxy_request_count.count,count,,request,,query count per upstream at a moment,1,coredns,proxy request count
+coredns.cache_hits_count,gauge,,hits,,Counter of cache hits by cache type,1,coredns,cache hit


Unit name should be hit not hits

https://docs.datadoghq.com/developers/metrics/#units

l0k0ms · 2018-08-22T06:41:12Z

coredns/metadata.csv

+coredns.proxy_request_count,gauge,,request,,query count per upstream.,1,coredns,proxy request count
+coredns.proxy_request_count.count,count,,request,,query count per upstream at a moment,1,coredns,proxy request count
+coredns.cache_hits_count,gauge,,hits,,Counter of cache hits by cache type,1,coredns,cache hit
+coredns.cache_hits_count.count,count,,hits,,Counter of cache hits by cache type at a moment,1,coredns,cache hit


unit_name should be hit not hits

https://docs.datadoghq.com/developers/metrics/#units

l0k0ms · 2018-08-22T06:41:35Z

coredns/metadata.csv

+coredns.proxy_request_count.count,count,,request,,query count per upstream at a moment,1,coredns,proxy request count
+coredns.cache_hits_count,gauge,,hits,,Counter of cache hits by cache type,1,coredns,cache hit
+coredns.cache_hits_count.count,count,,hits,,Counter of cache hits by cache type at a moment,1,coredns,cache hit
+coredns.cache_misses_count,gauge,,misses,,Counter of cache misses.,1,coredns,cache miss


r/misses/miss

l0k0ms · 2018-08-22T06:41:51Z

coredns/metadata.csv

+coredns.cache_hits_count,gauge,,hits,,Counter of cache hits by cache type,1,coredns,cache hit
+coredns.cache_hits_count.count,count,,hits,,Counter of cache hits by cache type at a moment,1,coredns,cache hit
+coredns.cache_misses_count,gauge,,misses,,Counter of cache misses.,1,coredns,cache miss
+coredns.cache_misses_count.count,count,,misses,,Counter of cache misses at a moment.,1,coredns,cache miss


r/misses/miss

l0k0ms · 2018-08-22T06:42:13Z

coredns/metadata.csv

+coredns.request_count.count,count,,request,,total query count at a moment,1,coredns,request count
+coredns.request_type_count,gauge,,request type,,counter of queries per zone and type,1,coredns,request type
+coredns.request_type_count.count,count,,request type,,counter of queries per zone and type,1,coredns,request type
+coredns.request_duration.seconds.sum,gauge,,seconds,,duration to process each query,-1,coredns,proxy request duration


r/seconds/second

and for the next 3 metrics too

l0k0ms · 2018-08-22T06:46:02Z

coredns/README.md

+Get metrics from CoreDNS in real time to:
+
+* Visualize and monitor DNS failures and cache hits/misses
+## Setup


Since this integration is in integration-core.
The CoreDNS check is packaged with the Agent? if so, no need to manually download check.py and check.conf files.

l0k0ms · 2018-08-22T06:47:26Z

coredns/README.md

+5. [Restart the Agent][3] to begin sending CoreDNS metrics to Datadog.
+
+### Validation
+


Validation section should follow the same layout as all other integrations in integrations core:
https://github.com/DataDog/integrations-core/tree/master/apache#validation

We don't display the result since it differs between agent v5 and v6.

l0k0ms · 2018-08-22T06:48:41Z

coredns/README.md

+    [...]
+```
+
+## Compatibility


No need to add the compatibility section in the readme. The information is already in the manifest.json file and has a specific display on the public documentation:

https://docs.datadoghq.com/integrations/apache/

l0k0ms · 2018-08-22T06:50:28Z

coredns/README.md

+See [metadata.csv][5] for a list of metrics provided by this integration.
+
+### Events
+The CoreDNS check does not include any event at this time.


We don't need to say "at this time" here. The documentation represents now, not a hypothetical future.

The CoreDNS check does not include any events.

l0k0ms · 2018-08-22T06:50:44Z

coredns/README.md

+The CoreDNS check does not include any event at this time.
+
+### Service Checks
+The CoreDNS check does not include any service checks at this time.


Same comment as for events subsections.

gmmeyer · 2018-08-22T15:57:45Z

@shraykay can you do it in this one? That PR should be merged soon!

gmmeyer

Thanks again for this! This is great! 😄

I have a few small changes to request, I'll take a second look once you migrate this to the new stuff.

One thing I wanna ask if if you could create an integration test for this -- as in, actually spin coredns up in a docker container and grab metrics from it. There's a bunch of examples of this in the repo. Nginx is a fairly simple one that should be easy to follow: https://github.com/DataDog/integrations-core/tree/master/nginx/tests

gmmeyer · 2018-08-22T16:00:01Z

coredns/tox.ini

+[testenv]
+platform = linux2|darwin
+deps =
+    datadog-checks-base


can you change this to -e../datadog_checks_base[deps]? That will, I believe, make the test work.

gmmeyer · 2018-08-22T16:01:16Z

coredns/datadog_checks/coredns/coredns.py

+        super(CoreDNSCheck, self).__init__(name, init_config, agentConfig, instances)
+        self.NAMESPACE = 'coredns'
+
+        self.metrics_mapper = {


can we make this configurable in the yaml? making this the default and then merging a yaml metrics_mapper map with this one would be a good idea I think

gmmeyer · 2018-08-22T16:05:41Z

coredns/datadog_checks/coredns/coredns.py

+            # https://coredns.io/plugins/cache/
+            'coredns_cache_size': 'cache_size.count',
+        }
+        self.ignore_metrics = {


do we wanna ignore all of these? (people can find lots of metrics useful for a lot of reasons)

gzussa

Thanks a lot for this contribution 💯 I made some additional comments. I think this is on good tracks 😄.

gzussa · 2018-08-23T08:28:38Z

coredns/requirements-dev.txt

@@ -0,0 +1,2 @@
+mock==2.0.0


You can just import datadog-checks-dev instead of those two

gzussa · 2018-08-23T08:30:13Z

coredns/setup.py

+    packages=['datadog_checks.coredns'],
+
+    # Run-time dependencies
+    install_requires=get_requirements('requirements.in')+[


You can remove the get_requirements('requirements.in') since it has nothing in it.

gzussa · 2018-08-23T08:36:44Z

coredns/tests/test_check.py

+
+
+@pytest.fixture
+def aggregator():


You can remove this function since it is part of datadog_check_base.
You can look at the Vault integrations for an example.

BrianChristie · 2018-08-23T18:52:30Z

Thanks for this work!

@ datadog folks, it would be great to merge this soon because as you can see in the Kubernetes V1.11 release notes https://kubernetes.io/docs/setup/release/notes/#sig-cluster-lifecycle , CoreDNS has replaced kube-dns as the default DNS provider so all new clusters have only CoreDNS running.

shraykay · 2018-08-23T18:53:24Z

i will work on this today/tomorrow! will have an updated version for you soon.

shraykay · 2018-08-24T04:33:35Z

I pushed a newer version, however I'm having trouble running an integration test using docker-compose. can anyone provide some pointers?

gzussa · 2018-08-24T09:41:18Z

coredns/tox.ini

+
+[common]
+deps =
+    ../datadog_checks_base


You should be able to replace both of those lines with -e../datadog_checks_base[deps]. See this example

gzussa · 2018-08-24T09:53:35Z

coredns/tests/test_check.py

+    }
+
+@pytest.fixture
+def aggregator():


aggregator is globally available with datadog_checks_dev there so you don't need to add it here :)

gzussa · 2018-08-24T10:00:31Z

coredns/tests/test_check.py

+
+@pytest.fixture
+def coredns():
+    env = os.environ


Instead of calling subprocess to start your container, you can leverage utilities in this file. You can find good examples here or there. Feel free to take a look at what the datadog_checks_dev package contains since it holds a lot of useful methods :)

shraykay · 2018-08-29T00:07:43Z

@gzussa thanks! was able to get docker-compose going with your help, could you help me figure out why flake8 isn't running? the error seems a bit vague

edit: disregard, was whitespaces

codecov-io · 2018-08-29T06:43:51Z

Codecov Report

Merging #2091 into master will increase coverage by 3.49%.
The diff coverage is 89.28%.

l0k0ms

LGTM 👌 ping @cswatt for a final doc review

ofek

Excellent work!

ofek · 2018-08-29T16:55:05Z

coredns/datadog_checks/coredns/coredns.py

+
+        return instance
+
+    def submit_as_gauge_and_monotonic_count(self, metric_suffix, message, scraper_config):


I think these should only be submitted as monotonic_count. See #1303

cc @mfpierre to verify

I agree we should use the send_monotonic_counter to True here I double-checked all the affected metrics are of the counter type.
As a result we can remove all the custom functions and METRIC_TRANSFORMERS + drop the extra .count gauges in the metadata.csv file

gmmeyer

I'm happy with this! it looks great! Thanks a lot.

coredns/MANIFEST.in

ofek · 2018-08-29T17:01:47Z

coredns/datadog_checks/coredns/coredns.py

+
+        return instance
+
+    def submit_as_gauge_and_monotonic_count(self, metric_suffix, message, scraper_config):


cc @mfpierre to verify

coredns/setup.py

ofek · 2018-08-29T17:06:17Z

coredns/setup.py

+    keywords='datadog agent check',
+    url='https://github.com/DataDog/integrations-core',
+    author='CoreDNS',
+    author_email='shray@namely.com',


author='Datadog', author_email='packages@datadoghq.com',

ofek · 2018-08-29T17:10:22Z

coredns/setup.py

+
+    # Extra files to ship with the wheel package
+    package_data={'datadog_checks.coredns': ['conf.yaml.example']},
+    include_package_data=True,


# The package we're going to ship packages=['datadog_checks', 'datadog_checks.coredns'], # Run-time dependencies install_requires=[CHECKS_BASE_REQ], # Extra files to ship with the wheel package include_package_data=True,

ofek · 2018-08-29T17:12:52Z

coredns/tests/test_check.py

+
+
+@pytest.fixture(scope="session")
+def spin_up_coredns():


Can you please put all these fixtures in a conftest.py? At that point they will be globally available.

ofek · 2018-08-29T17:14:12Z

coredns/tests/test_check.py

+                res = requests.get(URL)
+                # create some metrics by using dig
+                subprocess.check_call(DIG_ARGS, env=env)
+                res.raise_for_status


res.raise_for_status()

ofek · 2018-08-29T17:15:05Z

coredns/tests/test_check.py

+
+    compose_file = os.path.join(HERE, 'docker', 'docker-compose.yml')
+    env = os.environ
+    env['COREDNS_CONFIG_FOLDER'] = CONFIG_FOLDER


env = {'COREDNS_CONFIG_FOLDER': CONFIG_FOLDER}

ofek · 2018-08-29T17:16:36Z

coredns/tests/test_check.py

+    with open(mesh_file_path, 'rb') as f:
+        text_data = f.read()
+
+    p = mock.patch('requests.get', return_value=MockResponse(text_data, 'text/plain; version=0.0.4'), __name__='get')


with mock.patch(...): yield

rishabh · 2018-08-29T17:26:29Z

coredns/datadog_checks/coredns/data/conf.yaml.example

+
+instances:
+    # url of the metrics endpoint of prometheus
+  - prometheus_endpoint: http://localhost:9153/metrics


Can you please use prometheus_url instead of prometheus_endpoint?

rishabh · 2018-08-29T17:26:33Z

coredns/datadog_checks/coredns/coredns.py

+        """
+        Set up coredns instance so it can be used in OpenMetricsBaseCheck
+        """
+        endpoint = instance.get('prometheus_endpoint')


This should also be prometheus_url.

rishabh · 2018-08-29T17:26:35Z

coredns/datadog_checks/coredns/coredns.py

+        """
+        endpoint = instance.get('prometheus_endpoint')
+        if endpoint is None:
+            raise CheckException("Unable to find prometheus endpoint in config file.")


And we can just change this to prometheus_url.

rishabh · 2018-08-29T17:26:54Z

coredns/datadog_checks/coredns/coredns.py

+        super(CoreDNSCheck, self).__init__(name, init_config, agentConfig, instances=generic_instances)
+
+    def check(self, instance):
+        endpoint = instance.get('prometheus_endpoint')


And here as well prometheus_url.

rishabh · 2018-08-29T17:28:59Z

coredns/datadog_checks/coredns/coredns.py

+            # Primarily, metrics are emitted by the prometheus plugin: https://coredns.io/plugins/metrics/
+            # Note: the count metrics were moved to specific functions
+            # below to be submitted as both gauges and monotonic_counts
+            # 'coredns_dns_response_rcode_count_total': self.coredns_dns_response_rcode_count_total,


Since we're not using the transformer functions, we can just get rid of this dictionary.
As per #2091 (comment).

rishabh · 2018-08-29T17:29:15Z

coredns/datadog_checks/coredns/coredns.py

+    def check(self, instance):
+        endpoint = instance.get('prometheus_endpoint')
+        scraper_config = self.config_map[endpoint]
+        self.process(scraper_config, metric_transformers=self.METRIC_TRANSFORMERS)


And then we can also get rid of the metric_transformers named argument.

rishabh · 2018-08-29T17:30:05Z

coredns/datadog_checks/coredns/data/auto_conf.yaml

+instances:
+  # To enable CoreDNS metrics you must specify the prometheus url and enable the plugin within coredns
+  # See: https://coredns.io/plugins/metrics/
+  - prometheus_endpoint: "http://%%host%%:9153/metrics"


And this should also be prometheus_url.

rishabh · 2018-08-29T17:30:59Z

coredns/README.md

+  annotations:
+    ad.datadoghq.com/coredns.check_names: '["coredns"]'
+    ad.datadoghq.com/coredns.init_configs: '[{}]'
+    ad.datadoghq.com/coredns.instances: '[[{"prometheus_endpoint":"http://%%host%%:9153/metrics", "tags":["dns-pod:%%host%%"]}]]'


And this one, too :)

rishabh · 2018-08-29T17:31:50Z

coredns/tests/test_check.py

+from datadog_checks.utils.common import get_docker_hostname
+
+instance = {
+    'prometheus_endpoint': 'http://localhost:9153/metrics',


Let's fix the tests as well to use prometheus_url.

rishabh · 2018-08-29T17:31:53Z

coredns/tests/test_check.py

+@pytest.fixture
+def dockerinstance():
+    return {
+        'prometheus_endpoint': URL,


And let's also fix the docker instance config to use prometheus_url.

mfpierre · 2018-08-29T17:40:56Z

coredns/tests/test_check.py

+        NAMESPACE + '.proxy_request_duration.seconds.sum',
+        NAMESPACE + '.cache_size.count',
+    ]
+    COUNT_METRICS = [


we should remove this empty list

gmmeyer

this looks amazing! Thanks a lot!

shraykay · 2018-09-10T14:52:27Z

greetings! just following up on this, is there anything else required or can we get this merged in? :)

shraykay requested review from a team as code owners August 22, 2018 04:37

l0k0ms suggested changes Aug 22, 2018

View reviewed changes

gmmeyer suggested changes Aug 22, 2018

View reviewed changes

gzussa reviewed Aug 23, 2018

View reviewed changes

shraykay force-pushed the coredns branch from 740d2d6 to d84db14 Compare August 24, 2018 04:29

gzussa reviewed Aug 24, 2018

View reviewed changes

masci added community kind/new-integration labels Aug 24, 2018

shraykay force-pushed the coredns branch from 129ff8c to 0a897c0 Compare August 29, 2018 06:32

l0k0ms previously approved these changes Aug 29, 2018

View reviewed changes

ofek requested changes Aug 29, 2018

View reviewed changes

gmmeyer previously approved these changes Aug 29, 2018

View reviewed changes

ofek requested changes Aug 29, 2018

View reviewed changes

rishabh requested changes Aug 29, 2018

View reviewed changes

mfpierre added the containers label Aug 29, 2018

mfpierre reviewed Aug 29, 2018

View reviewed changes

shraykay dismissed stale reviews from gmmeyer and l0k0ms via f236fe4 August 30, 2018 00:56

shraykay force-pushed the coredns branch 2 times, most recently from bdbca94 to ac8517f Compare August 30, 2018 01:09

shraykay and others added 9 commits August 31, 2018 14:20

Fixing Manifest.json

bd4a32b

Addressing comments in README

6dd72f1

Fixing reference link

ea84ed8

wording update

334bbd9

Addressing Comments

9762171

Addressing Comments part 2

bf6fb42

Fixing whitespace

663b65e

If no metrics are set, use a default set

f5f7940

Addressing comments pt3

65b9e52

shraykay force-pushed the coredns branch from ae365a8 to 65b9e52 Compare August 31, 2018 23:58

shraykay and others added 6 commits August 31, 2018 18:22

adding doc line for params

f1edb47

fixing indentation in conf.yaml file

03ee997

fixing indentation in auto_conf.yaml

642f44a

Fixing parameter indentation

7213c3c

prometheus_endpoint -> url

55177c8

Update CHANGELOG.md

4d2c4d6

ofek previously approved these changes Sep 4, 2018

View reviewed changes

gmmeyer previously approved these changes Sep 4, 2018

View reviewed changes

grammar update in conf file

5118561

l0k0ms dismissed stale reviews from gmmeyer and ofek via 5118561 September 5, 2018 06:42

fixing true capitalization

597f984

gmmeyer approved these changes Sep 10, 2018

View reviewed changes

gmmeyer merged commit ac9cf85 into DataDog:master Sep 10, 2018

ofek changed the title ~~Adding CoreDNS integration to Datadog~~ Add CoreDNS integration Oct 13, 2018

ofek added changelog/Added integration/coredns labels Oct 13, 2018

nmuesch pushed a commit that referenced this pull request Nov 1, 2018

Adding CoreDNS integration to Datadog (#2091)

078af55

fanny-jiang mentioned this pull request Jan 24, 2022

Fix License Header Dates #11200

Merged

4 tasks

		5. [Restart the Agent][3] to begin sending CoreDNS metrics to Datadog.

		### Validation


		return instance

		def submit_as_gauge_and_monotonic_count(self, metric_suffix, message, scraper_config):

Add CoreDNS integration #2091

Add CoreDNS integration #2091

Conversation

shraykay commented Aug 22, 2018 • edited by l0k0ms Loading

What does this PR do?

Motivation

Review checklist

Additional Notes

shraykay commented Aug 22, 2018

l0k0ms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmmeyer commented Aug 22, 2018

gmmeyer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzussa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianChristie commented Aug 23, 2018

shraykay commented Aug 23, 2018

shraykay commented Aug 24, 2018

gzussa Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzussa Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

shraykay commented Aug 29, 2018 • edited Loading

codecov-io commented Aug 29, 2018 • edited Loading

Codecov Report

l0k0ms left a comment

Choose a reason for hiding this comment

ofek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfpierre Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

gmmeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofek Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabh Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

rishabh Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmmeyer left a comment

Choose a reason for hiding this comment

shraykay commented Sep 10, 2018

shraykay commented Aug 22, 2018 •

edited by l0k0ms

Loading

gmmeyer left a comment •

edited

Loading

gzussa Aug 24, 2018 •

edited

Loading

gzussa Aug 24, 2018 •

edited

Loading

shraykay commented Aug 29, 2018 •

edited

Loading

codecov-io commented Aug 29, 2018 •

edited

Loading

mfpierre Aug 29, 2018 •

edited

Loading

ofek Aug 29, 2018 •

edited

Loading

rishabh Aug 29, 2018 •

edited

Loading

rishabh Aug 29, 2018 •

edited

Loading