Optionally add OM unit

[vesari]

This PR adds support for unit for Open Metrics. It is ready for review, but not ready to merge (see my long comment below). Fixes #684.

[ArthurSens]

Hey @vesari , just passing by to ask if there is anything we could do to unblock you 👋

[vesari]

Hey @vesari , just passing by to ask if there is anything we could do to unblock you 👋

@ArthurSens hello! I think I'm just using a Go version which is too new, I plan on fixing that later today. Thanks a lot for checking on me, much appreciated! :)

[vesari]

@ArthurSens hello again! Here are a few considerations to give you some insight about what I’ve done here and why.
I had to upgrade Go in the GitHub workflows so that the version is at least 1.21, as this is needed by common 0.50.0 which, as you know, contains the necessary implementation for unit. I also changed the Makefile and the go.mod and main.go files in the tutorial folder accordingly. I have not touched the .bingo file but I guess the version should be upgraded there too.
However, Tests(1.20) is still expected to run among the checks, and, as you can see, just hangs, but I suppose that’s because of the repo settings, so I’ll need your help there.

The lint was failing on a supposedly unused test function unrelated to my changes (withDebugMetrics), so I removed it. Let me know if I should instead just have the lint ignore it.

I made the changes needed where an OM format can be passed to expfmt.NewEncoder, and also all the others stemming from the field Unit being added to the type prometheus.Desc. In some cases, I chose to populate this field with the unit which was already included in the metric family name, but I could have also just put an empty string, and I’m not sure what the preferred behaviour would be or if it doesn’t make any difference. I’m talking especially about the various collectors files, like prometheus/collector/dbstats_collector.go , prometheus/go_collector.go or prometheus/go_collector_latest.go etc. I’d be grateful if you could advise me on this.

Also, so far I haven’t added any new tests to promhttp/http_test.go, considering not even the EnableOpenMetrics option has been tested there. Let me know if I should.

Lastly, I happened to push a lot of commits, let me know if you want me to rebase and squash them.

As per usual, many thanks for your time and help! :)

[ArthurSens]

Oh sorry for the silence here, somehow I missed your last comment.

I had to upgrade Go in the GitHub workflows so that the version is at least 1.21, as this is needed by common 0.50.0 which, as you know, contains the necessary implementation for unit.

Yeah, that's a problem actually. We promise support for the 3 latest go versions, so we can't bump our go version to 1.21 until mid-August when go 1.23 is released. I've started conversations about how we can improve this situation with common.

The lint was failing on a supposedly unused test function unrelated to my changes (withDebugMetrics), so I removed it. Let me know if I should instead just have the lint ignore it.

This is another problem that I've been delaying for way too long 😬 From my understanding it doesn't block PRs though. But anyway, I'll try to get back to this soon, no need to remove it now :)

---

Regarding the changes that are related to OpenMetrics units, I haven't taken a deep look yet 😬. I'm mostly concerned with enabling the PR first and will review the changes once we solve all the blockers

[vesari]

Thanks a lot for the detailed update! Much appreciated

[ArthurSens]

Hi @vesari, all blockers have been resolved finally :)

The unused function is now used, and we can update prometheus/common without giving up support of go 1.20. If you rebase your PR it should be fine :)

[ArthurSens]

In #1408, we're changing a bit the structure of the HandlerOpts regarding OpenMetrics functionality. We'll probably want something similar here

[vesari]

@ArthurSens I made some changes to my code accordingly, but I think it would be better for your PR to be merged before mine, as yours has already been approved and also because in yours you explain the rationale behind the new HandlerOpts struct and what options to prefer. I can then take it from there and finish mine. What do you think? :)

[vesari]

Awesome! Thank you very much! I'm on it.

[vesari]

@ArthurSens , I've marked this as "ready for review", but I'm still assuming your PR should be merged first?

[bwplotka]

Thanks for this!

I don't think this PR is blocked on #1408, but I think I would challenge adding units in this way (or adding units at all).

First of all, we can't change NewDesc, it's breaking change.

Then the explicit unit in v2.NewDesc and all metric type constructors' options. Essentially, are we sure asking users to add unit to all their definitions (given majority of the ecosystem metrics have unit in the metric name already) is a good use of their time for the benefit (and what is the benefit actually?)

BTW @vesari or anyone, feel free to challenge me! I might miss some use cases etc! 🤗

So there are two questions to answer:

1. What's the benefit of having UNIT entry in metadata for OpenMetrics? Essentially what users would gain by using OpenMetrics with Units metadata entry? Right now, I see near zero value 🤔
   a. They would gain "formal compatibility with OpenMetrics 1.0, which means little (no scraper blocks client_golang OpenMetrics at the moment, so what's the point?) and we break compatibility for reasons (we think it would be damaging for Go Prometheus ecosystem to add e.g. _created metrics by default).
   b. Some third party systems would use it. Can we wait for actual use cases come to us? or maybe you have use case?

Wonder, what's your motivation to add units?

2. What's the additional friction for defining metrics. How we can reduce it or automate it. Howe can reduce the things they need to touch, not increase in their code?

Just looking on changing client golang metrics, we have dozens of changes like this:

Also:

Now, do we think ALL of 118,491 repositories will check and be happy with extra line to every metric definition that literally duplicate a string. That's like billion code lines in total if everyone would just add one line to every metric with some unit across all our users, JUST in open source. We have to be careful here.

One idea would be to work on some autodetection mechanism that would try to detect units in metric names IF somebody enabled UNITs AND OpenMetrics format 🤔 Ideally we could have some bigger design/proposal for it. However, first question - remains. Do we need UNIT metadata entry?

To sum up, sorry for those challenging questions. Your code is top quality, thanks for massive work on this. It's just a controversial change a bit, and we have large user base, so we have to be more careful here 🤗 DMed you on Slack too, feel bad a bit I didn't respond earlier.

[bwplotka]

Next time, this probably should be done in a separate PR 🤗

[bwplotka]

Perhaps we could remove this field and use OpenMetricsOptions *OpenMetricsOptions to ensure it's optional? We cannot remove EnableOpenMetrics anyway so we could still use that, OR when OpenMetricsOptions is allocated assume OM is enabled? 🤔

[bwplotka]

Unfortunately this is a breaking change. We can't change this function.

Not sure how to solve it. Probably NewDescWithUnit, or changing v2 only would make sense.

[bwplotka]

I realized we already kind of agreed to supporting this... #684 so probably too late to challenge it cc @beorn7 @ArthurSens - do you remember what's the benefit other than "compatibility"?

The friction comment and breaking change stays. I feel we could do better than adding Unit field and asking everybody to duplicate this information 🤔

Perhaps some automation AND clear guidance e.g. in comment to NOT add it if you have correct unit in name would help?

[vesari]

@bwplotka thanks, I'll await @beorn7 and @ArthurSens 's feedback on your questions, as they know the original motivation :)