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

Added Prometheus stats support for dnsdist #6901

Merged
merged 16 commits into from Sep 3, 2018
Merged

Added Prometheus stats support for dnsdist #6901

merged 16 commits into from Sep 3, 2018

Conversation

pavel-odintsov
Copy link

@pavel-odintsov pavel-odintsov commented Aug 30, 2018

Short description

My patch based on #6343 prepared by @giganteous

I made number of changes for it:

  • Ported code to current master branch
  • Introduced class which stores meta information about metrics (metric type, description). I do not like idea about mixing atomic counters and meta fields with big text blobs and I divided them
  • Slightly changed code formatting
  • Changed endpoint to /metrics as suggested by @dannyk81
  • Tested code and it works quite well for my case

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)

Thank you!

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR! A few nits but the code looks sane. A test in the test_API part of the regression tests would be nice but is not required for merging.
I'm not really familiar with Prometheus though, perhaps @wojas might have a quick look?

pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist.hh Outdated Show resolved Hide resolved
pdns/dnsdist.hh Show resolved Hide resolved
pdns/dnsdist.hh Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
@pavel-odintsov
Copy link
Author

I think I fixed all issues discovered during review.

Copy link
Member

@wojas wojas left a comment

Choose a reason for hiding this comment

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

Great work on this PR! I added a few comments, but other than that I think it's good to merge.

We're picky because we care and want to add the same functionality to the recursor in a consistent way. :-)

pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
pdns/dnsdist-web.cc Outdated Show resolved Hide resolved
@pavel-odintsov
Copy link
Author

I fixed all issues again. Also, I updated example endpoint output to match new code.

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

Code looks good, if @wojas is on board with the changes I'd be happy to merge this!

@pavel-odintsov
Copy link
Author

pavel-odintsov commented Aug 31, 2018

Thank you for reviewing it! :)

@rgacogne rgacogne merged commit 99a542e into PowerDNS:master Sep 3, 2018
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.

None yet

6 participants