[Metrics] Perform async Re-Aggregation when Spatial Attributes are dropped #3691
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3691 +/- ##
==========================================
- Coverage 89.95% 89.75% -0.20%
==========================================
Files 225 226 +1
Lines 7273 7326 +53
==========================================
+ Hits 6542 6575 +33
- Misses 731 751 +20
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the issue of performing async re-aggregation when spatial attributes are dropped in OpenTelemetry metrics. The main purpose is to ensure that when attributes are filtered out by attribute processors in async counters, the measurements are properly reaggregated to combine values that now have identical attribute sets.
Key changes include:
- Introduced a new
MeasurementAttributestype for better handling of attribute-value mappings - Modified async metric storage to perform reaggregation during attribute processing
- Updated observer result handling to use the new measurement structure
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/include/opentelemetry/sdk/metrics/state/measurement_attributes_map.h | New header defining custom hash/equality functions and MeasurementAttributes type for attribute maps |
| sdk/include/opentelemetry/sdk/metrics/observer_result.h | Updated ObserverResultT to use new MeasurementAttributes type and removed dependency on AttributesProcessor |
| sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h | Added ConvertMeasurementsToMetricAttributes method to handle reaggregation and attributes processor integration |
| sdk/include/opentelemetry/sdk/metrics/state/metric_storage.h | Updated AsyncWritableMetricStorage interface to use MeasurementAttributes type |
| sdk/include/opentelemetry/sdk/metrics/state/multi_metric_storage.h | Updated AsyncMultiMetricStorage to use new MeasurementAttributes type |
| sdk/src/metrics/meter.cc | Modified async metric storage registration to pass attributes processor |
| sdk/src/metrics/state/observable_registry.cc | Updated to create ObserverResultT without attributes processor parameter |
| sdk/test/metrics/async_metric_storage_test.cc | Updated tests to use new types and added attributes processor setup |
| sdk/test/metrics/observer_result_test.cc | Simplified test to use new ObserverResultT constructor |
| sdk/test/metrics/sum_aggregation_test.cc | Added comprehensive tests for observable counter reaggregation scenarios |
| return std::hash<nostd::string_view>{}(nostd::get<nostd::string_view>(value)); | ||
| } | ||
|
|
||
| return 0; // fallback for other types |
There was a problem hiding this comment.
The fallback hash value of 0 for unsupported types can cause hash collisions. Consider using a more distinctive hash value or throwing an exception for unsupported types to make the limitation explicit.
| return 0; // fallback for other types | |
| // Fallback for other types: use the index of the variant as a distinctive hash value | |
| return std::hash<size_t>{}(value.index()); |
Copilot uses AI. Check for mistakes.
| return nostd::get<nostd::string_view>(a) == nostd::get<nostd::string_view>(b); | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
The fallback return value of false for unsupported types in equality comparison can lead to incorrect behavior. Consider throwing an exception for unsupported types to make the limitation explicit, similar to the hash function concern.
Copilot uses AI. Check for mistakes.
|
Assigned it to self. Will review during this week. |
2 participants
Fixes #1724
Performing Reaggregation when spatial dimensions are dropped in async counters. There are also unit tests provided to the change.
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes