feat: add range based exponential buckets in histogram #233
Conversation
Signed-off-by: Sidhant Kohli <[email protected]>
|
@mxinden please check! |
src/metrics/histogram.rs
Outdated
| if length < 1 { | ||
| panic!("ExponentialBucketsRange length needs a positive length"); | ||
| } |
There was a problem hiding this comment.
You could use a NonZeroU16 instead of a u16. That said, that would be inconsistent with exponential_buckets. Thoughts?
There was a problem hiding this comment.
I guess better to keep as u16 to keep consistent with exponential_buckets.
We can keep the additional check in the code for correctness.
What do you think?
src/metrics/histogram.rs
Outdated
| if min <= 0.0 { | ||
| panic!("ExponentialBucketsRange min needs to be greater than 0"); | ||
| } |
There was a problem hiding this comment.
We don't panic anywhere else in the crate. Reason being, that given that instrumentation is auxiliary only, folks likely don't want a panic. Would an empty Iterator work too?
There was a problem hiding this comment.
Sure! I have removed panics, but instead of an empty iterator I have added defaults with a warning.
Empty iterator might have have side-effects in histogram functionality.
Wdyt?
Signed-off-by: Sidhant Kohli <[email protected]>
Signed-off-by: Sidhant Kohli <[email protected]>
|
@mxinden Thanks for the review! Have addressed them with some small follow ups, please take a look whenever you get a chance. |
|
@mxinden Just following up on this? Let me know if any other changes are required |
The current implementation does not have a range based exponential bucket function.
This can be useful in scenarios where the end user doesn't want to perform manual calculations for extracting the required buckets, especially when multiple different ranges are to be used for different histograms.
Similar implementation is provided in the golang client as well.
https://github.com/prometheus/client_golang/blob/5d584e2717ef525673736d72cd1d12e304f243d7/prometheus/histogram.go#L125