search

miekg / caddy-prometheus

Prometheus metrics middleware for caddy
Apache License 2.0
65 stars 26 forks source link

Add path label in metrics #50

Open lforite opened 5 years ago

lforite commented 5 years ago

On top of hostname, we would like to segregate our metrics by path such as:

If it's fine for you I can open a PR

miekg commented 5 years ago

I don't think that's a good idea considering label cardinality. If you need this data you prolly want to get it from your logs

On Mon, 3 Jun 2019, 16:48 Louis Forite, notifications@github.com wrote:

We would like to segregate our metrics by path such as:

  • caddy_http_request_count_total{host, family, proto, path}
  • caddy_http_request_duration_seconds{host, family, proto, path}
  • caddy_http_response_latency_seconds{host, family, proto, path, status}
  • caddy_http_response_size_bytes{host, family, proto, path, status}
  • caddy_http_response_status_count_total{host, family, proto, path, status}

If it's fine for you I can open a PR

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/miekg/caddy-prometheus/issues/50?email_source=notifications&email_token=AACWIW43JARCNWSQPECFYNDPYU4NRA5CNFSM4HSJMKL2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4GXKHPVQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AACWIW2NPVDH5HDQCOXJB7TPYU4NRANCNFSM4HSJMKLQ .

hairyhenderson commented 5 years ago

I was actually also thinking of this a few weeks ago. Our use-case is where we want to make sure that Caddy is serving files within a certain latency, but only from certain paths.

The cardinality is a concern, however the usual way we deal with this with other services is by setting the path value not to the final path, but to the endpoint that handled the request (i.e. the proxy from, or rewrite basepath, etc...).

Also it's probably important to omit the path label for non-matching requests - ones that would result in a 404.

For compatibility, this should definitely be an opt-in feature, not enabled by default.

lforite commented 5 years ago

Even for a 404 this information would be useful: you could see which resource resulted in a 404 which might be a useful piece of information.

@miekg what about making it opt-in ?

miekg commented 5 years ago

Paths are basically unbounded and as such not applicable for use as a label value.

On Tue, 4 Jun 2019, 08:00 Louis Forite, notifications@github.com wrote:

Even for a 404 this information would be useful: you could see which resource resulted in a 404 which might be a useful piece of information.

@miekg https://github.com/miekg what about making it opt-in ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/miekg/caddy-prometheus/issues/50?email_source=notifications&email_token=AACWIWYKIFLAQOXCCEPKIX3PYYHHDA5CNFSM4HSJMKL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW3TUYY#issuecomment-498547299, or mute the thread https://github.com/notifications/unsubscribe-auth/AACWIW74I7AXEWK3VJRNNMDPYYHHDANCNFSM4HSJMKLQ .

lforite commented 5 years ago

you mean size wise ?

lforite commented 5 years ago

Probably you refer to: https://github.com/prometheus/client_golang/issues/491 ?

hairyhenderson commented 5 years ago

you mean size wise ?

He's referring to cardinality, which is true. Unbounded cardinality can be a bit of an Achilles' heel for Prometheus.

The cardinality could be bounded by setting a static value per endpoint. Not sure about how to actually implement that, though 😉