Skip to content

Add forward metrics - #4850

Merged
FlorianVeaux merged 1 commit into
DataDog:masterfrom
therc:patch-2
Oct 29, 2019
Merged

Add forward metrics#4850
FlorianVeaux merged 1 commit into
DataDog:masterfrom
therc:patch-2

Conversation

@therc

@therc therc commented Oct 21, 2019

Copy link
Copy Markdown
Contributor

What does this PR do?

Add CoreDNS metrics for the forward plugin

Motivation

The proxy plugin is deprecated and has been removed in recent versions, with users expected to replace it with the forward plugin.

Additional Notes

I couldn't grab data for all tests: healtcheck failures are missing.

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@therc
therc requested review from a team as code owners October 21, 2019 17:58
@therc

therc commented Oct 21, 2019

Copy link
Copy Markdown
Contributor Author

The Docker test fails because it runs an ancient version of CoreDNS, 1.2.0. Should there be two Docker tests?

@therc

therc commented Oct 21, 2019

Copy link
Copy Markdown
Contributor Author

I got it to work by changing the configuration to use both plugins and triggering two DNS lookups.

@hithwen

hithwen commented Oct 24, 2019

Copy link
Copy Markdown
Contributor

You can try including another coredns version to test in the tox file:

    py{27,37}-{1.2.0,1.6.4}

and then adapting the test for different versions.
You can find examples in other integrations that are version based like postgres

@therc

therc commented Oct 24, 2019

Copy link
Copy Markdown
Contributor Author

It doesn't look like we need to test two versions, the forward plugin already existed in 1.2.0.

@therve therve left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good, some minor comments.

Comment thread coredns/tests/conftest.py Outdated

DIG_ARGS = ["dig", "google.com", "@127.0.0.1", "-p", "54"]
# One lookup each for the forward and proxy plugins
DIG_ARGS = ["dig", "google.com", "@127.0.0.1", "example.com", "@127.0.0.1", "-p", "54"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you pass "HOST" instead of "127.0.0.1" ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

coredns_forward_response_rcode_count_total{rcode="NXDOMAIN",to="10.0.0.2:53"} 20482
# HELP coredns_forward_sockets_open Gauge of open sockets per upstream.
# TYPE coredns_forward_sockets_open gauge
coredns_forward_sockets_open{to="10.0.0.2:53"} 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is that not a useful metric to surface?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

Comment thread coredns/metadata.csv Outdated
The proxy plugin is deprecated and will be replaced by forward.
@therc

therc commented Oct 28, 2019 via email

Copy link
Copy Markdown
Contributor Author

@FlorianVeaux FlorianVeaux changed the title Add coredns forward metrics Add forward metrics Oct 29, 2019

@FlorianVeaux FlorianVeaux left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Merging and releasing the integration, thanks!
PS: The PR title is used in coredns changelog as is, I removed 'coredns' from it as we don't need to specify the integration name here.

@FlorianVeaux
FlorianVeaux merged commit 9b23b8f into DataDog:master Oct 29, 2019
@FlorianVeaux

FlorianVeaux commented Oct 29, 2019

Copy link
Copy Markdown
Member

This has been released and will be automatically included in Agent 6.16, until then you can install it manually with the command datadog-agent integration install datadog-coredns==1.3.0

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.

5 participants