search

discourse / prometheus_exporter

A framework for collecting and aggregating prometheus metrics
MIT License
539 stars 156 forks source link

Do not include status label everywhere #216

Closed MaximeD closed 2 years ago

MaximeD commented 2 years ago

The status label is included in every metrics, and not only in http_requests_total, even though the README states that

All metrics have a `controller` and an `action` label.
`http_requests_total` additionally has a (HTTP response) `status` label.

This can be a problem as this status label can quickly increase the cardinality of the metrics.

This change of behavior was introduced in #148. But I believe this is not the best pattern as it is not useful to have it everywhere, and it can quickly blow up the cardinality. See those slides about Containing your Cardinality, especially page 14.

SamSaffron commented 2 years ago

Is there any way to amend our internals so we only explicitly add the label where we need it vs removing it later on?

MaximeD commented 2 years ago

Sure thing! I reverted part of what was done in #148, that is the status label is now again at the root of obj instead of being nested in default_labels.

SamSaffron commented 2 years ago

cool, looks great!

MaximeD commented 2 years ago

Awesome! By the way, do you plan to release a new version of this gem in the coming days?

Thank you again!

SamSaffron commented 2 years ago

I have been a bit afraid to do a new release cause we had a few breaking changes ... that said .. got to rip off the cord at some point!

MaximeD commented 2 years ago

Yes, only one way to battle test the new changes I guess!

salzig commented 2 years ago

Another changes that makes harder to upgrade our dashboards. Thanks.

We used to have a dashboard that shows duration per status code, as sometimes "sideeffects" (3rd Parties) can influence our app which is often seen very well isolated for a status code we're returning.