Create functions to create u64 and f64 exponential histograms #94
Conversation
| ($method: ident, $ty:ty, $var_name:literal, $usage:literal) => { | ||
| ($method: ident, $ty:ty, $var_name:literal, $usage:literal, $show_header: ident) => { | ||
| concat!( | ||
| "Wrapper for [`Meter::", stringify!($method), "`] using Pydantic Logfire's global meter. | ||
| metric_doc_header!($show_header, $method), | ||
| " |
There was a problem hiding this comment.
It would be nice to have an alternative header for the exponential histograms, which explains what they are, given there isn't an upstream doc to link to.
There was a problem hiding this comment.
Makes sense. Will add it
There was a problem hiding this comment.
Added
src/metrics.rs
Outdated
| let mut histograms = EXPONENTIAL_HISTOGRAMS | ||
| .write() | ||
| .unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| let name = name.into(); | ||
| histograms.insert(name.clone(), scale); | ||
| u64_histogram(name) |
There was a problem hiding this comment.
Should we potentially wrap the return value as a struct ExponentialHistogram so that on drop it can remove the name from the global? Might be cleaner.
There was a problem hiding this comment.
We could also remove from the map after the histogram is passed to the view function. I'm not completely sure that removing from the map is safe since the spec doesn't say how many times the view function will be called for each metric.
There was a problem hiding this comment.
Agreed, I think if we clean up we should do it when the whole instrument is dropped.
There was a problem hiding this comment.
Implemented Drop to clean up
src/metrics.rs
Outdated
| .unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| let name = name.into(); | ||
| histograms.insert(name.clone(), scale); | ||
| f64_histogram(name) |
There was a problem hiding this comment.
How does the base2 aggregation affect the bucketing going on here? Should this use a different underlying instrument to avoid any bucketing here being a problem?
|
@PoorlyDefinedBehaviour let's get CI green then we can merge this. We'll test it in our codebase and see what the data looks like once it's exported, etc. Sound good? |
Sounds good, hadn't seen this comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
4 participants
SdkMeterProviderAggregation::Base2ExponentialHistogramThe feature
spec_unstable_metrics_viewsis required to use the view functionality.