-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Optionally add OM unit #1392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Optionally add OM unit #1392
Conversation
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
|
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! :) |
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Either this or anything the team deem useful. Whatever you think needs attention, I’ll be happy to work on :) Feel free to DM me on Slack if necessary. Thank you! |
|
As agreed with @bwplotka, we want to add units eventually, but this needs more thought on how to avoid adding significant boilerplate to all users for that; help wanted, closing for now. |
|
We might want to ressurect this PR now that prometheus/proposals#39 adds a purpose to units in Prometheus :) |
|
I'll be happy to do so |
|
@kakkoyun ignore the request for review for now, as it was sent automatically once I reopened the PR. There's still a lot to catch up on here and work to do. |
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
bwplotka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing. Solid work, thanks for finding way to not break compatibility.
Added a few suggestions and ideas, hope those help! Overall this goes in a good direction 👍🏽
| enc = expfmt.NewEncoder(w, contentType) | ||
| encOpts = append(encOpts, expfmt.WithCreatedLines()) | ||
| } | ||
| if opts.EnableOpenMetricsUnit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this manual flag?
Why can't we simply add UNIT line if someone enabled OM? Shouldn't this be safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_totalwith 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing. Opened the corresponding PR on common today.
| encOpts = append(encOpts, expfmt.WithCreatedLines()) | ||
| } | ||
| if opts.EnableOpenMetricsUnit { | ||
| encOpts = append(encOpts, expfmt.WithUnit()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| EnableOpenMetricsTextCreatedSamples bool | ||
| // EnableOpenMetricsUnit enables unit metadata in the OpenMetrics output format. | ||
| // This is only applicable when OpenMetrics format is negotiated. | ||
| EnableOpenMetricsUnit bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, I wonder if we could skip this (reduce complexity and effort for everyone, users already need to manually add Unit field).
prometheus/promhttp/http_test.go
Outdated
| } | ||
|
|
||
| wantMsg := `error gathering metrics: error collecting metric Desc{fqName: "invalid_metric", help: "not helpful", constLabels: {}, variableLabels: {}}: collect error | ||
| wantMsg := `error gathering metrics: error collecting metric Desc{fqName: "invalid_metric", help: "not helpful", unit: "", constLabels: {}, variableLabels: {}}: collect error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
prometheus/desc.go
Outdated
| // For constLabels, the label values are constant. Therefore, they are fully | ||
| // specified in the Desc. See the Collector example for a usage pattern. | ||
| func (v2) NewDesc(fqName, help string, variableLabels ConstrainableLabels, constLabels Labels) *Desc { | ||
| func (v2) NewDesc(fqName, help string, variableLabels ConstrainableLabels, constLabels Labels, unit ...string) *Desc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
NewDescwe do (B) - For V2 we do (A) ?
prometheus/desc.go
Outdated
| // label names. | ||
| xxh.Reset() | ||
| xxh.WriteString(help) | ||
| xxh.WriteString(optionalUnitValue(unit...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's eval unit as soon as we have it from descV2Opts or something
prometheus/desc.go
Outdated
| } | ||
| return fmt.Sprintf( | ||
| "Desc{fqName: %q, help: %q, constLabels: {%s}, variableLabels: {%s}}", | ||
| "Desc{fqName: %q, help: %q, unit: %q, constLabels: {%s}, variableLabels: {%s}}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, optionally print it?
| sampleBuf = append(sampleBuf, metrics.Sample{Name: d.Name}) | ||
| sampleMap[d.Name] = &sampleBuf[len(sampleBuf)-1] | ||
|
|
||
| // Extract unit from the runtime/metrics name (e.g., "/gc/heap/allocs:bytes" -> "bytes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's nice!
Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Arianna Vespri <36129782+vesari@users.noreply.github.com>
Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com> Signed-off-by: Arianna Vespri <36129782+vesari@users.noreply.github.com>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Thank you very much, I'm working on addressing your requests for changes and your observations. I'll get back to you once I'm done :) |
…t is given Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
…c argument in V2 NewDesc Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>
|
This is now ready for review :) |
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.