Do we need this manual flag?

Why can't we simply add UNIT line if someone enabled OM? Shouldn't this be safe?

We added flag for EnableOpenMetricsTextCreatedSamples because we believe CT line is potentially breaking to the ecosystem (lot's of problems). But with UNIT metadata, I'd expect OM compatible parsers to either ignore it or parse. Even TEXT formats should work, because it's just a # .. comment.

The only potential risk is the slight increase of payload size, but this is actually limited by the user who need to manually add Unit: <....> fields to their metric definitions, no?

I see your point. However, as things stand, the flag is necessary because prometheus/common's expfmt encoder requires explicit opt-in via WithUnit() to output units. Without it, units are silently ignored even if set in the MetricFamily. I think this was to avoid potential changes to metric names to happen automatically. In fact, because of this logic

if we:

• Opt in with WithUnit()
• Have a metric foo_total with Unit: "seconds"
• The metric name doesn't already end with _seconds

The metric name becomes foo_seconds_total. I believe the consensus back then was this should be the result of a very explicit decision by the user.
Of course, if - for example - we decide we’re allowed not to enforce the unit suffix anymore or that we deem acceptable to silently add unit suffixes, then the flag becomes superfluous, and can be removed as soon as the related code in common gets modified accordingly. I’ll be happy to do that myself, if consensus is ever reached in that sense.

I think we agree it's a bit toxic to change name depending on the unit, even though it's what OM 1.0 kind of requires (but there tons of other way to handle this e.g. return error on registration e.g. using pedantic registry).

To proceed I think we need to simplify this for users and make sure there's a mode in expfmt that do not change metric names, perhaps even to _total. Then we also need to reduce risk this mode will be not alter.

The problem with adding a feature flag "for now", it's because it's for forever (v1) and it's also adding a flag with something that we don't recommend and can have consequences. So let's try to alter common then?

Sure thing. Opened the corresponding PR on common today.

I wonder, does this print UNIT line if UNIT is unknown (empty string?)

Can we ensure this does NOT add UNIT if it's unknown? Just to safe space and limit the risks mentioned in https://github.com/prometheus/client_golang/pull/1392/files#r2580622660

again, I wonder if we could skip this (reduce complexity and effort for everyone, users already need to manually add Unit field).

Can we ensure everywhere we don't print empty string? (and just skip it?). It does not give a lot of info in an empty string form

Here, I wonder if we don't want to break compatibility. 🤔 It's V2 and I wished we marked it as experimental somewhere visibility (it was meant to experiment on a new API). However, I bet downstream ecosystem already started using it.

I'd suggest, (see A, B options in https://github.com/prometheus/client_golang/pull/1392/files#r2580668173)

• For NewDesc we do (B)
• For V2 we do (A) ?

let's eval unit as soon as we have it from descV2Opts or something

ditto, optionally print it?

That's nice!

Probably needs commentary (public structure)

I'd inline this

I'd add this inline and show unit is empty? We expanded the data structure so we can expand the error too

Info likely does not have unit?

As found in https://gist.github.com/bwplotka/a38c33a74fad88a46c4623e924fa58c8#file-prometheusclient_golang1392-md, I think indeed we should handle the case of no unit here gracefully (-1 index response):

> Unsafe Substring Slice during Runtime Metric Unit Extraction

File with exact line number: prometheus/go_collector_latest.go:L213
Link to a file line on GitHub PR: https://github.com/prometheus/client_golang/pull/1392/files#diff-8360f0c0ae2f0d98fb9ecdf37119d6d333dc17ec23f0547055c1ddfdf4ed0759R213
Problem: The unit is extracted using d.Name[strings.IndexRune(d.Name, ':')+1:]. If a runtime metric is introduced without a :, strings.IndexRune evaluates to -1 resulting in -1+1 = 0. The unit silently turns into the entire metric name, bypassing obvious fault detection. (Note: this extraction assumption existed before the PR for histograms, but was generalized here).
Suggestion: Consider checking if strings.ContainsRune(d.Name, ':') before slicing to future-proof the collector against misconfigured runtime metrics.