-
Notifications
You must be signed in to change notification settings - Fork 872
Fix float histogram bucket precision #6866
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?
Fix float histogram bucket precision #6866
Conversation
Signed-off-by: Pratik Mahalle <mahallepratik683@gmail.com>
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Signed-off-by: Pratik Mahalle <mahallepratik683@gmail.com>
Signed-off-by: Pratik Mahalle <mahallepratik683@gmail.com>
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.
Pull request overview
This pull request fixes a floating-point precision issue when using Histogram<float> with custom HistogramBucketBoundaries. The problem occurred when float values like 0.025f were converted to double, resulting in precision errors like 0.02500000037252903 instead of the intended 0.025.
Changes:
- Modified float-to-double conversion for histogram bucket boundaries to preserve decimal precision
- Added comprehensive test to verify the precision fix works correctly
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/OpenTelemetry/Metrics/MetricStreamIdentity.cs | Implemented float-to-string-to-double conversion to preserve decimal precision for float histogram boundaries |
| test/OpenTelemetry.Tests/Metrics/MetricViewTests.cs | Added test to verify float histogram boundaries maintain proper precision |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Pratik Mahalle <mahallepratik683@gmail.com>
Signed-off-by: Pratik Mahalle <mahallepratik683@gmail.com>
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Record some values | ||
| histogram.Record(0.02f); | ||
| histogram.Record(0.5f); | ||
| histogram.Record(5.0f); | ||
|
|
||
| meterProvider.ForceFlush(MaxTimeToAllowForFlush); | ||
| Assert.Single(exportedItems); | ||
| var metric = exportedItems[0]; | ||
|
|
||
| List<MetricPoint> metricPoints = []; | ||
| foreach (ref readonly var mp in metric.GetMetricPoints()) | ||
| { | ||
| metricPoints.Add(mp); | ||
| } | ||
|
|
||
| Assert.Single(metricPoints); | ||
| var histogramPoint = metricPoints[0]; | ||
|
|
||
| // Verify the bucket boundaries maintain proper precision | ||
| // The key assertion is that the boundaries should be clean decimal values | ||
| // not values with floating-point precision errors like 0.02500000037252903 | ||
|
|
||
| // Expected clean boundaries (converted from float to double with proper precision) | ||
| var expectedBoundaries = new double[] | ||
| { | ||
| 0.001, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 120, | ||
| }; | ||
|
|
||
| var index = 0; | ||
| foreach (var histogramMeasurement in histogramPoint.GetHistogramBuckets()) | ||
| { | ||
| if (index < expectedBoundaries.Length) | ||
| { | ||
| // Verify each boundary is the expected clean value | ||
| Assert.Equal(expectedBoundaries[index], histogramMeasurement.ExplicitBound); | ||
| } | ||
| else | ||
| { | ||
| // Verify the last bucket is positive infinity | ||
| Assert.Equal(double.PositiveInfinity, histogramMeasurement.ExplicitBound); | ||
| } | ||
|
|
||
| index++; | ||
| } | ||
|
|
||
| // Verify we got the expected number of buckets | ||
| Assert.Equal(expectedBoundaries.Length + 1, index); | ||
| } |
Copilot
AI
Feb 8, 2026
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.
This test validates that exported ExplicitBound values look “clean”, but it doesn’t guard against a potential semantic regression where recording a value exactly equal to a float boundary (e.g., histogram.Record(0.025f) or 0.1f) no longer increments the expected bucket due to the float->double boundary conversion. Add an assertion that a boundary-equal float measurement lands in the correct (inclusive) bucket to catch this.
| // For float types, convert to string first to preserve the intended decimal precision | ||
| // and avoid floating-point representation issues (e.g., 0.025f -> 0.025 instead of 0.02500000037252903) | ||
| explicitBucketBoundaries[i] = typeof(T) == typeof(float) | ||
| ? double.Parse( | ||
| Convert.ToString(adviceExplicitBucketBoundaries[i], CultureInfo.InvariantCulture)!, | ||
| CultureInfo.InvariantCulture) | ||
| : Convert.ToDouble( | ||
| adviceExplicitBucketBoundaries[i], | ||
| CultureInfo.InvariantCulture); |
Copilot
AI
Feb 8, 2026
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.
The new float-handling path converts bucket boundaries from float -> string -> double. This changes the numeric boundary used for bucketing (e.g., 0.1f is recorded as 0.10000000149011612 via the float measurement callback, but the boundary becomes 0.1), which can shift measurements at/near the boundary into a different bucket (breaking the “upper bound inclusive” behavior for values equal to the provided float boundary). Consider keeping internal bucketing bounds as the exact float value (float-cast-to-double) and only “cleaning” the values at export/serialization time (or store separate arrays for lookup vs. exported explicit bounds).
| // For float types, convert to string first to preserve the intended decimal precision | |
| // and avoid floating-point representation issues (e.g., 0.025f -> 0.025 instead of 0.02500000037252903) | |
| explicitBucketBoundaries[i] = typeof(T) == typeof(float) | |
| ? double.Parse( | |
| Convert.ToString(adviceExplicitBucketBoundaries[i], CultureInfo.InvariantCulture)!, | |
| CultureInfo.InvariantCulture) | |
| : Convert.ToDouble( | |
| adviceExplicitBucketBoundaries[i], | |
| CultureInfo.InvariantCulture); | |
| if (typeof(T) == typeof(float)) | |
| { | |
| // For float types, preserve the exact numeric value by casting directly from float to double. | |
| // This avoids changing the numeric boundary (e.g., 0.10000000149011612f) while still promoting | |
| // to double for internal bucketing comparisons. | |
| explicitBucketBoundaries[i] = (double)(float)(object)adviceExplicitBucketBoundaries[i]!; | |
| } | |
| else | |
| { | |
| explicitBucketBoundaries[i] = Convert.ToDouble( | |
| adviceExplicitBucketBoundaries[i], | |
| CultureInfo.InvariantCulture); | |
| } |
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.
The current fix (converting float→string→double) changes the numeric boundary used for bucketing, which could cause boundary-equal measurements to land in the wrong bucket:
When recording 0.025f, the value becomes 0.02500000037252903 (float cast to double)
With this fix, the boundary becomes 0.025 (clean double)
Result: 0.02500000037252903 > 0.025, so the measurement lands in the NEXT bucket
Options:
- Fix at export time only - Keep exact float values for internal bucketing, but clean them when exporting/serializing bucket labels
- Document as expected behavior - Users should use Histogram with double boundaries to avoid precision issues
What should I do ? @martincostello @cijothomas
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.
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.
okay, Iets see, that works or not
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.
Signed-off-by: Pratik Mahalle <mahallepratik683@gmail.com>
Signed-off-by: Pratik Mahalle <mahallepratik683@gmail.com>
Fixes #6803
Changes
Fixed precision issues when using
Histogram<float>with customHistogramBucketBoundaries.Problem: Float values like
0.025fwere being converted to double with precision errors, resulting in bucket boundaries like0.02500000037252903.Solution: For float types, convert to string first (which preserves the intended decimal representation) then parse as double. This ensures clean bucket boundaries (e.g.,
0.025f→0.025).Other numeric types (int, long, short, byte, double) continue using the existing
Convert.ToDouble()approach.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes