-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Add support for metrics with Prometheus #3709
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the metric naming is pretty good. A couple of comments.
One thing I would like to promote is that we keep the configuration as simple as possible to start with. There's no need to have too many knobs to control metric output without having some good justification for those knobs. The more we make metrics output consistent, the easier it is to support. Both in terms of code, but also in terms of dashboards, alerts, and comparing one Caddy install to another. |
Thanks for looking at this, @SuperQ!
Yup, I agree. I'll defer any to a future PR. |
4164cc0
to
98f39f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice so far! I'm looking forward to getting this in, as are many others I'm sure.
Just did a high level first pass.
As I don't have experience with Prometheus, would it be possible to get at least one other experienced Prometheus user to do a review before we merge it? If you can find somebody and point them this way, that would be ideal. I can tweet about it too if you'd like.
Prometheus person here, already looking at this. 😄 Maybe also @beorn7 or @roidelapluie would be able to comment. |
I'm happy to have a look. Just super busy these days. Hopefully tomorrow… |
04fab35
to
4748481
Compare
Thanks for the review @SuperQ @mholt @francislavoie - I've addressed most of the feedback, just a few outstanding comments where I'm waiting for feedback 🙂. PTAAL! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, we're almost there! I just have a few remaining suggestions and questions.
Will still try to give this a review tomorrow… |
f3c870f
to
6b478d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I will await a final pass (or however many are necessary) or OK from the other reviewer(s) before merging this.
My last two suggestions here are just totally unimportant nits, doesn't matter to me either way.
Signed-off-by: Dave Henderson <dhenderson@gmail.com>
8dd5dc2
to
3178fb1
Compare
Thanks for all the great discussions. I have reached the end of the day, and again didn't find time to look at this in detail. I guess you should just merge now, and I'll earmark this for looking at this later and will let you know if I have any follow-ups to suggest. |
And BTW, super happy to see Prometheus metrics in Caddy. I kind of thought that would be a great thing to have ever since I noticed Caddy. |
This is good but I think we should initialize some of the counters with the most common http codes. |
Thanks @roidelapluie - that's a good point. I'll keep this for a follow-up PR I think. There are a number of dynamic labels and I want to be smart about how I initialize all the various combinations... |
I'll merge this in a couple hours unless there are any objections. Thanks all, for your reviews! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks all for the reviews and guidance! I really appreciated it. Very excited to have this in finally 😂 🎉 |
(edited for brevity - see previous edits if you're interested in the history!)
Fixes #3390
At long last I've found time to work on this. Sorry about the delay - it's been a bit of a rough summer!
This adds PR does a number of things:
admin.api.metrics
module for scraping metrics on the admin API endpoint at/metrics
. This is always mounted as long as the admin API is enabled, giving Caddy automatic support for Prometheus metrics.http.handlers.metrics
module for configuring which port/path metrics should be scraped on. This will be mounted as-needed, and can be used when the admin API is disabled. This module may get more configurable in future PRs.go_build_info
metric (seeprometheus.NewBuildInfoCollector
)metrics
directive to CaddyfileIt's worth noting also that this fully enables any module author to register and observe their own custom Prometheus metrics. I can add a brief tutorial in the docs at some point to that end (in short: just register them with
prometheus.DefaultRegisterer
).Many thanks to @mholt who helped me understand heaps more about the Caddy internals and make decisions on how to put the modules together!
Expand to see a sample (warning: verbose!)
Config file:
Doing a few requests to seed some metrics observations:
Gather the metrics from the admin API, specifying OpenMetrics format (optional):
Gathering metrics from the
metrics
handler. Note thatadmin_http_requests_total
is now present (since the last call to the admin metrics endpoint):Signed-off-by: Dave Henderson dhenderson@gmail.com