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

Exponential histogram downscaling can go into an infinite loop #6089

Closed
axw opened this issue Dec 21, 2023 · 3 comments · Fixed by #6093
Closed

Exponential histogram downscaling can go into an infinite loop #6089

axw opened this issue Dec 21, 2023 · 3 comments · Fixed by #6093
Labels
Bug Something isn't working

Comments

@axw
Copy link

axw commented Dec 21, 2023

Describe the bug

When configuring a base-2 exponential bucket histogram with a maximum of 1 bucket, DoubleHistogram.record may spin and never return.

Steps to reproduce

Run this example: axw/opentelemetry-java-examples@a1bf2e7

What did you expect to see?

I expect the the program not to spin.

I don't know what the correct logic should be. Require > 1 max buckets? Or otherwise automatically increase the zero threshold to 1.0 as needed? This probably needs clarification in https://opentelemetry.io/docs/specs/otel/metrics/sdk/#base2-exponential-bucket-histogram-aggregation

What did you see instead?

The program spins. Attaching a debugger shows that it's stuck in an infinite loop in DoubleHistogram.record, due to the start index being -1 and the end index shifting down to 0 -- so the delta converges on 2, which is always greater than a maximum of 1 bucket.

What version and what artifacts are you using?
Artifacts: opentelemetry-api, opentelemetry-sdk
Version: 1.33.0

Environment
Compiler: OpenJDK 17
OS: Ubuntu 22.04

@axw axw added the Bug Something isn't working label Dec 21, 2023
@jack-berg
Copy link
Member

Thanks for the report and the repro. Will dig into this.

@jack-berg
Copy link
Member

jack-berg commented Dec 21, 2023

I don't know what the correct logic should be. Require > 1 max buckets?

I think we need to require > 1 max buckets. Specifying a single bucket defeats the purpose of exponential bucket histogram since there is effectively no ability to rescale to accommodate the range of measurements. With a single bucket, the measurements MUST fit into that bucket, and because the formula for bucket boundaries always results in a boundary falling on 1, there is no scale which can accommodate a measurement <= 1 and any measurement >= 1 in a single bucket.

To demonstrate, try running your example with:

histogram.record(1.0);
histogram.record(1.00001);

This will hang, while the following will resolve:

histogram.record(1.00001);
histogram.record(Double.MAX_VALUE);

To your point, we could also increase the zero threshold to 1.0.

But what is the point of a single bucket exponential histogram? What information do you gain over an explicit bucket histogram with a single bucket (which records min, max, sum, count)?

As an additional data point, dot net requires the number of buckets to be >= 2: https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Metrics/Base2ExponentialBucketHistogramConfiguration.cs#L29-L34

@axw
Copy link
Author

axw commented Dec 21, 2023

But what is the point of a single bucket exponential histogram? What information do you gain over an explicit bucket histogram with a single bucket (which records min, max, sum, count)?

Agreed, it's fairly pointless. I was just messing around with the parameters and happened to notice this behaviour. I think it's fair to require more than one bucket - that should be simplest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants