-
Notifications
You must be signed in to change notification settings - Fork 925
Define conversion of prometheus native histograms to OpenTelemetry exponential histograms #4561
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?
Conversation
1a3a046
to
9eb6672
Compare
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.
thank you for starting this, I haven't got around to it yet
…ponential histograms
Co-authored-by: George Krajcsovits <[email protected]>
5ac176e
to
bd56bea
Compare
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.
almost there, a few tweaks
Co-authored-by: George Krajcsovits <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
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.
Approve with a small nit.
Schemas outside of range are dropped
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.
Thank you!
- `StartTimeUnixNano` is set to the `Created` timestamp, if available. | ||
- `AggregationTemporality` is set to `cumulative`. | ||
|
||
Native histograms of the float or gauge flavors MUST be dropped. Native Histograms with `Schema` outside of the range [-4, 8] MUST be dropped. |
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 noticed in the NHCB PR that the PRW 2.0 receiver accepts float histograms, but rounds down floats to the nearest integer. This language here differs from that implementation. Do we want to specify that float histograms should be rounded down to the nearest integer instead?
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.
IMO, OTel should add these concepts, and to drop them is appropriate.
- `StartTimeUnixNano` is set to the `Created` timestamp, if available. | ||
- `AggregationTemporality` is set to `cumulative`. | ||
|
||
Native histograms of the float or gauge flavors MUST be dropped. Native Histograms with `Schema` outside of the range [-4, 8] MUST be dropped. |
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.
IMO, OTel should add these concepts, and to drop them is appropriate.
Part of #4494
Changes
This conversion is the inverse of the exp -> native conversion defined here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#exponential-histograms.
CHANGELOG.md
file updated for non-trivial changes@open-telemetry/prometheus-interoperability @ywwg @krajorama