fix: Metrics Views - fix a bug that causes unit, description to be lost when applying views that influence other aspects #2981
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2981 +/- ##
=======================================
+ Coverage 81.2% 81.4% +0.1%
=======================================
Files 126 126
Lines 24394 24571 +177
=======================================
+ Hits 19827 20005 +178
+ Misses 4567 4566 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 1)] | ||
| async fn view_test_rename() { |
There was a problem hiding this comment.
These tests and the helper method need not be async.
| otel_debug!( | ||
| name : "Metrics.InstrumentCreated", | ||
| instrument_name = stream.name.as_ref(), | ||
| instrument_name = stream.name.clone().unwrap_or_default().as_ref(), |
There was a problem hiding this comment.
We are being forced to introduce a lot of unwrap_or_default() method calls even though we know that the stream provided to cached_aggregator method would always have some name, unit, and description.
We should ideally create another internal struct that would have the same fields as Stream but they wouldn't be optional. Using such a struct instead of Stream would allow to get rid of these unwrap_or_default() calls which have weakened the contract for cached_aggregator method.
There was a problem hiding this comment.
good point! we can modify this separately, as we use the unwrap_default for buckets, attribute_keys too.
|
|
||
| let match_fn: Box<dyn Fn(&Instrument) -> bool + Send + Sync> = if contains_wildcard { | ||
| if !mask.name.is_empty() { | ||
| if mask.name.is_some() { |
There was a problem hiding this comment.
We still might have to check for empty string. A user could still provide an empty string as Some(String::new()).
2 participants
unit, desc were lost when the View did not explicitly specify them. This PR fixes this bug. If any aspect is not specified in the View's Stream configuration, it'll use the default (if advisory is available, that is considered first before default)