Add units to prometheus metric lines #535
Conversation
This adds units to the prometheus exporter metric lines as the prometheus naming conventions specify: https://prometheus.io/docs/practices/naming/ The units without a prometheus equivalent such as kilobytes of bytes_per_second were left as is in order to not lose information given by the user.
There was a problem hiding this comment.
So at a high-level, I'm not opposed to this although it will definitely need to live behind a flag to enable it, with it disabled by default.
Separately, though, I'm curious for your thoughts around how to close the gap between what this PR does and what the ideal semantic suffixing strategy is, as defined by the "Metric and label naming" guide.
For example, one thing is base units. The guide recommends avoiding using non-base units (i.e., prefer seconds over milliseconds/nanoseconds/etc) but in this PR, we're just tacking on whatever the defined unit is. Should we actually be adjusting the value to normalize it to seconds? And do a similar thing for bytes, etc.
(I originally commented about counters and _total but glossed over the fact you've already mentioned that in the PR description.)
tobz
added
C-exporter
|
I think a flag is a good idea. But just to confirm, you do mean a runtime flag right? I think it would be a good idea in general to normalize the units, though it will require some refactoring. Let me know if you would like me to add it here. What would make this process easier is if One incongruity here is the fact that counters can only take an integer value. For instance, this makes it such that counters in seconds do not make much sense. However, for example prometheus uses f64, sentry uses f64 and otlp allows for the use of either f64 or i64. Perhaps then this should change in Another concern are the per second metrics. The advice from prometheus is to let the prometheus implementation compute the rate by itself. However, the prometheus guidance specifies the "power" unit, which is aggregated over time. So normalizing these but otherwise leaving them as-is could be an acceptable solution. |
This adds units to the prometheus exporter metric lines as the prometheus naming conventions specify:
https://prometheus.io/docs/practices/naming/
The units without a prometheus equivalent such as kilobytes or bytes_per_second were left as is in order to not lose information given by the user.
I also think
_totalshould be appended to all counters. However, that is for a future pull request.Closes #426.