Skip to content
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

Add ExponentialHistogram to Metrics data model #1935

Merged
merged 26 commits into from
Oct 7, 2021

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Sep 16, 2021

Fixes #1932.

Changes

Adds statements describing the ExponentialHistogram data point introduced by open-telemetry/opentelemetry-proto#322.

This is meant to capture details about acceptable precision limits, how to handle out-of-range values for producers and consumers. See open-telemetry/opentelemetry-proto#322 (comment)

Related OTEPs:
https://github.com/open-telemetry/oteps/blob/main/text/0149-exponential-histogram.md

@reyang reyang added area:data-model For issues related to data model spec:metrics Related to the specification/metrics directory labels Sep 17, 2021
@jmacd jmacd marked this pull request as ready for review September 20, 2021 21:01
@jmacd jmacd requested review from a team as code owners September 20, 2021 21:01
@jmacd
Copy link
Contributor Author

jmacd commented Sep 20, 2021

@open-telemetry/specs-metrics-approvers this is ready for review.

@jmacd
Copy link
Contributor Author

jmacd commented Sep 21, 2021

@yzhuge and @oertl please review

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Sep 21, 2021

FYI I have posted reference implementations that calculate bucket indexes described by the formulas here. These were captured from the review thread of open-telemetry/opentelemetry-proto#322 here, now available:
open-telemetry/oteps#179

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Show resolved Hide resolved
Copy link
Contributor Author

@jmacd jmacd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @yzhuge!

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
specification/metrics/datamodel.md Outdated Show resolved Hide resolved
@yzhuge
Copy link

yzhuge commented Sep 24, 2021

@jmacd all looks good now.

@yzhuge
Copy link

yzhuge commented Oct 5, 2021

@jmacd "the lower-boundary of index -68 cannot be represented using a double".
In negative scales, the lowest bucket may be a "partial bucket" (partially representable by "double" range).
An implementation's lower bound function will need to guard against floating point underflow. In NrSketch, I have https://github.com/newrelic-experimental/newrelic-sketch-java/blob/main/src/main/java/com/newrelic/nrsketch/indexer/ExponentIndexer.java#L43

Similarly, to be bullet proof, we need guard against overflow at the high end of "double":
https://github.com/newrelic-experimental/newrelic-sketch-java/blob/main/src/main/java/com/newrelic/nrsketch/indexer/ScaledExpIndexer.java#L114

Not sure it is worth mentioning such extreme cases in the spec though. Maybe it is, spec has to be bullet proof.

"Should the producer bother to check that an index can be reverse-mapped before using it?"
I think producer should still map it to the lowest (partial) bucket. Its other choices are: round off to zero; throw an exception. None is great.

And I assume you are talking about the exponent extraction method. The log method generally does not work well in the subnormal range. I had to skip it in subnormal tests (https://github.com/newrelic-experimental/newrelic-sketch-java/blob/main/src/test/java/com/newrelic/nrsketch/indexer/BucketIndexerTest.java#L400)

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specification/metrics/datamodel.md Outdated Show resolved Hide resolved
Co-authored-by: Aaron Abbott <aaronabbott@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:data-model For issues related to data model spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify data model details for ExponentialHistogram
9 participants