From d06a13863893d84fbdbeda094b59a47726bb1a09 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 14 Apr 2026 21:21:53 +0800 Subject: [PATCH] [CODE HEALTH] Fix clang-tidy warnings in base2 exponential histogram aggregation Clear the seven clang-tidy warnings reported under #3979 in sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc: * GetScaleReduction (L35): widen the loop condition to int64_t so that end_index - start_index + 1 cannot overflow int32_t at max_scale_=20 with extreme-span histograms, and so bugprone-misplaced-widening-cast stops firing. * Downscale (L215): wrap point_data_.scale_ -= by with static_cast(point_data_.scale_ - by). * Merge (L327-329) and Diff (L412-413): same static_cast wrap on the five remaining int32_t -= uint32_t sites that land back into the scale_ int32_t field. Behaviour is preserved: all nine existing Base2Exponential* cases in sdk/test/metrics/aggregation_test.cc pass before and after the change. The MergeCountInvariant test in particular exercises the scale-reduction branch this PR touches. Decrement the clang-tidy warning_limit by 7 for both all-options-abiv1-preview and all-options-abiv2-preview matrix entries to ratchet the limit down to the new measured count. Resolves #3979. Part of #2053. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .github/workflows/clang-tidy.yaml | 4 ++-- CHANGELOG.md | 3 +++ ...base2_exponential_histogram_aggregation.cc | 14 +++++------ sdk/test/metrics/aggregation_test.cc | 24 +++++++++++++++++++ 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/.github/workflows/clang-tidy.yaml b/.github/workflows/clang-tidy.yaml index 85d7210a4f..9ce1dffa29 100644 --- a/.github/workflows/clang-tidy.yaml +++ b/.github/workflows/clang-tidy.yaml @@ -17,9 +17,9 @@ jobs: matrix: include: - cmake_options: all-options-abiv1-preview - warning_limit: 42 + warning_limit: 35 - cmake_options: all-options-abiv2-preview - warning_limit: 44 + warning_limit: 37 env: CC: /usr/bin/clang-18 CXX: /usr/bin/clang++-18 diff --git a/CHANGELOG.md b/CHANGELOG.md index 87a519613f..bac0392acc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,9 @@ Increment the: * [CODE HEALTH] Fix misc clang-tidy warnings [#3993](https://github.com/open-telemetry/opentelemetry-cpp/pull/3993) +* [CODE HEALTH] Fix clang-tidy warnings in base2 exponential histogram aggregation + [#3997](https://github.com/open-telemetry/opentelemetry-cpp/pull/3997) + Important changes: * Enable WITH_OTLP_RETRY_PREVIEW by default diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index 70d915bb04..140eff668e 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -32,7 +32,7 @@ namespace uint32_t GetScaleReduction(int32_t start_index, int32_t end_index, size_t max_buckets) noexcept { uint32_t scale_reduction = 0; - while (static_cast(end_index - start_index + 1) > max_buckets) + while (static_cast(end_index) - start_index + 1 > static_cast(max_buckets)) { start_index >>= 1; end_index >>= 1; @@ -212,7 +212,7 @@ void Base2ExponentialHistogramAggregation::Downscale(uint32_t by) noexcept DownscaleBuckets(point_data_.negative_buckets_, by); } - point_data_.scale_ -= by; + point_data_.scale_ -= static_cast(by); indexer_ = Base2ExponentialHistogramIndexer(point_data_.scale_); } @@ -324,9 +324,9 @@ std::unique_ptr Base2ExponentialHistogramAggregation::Merge( DownscaleBuckets(high_res.positive_buckets_, scale_reduction); DownscaleBuckets(low_res.negative_buckets_, scale_reduction); DownscaleBuckets(high_res.negative_buckets_, scale_reduction); - low_res.scale_ -= scale_reduction; - high_res.scale_ -= scale_reduction; - result_value.scale_ -= scale_reduction; + low_res.scale_ -= static_cast(scale_reduction); + high_res.scale_ -= static_cast(scale_reduction); + result_value.scale_ -= static_cast(scale_reduction); } } @@ -409,8 +409,8 @@ std::unique_ptr Base2ExponentialHistogramAggregation::Diff( { DownscaleBuckets(right.negative_buckets_, scale_reduction); } - left.scale_ -= scale_reduction; - right.scale_ -= scale_reduction; + left.scale_ -= static_cast(scale_reduction); + right.scale_ -= static_cast(scale_reduction); } Base2ExponentialHistogramPointData result_value; diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index b60d95a4e7..9e76c2b292 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -608,3 +608,27 @@ TEST(Aggregation, Base2ExponentialHistogramAggregationMergeCountInvariant) verify_invariant(point, expected_count, "Cycle 6: indices 5-9"); } } + +TEST(Aggregation, Base2ExponentialHistogramAggregationDiffDownscale) +{ + // Force the scale-reduction branch in Diff() by giving left and right + // bucket indices whose combined span exceeds max_buckets. Each side + // individually has only one bucket so neither triggers Aggregate's + // internal Downscale; the reduction must happen inside Diff(). + Base2ExponentialHistogramAggregationConfig config; + config.max_scale_ = 0; + config.max_buckets_ = 5; + + Base2ExponentialHistogramAggregation left(&config); + left.Aggregate(2.0, {}); // 2^1 -> index 0 + + Base2ExponentialHistogramAggregation right(&config); + right.Aggregate(128.0, {}); // 2^7 -> index 6 + + // Combined span 0..6 (7 buckets) > max_buckets=5, so Diff() must downscale. + // GetScaleReduction(0, 6, 5) returns 1, so the result scale drops from 0 to -1. + auto diffed = left.Diff(right); + auto diffed_point = nostd::get(diffed->ToPoint()); + + EXPECT_EQ(diffed_point.scale_, -1); +}