[marcalff]

Fixes # (issue)

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

❌ Patch coverage is 74.39024% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.73%. Comparing base (ac7b87a) to head (8ae93b4).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
exporters/prometheus/src/exporter_utils.cc	62.97%	10 Missing ⚠️
sdk/src/metrics/aggregation/sum_aggregation.cc	82.15%	5 Missing ⚠️
exporters/ostream/src/metric_exporter.cc	75.00%	2 Missing ⚠️
...ude/opentelemetry/sdk/metrics/data/exemplar_data.h	66.67%	2 Missing ⚠️
exporters/otlp/src/otlp_metric_utils.cc	88.89%	1 Missing ⚠️
...pentelemetry/sdk/metrics/exemplar/reservoir_cell.h	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4731      +/-   ##
==========================================
+ Coverage   86.69%   86.73%   +0.04%     
==========================================
  Files         527      527              
  Lines       20902    20900       -2     
==========================================
+ Hits        18119    18125       +6     
+ Misses       2783     2775       -8     

Files with missing lines	Coverage Δ
...etry/sdk/metrics/aggregation/default_aggregation.h	86.59% <100.00%> (ø)
...elemetry/sdk/metrics/aggregation/sum_aggregation.h	0.00% <ø> (ø)
...nclude/opentelemetry/sdk/metrics/data/point_data.h	58.70% <ø> (ø)
exporters/otlp/src/otlp_metric_utils.cc	92.15% <88.89%> (+0.17%)	⬆️
...pentelemetry/sdk/metrics/exemplar/reservoir_cell.h	97.83% <50.00%> (ø)
exporters/ostream/src/metric_exporter.cc	85.81% <75.00%> (-0.55%)	⬇️
...ude/opentelemetry/sdk/metrics/data/exemplar_data.h	71.43% <66.67%> (-28.57%)	⬇️
sdk/src/metrics/aggregation/sum_aggregation.cc	82.36% <82.15%> (+15.36%)	⬆️
exporters/prometheus/src/exporter_utils.cc	87.50% <62.97%> (-1.70%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marcalff]

Refactoring to discuss

Before

• SumPointData contains a ValueType variant with both int64_t and double.
• LongSumAggregation contains ValueType, but only uses int64_t
• DoubleSumAggregation contains ValueType, but only uses double
• Code using Long/Double aggregation needs to extract the proper value from the variant, all the time.
• ABIv1 warnings: 198
• ABIv2 warnings: 212

After

• LongSumPointData = SumPointData (int64_t) only contains long
• DoubleSumPointData = SumPointData (double) only contains double
• LongSumAggregation only contains LongSumPointData (int64_t)
• DoubleSumAggregation only contains DoubleSumPointData (double)
• Code using Long/Double aggregation sees a value with the proper type.
• ABIv1 warnings: 187
• ABIv2 warnings: 201

Proposal

• Change the Sum Aggregation as this PR
• Follow up with similar refactoring for other aggregation types

This simplifies the code base in general.
This avoids uses of variants, and avoids exceptions related to std::get or conditionals with std::get_if

[mateenali66]

the typed aggregation reads much better, and the loss-of-type branches in Merge and Diff go away.

the cost lands downstream. SumPointData and PointType are what out-of-tree exporters read, and both stop compiling here. contrib's geneva exporter calls holds_alternative<SumPointData> and get<SumPointData> (exporter.cc:79-81), and google-cloud-cpp visits PointType with an operator()(SumPointData const&) (time_series.cc:104). google-cloud-cpp already gates its exponential histogram overload on OPENTELEMETRY_VERSION_MINOR >= 21, so a PointType change has hit them once.

compared with the clang-tidy run on main, 10 of the 11 warnings this drops are inside sum_aggregation.cc (8 new in the loss-of-type branches, 2 Aggregate). the 11th is ConvertSumMetric.

would typed fields inside the two aggregations only, building SumPointData in ToPoint(), get those 10 without the break? if the break is wanted, does it go through deprecation-process.md, and land with the last value and histogram follow-ups in one release?