Update OpenTelemetry Exponential Histogram to Prometheus Native Histogram translation#5125
Update OpenTelemetry Exponential Histogram to Prometheus Native Histogram translation#5125krajorama wants to merge 10 commits into
Conversation
…stogram translation Mark the OTLP Exponential Histogram to Prometheus Native Histogram (standard exponential schema) conversion section as Stable. Restructure the bullets to mirror the Histograms section established in open-telemetry#5091, splitting the NoRecordedValue handling into explicit true/false branches. Fix the NoRecordedValue handling: the Native Histogram Count is a uint64 and MUST be set to zero (not the Stale NaN value) on staleness, with ZeroCount/PositiveSpans/PositiveDeltas/NegativeSpans/NegativeDeltas left empty. Set the Native Histogram ResetHint to UNKNOWN since OpenTelemetry does not carry an explicit reset flag. Add StartTimeUnixNano handling consistent with the Histograms section, and note that fields not explicitly referenced are left at their zero value. Relax the delta-temporality rule from "MUST be dropped" to "MAY be aggregated to cumulative or MUST be dropped", matching the (now Stable) Histograms section. Fixes open-telemetry#4922 Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
a0d967f to
e9346fe
Compare
Mirror the detailed explanation added for the OpenTelemetry Histogram to Prometheus NHCB conversion in open-telemetry#5091: split the sparse-layout conversion into separate bullets covering which counts are converted, delta encoding, span Offset/Length semantics, and a worked example. Provide an explicit formula (`Offset + i + 1`) for mapping an Exponential Histogram bucket array position to a Native Histogram bucket index, since the indexing convention differs by one between the two formats. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
e9346fe to
2923849
Compare
|
The markdown-link-check fails on |
There was a problem hiding this comment.
Pull request overview
This PR stabilizes the specification for translating OTLP Exponential Histogram datapoints into Prometheus Native Histograms (standard exponential schema) in the Prometheus/OpenMetrics compatibility document, aligning structure and semantics with the (recently stabilized) histogram translation guidance.
Changes:
- Mark OTLP Exponential Histogram → Prometheus Native Histogram translation as Stable and reorganize conversion bullets to a more explicit, symmetric structure.
- Clarify
NoRecordedValuehandling (explicit true/false branches), reset-hint behavior (UNKNOWN), and addStartTimeUnixNano→ PrometheusStartTimemapping. - Expand the dense-to-sparse bucket conversion description with span/delta semantics and a worked example; update the changelog accordingly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| specification/compatibility/prometheus_and_openmetrics.md | Marks Exponential Histogram translation as Stable and refines/expands the normative conversion rules to Prometheus Native Histograms. |
| CHANGELOG.md | Adds an Unreleased changelog entry noting stabilization of the Exponential Histogram → Native Histogram translation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - The `ResetHint` (or `CounterResetHint`) in the Native Histogram MUST be set | ||
| to `UNKNOWN`. OpenTelemetry does not carry an explicit reset flag, so | ||
| `UNKNOWN` lets Prometheus auto-detect resets from the values and avoids any | ||
| semantic divergence with OpenTelemetry's reset model. |
There was a problem hiding this comment.
Same comment as #5091 (comment).
I believe OTel has carries explicit reset information, so we can probably do better here.
There was a problem hiding this comment.
Unresolving it because this does not appear to be addressed.
There was a problem hiding this comment.
Looking at the PR getting merged, looks like we're moving forward with MUST. Could you elaborate on what is not addressed?
There was a problem hiding this comment.
OpenTelemetry does not carry an explicit reset flag
@jmacd argued here that OTel carries implicit reset information: #5091 (comment)
I didn't see an obvious resolution to that conversation
There was a problem hiding this comment.
I thought we had strong enough arguments against using that implicit reset information. @jmacd wdyt?
There was a problem hiding this comment.
I think we should declare a bug in the specification, detailed by @krajorama here: #5091 (comment)
That is to say, I've read the list of reset-flags in the document https://prometheus.io/docs/specs/native_histograms/#counter-reset-considerations and I think we can write well-defined rules for generating them all; what we write in the data model document is not the same as what we write in an SDK specification. The SDK knows more in general than a Collector does in this situation, so I would say the SDK has better options than to set UNKNOWN, at least.
There was a problem hiding this comment.
Setting UNKNOWN is never a bug from Prometheus point of view. If you think we're going to miss some resets, then that could be considered a bug from OTEL point of view.
Prometheus will honor if you set RESET hint (or GAUGE hint - it's a different discussion for delta temporality). There is a danger that if you abuse the RESET hint, it will generate a lot of overhead in Prometheus without benefit - because storage units (chunks) are cut on every RESET hint.
Prometheus will not honor NOTRESET hint, because it depends on it being 100% accurate, so it will validate the input anyway.
what we write in the data model document is not the same as what we write in an SDK specification
I'm not sure I follow, shouldn't the SDK spec follow the data model?
There was a problem hiding this comment.
If we're going to deal with this in a separate issue, can we merge this PR? Note: this PR does not stabilize this part of the spec anymore, that was removed from the PR and deferred. @jmacd
There was a problem hiding this comment.
And finally: it would be nice to hash these out in our upcoming OTEL Prometheus WG meeting on the 17th of July. See the OTEL calendar.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
I think this is ready to merge. Markdown fail due to github.com gateway timeout. |
cijothomas
left a comment
There was a problem hiding this comment.
Thanks for driving this! One process question: this PR combines the status flip with actual wording changes. Could you split into (1) the wording changes while still Experimental, then (2) a clean flip-to-Stable PR that only changes the status?
Not objecting to the stabilization or the contents - just trying to keep stabilization PRs scoped to "flip the status, nothing more" so reviewers don't have to evaluate both at once.
…gram-translation Also undo stabilization part. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
I've updated the PR to not do the stabilization , just update the text. |
Concern addressed by removing stabilization status change from this PR
Related to #4922
Changes
Update the OTLP Exponential Histogram to Prometheus Native Histogram (standard exponential schema) conversion section and restructure the bullets to mirror the Histograms section established in #5091.
Notable substantive changes alongside the status flip:
NoRecordedValuehandling into explicittrue/falsebranches, matching the NHCB structure in Stabilize Histogram to Prometheus translation and specify NHCB conversion #5091.NoRecordedValue == truehandling: the Native HistogramCountis auint64, so it MUST be set to0(not the Stale NaN value).ZeroCount,PositiveSpans,PositiveDeltas,NegativeSpans, andNegativeDeltasMUST be left empty. This is the follow-up flagged in Stabilize Histogram to Prometheus translation and specify NHCB conversion #5091 (comment) .ResetHint(orCounterResetHint) toUNKNOWN. OpenTelemetry does not carry an explicit reset flag, soUNKNOWNlets Prometheus auto-detect resets from the values and avoids any semantic divergence with OpenTelemetry's reset model.StartTimeUnixNano→ PrometheusStartTimemapping, consistent with the (now Stable) Histograms section. The previous text saidStartTimeUnixNanowas "not used", which contradicted the symmetric reverse-direction conversion.Offset/Lengthsemantics (absolute for the first span, relative for subsequent spans), and a worked example. An explicit formula maps an Exponential Histogram bucket array position to a Native Histogram bucket index (Offset + i + 1), since the indexing convention differs by one between the two formats.The detailed bucket-conversion explanation is open for discussion — see the inline review comment showing the shorter alternative, and #5091 for the equivalent NHCB wording.
CHANGELOG.mdfile updated for non-trivial changes[chore]in the PR title to skip the changelog checkCoded with Claude Opus 4.7.