Fix few Observable Counter and UpDownCounter bugs #1992
Conversation
| } | ||
| } | ||
| } | ||
|
|
||
| impl<T: Number<T>> ValueMap<T> { | ||
| fn measure(&self, measurement: T, attrs: &[KeyValue]) { | ||
| if attrs.is_empty() { | ||
| self.no_attribute_value.add(measurement); | ||
| if self.assign_only { |
There was a problem hiding this comment.
@stormshield-fabs Could you help refactor this to use generics, so we dont have this duplication everywhere and get better perf?
There was a problem hiding this comment.
Let's extract this repeated snippet to a method for now? It would also make it easier to review the future PR that would refactor it using generics.
| @@ -629,8 +667,9 @@ mod tests { | |||
| } | |||
|
|
|||
| #[tokio::test(flavor = "multi_thread", worker_threads = 1)] | |||
| #[ignore = "Spatial aggregation is not yet implemented."] | |||
There was a problem hiding this comment.
this got accidentally fixed in some other PR, but this PR breaks it. spatial aggregation is complex and needs a separate discussion altogether!
There was a problem hiding this comment.
If there is no issue to track this, probably good to have one.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1992 +/- ##
=======================================
- Coverage 75.1% 74.8% -0.3%
=======================================
Files 122 122
Lines 20642 20698 +56
=======================================
- Hits 15505 15485 -20
- Misses 5137 5213 +76 ☔ View full report in Codecov by Sentry. |
| - Fixed various Metric aggregation bug related to | ||
| ObservableCounter,UpDownCounter including | ||
| [1517](https://github.com/open-telemetry/opentelemetry-rust/issues/1517). | ||
| [#1990](https://github.com/open-telemetry/opentelemetry-rust/pull/1990) |
There was a problem hiding this comment.
| [#1990](https://github.com/open-telemetry/opentelemetry-rust/pull/1990) | |
| [#1992](https://github.com/open-telemetry/opentelemetry-rust/pull/1992) |
| @@ -25,40 +25,54 @@ struct ValueMap<T: Number<T>> { | |||
| has_no_value_attribute_value: AtomicBool, | |||
| no_attribute_value: T::AtomicTracker, | |||
| count: AtomicUsize, | |||
| assign_only: bool, // if true, only assign incoming value, instead of adding to existing value | |||
There was a problem hiding this comment.
Could we rename this to something more intuitive like is_sum_precomputed? I think that would better explain the reasoning behind the logic when reading the code:
if self.is_sum_precomputed {
value_to_update.store(measurement);
} else {
value_to_update.add(measurement);
}
| let default = T::default(); | ||
| let delta = self.value_map.no_attribute_value.get_value() | ||
| - *reported.get(&vec![]).unwrap_or(&default); | ||
| new_reported.insert(vec![], self.value_map.no_attribute_value.get_value()); |
There was a problem hiding this comment.
We should be storing the computed delta value here and not do another atomic read. Otherwise, we risk storing an incorrect value from another update thread.
There was a problem hiding this comment.
Good catch. The self.value_map.no_attribute_value fetched once in line 372 should be used as new_reported value instead of fetching again.
|
Closing in favor of #2004 |
Fixes #1517 and other bugs. Added tests.
Copying ideas from @stormshield-fabs 's original PR: #1644
Additionally, the previous implementation of the Observable relied on the assumption that the ValueMap is drained after ever collect. This is an area that could be tweaked in the future. This PR removed that dependency anyway, so this is better future proof.
Focused on fixing the bug only. The code is poorly structured now, and could benefit from a refactoring. The Aggregation could be made generic similar to what was done in #1644 or other ways to make it manageable. Currently, there are tons of code duplication. Will attempt this after ensuring all known bugs are fixed and there is good test coverage to confidently do major refactorings.