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 prometheus stats #6343

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@giganteous
Contributor

giganteous commented Mar 14, 2018

Short description

Rebase of the original PR#3935, which is disconnected, hence a new PR.
See discussion #4947, and more recently #6002 and #6088.

I think I'm terrible at C++, but here we go.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
@rgacogne

This comment has been minimized.

Member

rgacogne commented Mar 14, 2018

Note that Travis failures are unrelated to this PR.

@rgacogne

This comment has been minimized.

Member

rgacogne commented Mar 14, 2018

Code looks good at first glance, perhaps @wojas could chime in (no rush) to look at the Prometheus part?

@zeha

This comment has been minimized.

Collaborator

zeha commented Mar 14, 2018

had a quick look -> ❤️

@@ -128,7 +128,7 @@ static bool isAnAPIRequestAllowedWithWebAuth(const YaHTTP::Request& req)
static bool isAStatsRequest(const YaHTTP::Request& req)
{
return req.url.path == "/jsonstat";
return req.url.path == "/jsonstat" || req.url.path == "/prometheus";

This comment has been minimized.

@dannyk81

dannyk81 Mar 29, 2018

Really looking forward for this to be merged!!! 👍 🙌

This is not really a big deal, but Prometheus default to /metrics for metrics path, would be nice to stick to the standard /metrics unless there's a reason no to.

# The HTTP resource path on which to fetch metrics from targets.
[ metrics_path: <path> | default = /metrics ]

https://prometheus.io/docs/prometheus/latest/configuration/configuration/

@rgacogne rgacogne added this to the dnsdist-1.4.0 milestone Apr 3, 2018

@pavel-odintsov

This comment has been minimized.

pavel-odintsov commented Aug 17, 2018

Hello!

Are you going to finish this work? That's very useful change and it can improve dnsdist deployment experience a lot.

I can help with testing.

@Habbie

This comment has been minimized.

Member

Habbie commented Sep 3, 2018

Superseded by #6901 which includes this work, thanks!

@Habbie Habbie closed this Sep 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment