Add native histogram support#304
Conversation
Add native-only histograms and histograms that expose both classic and native buckets. Encode native sparse bucket spans and deltas through the Prometheus protobuf encoder while keeping text and OpenMetrics protobuf behavior on the classic representation when available. Signed-off-by: John Howard <john.howard@solo.io>
howardjohn
force-pushed
the
histogram/native2
branch
from
dc631bf to
d4882eb
Compare
|
Related issue #150 |
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
This looks great, I am planning to do some actual testing as well soon, but added a couple of comments/questions in the meantime!
| if inner.positive.len() + inner.negative.len() <= inner.max_buckets { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Can we follow what client_golang is doing here:
https://github.com/prometheus/client_golang/blob/02eaf492695d380c231404841c5652a39b06be84/prometheus/histogram.go#L923-L962
basically do one widening/doubling per call then reassess as
"very sparse buckets could lead to a low
reduction of the bucket count (or even no reduction at all)"
| } | ||
|
|
||
| /// Create a new classic [`Histogram`]. | ||
| pub fn new_classic(buckets: impl IntoIterator<Item = f64>) -> Self { |
There was a problem hiding this comment.
Is this the same as new?
| fn native_histogram_bounds() -> &'static [&'static [f64]; 9] { | ||
| &NATIVE_HISTOGRAM_BOUNDS | ||
| } |
There was a problem hiding this comment.
Is there any reason why we cannot just use NATIVE_HISTOGRAM_BOUNDS directly?
| if frac == 0.5 { | ||
| key -= 1; | ||
| } | ||
| let offset = (1_i32 << (-(schema as i32) as u32)) - 1; |
There was a problem hiding this comment.
We had #281 removing as casts, so maybe we should this pattern here and at a few other places? cc. @jalil-salame
| _native: NativeHistogram<'_>, | ||
| ) -> Result<(), std::fmt::Error> { | ||
| if buckets.is_empty() { | ||
| return Err(std::fmt::Error); |
There was a problem hiding this comment.
Can we have a typed error here and surface that the text encoder doesn't support native histograms?
| /// Set a custom zero threshold. | ||
| pub fn zero_threshold(mut self, zero_threshold: f64) -> Self { | ||
| assert!(zero_threshold.is_finite()); | ||
| assert!(zero_threshold >= 0.0 || zero_threshold == NATIVE_HISTOGRAM_ZERO_THRESHOLD_ZERO); | ||
| self.zero_threshold = zero_threshold; | ||
| self | ||
| } |
There was a problem hiding this comment.
Can we match the logic in client_go, where we have this: https://github.com/prometheus/client_golang/blob/02eaf492695d380c231404841c5652a39b06be84/prometheus/histogram.go#L576-L587
| @@ -173,15 +471,874 @@ pub fn linear_buckets(start: f64, width: f64, length: u16) -> impl Iterator<Item | |||
|
|
|||
| impl EncodeMetric for Histogram { | |||
There was a problem hiding this comment.
When using examplars this is skipped:
client_rust/src/metrics/exemplar.rs
Lines 275 to 285 in 0c3ffbd
and histogram.get() is classic only.
So maybe we can move this into a helper and use that from HistogramWithExemplars as well?
3 participants
This PR adds support for native histograms using the new prometheus protobuf support recently merged.
The implementation is basically 1:1 lifted from the Golang implementation as much as possible, including tests.
Users can make any of the 3: classic only, native only, or classic + native histogram.