Add support for metrics with Prometheus#3709
Conversation
SuperQ
left a comment
There was a problem hiding this comment.
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
mholt
left a comment
There was a problem hiding this comment.
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! |
mholt
left a comment
There was a problem hiding this comment.
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
mholt
left a comment
There was a problem hiding this comment.
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! |
|
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.metricsmodule 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.metricsmodule 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_infometric (seeprometheus.NewBuildInfoCollector)metricsdirective 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:
{ "apps": { "http": { "servers": { "srv0": { "listen": [":9180"], "routes": [ { "match": [{"path":["/metrics"]}], "handle": [{ "handler": "metrics" }] }, { "match": [{ "path": ["/foo", "/bar"] }], "handle": [{"handler": "static_response", "body": "Hello world!\n"}] }, { "match": [{ "path": ["/error"] }], "handle": [{ "handler": "error" }] }, { "match": [{ "path": ["*"]}], "handle": [{"handler":"static_response", "status_code": "404"}] } ] } } } } }Doing a few requests to seed some metrics observations:
Gather the metrics from the admin API, specifying OpenMetrics format (optional):
Gathering metrics from the
metricshandler. Note thatadmin_http_requests_totalis now present (since the last call to the admin metrics endpoint):Signed-off-by: Dave Henderson dhenderson@gmail.com