From 29d68f1146e0082bf774685a41ad201883461d88 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Thu, 14 Apr 2022 22:23:09 +0530 Subject: [PATCH] Implement Merge and Diff operation for Histogram Aggregation (#1303) --- .../aggregation/histogram_aggregation.h | 39 +++++++++++ .../aggregation/histogram_aggregation.cc | 33 +++++++--- sdk/test/metrics/aggregation_test.cc | 64 ++++++++++++++++++- 3 files changed, 126 insertions(+), 10 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h index 8f33fa27b4..b5cc2c349e 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h @@ -24,8 +24,15 @@ class LongHistogramAggregation : public Aggregation void Aggregate(double value, const PointAttributes &attributes = {}) noexcept override {} + /* Returns the result of merge of the existing aggregation with delta aggregation with same + * boundaries */ virtual std::unique_ptr Merge(const Aggregation &delta) const noexcept override; + /* Returns the new delta aggregation by comparing existing aggregation with next aggregation with + * same boundaries. Data points for `next` aggregation (sum , bucket-counts) should be more than + * the current aggregation - which is the normal scenario as measurements values are monotonic + * increasing. + */ virtual std::unique_ptr Diff(const Aggregation &next) const noexcept override; PointType ToPoint() const noexcept override; @@ -45,8 +52,15 @@ class DoubleHistogramAggregation : public Aggregation void Aggregate(double value, const PointAttributes &attributes = {}) noexcept override; + /* Returns the result of merge of the existing aggregation with delta aggregation with same + * boundaries */ virtual std::unique_ptr Merge(const Aggregation &delta) const noexcept override; + /* Returns the new delta aggregation by comparing existing aggregation with next aggregation with + * same boundaries. Data points for `next` aggregation (sum , bucket-counts) should be more than + * the current aggregation - which is the normal scenario as measurements values are monotonic + * increasing. + */ virtual std::unique_ptr Diff(const Aggregation &next) const noexcept override; PointType ToPoint() const noexcept override; @@ -56,6 +70,31 @@ class DoubleHistogramAggregation : public Aggregation mutable HistogramPointData point_data_; }; +template +void HistogramMerge(HistogramPointData ¤t, + HistogramPointData &delta, + HistogramPointData &merge) +{ + for (size_t i = 0; i < current.counts_.size(); i++) + { + merge.counts_[i] = current.counts_[i] + delta.counts_[i]; + } + merge.boundaries_ = current.boundaries_; + merge.sum_ = nostd::get(current.sum_) + nostd::get(delta.sum_); + merge.count_ = current.count_ + delta.count_; +} + +template +void HistogramDiff(HistogramPointData ¤t, HistogramPointData &next, HistogramPointData &diff) +{ + for (size_t i = 0; i < current.counts_.size(); i++) + { + diff.counts_[i] = next.counts_[i] - current.counts_[i]; + } + diff.boundaries_ = current.boundaries_; + diff.count_ = next.count_ - current.count_; +} + } // namespace metrics } // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index 60f65c51b1..27405999c9 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -14,7 +14,6 @@ namespace metrics LongHistogramAggregation::LongHistogramAggregation() { - point_data_.boundaries_ = std::list{0l, 5l, 10l, 25l, 50l, 75l, 100l, 250l, 500l, 1000l}; point_data_.counts_ = std::vector(nostd::get>(point_data_.boundaries_).size() + 1, 0); @@ -47,12 +46,22 @@ void LongHistogramAggregation::Aggregate(long value, const PointAttributes &attr std::unique_ptr LongHistogramAggregation::Merge( const Aggregation &delta) const noexcept { - return nullptr; + auto curr_value = nostd::get(ToPoint()); + auto delta_value = nostd::get( + (static_cast(delta).ToPoint())); + LongHistogramAggregation *aggr = new LongHistogramAggregation(); + HistogramMerge(curr_value, delta_value, aggr->point_data_); + return std::unique_ptr(aggr); } std::unique_ptr LongHistogramAggregation::Diff(const Aggregation &next) const noexcept { - return nullptr; + auto curr_value = nostd::get(ToPoint()); + auto next_value = nostd::get( + (static_cast(next).ToPoint())); + LongHistogramAggregation *aggr = new LongHistogramAggregation(); + HistogramDiff(curr_value, next_value, aggr->point_data_); + return std::unique_ptr(aggr); } PointType LongHistogramAggregation::ToPoint() const noexcept @@ -62,7 +71,6 @@ PointType LongHistogramAggregation::ToPoint() const noexcept DoubleHistogramAggregation::DoubleHistogramAggregation() { - point_data_.boundaries_ = std::list{0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, 500.0, 1000.0}; point_data_.counts_ = @@ -96,20 +104,27 @@ void DoubleHistogramAggregation::Aggregate(double value, const PointAttributes & std::unique_ptr DoubleHistogramAggregation::Merge( const Aggregation &delta) const noexcept { - // TODO - Implement me - return nullptr; + auto curr_value = nostd::get(ToPoint()); + auto delta_value = nostd::get( + (static_cast(delta).ToPoint())); + DoubleHistogramAggregation *aggr = new DoubleHistogramAggregation(); + HistogramMerge(curr_value, delta_value, aggr->point_data_); + return std::unique_ptr(aggr); } std::unique_ptr DoubleHistogramAggregation::Diff( const Aggregation &next) const noexcept { - // TODO - Implement me - return nullptr; + auto curr_value = nostd::get(ToPoint()); + auto next_value = nostd::get( + (static_cast(next).ToPoint())); + DoubleHistogramAggregation *aggr = new DoubleHistogramAggregation(); + HistogramDiff(curr_value, next_value, aggr->point_data_); + return std::unique_ptr(aggr); } PointType DoubleHistogramAggregation::ToPoint() const noexcept { - // TODO Implement me return point_data_; } diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index a32826b145..f8051776d1 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -90,6 +90,37 @@ TEST(Aggregation, LongHistogramAggregation) EXPECT_EQ(histogram_data.count_, 4); EXPECT_EQ(histogram_data.counts_[3], 2); EXPECT_EQ(histogram_data.counts_[8], 1); + + // Merge + LongHistogramAggregation aggr1; + aggr1.Aggregate(1l, {}); + aggr1.Aggregate(11l, {}); + aggr1.Aggregate(26l, {}); + + LongHistogramAggregation aggr2; + aggr2.Aggregate(2l, {}); + aggr2.Aggregate(3l, {}); + aggr2.Aggregate(13l, {}); + aggr2.Aggregate(28l, {}); + aggr2.Aggregate(105l, {}); + + auto aggr3 = aggr1.Merge(aggr2); + histogram_data = nostd::get(aggr3->ToPoint()); + + EXPECT_EQ(histogram_data.count_, 8); // 3 each from aggr1 and aggr2 + EXPECT_EQ(histogram_data.counts_[1], 3); // 1, 2, 3 + EXPECT_EQ(histogram_data.counts_[3], 2); // 11, 13 + EXPECT_EQ(histogram_data.counts_[4], 2); // 25, 28 + EXPECT_EQ(histogram_data.counts_[7], 1); // 105 + + // Diff + auto aggr4 = aggr1.Diff(aggr2); + histogram_data = nostd::get(aggr4->ToPoint()); + EXPECT_EQ(histogram_data.count_, 2); // aggr2:5 - aggr1:3 + EXPECT_EQ(histogram_data.counts_[1], 1); // aggr2(2, 3) - aggr1(1) + EXPECT_EQ(histogram_data.counts_[3], 0); // aggr2(13) - aggr1(11) + EXPECT_EQ(histogram_data.counts_[4], 0); // aggr2(28) - aggr1(25) + EXPECT_EQ(histogram_data.counts_[7], 1); // aggr2(105) - aggr1(0) } TEST(Aggregation, DoubleHistogramAggregation) @@ -116,5 +147,36 @@ TEST(Aggregation, DoubleHistogramAggregation) EXPECT_EQ(histogram_data.counts_[3], 2); EXPECT_EQ(histogram_data.counts_[8], 1); EXPECT_EQ(nostd::get(histogram_data.sum_), 377); + + // Merge + DoubleHistogramAggregation aggr1; + aggr1.Aggregate(1.0, {}); + aggr1.Aggregate(11.0, {}); + aggr1.Aggregate(25.1, {}); + + DoubleHistogramAggregation aggr2; + aggr2.Aggregate(2.0, {}); + aggr2.Aggregate(3.0, {}); + aggr2.Aggregate(13.0, {}); + aggr2.Aggregate(28.1, {}); + aggr2.Aggregate(105.0, {}); + + auto aggr3 = aggr1.Merge(aggr2); + histogram_data = nostd::get(aggr3->ToPoint()); + + EXPECT_EQ(histogram_data.count_, 8); // 3 each from aggr1 and aggr2 + EXPECT_EQ(histogram_data.counts_[1], 3); // 1.0, 2.0, 3.0 + EXPECT_EQ(histogram_data.counts_[3], 2); // 11.0, 13.0 + EXPECT_EQ(histogram_data.counts_[4], 2); // 25.1, 28.1 + EXPECT_EQ(histogram_data.counts_[7], 1); // 105.0 + + // Diff + auto aggr4 = aggr1.Diff(aggr2); + histogram_data = nostd::get(aggr4->ToPoint()); + EXPECT_EQ(histogram_data.count_, 2); // aggr2:5 - aggr1:3 + EXPECT_EQ(histogram_data.counts_[1], 1); // aggr2(2.0, 3.0) - aggr1(1.0) + EXPECT_EQ(histogram_data.counts_[3], 0); // aggr2(13.0) - aggr1(11.0) + EXPECT_EQ(histogram_data.counts_[4], 0); // aggr2(28.1) - aggr1(25.1) + EXPECT_EQ(histogram_data.counts_[7], 1); // aggr2(105.0) - aggr1(0) } -#endif \ No newline at end of file +#endif