Consider (optionally?) enforcing certain metric naming conventions

[bwplotka]

Hi,

I think it makes sense to have this verification on client-side in official Prometheus library - some rules are must-have like _total. I guess the problem is how to error... probably panic is the only option?

Alternatively, we can implement it on our (e.g Thanos side). Plus we can be even more opinionated. Some wrapper that will panic if:

• NewCounter is invoked without _total suffix
• NewTimeHistogram invoked without _seconds and NewSizeHistogram invoked without _bytes (we only have two types).

Alternatively (3rd) NewCounter could always add _total if not specified, but that's surprising. Sometimes we discover metrics from code.

Use case: 10000 bugs where we forgot to ensure _total of counter suffix in PR review time...

[brian-brazil]

It is perfectly valid to have a counter without _total in the Prometheus exposition format, though not encouraged.

[beorn7]

There is already some validation of metric names. And there is an established way of reporting errors. (They are reported during registration time of a metric, not during construction time. This might not be the most intuitive way, but error handling has organically grown here. It can be rethought for v2, but right now, we cannot really change it in a breaking way.)

Having said that, because the _total suffix is just a convention, not a requirement, introducing the requirement now would be a breaking change.

Things are a bit different on OpenMetrics. For that, there is #683 (which I still have to complete with my thoughts how to implement this without breaking a lot of current users, but input of others is welcome).

For v2, we can go wild, of course, and we could elect to be more restrictive for naming. For the current world order, I think the approach with promlint is the right way to test for legal but discouraged metric names.

I'd suggest to move this issue to the v2 milestone as it cannot really be implemented within v1. And I'd also word it as "Consider to…" as it still needs to be discussed if we want to be more restrictive in naming than what the exposition format allows. (Even OpenMetrics doesn't enforce seconds in the metric name of a histogram of durations.)

Note that there is also #426 which deals with formal validations that are currently not even possible during registration time (but will happen during scrape time).

[beorn7]

I adjusted the issue according to my suggestions above.

I guess if something is just a convention, it would be bad if an instrumentation library enforce it. But perhaps we can have an option somewhere in v2 where a user can ask the library to check/enforce certain conventions.

We don't have to decide that now. v2 needs more fundamental design decisions first. (I wanted to turn my Gophercon EU talk into a straw poll about opinions in the Go community, but oh well, the conference got postponed…)

[bwplotka]

Somehow related: thanos-io/thanos#2430

[rremer]

I realize this is an older thread, but I stumbled upon it while looking for a way to safely register metrics in a dynamic way which may or may not be compliant with the advertised allowable characters. Essentially what I was looking for is something in a prometheus package like:

// GetSafeMetricName takes an application prefix "myapp_" and prepends it to a valid prometheus metric name for the given unsanitized string
GetSafeMetricName(appPrefix string, unsanitizedMetricName string) string {}

// GetSafeMetricLabels strings represent potential metric label keys and returns a Labels map
GetSafeMetricLabels(labels ...string) Labels {}

This way when I'm instrumenting an app I can pass in more dynamic labels and such and know they'll be valid. The closest thing I could find was in "github.com/prometheus/common/model" which has LabelNameRE containing the compiled regex of the allowable characters. Promlint seems to have a similar thing, for validation of labels after they've been registered.