Prometheus: Add setting for recommended metric names

[michelhe]

This commit adds a setting to PrometheusBuilder to add the '_total' suffix to counter names, compatible with Prometheus naming best practices.

This is useful for migrating from the [prometheus-client] crate, (where the counter suffix is enforced) without breaking existing dashboards.

prometheus-client

[tobz]

👋🏻

I'd rather see this updated here:

metrics/metrics-exporter-prometheus/src/formatting.rs

Line 79 in 5bf2aee

Some(Unit::Count) | None => {}

Having two distinct settings for controlling metric name suffixes is too much. I think we would just add this for the Unit::Count case, though.

[michelhe]

👋🏻

I'd rather see this updated here:

metrics/metrics-exporter-prometheus/src/formatting.rs

Line 79 in 5bf2aee

Some(Unit::Count) | None => {}

Having two distinct settings for controlling metric name suffixes is too much. I think we would just add this for the Unit::Count case, though.

Only Unit::Count isn't enough to allow migration from prometheus-client . That crate formats unit-less counters with _total suffix and counters with unit information with the _{unit}_total suffix, following the Prometheus naming best practices document to the word.

See these lines from the above link:

http_requests_total (for a unit-less accumulating count)
process_cpu_seconds_total (for an accumulating count with unit)

If you don't want yet another setting, maybe we can have a set_prometheus_client_compat_mode that implictly enables the unit suffix as well?

[tobz]

Blah, sometimes I really dislike how prescriptive Prometheus is... 😅

Alright, yeah: I think what I ultimately want to work towards is being able to get rid of these opt-in ways of adding the unit suffixes, adding the _total suffix, etc... so I like your idea of an all-in-one method.

Some thoughts:

• I'd call it with_recommend_naming, but keep the existing signature as you have it
• have with_recommended_naming also enable unit suffixes, as you mentioned
• add #[deprecated(since = "0.18.0", note = "users should prefer `with_recommended_naming` which automatically enables unit suffixes")] to set_enable_unit_suffix

Then in 0.19.0, I can remove set_enable_unit_suffix and mark with_recommended_naming as deprecated, and in 0.20.0, remove with_recommended_naming and just make it the default behavior. 😌

[michelhe]

Yep, Sounds good to me overall — I like the direction.

My only concern is around deprecating or changing the default behavior too aggressively — there might be users who currently only enable unit suffixes, and if their dashboards are tied to that, it could be hard or even impossible for them to update (e.g. if the app is deployed to customers).

If breaking changes are introduced - might be worth keeping a way to retain the current behavior, even if it's opt-in.

[michelhe]

I pushed the changes you suggested, and also had to fix some stuff in formatting.rs.

Mainly, the fix I made in 80960de could be breaking existing metric names for users who use unit-suffixes :/

[tobz]

I need to refresh my memory on this but the existing support for adding a suffix in write_metric_line was just to add stuff for aggregated histograms/summaries (e.g., my_metric_name_sum, etc) so ... I think it may actually be broken as-is if people are using unit suffixes. 🤔

(The idea being that your change is now fixing it.)

[michelhe]

Well the current state is not idiomatic Prometheus but their dashboards are functioning.
With my fixes they will gonna need to make new dashboards

[tobz]

After thinking about it, I'm OK with the breakage because 1) we're still v0 and breaking changes in minor releases is acceptable when documented, and 2) it aligns closer to the Prometheus best practices.

I'll make sure the changelog calls this out specifically.