Add units to prometheus metric lines

[fayalalebrun]

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 _total should be appended to all counters. However, that is for a future pull request.

Closes #426.

[tobz]

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.)

[fayalalebrun]

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 metrics stored the unit and the magnitude separately.

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 metrics itself, as the limitation to integers does not match most metrics protocols. See:
https://docs.rs/sentry-core/0.32.3/sentry_core/metrics/type.CounterValue.html
https://docs.rs/opentelemetry-proto/latest/opentelemetry_proto/tonic/metrics/v1/struct.Sum.html

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.

[tobz]

I have a note to circle back around to this to re-review it and think about what we've discussed... time has just been hard to come by. 😅

[tobz]

Coming back to this again...

I think a flag is a good idea. But just to confirm, you do mean a runtime flag right?

Yes, something that would end up being configured by PrometheusBuilder.

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 metrics stored the unit and the magnitude separately.

Definitely something for later.

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 metrics itself, as the limitation to integers does not match most metrics protocols. See: https://docs.rs/sentry-core/0.32.3/sentry_core/metrics/type.CounterValue.html https://docs.rs/opentelemetry-proto/latest/opentelemetry_proto/tonic/metrics/v1/struct.Sum.html

I do realize that not using f64 makes this more difficult. I've yet to see a good explanation or example given where counting negative values makes sense, since allowing negative and positive values to be mixed ends up giving you something a lot more like a gauge. This is probably the most opinionated part of metrics: counters are monotonic, and we enforce it by making users deal with u64.

It's very unlikely that I will ever change this, at least before metrics v2.

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.

The per-second units are a hold over.. I don't actually remember why I initially added them, but they're sort of pointless at this stage. Users are typically able to compute rates at query time, so dealing with it client-side is unnecessary, and if you actually only have access to a pre-computed rate (like pulling in metrics from a dependent library to be exporter with your app metrics), then it's probably just easier to manually suffix them as such... but that's a tangent.

I'd say we could just pretend the per-second units don't exist because it's more likely than not that I'll remove them in the future.