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

Remove Aggregation.histogram(), rename explicitBucketHistogram -> histogram() #4199

Closed
anuraaga opened this issue Feb 22, 2022 · 2 comments
Closed
Labels
Feature Request Suggest an idea for this project

Comments

@anuraaga
Copy link
Contributor

First is I think we can remove this method that returns an abstract "best histogram"

https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/Aggregation.java#L70

It's a bit hard to reason about, and if the method opened up changing the implementation to a different one like exponential histogram in the future, it could make sense, but AFAIK the behavior would change so drastically (including the export SDK data type) that such a change couldn't actually be made. So we can probably just stick to explicit factories.

Then, we could either have explicitBucketHistogram() and exponentialHistogram(), or histogram() and exponentialHistogram(). I have a slight preference for the latter since I think most people will grok that a "standard histogram" is a fixed buckets one. If we had a full understanding of exponential histogram and many backends supported it, then histogram() and explicitBucketHistogram() could also be OK, but I think that's a bit too much for a V1, we should probably stick closer to current user expectations.

@anuraaga anuraaga added the Feature Request Suggest an idea for this project label Feb 22, 2022
@jack-berg
Copy link
Member

The best histogram language comes from the sdk spec. I agree that its hard to reason about, but also think that exponential histograms will likely be a much better default than explicit bucket in the long run because of their ability to represent distributions well at a wide range of scales.

Here's some discussion around the the original introduction of the "best histogram" language. It seems ideal if the SDK could reserve the right to switch to exponential histograms when they become available, but share your concern that that the change would be so drastic that it may not be possible. It may be worth discussing at the spec level since this type of behavioral change from explicit -> exponential would affect all languages.

@jack-berg
Copy link
Member

Resolved in #4218. The spec issue #2382 argues that a consistent definition for the histogram definition should be specified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

2 participants