feat: Improve support for exponential histogram #3259
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3259 +/- ##
=======================================
- Coverage 80.8% 80.5% -0.3%
=======================================
Files 129 129
Lines 23203 23272 +69
=======================================
Hits 18750 18750
- Misses 4453 4522 +69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@thomasstauffer666 let us know when this is ready for review! |
scottgerring
changed the title
thomasstauffer666
force-pushed
the
main
branch
2 times, most recently
from
7bfc771 to
2cd0f32
Compare
thomasstauffer666
force-pushed
the
main
branch
from
2cd0f32 to
70a29bb
Compare
|
@scottgerring would be ready for review codecov correctly reports that nothing I added to stdout is tested. AFAIK rust has no official way to unit test functions which use stdout. Possibilites I see:
|
|
hey @thomasstauffer666 great - cheers! I'll check it out tomorrow 💪 Apropos the stdout coverage; we try and be pragmatic with coverage, and my initial thought is that this is a case where we don't need to go wild chasing it, but I will form proper opinions shortly. |
scottgerring
left a comment
scottgerring
left a comment
There was a problem hiding this comment.
Hey @thomasstauffer666 thanks for the PR! Couple of minor notes inline
p.s. if you would like to chat about this or other otel-rust stuff, you can join us in #opentelemetry-rust in the CNCF slack 🤝
| } | ||
| }; | ||
|
|
||
| // for example 3 |
There was a problem hiding this comment.
It would be cool to be a bit more descriptive with the examples; "This is an exponential histogram, this is great for bucketed data that is exponential in nature" or something, I reckon. The example text on the others above is not so detailed, but this feels like it is introducing nuance, in that we have two examples of histograms now, and its not going to be obvious to all folks which to use when.
Likewise on the aggregation front down around L40-43, some brief comments on the tunables would be great; ultimately we want this all to feel overly descriptive / obvious, so there's less room for folks to waste time guessing at things!
| if *max_scale > EXPO_MAX_SCALE { | ||
| return Err(MetricError::Config(format!( | ||
| "aggregation: exponential histogram: max scale ({max_scale}) is greater than 20", | ||
| "aggregation: exponential histogram: max scale ({max_scale}) is greater than {}", EXPO_MAX_SCALE |
| let from = -base.powf(i as f64 + negative_offset as f64); | ||
| let to = -base.powf(i as f64 + negative_offset as f64 + 1.0f64); | ||
| println!( | ||
| "\t\t\t\t -> Bucket {} (> {:.3} to <= {:.3}) : {}", | ||
| i, from, to, count | ||
| ); |
There was a problem hiding this comment.
I think this gets the labels backwards for negative i - such that from > to
| print_hist_data_points(histogram.data_points()); | ||
| } | ||
|
|
||
| fn print_exponential_histogram<T: Debug + Copy>(histogram: &ExponentialHistogram<T>) { |
There was a problem hiding this comment.
Could you update opentelemetry-stdout/CHANGELOG.md with a brief note about this?
thomasstauffer666
force-pushed
the
main
branch
from
4d362c8 to
19bba98
Compare
thomasstauffer666
force-pushed
the
main
branch
from
19bba98 to
9317826
Compare
2 participants
Fixes #2111 and implements exponential histogram support in
opentelemetry-stdout.Design discussion issue (if applicable) #
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes