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

dnsdist: Support Prometheus latency histograms #7853

Merged
merged 1 commit into from Jun 3, 2019

Conversation

Projects
None yet
5 participants
@Marlinc
Copy link

commented May 27, 2019

Short description

This PR adds support for Prometheus latency histograms as described in: https://prometheus.io/docs/concepts/metric_types/#histogram

It adds 3 metrics, latency_sum, latency_count and latency_bucket. The latency_bucket metrics have a le label which has the lower and equal to latency of the request in ms.

This resolves #6088 for dnsdist.

Example graph:
image

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

@Marlinc Marlinc changed the title dnsdist: Add support for Prometheus histogram support for response la… dnsdist: Support Prometheus latency histograms May 27, 2019

@Habbie Habbie requested review from rgacogne and pieterlexis May 27, 2019

@Habbie Habbie added this to the dnsdist-1.4.0 milestone May 27, 2019

@rgacogne
Copy link
Member

left a comment

Thanks for this PR! I have a few comments but it looks good overall!

Show resolved Hide resolved pdns/dnsdist-web.cc Outdated
Show resolved Hide resolved pdns/dnsdist.hh Outdated
Show resolved Hide resolved pdns/dnsdist.hh Outdated

@Marlinc Marlinc force-pushed the Marlinc:dnsdist-prometheus-histogram branch 2 times, most recently from 419646b to 0957b46 May 28, 2019

@rgacogne
Copy link
Member

left a comment

A few nits, looks great otherwise!

Show resolved Hide resolved pdns/dnsdistdist/docs/statistics.rst Outdated
Show resolved Hide resolved pdns/dnsdistdist/docs/statistics.rst Outdated
Show resolved Hide resolved pdns/dnsdistdist/docs/statistics.rst Outdated

@Marlinc Marlinc force-pushed the Marlinc:dnsdist-prometheus-histogram branch from 0957b46 to e984744 May 28, 2019

@Marlinc Marlinc marked this pull request as ready for review May 28, 2019

@Marlinc

This comment has been minimized.

Copy link
Author

commented May 28, 2019

Fixes all the things!

@Marlinc

This comment has been minimized.

Copy link
Author

commented May 28, 2019

image

@wojas

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Great addition! Am I correct that the metrics are not prefixed with dnsdist_server_ unlike the other ones?

@Marlinc

This comment has been minimized.

Copy link
Author

commented May 28, 2019

That is a good one and is by mistake, I will fix that

@wojas

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@Marlinc Great! Correction, the prefix should be pdns_dnsdist_.

@Marlinc

This comment has been minimized.

Copy link
Author

commented May 28, 2019

Actually, what is the difference between the dnsdist_server_ metrics and dnsdist_? None of the current metrics start with pdns_dnsdist_.

@wojas

This comment has been minimized.

Copy link
Member

commented May 28, 2019

I just realize that this will conflict with the existing metrics...

How about something like pdns_dnsdist_response_latency_(bucket|sum|count)?

The server metrics are the ones derived from the per-server stats.

@wojas

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@Marlinc Sorry, you're right, I was looking at the output of an old dnsdist exporter instead of the new native dnsdist metrics... dnsdist_latency_bucket etc will be perfectly fine!

@Marlinc Marlinc force-pushed the Marlinc:dnsdist-prometheus-histogram branch from e984744 to 5765a71 May 28, 2019

@Marlinc

This comment has been minimized.

Copy link
Author

commented May 28, 2019

Added the dnsdist_ prefix and added HELP and TYPE definitions to dnsdist_latency_bucket

@Marlinc Marlinc force-pushed the Marlinc:dnsdist-prometheus-histogram branch from 5765a71 to 7c452fa May 28, 2019

Show resolved Hide resolved pdns/dnsdist-web.cc Outdated

@Marlinc Marlinc force-pushed the Marlinc:dnsdist-prometheus-histogram branch from 7c452fa to 7955ec3 May 28, 2019

@wojas

wojas approved these changes May 28, 2019

@pieterlexis
Copy link
Member

left a comment

code is good, but I don't see the use of the latency-sum metric directly.

Show resolved Hide resolved pdns/dnsdistdist/docs/statistics.rst Outdated
@Marlinc

This comment has been minimized.

Copy link
Author

commented May 29, 2019

I have now hopefully fixed the tests

@Marlinc Marlinc force-pushed the Marlinc:dnsdist-prometheus-histogram branch from 7955ec3 to f592012 May 29, 2019

Show resolved Hide resolved pdns/dnsdist-web.cc Outdated

@Marlinc Marlinc force-pushed the Marlinc:dnsdist-prometheus-histogram branch from f592012 to eb0335f May 31, 2019

@Marlinc

This comment has been minimized.

Copy link
Author

commented May 31, 2019

Applied the last feedback

@rgacogne rgacogne merged commit ac9e305 into PowerDNS:master Jun 3, 2019

18 checks passed

ci/circleci: build-auth Your tests passed on CircleCI!
Details
ci/circleci: build-recursor Your tests passed on CircleCI!
Details
ci/circleci: test-auth-algorithms Your tests passed on CircleCI!
Details
ci/circleci: test-auth-api Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-bind Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-gmysql Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-gpgsql Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-gsqlite3 Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-ldap Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-lmdb Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-mydns Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-odbc-mssql Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-odbc-sqlite3 Your tests passed on CircleCI!
Details
ci/circleci: test-auth-regress-tinydns Your tests passed on CircleCI!
Details
ci/circleci: test-recursor-api Your tests passed on CircleCI!
Details
ci/circleci: test-recursor-bulk Your tests passed on CircleCI!
Details
ci/circleci: test-recursor-regression Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Marlinc Marlinc deleted the Marlinc:dnsdist-prometheus-histogram branch Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.