-
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
Changes from all commits
d74a7fb
5d8c6de
f78b666
e876012
b1f5a86
788d53b
1692495
5971215
efb64f3
fafe45b
51861d5
1362436
a2bfd3a
7900714
19d7102
031e0d3
fd60a57
f7e2848
5f58996
ac9d1d8
3db440e
7752d37
47c8cf1
7312bd5
3644321
dca67a5
ab34546
717d3d3
5b5c4be
a635a5a
e5f8851
9eac68e
3441cb0
52eb82d
99ae579
1d93cb8
00d2201
5a07918
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,8 @@ type Desc struct { | |
| fqName string | ||
| // help provides some helpful information about this metric. | ||
| help string | ||
| // unit provides the unit of this metric. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| unit string | ||
| // constLabelPairs contains precalculated DTO label pairs based on | ||
| // the constant labels. | ||
| constLabelPairs []*dto.LabelPair | ||
|
|
@@ -66,6 +68,13 @@ type Desc struct { | |
| err error | ||
| } | ||
|
|
||
| func optionalUnitValue(unit ...string) string { | ||
| if len(unit) > 0 { | ||
| return unit[0] | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| // NewDesc allocates and initializes a new Desc. Errors are recorded in the Desc | ||
| // and will be reported on registration time. variableLabels and constLabels can | ||
| // be nil if no such labels should be set. fqName must not be empty. | ||
|
|
@@ -75,8 +84,8 @@ type Desc struct { | |
| // | ||
| // For constLabels, the label values are constant. Therefore, they are fully | ||
| // specified in the Desc. See the Collector example for a usage pattern. | ||
| func NewDesc(fqName, help string, variableLabels []string, constLabels Labels) *Desc { | ||
| return V2.NewDesc(fqName, help, UnconstrainedLabels(variableLabels), constLabels) | ||
| func NewDesc(fqName, help string, variableLabels []string, constLabels Labels, unit ...string) *Desc { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! I was about to comment we can't break compatibility, but you found a way. I am not a fan as it's a very one-time hack (: Some alternatives: A. Do |
||
| return V2.NewDesc(fqName, help, UnconstrainedLabels(variableLabels), constLabels, optionalUnitValue(unit...)) | ||
| } | ||
|
|
||
| // NewDesc allocates and initializes a new Desc. Errors are recorded in the Desc | ||
|
|
@@ -89,11 +98,12 @@ func NewDesc(fqName, help string, variableLabels []string, constLabels Labels) * | |
| // | ||
| // 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 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
|
||
| d := &Desc{ | ||
| fqName: fqName, | ||
| help: help, | ||
| variableLabels: variableLabels.compile(), | ||
| unit: optionalUnitValue(unit...), | ||
| } | ||
| //nolint:staticcheck // TODO: Don't use deprecated model.NameValidationScheme. | ||
| if !model.NameValidationScheme.IsValidMetricName(fqName) { | ||
|
|
@@ -150,10 +160,11 @@ func (v2) NewDesc(fqName, help string, variableLabels ConstrainableLabels, const | |
| d.id = xxh.Sum64() | ||
| // Sort labelNames so that order doesn't matter for the hash. | ||
| sort.Strings(labelNames) | ||
| // Now hash together (in this order) the help string and the sorted | ||
| // Now hash together (in this order) the help string, the unit string and the sorted | ||
| // label names. | ||
| xxh.Reset() | ||
| xxh.WriteString(help) | ||
| xxh.WriteString(optionalUnitValue(unit...)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's eval unit as soon as we have it from |
||
| xxh.Write(separatorByteSlice) | ||
| for _, labelName := range labelNames { | ||
| xxh.WriteString(labelName) | ||
|
|
@@ -211,9 +222,10 @@ func (d *Desc) String() string { | |
| } | ||
| } | ||
| return fmt.Sprintf( | ||
| "Desc{fqName: %q, help: %q, constLabels: {%s}, variableLabels: {%s}}", | ||
| "Desc{fqName: %q, help: %q, unit: %q, constLabels: {%s}, variableLabels: {%s}}", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto, optionally print it? |
||
| d.fqName, | ||
| d.help, | ||
| d.unit, | ||
| strings.Join(lpStrings, ","), | ||
| strings.Join(vlStrings, ","), | ||
| ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,7 +98,7 @@ type goCollector struct { | |
| // snapshot is always produced by Collect. | ||
| mu sync.Mutex | ||
|
|
||
| // Contains all samples that has to retrieved from runtime/metrics (not all of them will be exposed). | ||
| // Contains all samples that have to be retrieved from runtime/metrics (not all of them will be exposed). | ||
| sampleBuf []metrics.Sample | ||
| // sampleMap allows lookup for MemStats metrics and runtime/metrics histograms for exact sums. | ||
| sampleMap map[string]*metrics.Sample | ||
|
|
@@ -210,16 +210,19 @@ func NewGoCollector(opts ...func(o *internal.GoCollectorOptions)) Collector { | |
| 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") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's nice! |
||
| unit := d.Name[strings.IndexRune(d.Name, ':')+1:] | ||
|
|
||
| var m collectorMetric | ||
| if d.Kind == metrics.KindFloat64Histogram { | ||
| _, hasSum := opt.RuntimeMetricSumForHist[d.Name] | ||
| unit := d.Name[strings.IndexRune(d.Name, ':')+1:] | ||
| m = newBatchHistogram( | ||
| NewDesc( | ||
| BuildFQName(namespace, subsystem, name), | ||
| help, | ||
| nil, | ||
| nil, | ||
| unit, | ||
| ), | ||
| internal.RuntimeMetricsBucketsForUnit(bucketsMap[d.Name], unit), | ||
| hasSum, | ||
|
|
@@ -230,6 +233,7 @@ func NewGoCollector(opts ...func(o *internal.GoCollectorOptions)) Collector { | |
| Subsystem: subsystem, | ||
| Name: name, | ||
| Help: help, | ||
| Unit: unit, | ||
| }, | ||
| ) | ||
| } else { | ||
|
|
@@ -238,6 +242,7 @@ func NewGoCollector(opts ...func(o *internal.GoCollectorOptions)) Collector { | |
| Subsystem: subsystem, | ||
| Name: name, | ||
| Help: help, | ||
| Unit: unit, | ||
| }) | ||
| } | ||
| metricSet = append(metricSet, m) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,12 +226,17 @@ func HandlerForTransactional(reg prometheus.TransactionalGatherer, opts HandlerO | |
| rsp.Header().Set(contentEncodingHeader, encodingHeader) | ||
| } | ||
|
|
||
| var enc expfmt.Encoder | ||
| var ( | ||
| enc expfmt.Encoder | ||
| encOpts []expfmt.EncoderOption | ||
| ) | ||
| if opts.EnableOpenMetricsTextCreatedSamples { | ||
| enc = expfmt.NewEncoder(w, contentType, expfmt.WithCreatedLines()) | ||
| } else { | ||
| enc = expfmt.NewEncoder(w, contentType) | ||
| encOpts = append(encOpts, expfmt.WithCreatedLines()) | ||
| } | ||
| if opts.EnableOpenMetricsUnit { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We added flag for The only potential risk is the slight increase of payload size, but this is actually limited by the user who need to manually add |
||
| encOpts = append(encOpts, expfmt.WithUnit()) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
| enc = expfmt.NewEncoder(w, contentType, encOpts...) | ||
|
|
||
| // handleError handles the error according to opts.ErrorHandling | ||
| // and returns true if we have to abort after the handling. | ||
|
|
@@ -461,6 +466,9 @@ type HandlerOpts struct { | |
| // Prometheus introduced the feature flag 'created-timestamp-zero-ingestion' | ||
| // in version 2.50.0 to handle this situation. | ||
| EnableOpenMetricsTextCreatedSamples bool | ||
| // EnableOpenMetricsUnit enables unit metadata in the OpenMetrics output format. | ||
| // This is only applicable when OpenMetrics format is negotiated. | ||
| EnableOpenMetricsUnit bool | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| // ProcessStartTime allows setting process start timevalue that will be exposed | ||
| // with "Process-Start-Time-Unix" response header along with the metrics | ||
| // payload. This allow callers to have efficient transformations to cumulative | ||
|
|
||
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 here is ok, or even after name.