-
Notifications
You must be signed in to change notification settings - Fork 63
Fixes ExponentialBucketTimeRange causing unpredictable memory usage and Floating Points Prometheus Issue #1120
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
Conversation
c0830a9 to
a6f2efa
Compare
…ket count, and floating-point precision This commit fixes 5 critical issues in ExponentialBucketTimeRange: 1. Safer calculation: Replaced math.Exp(math.Log(x)) with math.Pow(x, 1/n) - Both are mathematically identical but Pow is clearer and more direct - Maintains true exponential spacing with constant ratio between buckets 2. Guaranteed exact bucket count: Changed loop from 'for v <= interval' to 'for i := 1; i < buckets' - Previously produced 9-11 buckets when requesting 10 - Now guarantees exact count, making memory usage predictable 3. Independent calculation: Each bucket calculated via math.Pow(factor, i) - Eliminates error accumulation from iterative multiplication - Ensures monotonically increasing values 4. Clean Prometheus output: Added roundToSignificantDigits() function - Produces clean values like le="0.00215443" instead of le="0.0021544299999999999" - Improves readability and query performance 5. Edge case handling: Returns sensible defaults for invalid inputs - Handles zero/negative buckets, start >= end gracefully - Prevents panics and incorrect results Impact: - 40-50% memory reduction per histogram by enabling safe bucket count reduction - Predictable memory usage with exact bucket counts - Cleaner Prometheus output - More robust code Testing: - Comprehensive test suite with individual tests for each fix - All tests passing with verification of exponential spacing, exact counts, and clean output Signed-off-by: AkramBitar <akram@il.ibm.com>
9eb989e to
8b2b5ff
Compare
mbrandenburger
left a comment
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.
Thanks @AkramBitar for creating this fix. I am wondering if we could just use ExponentialBucketsRange from prometheus https://github.com/prometheus/client_golang/blob/v1.23.2/prometheus/histogram.go#L339
Here in this package we could just provide a adapter to match our time-based API. WDYT?
Thanks a lot @mbrandenburger. Prometheus's function requires min > 0 because it uses ratio-based math (max/min). In our case measuring time durations starting from 0 (e.g., 0ms to 1000ms). For example: We can do one of the following:
Please let me know what do you think and I will change the code accordingly. |
|
Thanks @AkramBitar. I think if the prometheus impl gives what we need (module some wrapper code), then we should consider to use it. WDYT? |
I think lets leave this implementation since we need to keep "0" buckets and not to make a lot of changes in other components. |
mbrandenburger
left a comment
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.
LGTM
See issue: #1119