From 85d43d60550fadf37f164234e6c44aab709bcde4 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Sat, 27 May 2023 01:34:40 -0700 Subject: [PATCH 01/16] Add ExponentialHistogramAggregation --- .../metrics/aggregation/aggregation_config.h | 9 + .../base2_exponential_histogram_aggregation.h | 54 +++++ .../metrics/aggregation/default_aggregation.h | 5 + .../sdk/metrics/data/metric_data.h | 7 +- .../sdk/metrics/data/point_data.h | 25 +- .../opentelemetry/sdk/metrics/instruments.h | 3 +- ...base2_exponential_histogram_aggregation.cc | 215 ++++++++++++++++++ .../histogram_aggregation_benchmark.cc | 57 ++++- 8 files changed, 366 insertions(+), 9 deletions(-) create mode 100644 sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h create mode 100644 sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index f5a48d7ddb..0cae37f39e 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -24,6 +24,15 @@ class HistogramAggregationConfig : public AggregationConfig std::vector boundaries_; bool record_min_max_ = true; }; + +class Base2ExponentialHistogramAggregationConfig : public AggregationConfig +{ +public: + size_t max_buckets_ = 160; + int32_t max_scale_ = 20; + bool record_min_max_ = true; +}; + } // namespace metrics } // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h new file mode 100644 index 0000000000..3ae51a9a31 --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h @@ -0,0 +1,54 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#pragma once + +#include "opentelemetry/common/spin_lock_mutex.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" +#include "opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_indexer.h" + +#include +#include + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +class Base2ExponentialHistogramAggregation : public Aggregation +{ +public: + Base2ExponentialHistogramAggregation(const AggregationConfig *aggregation_config = nullptr); + + void Aggregate(int64_t value, const PointAttributes &attributes = {}) noexcept override; + void Aggregate(double value, const PointAttributes &attributes = {}) noexcept override; + + /* Returns the result of merge of the existing aggregation with delta + * aggregation with same boundaries */ + 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. + */ + std::unique_ptr Diff(const Aggregation &next) const noexcept override; + + PointType ToPoint() const noexcept override; + +private: + Base2ExponentialHistogramAggregation(ExponentialHistogramPointData point_data); + + void AggregateIntoBuckets(AdaptingCircularBufferCounter *buckets, double value) noexcept; + void Downscale(uint32_t by) noexcept; + + mutable opentelemetry::common::SpinLockMutex lock_; + ExponentialHistogramPointData point_data_; + Base2ExponentialHistogramIndexer indexer_; +}; + +} // namespace metrics +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h index 9b1b1c2d7d..7525ee639d 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h @@ -6,6 +6,7 @@ #include #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" +#include "opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/drop_aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/lastvalue_aggregation.h" @@ -80,6 +81,10 @@ class DefaultAggregation return std::unique_ptr(new DoubleHistogramAggregation(aggregation_config)); } break; + case AggregationType::kBase2ExponentialHistogram: + return std::unique_ptr( + new Base2ExponentialHistogramAggregation(aggregation_config)); + break; case AggregationType::kLastValue: if (instrument_descriptor.value_type_ == InstrumentValueType::kLong) { diff --git a/sdk/include/opentelemetry/sdk/metrics/data/metric_data.h b/sdk/include/opentelemetry/sdk/metrics/data/metric_data.h index ad1084f581..0c860d3b37 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/metric_data.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/metric_data.h @@ -18,8 +18,11 @@ namespace metrics { using PointAttributes = opentelemetry::sdk::common::OrderedAttributeMap; -using PointType = opentelemetry::nostd:: - variant; +using PointType = opentelemetry::nostd::variant; struct PointDataAttributes { diff --git a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h index 32853316a5..ee97a61460 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h @@ -17,7 +17,8 @@ namespace metrics using ValueType = nostd::variant; -// TODO: remove ctors and initializers from below classes when GCC<5 stops shipping on Ubuntu +// TODO: remove ctors and initializers from below classes when GCC<5 stops +// shipping on Ubuntu class SumPointData { @@ -64,6 +65,28 @@ class HistogramPointData bool record_min_max_ = true; }; +class ExponentialHistogramPointData +{ +public: + // TODO: remove ctors and initializers when GCC<5 stops shipping on Ubuntu + ExponentialHistogramPointData(ExponentialHistogramPointData &&) = default; + ExponentialHistogramPointData &operator=(ExponentialHistogramPointData &&) = default; + ExponentialHistogramPointData(const ExponentialHistogramPointData &) = default; + ExponentialHistogramPointData() = default; + + uint64_t count_ = {}; + double sum_ = {}; + int32_t scale_ = {}; + uint64_t zero_count_ = {}; + AdaptingCircularBufferCounter positive_buckets_{0}; + AdaptingCircularBufferCounter negative_buckets_{0}; + double min_ = {}; + double max_ = {}; + double zero_threshold_ = {}; + bool record_min_max_ = true; + size_t max_buckets_ = {}; +}; + class DropPointData { public: diff --git a/sdk/include/opentelemetry/sdk/metrics/instruments.h b/sdk/include/opentelemetry/sdk/metrics/instruments.h index 6473a267f2..9e3c843f5e 100644 --- a/sdk/include/opentelemetry/sdk/metrics/instruments.h +++ b/sdk/include/opentelemetry/sdk/metrics/instruments.h @@ -45,7 +45,8 @@ enum class AggregationType kHistogram, kLastValue, kSum, - kDefault + kDefault, + kBase2ExponentialHistogram }; enum class AggregationTemporality diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc new file mode 100644 index 0000000000..7640e4d28b --- /dev/null +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -0,0 +1,215 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include "opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h" +#include "opentelemetry/sdk/metrics/aggregation/aggregation.h" +#include "opentelemetry/sdk/metrics/data/circular_buffer.h" +#include "opentelemetry/version.h" + +#include +#include +#include +#include +#include +#include + +#include + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ + +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) + { + start_index >>= 1; + end_index >>= 1; + scale_reduction++; + } + return scale_reduction; +} + +void DownscaleBuckets(AdaptingCircularBufferCounter *buckets, uint32_t by) noexcept +{ + if (buckets->Empty()) + { + return; + } + + // We want to preserve other optimisations here as well, e.g. integer size. + // Instead of creating a new counter, we copy the existing one (for bucket size + // optimisations), and clear the values before writing the new ones. + AdaptingCircularBufferCounter new_buckets = *buckets; + new_buckets.Clear(); + + for (int i = buckets->StartIndex(); i <= buckets->EndIndex(); i++) + { + const uint64_t count = buckets->Get(i); + if (count > 0) + { + buckets->Increment(i >> by, count); + } + } + *buckets = std::move(new_buckets); +} + +} // namespace + +Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( + const AggregationConfig *aggregation_config) +{ + const Base2ExponentialHistogramAggregationConfig default_config; + auto ac = static_cast(aggregation_config); + if (!ac) + { + ac = &default_config; + } + + point_data_.max_buckets_ = ac->max_buckets_; + point_data_.scale_ = ac->max_scale_; + point_data_.record_min_max_ = ac->record_min_max_; + point_data_.min_ = std::numeric_limits::max(); + point_data_.max_ = std::numeric_limits::min(); + + indexer_ = Base2ExponentialHistogramIndexer(point_data_.scale_); +} + +Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( + ExponentialHistogramPointData point_data) + : point_data_{std::move(point_data)}, indexer_(point_data.scale_) +{} + +void Base2ExponentialHistogramAggregation::Aggregate( + int64_t value, + const PointAttributes & /* attributes */) noexcept +{ + Aggregate(double(value)); +} + +void Base2ExponentialHistogramAggregation::Aggregate( + double value, + const PointAttributes & /* attributes */) noexcept +{ + const std::lock_guard locked(lock_); + point_data_.sum_ += value; + point_data_.min_ = std::min(point_data_.min_, value); + point_data_.max_ = std::max(point_data_.max_, value); + point_data_.count_++; + + if (value == 0) + { + point_data_.zero_count_++; + return; + } + AggregateIntoBuckets(value > 0 ? &point_data_.positive_buckets_ : &point_data_.negative_buckets_, + value); +} + +void Base2ExponentialHistogramAggregation::AggregateIntoBuckets( + AdaptingCircularBufferCounter *buckets, + double value) noexcept +{ + if (buckets->MaxSize() == 0) + { + *buckets = AdaptingCircularBufferCounter{point_data_.max_buckets_}; + } + + const int32_t index = indexer_.ComputeIndex(value); + if (!buckets->Increment(index, 1)) + { + const int32_t start_index = std::min(buckets->StartIndex(), index); + const int32_t end_index = std::max(buckets->EndIndex(), index); + const uint32_t scale_reduction = + GetScaleReduction(start_index, end_index, point_data_.max_buckets_); + Downscale(scale_reduction); + + buckets->Increment(index >> scale_reduction, 1); + } +} + +void Base2ExponentialHistogramAggregation::Downscale(uint32_t by) noexcept +{ + if (by == 0) + { + return; + } + + DownscaleBuckets(&point_data_.positive_buckets_, by); + DownscaleBuckets(&point_data_.negative_buckets_, by); + + // std::cerr << "[" << std::hex << this << "] downscale from " << std::dec << point_data_.scale_ + // << " by " << by << ", count: " << point_data_.count_ << std::endl; + point_data_.scale_ -= by; + indexer_ = Base2ExponentialHistogramIndexer(point_data_.scale_); +} + +std::unique_ptr Base2ExponentialHistogramAggregation::Merge( + const Aggregation &delta) const noexcept +{ + auto curr_value = nostd::get(ToPoint()); + auto delta_value = nostd::get( + (static_cast(delta).ToPoint())); + Base2ExponentialHistogramAggregationConfig agg_config; + agg_config.max_scale_ = std::min(curr_value.scale_, delta_value.scale_); + agg_config.max_buckets_ = curr_value.max_buckets_; + agg_config.record_min_max_ = curr_value.record_min_max_ && delta_value.record_min_max_; + std::unique_ptr result{ + new Base2ExponentialHistogramAggregation(&agg_config)}; + + result->point_data_.count_ = curr_value.count_ + delta_value.count_; + result->point_data_.sum_ = curr_value.sum_ + delta_value.sum_; + result->point_data_.zero_count_ = curr_value.zero_count_ + delta_value.zero_count_; + result->point_data_.min_ = std::min(curr_value.min_, delta_value.min_); + result->point_data_.max_ = std::max(curr_value.max_, delta_value.max_); + result->point_data_.positive_buckets_ = std::move(curr_value.positive_buckets_); + // if (!delta_value.positive_buckets_.Empty()) + // { + // } + result->point_data_.negative_buckets_ = std::move(curr_value.negative_buckets_); + // if (!delta_value.negative_buckets_.Empty()) + // { + // for (int i = delta_value.negative_buckets_.StartIndex(); + // i <= delta_value.negative_buckets_.EndIndex(); i++) + // { + // result->point_data_.negative_buckets_.Increment(i, delta); + // } + // } + + return result; +} + +std::unique_ptr Base2ExponentialHistogramAggregation::Diff( + const Aggregation &next) const noexcept +{ + auto curr_value = nostd::get(ToPoint()); + auto next_value = nostd::get( + (static_cast(next).ToPoint())); + Base2ExponentialHistogramAggregationConfig agg_config; + agg_config.max_scale_ = std::min(curr_value.scale_, next_value.scale_); + agg_config.max_buckets_ = curr_value.max_buckets_; + agg_config.record_min_max_ = false; + std::unique_ptr result{ + new Base2ExponentialHistogramAggregation(&agg_config)}; + + result->point_data_.count_ = next_value.count_ - curr_value.count_; + result->point_data_.sum_ = next_value.sum_ - curr_value.sum_; + result->point_data_.zero_count_ = next_value.zero_count_ - curr_value.zero_count_; + return result; +} + +PointType Base2ExponentialHistogramAggregation::ToPoint() const noexcept +{ + const std::lock_guard locked(lock_); + return point_data_; +} + +} // namespace metrics +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/test/metrics/histogram_aggregation_benchmark.cc b/sdk/test/metrics/histogram_aggregation_benchmark.cc index b66538042c..1b84670f82 100644 --- a/sdk/test/metrics/histogram_aggregation_benchmark.cc +++ b/sdk/test/metrics/histogram_aggregation_benchmark.cc @@ -25,6 +25,8 @@ #include "opentelemetry/sdk/metrics/meter_provider.h" #include "opentelemetry/sdk/metrics/metric_reader.h" #include "opentelemetry/sdk/metrics/push_metric_exporter.h" +#include "opentelemetry/sdk/metrics/view/instrument_selector.h" +#include "opentelemetry/sdk/metrics/view/view_registry.h" using namespace opentelemetry; using namespace opentelemetry::sdk::instrumentationscope; @@ -32,9 +34,11 @@ using namespace opentelemetry::sdk::metrics; namespace { -void BM_HistogramAggregation(benchmark::State &state) + +template +void HistogramAggregation(benchmark::State &state, std::unique_ptr views) { - MeterProvider mp; + MeterProvider mp(std::move(views)); auto m = mp.GetMeter("meter1", "version1", "schema1"); std::unique_ptr exporter(new MockMetricExporter()); @@ -50,7 +54,7 @@ void BM_HistogramAggregation(benchmark::State &state) { measurements[i] = static_cast(distribution(generator)); } - std::vector actuals; + std::vector actuals; std::vector collectionThreads; std::function collectMetrics = [&reader, &actuals]() { reader->Collect([&](ResourceMetrics &rm) { @@ -60,7 +64,7 @@ void BM_HistogramAggregation(benchmark::State &state) { for (const PointDataAttributes &dp : md.point_data_attr_) { - actuals.push_back(opentelemetry::nostd::get(dp.point_data)); + actuals.push_back(opentelemetry::nostd::get(dp.point_data)); } } } @@ -68,7 +72,7 @@ void BM_HistogramAggregation(benchmark::State &state) }); }; - while (state.KeepRunning()) + while (state.KeepRunningBatch(TOTAL_MEASUREMENTS)) { for (size_t i = 0; i < TOTAL_MEASUREMENTS; i++) { @@ -85,7 +89,50 @@ void BM_HistogramAggregation(benchmark::State &state) } } +void BM_HistogramAggregation(benchmark::State &state) +{ + std::unique_ptr views{new ViewRegistry()}; + HistogramAggregation(state, std::move(views)); +} + BENCHMARK(BM_HistogramAggregation); +void BM_Base2ExponentialHistogramAggregation(benchmark::State &state) +{ + std::unique_ptr histogram_instrument_selector{ + new InstrumentSelector(InstrumentType::kHistogram, ".*")}; + std::unique_ptr histogram_meter_selector{ + new MeterSelector("meter1", "version1", "schema1")}; + std::unique_ptr histogram_view{ + new View("base2_expohisto", "description", AggregationType::kBase2ExponentialHistogram)}; + + std::unique_ptr views{new ViewRegistry()}; + views->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), + std::move(histogram_view)); + HistogramAggregation(state, std::move(views)); +} + +BENCHMARK(BM_Base2ExponentialHistogramAggregation); + +void BM_Base2ExponentialHistogramAggregationZeroScale(benchmark::State &state) +{ + std::unique_ptr histogram_instrument_selector{ + new InstrumentSelector(InstrumentType::kHistogram, ".*")}; + std::unique_ptr histogram_meter_selector{ + new MeterSelector("meter1", "version1", "schema1")}; + Base2ExponentialHistogramAggregationConfig config; + config.max_scale_ = 0; + std::unique_ptr histogram_view{ + new View("base2_expohisto", "description", AggregationType::kBase2ExponentialHistogram, + std::make_shared(config))}; + + std::unique_ptr views{new ViewRegistry()}; + views->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), + std::move(histogram_view)); + HistogramAggregation(state, std::move(views)); +} + +BENCHMARK(BM_Base2ExponentialHistogramAggregationZeroScale); + } // namespace BENCHMARK_MAIN(); From 81208c0374fd414aa024e19111839c7ccf5b3654 Mon Sep 17 00:00:00 2001 From: Ruslan Nigmatullin Date: Fri, 15 Mar 2024 17:32:51 -0700 Subject: [PATCH 02/16] iterate --- .../base2_exponential_histogram_aggregation.h | 5 +- .../metrics/aggregation/default_aggregation.h | 3 + .../sdk/metrics/data/metric_data.h | 2 +- .../sdk/metrics/data/point_data.h | 10 +-- ...base2_exponential_histogram_aggregation.cc | 85 ++++++++++++------- .../histogram_aggregation_benchmark.cc | 4 +- 6 files changed, 66 insertions(+), 43 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h index 3ae51a9a31..a44e402dd7 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h @@ -21,6 +21,7 @@ class Base2ExponentialHistogramAggregation : public Aggregation { public: Base2ExponentialHistogramAggregation(const AggregationConfig *aggregation_config = nullptr); + Base2ExponentialHistogramAggregation(Base2ExponentialHistogramPointData point_data); void Aggregate(int64_t value, const PointAttributes &attributes = {}) noexcept override; void Aggregate(double value, const PointAttributes &attributes = {}) noexcept override; @@ -39,13 +40,11 @@ class Base2ExponentialHistogramAggregation : public Aggregation PointType ToPoint() const noexcept override; private: - Base2ExponentialHistogramAggregation(ExponentialHistogramPointData point_data); - void AggregateIntoBuckets(AdaptingCircularBufferCounter *buckets, double value) noexcept; void Downscale(uint32_t by) noexcept; mutable opentelemetry::common::SpinLockMutex lock_; - ExponentialHistogramPointData point_data_; + Base2ExponentialHistogramPointData point_data_; Base2ExponentialHistogramIndexer indexer_; }; diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h index 7525ee639d..70f6f8ee74 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h @@ -144,6 +144,9 @@ class DefaultAggregation return std::unique_ptr( new DoubleHistogramAggregation(nostd::get(point_data))); } + case AggregationType::kBase2ExponentialHistogram: + return std::unique_ptr(new Base2ExponentialHistogramAggregation( + nostd::get(point_data))); case AggregationType::kLastValue: if (instrument_descriptor.value_type_ == InstrumentValueType::kLong) { diff --git a/sdk/include/opentelemetry/sdk/metrics/data/metric_data.h b/sdk/include/opentelemetry/sdk/metrics/data/metric_data.h index 0c860d3b37..e1da0b7f95 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/metric_data.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/metric_data.h @@ -20,7 +20,7 @@ namespace metrics using PointAttributes = opentelemetry::sdk::common::OrderedAttributeMap; using PointType = opentelemetry::nostd::variant; diff --git a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h index ee97a61460..ff47ffaa9c 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h @@ -65,14 +65,14 @@ class HistogramPointData bool record_min_max_ = true; }; -class ExponentialHistogramPointData +class Base2ExponentialHistogramPointData { public: // TODO: remove ctors and initializers when GCC<5 stops shipping on Ubuntu - ExponentialHistogramPointData(ExponentialHistogramPointData &&) = default; - ExponentialHistogramPointData &operator=(ExponentialHistogramPointData &&) = default; - ExponentialHistogramPointData(const ExponentialHistogramPointData &) = default; - ExponentialHistogramPointData() = default; + Base2ExponentialHistogramPointData(Base2ExponentialHistogramPointData &&) = default; + Base2ExponentialHistogramPointData &operator=(Base2ExponentialHistogramPointData &&) = default; + Base2ExponentialHistogramPointData(const Base2ExponentialHistogramPointData &) = default; + Base2ExponentialHistogramPointData() = default; uint64_t count_ = {}; double sum_ = {}; diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index 7640e4d28b..b4cb550cc1 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -4,6 +4,7 @@ #include "opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" #include "opentelemetry/sdk/metrics/data/circular_buffer.h" +#include "opentelemetry/sdk/metrics/data/point_data.h" #include "opentelemetry/version.h" #include @@ -36,6 +37,20 @@ uint32_t GetScaleReduction(int32_t start_index, int32_t end_index, size_t max_bu return scale_reduction; } +uint32_t GetScaleReduction(const AdaptingCircularBufferCounter &first, + const AdaptingCircularBufferCounter &second, + size_t max_buckets) +{ + if (first.Empty() || second.Empty()) + { + return 0; + } + + const int32_t start_index = std::min(first.StartIndex(), second.StartIndex()); + const int32_t end_index = std::max(first.EndIndex(), second.EndIndex()); + return GetScaleReduction(start_index, end_index, max_buckets); +} + void DownscaleBuckets(AdaptingCircularBufferCounter *buckets, uint32_t by) noexcept { if (buckets->Empty()) @@ -46,6 +61,7 @@ void DownscaleBuckets(AdaptingCircularBufferCounter *buckets, uint32_t by) noexc // We want to preserve other optimisations here as well, e.g. integer size. // Instead of creating a new counter, we copy the existing one (for bucket size // optimisations), and clear the values before writing the new ones. + // TODO(euroelessar): Do downscaling in-place. AdaptingCircularBufferCounter new_buckets = *buckets; new_buckets.Clear(); @@ -82,7 +98,7 @@ Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( } Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( - ExponentialHistogramPointData point_data) + Base2ExponentialHistogramPointData point_data) : point_data_{std::move(point_data)}, indexer_(point_data.scale_) {} @@ -108,8 +124,14 @@ void Base2ExponentialHistogramAggregation::Aggregate( point_data_.zero_count_++; return; } - AggregateIntoBuckets(value > 0 ? &point_data_.positive_buckets_ : &point_data_.negative_buckets_, - value); + else if (value > 0) + { + AggregateIntoBuckets(&point_data_.positive_buckets_, value); + } + else + { + AggregateIntoBuckets(&point_data_.negative_buckets_, -value); + } } void Base2ExponentialHistogramAggregation::AggregateIntoBuckets( @@ -144,8 +166,6 @@ void Base2ExponentialHistogramAggregation::Downscale(uint32_t by) noexcept DownscaleBuckets(&point_data_.positive_buckets_, by); DownscaleBuckets(&point_data_.negative_buckets_, by); - // std::cerr << "[" << std::hex << this << "] downscale from " << std::dec << point_data_.scale_ - // << " by " << by << ", count: " << point_data_.count_ << std::endl; point_data_.scale_ -= by; indexer_ = Base2ExponentialHistogramIndexer(point_data_.scale_); } @@ -153,55 +173,56 @@ void Base2ExponentialHistogramAggregation::Downscale(uint32_t by) noexcept std::unique_ptr Base2ExponentialHistogramAggregation::Merge( const Aggregation &delta) const noexcept { - auto curr_value = nostd::get(ToPoint()); - auto delta_value = nostd::get( + auto curr_value = nostd::get(ToPoint()); + auto delta_value = nostd::get( (static_cast(delta).ToPoint())); Base2ExponentialHistogramAggregationConfig agg_config; agg_config.max_scale_ = std::min(curr_value.scale_, delta_value.scale_); agg_config.max_buckets_ = curr_value.max_buckets_; agg_config.record_min_max_ = curr_value.record_min_max_ && delta_value.record_min_max_; - std::unique_ptr result{ - new Base2ExponentialHistogramAggregation(&agg_config)}; - - result->point_data_.count_ = curr_value.count_ + delta_value.count_; - result->point_data_.sum_ = curr_value.sum_ + delta_value.sum_; - result->point_data_.zero_count_ = curr_value.zero_count_ + delta_value.zero_count_; - result->point_data_.min_ = std::min(curr_value.min_, delta_value.min_); - result->point_data_.max_ = std::max(curr_value.max_, delta_value.max_); - result->point_data_.positive_buckets_ = std::move(curr_value.positive_buckets_); + + Base2ExponentialHistogramPointData result_value; + result_value.count_ = curr_value.count_ + delta_value.count_; + result_value.sum_ = curr_value.sum_ + delta_value.sum_; + result_value.zero_count_ = curr_value.zero_count_ + delta_value.zero_count_; + result_value.min_ = std::min(curr_value.min_, delta_value.min_); + result_value.max_ = std::max(curr_value.max_, delta_value.max_); + result_value.positive_buckets_ = std::move(curr_value.positive_buckets_); + result_value.record_min_max_ = curr_value.record_min_max_ && delta_value.record_min_max_; // if (!delta_value.positive_buckets_.Empty()) // { // } - result->point_data_.negative_buckets_ = std::move(curr_value.negative_buckets_); + result_value.negative_buckets_ = std::move(curr_value.negative_buckets_); // if (!delta_value.negative_buckets_.Empty()) // { // for (int i = delta_value.negative_buckets_.StartIndex(); // i <= delta_value.negative_buckets_.EndIndex(); i++) // { - // result->point_data_.negative_buckets_.Increment(i, delta); + // result.negative_buckets_.Increment(i, delta); // } // } - return result; + return std::unique_ptr{ + new Base2ExponentialHistogramAggregation(std::move(result_value))}; } std::unique_ptr Base2ExponentialHistogramAggregation::Diff( const Aggregation &next) const noexcept { - auto curr_value = nostd::get(ToPoint()); - auto next_value = nostd::get( + auto curr_value = nostd::get(ToPoint()); + auto next_value = nostd::get( (static_cast(next).ToPoint())); - Base2ExponentialHistogramAggregationConfig agg_config; - agg_config.max_scale_ = std::min(curr_value.scale_, next_value.scale_); - agg_config.max_buckets_ = curr_value.max_buckets_; - agg_config.record_min_max_ = false; - std::unique_ptr result{ - new Base2ExponentialHistogramAggregation(&agg_config)}; - - result->point_data_.count_ = next_value.count_ - curr_value.count_; - result->point_data_.sum_ = next_value.sum_ - curr_value.sum_; - result->point_data_.zero_count_ = next_value.zero_count_ - curr_value.zero_count_; - return result; + + Base2ExponentialHistogramPointData result_value; + result_value.scale_ = curr_value.scale_; + result_value.max_buckets_ = curr_value.max_buckets_; + result_value.record_min_max_ = false; + result_value.count_ = next_value.count_ - curr_value.count_; + result_value.sum_ = next_value.sum_ - curr_value.sum_; + result_value.zero_count_ = next_value.zero_count_ - curr_value.zero_count_; + + return std::unique_ptr{ + new Base2ExponentialHistogramAggregation(std::move(result_value))}; } PointType Base2ExponentialHistogramAggregation::ToPoint() const noexcept diff --git a/sdk/test/metrics/histogram_aggregation_benchmark.cc b/sdk/test/metrics/histogram_aggregation_benchmark.cc index 1b84670f82..468a6ff220 100644 --- a/sdk/test/metrics/histogram_aggregation_benchmark.cc +++ b/sdk/test/metrics/histogram_aggregation_benchmark.cc @@ -109,7 +109,7 @@ void BM_Base2ExponentialHistogramAggregation(benchmark::State &state) std::unique_ptr views{new ViewRegistry()}; views->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), std::move(histogram_view)); - HistogramAggregation(state, std::move(views)); + HistogramAggregation(state, std::move(views)); } BENCHMARK(BM_Base2ExponentialHistogramAggregation); @@ -129,7 +129,7 @@ void BM_Base2ExponentialHistogramAggregationZeroScale(benchmark::State &state) std::unique_ptr views{new ViewRegistry()}; views->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), std::move(histogram_view)); - HistogramAggregation(state, std::move(views)); + HistogramAggregation(state, std::move(views)); } BENCHMARK(BM_Base2ExponentialHistogramAggregationZeroScale); From a68bd4b9dc1d2952acd4f4992a0b0bad04c249bc Mon Sep 17 00:00:00 2001 From: "Felipe C. Dos Santos" Date: Wed, 2 Apr 2025 13:42:09 +0000 Subject: [PATCH 03/16] Fix rebase and compile errors --- .../opentelemetry/sdk/metrics/data/point_data.h | 1 + sdk/src/metrics/CMakeLists.txt | 1 + sdk/test/metrics/histogram_aggregation_benchmark.cc | 10 ++++++---- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h index ff47ffaa9c..cc84a3af8d 100644 --- a/sdk/include/opentelemetry/sdk/metrics/data/point_data.h +++ b/sdk/include/opentelemetry/sdk/metrics/data/point_data.h @@ -7,6 +7,7 @@ #include "opentelemetry/common/timestamp.h" #include "opentelemetry/nostd/variant.h" +#include "opentelemetry/sdk/metrics/data/circular_buffer.h" #include "opentelemetry/version.h" OPENTELEMETRY_BEGIN_NAMESPACE diff --git a/sdk/src/metrics/CMakeLists.txt b/sdk/src/metrics/CMakeLists.txt index f1ba16a116..e1c8b6e0e9 100644 --- a/sdk/src/metrics/CMakeLists.txt +++ b/sdk/src/metrics/CMakeLists.txt @@ -20,6 +20,7 @@ add_library( state/observable_registry.cc state/sync_metric_storage.cc state/temporal_metric_storage.cc + aggregation/base2_exponential_histogram_aggregation.cc aggregation/base2_exponential_histogram_indexer.cc aggregation/histogram_aggregation.cc aggregation/lastvalue_aggregation.cc diff --git a/sdk/test/metrics/histogram_aggregation_benchmark.cc b/sdk/test/metrics/histogram_aggregation_benchmark.cc index 468a6ff220..4af6024d8b 100644 --- a/sdk/test/metrics/histogram_aggregation_benchmark.cc +++ b/sdk/test/metrics/histogram_aggregation_benchmark.cc @@ -99,12 +99,13 @@ BENCHMARK(BM_HistogramAggregation); void BM_Base2ExponentialHistogramAggregation(benchmark::State &state) { + std::string instrument_unit = "ms"; std::unique_ptr histogram_instrument_selector{ - new InstrumentSelector(InstrumentType::kHistogram, ".*")}; + new InstrumentSelector(InstrumentType::kHistogram, ".*", instrument_unit)}; std::unique_ptr histogram_meter_selector{ new MeterSelector("meter1", "version1", "schema1")}; std::unique_ptr histogram_view{ - new View("base2_expohisto", "description", AggregationType::kBase2ExponentialHistogram)}; + new View("base2_expohisto", "description", instrument_unit, AggregationType::kBase2ExponentialHistogram)}; std::unique_ptr views{new ViewRegistry()}; views->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), @@ -116,14 +117,15 @@ BENCHMARK(BM_Base2ExponentialHistogramAggregation); void BM_Base2ExponentialHistogramAggregationZeroScale(benchmark::State &state) { + std::string instrument_unit = "ms"; std::unique_ptr histogram_instrument_selector{ - new InstrumentSelector(InstrumentType::kHistogram, ".*")}; + new InstrumentSelector(InstrumentType::kHistogram, ".*", instrument_unit)}; std::unique_ptr histogram_meter_selector{ new MeterSelector("meter1", "version1", "schema1")}; Base2ExponentialHistogramAggregationConfig config; config.max_scale_ = 0; std::unique_ptr histogram_view{ - new View("base2_expohisto", "description", AggregationType::kBase2ExponentialHistogram, + new View("base2_expohisto", "description", instrument_unit, AggregationType::kBase2ExponentialHistogram, std::make_shared(config))}; std::unique_ptr views{new ViewRegistry()}; From 19fbe9d44c9a8521d81b8499d04aa97ccd334670 Mon Sep 17 00:00:00 2001 From: "Felipe C. Dos Santos" Date: Wed, 2 Apr 2025 15:07:42 +0000 Subject: [PATCH 04/16] benchmark: Changed the aggregate unit be the same as the histogram --- sdk/test/metrics/histogram_aggregation_benchmark.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/test/metrics/histogram_aggregation_benchmark.cc b/sdk/test/metrics/histogram_aggregation_benchmark.cc index 4af6024d8b..7e06f150ab 100644 --- a/sdk/test/metrics/histogram_aggregation_benchmark.cc +++ b/sdk/test/metrics/histogram_aggregation_benchmark.cc @@ -99,7 +99,7 @@ BENCHMARK(BM_HistogramAggregation); void BM_Base2ExponentialHistogramAggregation(benchmark::State &state) { - std::string instrument_unit = "ms"; + std::string instrument_unit = "histogram1_unit"; std::unique_ptr histogram_instrument_selector{ new InstrumentSelector(InstrumentType::kHistogram, ".*", instrument_unit)}; std::unique_ptr histogram_meter_selector{ @@ -117,7 +117,7 @@ BENCHMARK(BM_Base2ExponentialHistogramAggregation); void BM_Base2ExponentialHistogramAggregationZeroScale(benchmark::State &state) { - std::string instrument_unit = "ms"; + std::string instrument_unit = "histogram1_unit"; std::unique_ptr histogram_instrument_selector{ new InstrumentSelector(InstrumentType::kHistogram, ".*", instrument_unit)}; std::unique_ptr histogram_meter_selector{ From 849316fcf26415a860c69cd2afd45cbe1a66e3ed Mon Sep 17 00:00:00 2001 From: etmcdonald Date: Wed, 2 Apr 2025 21:21:59 +0000 Subject: [PATCH 05/16] change bucket logic to avoid overwriting existing buckets. add initial expo histo test case --- ...base2_exponential_histogram_aggregation.cc | 4 +- sdk/test/metrics/aggregation_test.cc | 43 +++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index b4cb550cc1..e686fdd359 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -70,7 +70,7 @@ void DownscaleBuckets(AdaptingCircularBufferCounter *buckets, uint32_t by) noexc const uint64_t count = buckets->Get(i); if (count > 0) { - buckets->Increment(i >> by, count); + new_buckets.Increment(i >> by, count); } } *buckets = std::move(new_buckets); @@ -83,7 +83,7 @@ Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( { const Base2ExponentialHistogramAggregationConfig default_config; auto ac = static_cast(aggregation_config); - if (!ac) + if (!ac) { ac = &default_config; } diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index 92855863a8..8b3182fabe 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -11,6 +11,7 @@ #include "opentelemetry/sdk/metrics/aggregation/aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/aggregation_config.h" #include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" +#include "opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/lastvalue_aggregation.h" #include "opentelemetry/sdk/metrics/aggregation/sum_aggregation.h" #include "opentelemetry/sdk/metrics/data/point_data.h" @@ -223,3 +224,45 @@ TEST(Aggregation, DoubleHistogramAggregation) 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) } + +TEST(aggregation, Base2ExponentialHistogramAggregation) +{ + auto MAX_SCALE = 20; + auto MAX_BUCKETS = 100; + Base2ExponentialHistogramAggregationConfig config; + config.max_scale_ = MAX_SCALE; + config.max_buckets_ = MAX_BUCKETS; + config.record_min_max_ = true; + Base2ExponentialHistogramAggregation aggr = Base2ExponentialHistogramAggregation(&config); + auto point = aggr.ToPoint(); + ASSERT_TRUE(nostd::holds_alternative(point)); + auto histo_point = nostd::get(point); + EXPECT_EQ(histo_point.count_, 0); + EXPECT_EQ(histo_point.sum_, 0.0); + EXPECT_EQ(histo_point.zero_count_, 0); + EXPECT_EQ(histo_point.min_, std::numeric_limits::max()); + EXPECT_EQ(histo_point.max_, std::numeric_limits::min()); + EXPECT_EQ(histo_point.scale_, MAX_SCALE); + EXPECT_EQ(histo_point.max_buckets_, MAX_BUCKETS); + ASSERT_TRUE(histo_point.positive_buckets_.Empty()); + + aggr.Aggregate(0.0, {}); + histo_point = nostd::get(aggr.ToPoint()); + EXPECT_EQ(histo_point.count_, 1); + EXPECT_EQ(histo_point.zero_count_, 1); + + aggr.Aggregate(3.0, {}); + aggr.Aggregate(4.5, {}); + histo_point = nostd::get(aggr.ToPoint()); + EXPECT_EQ(histo_point.count_, 3); + EXPECT_EQ(histo_point.sum_, 7.5); + EXPECT_EQ(histo_point.min_, 0.0); + EXPECT_EQ(histo_point.max_, 4.5); + ASSERT_FALSE(histo_point.positive_buckets_.Empty()); + auto start_index = histo_point.positive_buckets_.StartIndex(); + auto end_index = histo_point.positive_buckets_.EndIndex(); + EXPECT_EQ(start_index, 202); + EXPECT_EQ(end_index, 277); + EXPECT_EQ(histo_point.positive_buckets_.Get(start_index), 1); + EXPECT_EQ(histo_point.positive_buckets_.Get(end_index), 1); +} \ No newline at end of file From 7171c497d0a419f91796cf4faef7621ce46d55ec Mon Sep 17 00:00:00 2001 From: etmcdonald Date: Thu, 3 Apr 2025 06:13:52 +0000 Subject: [PATCH 06/16] add merge --- ...base2_exponential_histogram_aggregation.cc | 67 +++++++++------ sdk/test/metrics/aggregation_test.cc | 81 ++++++++++++++----- 2 files changed, 102 insertions(+), 46 deletions(-) diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index e686fdd359..2746841696 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -173,34 +173,51 @@ void Base2ExponentialHistogramAggregation::Downscale(uint32_t by) noexcept std::unique_ptr Base2ExponentialHistogramAggregation::Merge( const Aggregation &delta) const noexcept { - auto curr_value = nostd::get(ToPoint()); - auto delta_value = nostd::get( + auto left = nostd::get(ToPoint()); + auto right = nostd::get( (static_cast(delta).ToPoint())); - Base2ExponentialHistogramAggregationConfig agg_config; - agg_config.max_scale_ = std::min(curr_value.scale_, delta_value.scale_); - agg_config.max_buckets_ = curr_value.max_buckets_; - agg_config.record_min_max_ = curr_value.record_min_max_ && delta_value.record_min_max_; + + auto low_res = left.scale_ < right.scale_ ? left : right; + auto high_res = left.scale_ < right.scale_ ? right : left; + auto scale_reduction = GetScaleReduction(low_res.positive_buckets_, high_res.positive_buckets_, + low_res.max_buckets_); + if (scale_reduction > 0) + { + DownscaleBuckets(&high_res.positive_buckets_, scale_reduction); + DownscaleBuckets(&high_res.negative_buckets_, scale_reduction); + high_res.scale_ -= scale_reduction; + } + + // Merge the two histograms Base2ExponentialHistogramPointData result_value; - result_value.count_ = curr_value.count_ + delta_value.count_; - result_value.sum_ = curr_value.sum_ + delta_value.sum_; - result_value.zero_count_ = curr_value.zero_count_ + delta_value.zero_count_; - result_value.min_ = std::min(curr_value.min_, delta_value.min_); - result_value.max_ = std::max(curr_value.max_, delta_value.max_); - result_value.positive_buckets_ = std::move(curr_value.positive_buckets_); - result_value.record_min_max_ = curr_value.record_min_max_ && delta_value.record_min_max_; - // if (!delta_value.positive_buckets_.Empty()) - // { - // } - result_value.negative_buckets_ = std::move(curr_value.negative_buckets_); - // if (!delta_value.negative_buckets_.Empty()) - // { - // for (int i = delta_value.negative_buckets_.StartIndex(); - // i <= delta_value.negative_buckets_.EndIndex(); i++) - // { - // result.negative_buckets_.Increment(i, delta); - // } - // } + result_value.count_ = low_res.count_ + high_res.count_; + result_value.sum_ = low_res.sum_ + high_res.sum_; + result_value.zero_count_ = low_res.zero_count_ + high_res.zero_count_; + result_value.min_ = std::min(low_res.min_, high_res.min_); + result_value.max_ = std::max(low_res.max_, high_res.max_); + result_value.scale_ = std::min(low_res.scale_, high_res.scale_); + result_value.max_buckets_ = low_res.max_buckets_; + result_value.record_min_max_ = low_res.record_min_max_ && high_res.record_min_max_; + if (!high_res.positive_buckets_.Empty()) + { + for (int i = high_res.positive_buckets_.StartIndex(); + i <= high_res.positive_buckets_.EndIndex(); i++) + { + low_res.positive_buckets_.Increment(i, high_res.positive_buckets_.Get(i)); + } + } + result_value.positive_buckets_ = std::move(low_res.positive_buckets_); + + if (!high_res.negative_buckets_.Empty()) + { + for (int i = high_res.negative_buckets_.StartIndex(); + i <= high_res.negative_buckets_.EndIndex(); i++) + { + low_res.negative_buckets_.Increment(i, high_res.negative_buckets_.Get(i)); + } + } + result_value.negative_buckets_ = std::move(low_res.negative_buckets_); return std::unique_ptr{ new Base2ExponentialHistogramAggregation(std::move(result_value))}; diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index 8b3182fabe..b46351937a 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -227,14 +227,15 @@ TEST(Aggregation, DoubleHistogramAggregation) TEST(aggregation, Base2ExponentialHistogramAggregation) { - auto MAX_SCALE = 20; - auto MAX_BUCKETS = 100; - Base2ExponentialHistogramAggregationConfig config; - config.max_scale_ = MAX_SCALE; - config.max_buckets_ = MAX_BUCKETS; - config.record_min_max_ = true; - Base2ExponentialHistogramAggregation aggr = Base2ExponentialHistogramAggregation(&config); - auto point = aggr.ToPoint(); + // Low res histo + auto SCALE0 = 0; + auto MAX_BUCKETS0 = 7; + Base2ExponentialHistogramAggregationConfig scale0_config; + scale0_config.max_scale_ = SCALE0; + scale0_config.max_buckets_ = MAX_BUCKETS0; + scale0_config.record_min_max_ = true; + Base2ExponentialHistogramAggregation scale0_aggr = Base2ExponentialHistogramAggregation(&scale0_config); + auto point = scale0_aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(point)); auto histo_point = nostd::get(point); EXPECT_EQ(histo_point.count_, 0); @@ -242,27 +243,65 @@ TEST(aggregation, Base2ExponentialHistogramAggregation) EXPECT_EQ(histo_point.zero_count_, 0); EXPECT_EQ(histo_point.min_, std::numeric_limits::max()); EXPECT_EQ(histo_point.max_, std::numeric_limits::min()); - EXPECT_EQ(histo_point.scale_, MAX_SCALE); - EXPECT_EQ(histo_point.max_buckets_, MAX_BUCKETS); + EXPECT_EQ(histo_point.scale_, SCALE0); + EXPECT_EQ(histo_point.max_buckets_, MAX_BUCKETS0); ASSERT_TRUE(histo_point.positive_buckets_.Empty()); - aggr.Aggregate(0.0, {}); - histo_point = nostd::get(aggr.ToPoint()); + // zero point + scale0_aggr.Aggregate(0.0, {}); + histo_point = nostd::get(scale0_aggr.ToPoint()); EXPECT_EQ(histo_point.count_, 1); EXPECT_EQ(histo_point.zero_count_, 1); - aggr.Aggregate(3.0, {}); - aggr.Aggregate(4.5, {}); - histo_point = nostd::get(aggr.ToPoint()); + // Two recordings in the same bucket (bucket 1 at scale 0) + scale0_aggr.Aggregate(3.0, {}); + scale0_aggr.Aggregate(3.5, {}); + histo_point = nostd::get(scale0_aggr.ToPoint()); EXPECT_EQ(histo_point.count_, 3); - EXPECT_EQ(histo_point.sum_, 7.5); + EXPECT_EQ(histo_point.sum_, 6.5); EXPECT_EQ(histo_point.min_, 0.0); - EXPECT_EQ(histo_point.max_, 4.5); + EXPECT_EQ(histo_point.max_, 3.5); ASSERT_FALSE(histo_point.positive_buckets_.Empty()); auto start_index = histo_point.positive_buckets_.StartIndex(); auto end_index = histo_point.positive_buckets_.EndIndex(); - EXPECT_EQ(start_index, 202); - EXPECT_EQ(end_index, 277); - EXPECT_EQ(histo_point.positive_buckets_.Get(start_index), 1); - EXPECT_EQ(histo_point.positive_buckets_.Get(end_index), 1); + EXPECT_EQ(start_index, 1); + EXPECT_EQ(end_index, 1); + EXPECT_EQ(histo_point.positive_buckets_.Get(start_index), 2); + + // Recording in a different bucket (bucket -2 at scale 0) + scale0_aggr.Aggregate(-0.3, {}); + histo_point = nostd::get(scale0_aggr.ToPoint()); + EXPECT_EQ(histo_point.count_, 4); + EXPECT_EQ(histo_point.sum_, 6.2); + EXPECT_EQ(histo_point.min_, -0.3); + EXPECT_EQ(histo_point.max_, 3.5); + EXPECT_EQ(histo_point.negative_buckets_.Get(-2), 1); + EXPECT_EQ(histo_point.positive_buckets_.Get(1), 2); + + Base2ExponentialHistogramAggregationConfig scale1_config; + scale1_config.max_scale_ = 1; + scale1_config.max_buckets_ = 14; + scale1_config.record_min_max_ = true; + Base2ExponentialHistogramAggregation scale1_aggr; + + scale1_aggr.Aggregate(0.0, {}); + scale1_aggr.Aggregate(3.0, {}); + scale1_aggr.Aggregate(3.5, {}); + scale1_aggr.Aggregate(0.3, {}); + auto scale1_point = nostd::get(scale1_aggr.ToPoint()); + EXPECT_EQ(scale1_point.count_, 4); + EXPECT_EQ(scale1_point.sum_, 6.8); + EXPECT_EQ(scale1_point.zero_count_, 1); + EXPECT_EQ(scale1_point.min_, 0.0); + EXPECT_EQ(scale1_point.max_, 3.5); + + auto merged = scale0_aggr.Merge(scale1_aggr); + auto merged_point = nostd::get(merged->ToPoint()); + EXPECT_EQ(merged_point.count_, 8); + EXPECT_EQ(merged_point.sum_, 13.0); + EXPECT_EQ(merged_point.zero_count_, 2); + EXPECT_EQ(merged_point.min_, -0.3); + EXPECT_EQ(merged_point.max_, 3.5); + EXPECT_EQ(merged_point.scale_, 0); + EXPECT_EQ(merged_point.positive_buckets_.Get(1), 4); } \ No newline at end of file From 223ca7b98a3aaa7864bdbe4390e39fa4010c26a8 Mon Sep 17 00:00:00 2001 From: etmcdonald Date: Thu, 3 Apr 2025 06:28:53 +0000 Subject: [PATCH 07/16] update merge test --- .../aggregation/base2_exponential_histogram_aggregation.cc | 3 +-- sdk/test/metrics/aggregation_test.cc | 6 ++++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index 2746841696..0b1d5f893e 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -179,8 +179,7 @@ std::unique_ptr Base2ExponentialHistogramAggregation::Merge( auto low_res = left.scale_ < right.scale_ ? left : right; auto high_res = left.scale_ < right.scale_ ? right : left; - auto scale_reduction = GetScaleReduction(low_res.positive_buckets_, high_res.positive_buckets_, - low_res.max_buckets_); + auto scale_reduction = high_res.scale_ - low_res.scale_; if (scale_reduction > 0) { diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index b46351937a..2254dffca1 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -282,7 +282,7 @@ TEST(aggregation, Base2ExponentialHistogramAggregation) scale1_config.max_scale_ = 1; scale1_config.max_buckets_ = 14; scale1_config.record_min_max_ = true; - Base2ExponentialHistogramAggregation scale1_aggr; + Base2ExponentialHistogramAggregation scale1_aggr = Base2ExponentialHistogramAggregation(&scale1_config); scale1_aggr.Aggregate(0.0, {}); scale1_aggr.Aggregate(3.0, {}); @@ -303,5 +303,7 @@ TEST(aggregation, Base2ExponentialHistogramAggregation) EXPECT_EQ(merged_point.min_, -0.3); EXPECT_EQ(merged_point.max_, 3.5); EXPECT_EQ(merged_point.scale_, 0); - EXPECT_EQ(merged_point.positive_buckets_.Get(1), 4); + EXPECT_EQ(merged_point.positive_buckets_.Get(1), 4); + EXPECT_EQ(merged_point.negative_buckets_.Get(-2), 1); + EXPECT_EQ(merged_point.positive_buckets_.Get(2), 0); } \ No newline at end of file From 70323b4258267980dc840c20aa813a17014115ce Mon Sep 17 00:00:00 2001 From: etmcdonald Date: Thu, 3 Apr 2025 15:08:02 +0000 Subject: [PATCH 08/16] add diff --- ...base2_exponential_histogram_aggregation.cc | 67 ++++++++++++++++--- sdk/test/metrics/aggregation_test.cc | 10 +++ 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index 0b1d5f893e..69dc688af4 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -188,7 +188,6 @@ std::unique_ptr Base2ExponentialHistogramAggregation::Merge( high_res.scale_ -= scale_reduction; } - // Merge the two histograms Base2ExponentialHistogramPointData result_value; result_value.count_ = low_res.count_ + high_res.count_; result_value.sum_ = low_res.sum_ + high_res.sum_; @@ -223,24 +222,74 @@ std::unique_ptr Base2ExponentialHistogramAggregation::Merge( } std::unique_ptr Base2ExponentialHistogramAggregation::Diff( - const Aggregation &next) const noexcept + const Aggregation &next) const noexcept { - auto curr_value = nostd::get(ToPoint()); - auto next_value = nostd::get( + auto left = nostd::get(ToPoint()); + auto right = nostd::get( (static_cast(next).ToPoint())); + + auto low_res = left.scale_ < right.scale_ ? left : right; + auto high_res = left.scale_ < right.scale_ ? right : left; + auto scale_reduction = high_res.scale_ - low_res.scale_; + + if (scale_reduction > 0) + { + DownscaleBuckets(&high_res.positive_buckets_, scale_reduction); + DownscaleBuckets(&high_res.negative_buckets_, scale_reduction); + high_res.scale_ -= scale_reduction; + } Base2ExponentialHistogramPointData result_value; - result_value.scale_ = curr_value.scale_; - result_value.max_buckets_ = curr_value.max_buckets_; + result_value.scale_ = low_res.scale_; + result_value.max_buckets_ = low_res.max_buckets_; result_value.record_min_max_ = false; - result_value.count_ = next_value.count_ - curr_value.count_; - result_value.sum_ = next_value.sum_ - curr_value.sum_; - result_value.zero_count_ = next_value.zero_count_ - curr_value.zero_count_; + // caution for underflow + result_value.count_ = (left.count_ >= right.count_) ? (left.count_ - right.count_) : 0; + result_value.sum_ = (left.sum_ >= right.sum_) ? (left.sum_ - right.sum_) : 0.0; + result_value.zero_count_ = (left.zero_count_ >= right.zero_count_) ? (left.zero_count_ - right.zero_count_) : 0; + if (!high_res.positive_buckets_.Empty()) + { + for (int i = high_res.positive_buckets_.StartIndex(); + i <= high_res.positive_buckets_.EndIndex(); i++) + { + low_res.positive_buckets_.Increment(i, 0-high_res.positive_buckets_.Get(i)); + } + } + result_value.positive_buckets_ = std::move(low_res.positive_buckets_); + + if (!high_res.negative_buckets_.Empty()) + { + for (int i = high_res.negative_buckets_.StartIndex(); + i <= high_res.negative_buckets_.EndIndex(); i++) + { + low_res.negative_buckets_.Increment(i, 0-high_res.negative_buckets_.Get(i)); + } + } + result_value.negative_buckets_ = std::move(low_res.negative_buckets_); return std::unique_ptr{ new Base2ExponentialHistogramAggregation(std::move(result_value))}; } +// std::unique_ptr Base2ExponentialHistogramAggregation::Diff( +// const Aggregation &next) const noexcept +// { +// auto curr_value = nostd::get(ToPoint()); +// auto next_value = nostd::get( +// (static_cast(next).ToPoint())); + +// Base2ExponentialHistogramPointData result_value; +// result_value.scale_ = curr_value.scale_; +// result_value.max_buckets_ = curr_value.max_buckets_; +// result_value.record_min_max_ = false; +// result_value.count_ = next_value.count_ - curr_value.count_; +// result_value.sum_ = next_value.sum_ - curr_value.sum_; +// result_value.zero_count_ = next_value.zero_count_ - curr_value.zero_count_; + +// return std::unique_ptr{ +// new Base2ExponentialHistogramAggregation(std::move(result_value))}; +// } + PointType Base2ExponentialHistogramAggregation::ToPoint() const noexcept { const std::lock_guard locked(lock_); diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index 2254dffca1..a439979c7b 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -306,4 +306,14 @@ TEST(aggregation, Base2ExponentialHistogramAggregation) EXPECT_EQ(merged_point.positive_buckets_.Get(1), 4); EXPECT_EQ(merged_point.negative_buckets_.Get(-2), 1); EXPECT_EQ(merged_point.positive_buckets_.Get(2), 0); + + auto diffd = merged->Diff(scale1_aggr); + auto diffd_point = nostd::get(diffd->ToPoint()); + EXPECT_EQ(diffd_point.count_, 4); + EXPECT_EQ(diffd_point.sum_, 6.2); + EXPECT_EQ(diffd_point.zero_count_, 1); + EXPECT_EQ(diffd_point.scale_, 0); + EXPECT_EQ(diffd_point.positive_buckets_.Get(1), 2); + EXPECT_EQ(diffd_point.negative_buckets_.Get(-2), 1); + EXPECT_EQ(diffd_point.positive_buckets_.Get(2), 0); } \ No newline at end of file From 8a901be02447aeb74f9860cfbb50097a4d29deb3 Mon Sep 17 00:00:00 2001 From: "Felipe C. Dos Santos" Date: Thu, 3 Apr 2025 15:24:43 +0000 Subject: [PATCH 09/16] Created an sample test --- .vscode/launch.json | 47 ++++++----- .../common/metrics_foo_library/foo_library.cc | 18 +++++ .../common/metrics_foo_library/foo_library.h | 1 + examples/otlp/grpc_metric_main.cc | 77 +++++++++++++++++-- exporters/otlp/src/otlp_metric_utils.cc | 5 ++ exporters/prometheus/src/exporter_utils.cc | 4 + ...base2_exponential_histogram_aggregation.cc | 6 +- 7 files changed, 131 insertions(+), 27 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 3532ca4d2a..c7ad6761dd 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -1,24 +1,33 @@ { "version": "0.2.0", "configurations": [ - { - "name": "Debug on Windows", - "type": "cppvsdbg", - "request": "launch", - "program": "${workspaceFolder}/build/", - "args": [], - "stopAtEntry": false, - "cwd": "${workspaceFolder}", - "environment": [], - "externalConsole": false - }, - { - "name": "Debug on Linux", - "type": "gdb", - "request": "launch", - "target": "${workspaceFolder}/bazel-bin/", - "cwd": "${workspaceRoot}", - "valuesFormatting": "parseText" - } + { + "name": "(ctest) Launch", + "type": "cppdbg", + "cwd": "${cmake.testWorkingDirectory}", + "request": "launch", + "program": "${cmake.testProgram}", + "args": [ "${cmake.testArgs}" ], + // other options... + }, + { + "name": "Debug on Windows", + "type": "cppvsdbg", + "request": "launch", + "program": "${workspaceFolder}/build/", + "args": [], + "stopAtEntry": false, + "cwd": "${workspaceFolder}", + "environment": [], + "externalConsole": false + }, + { + "name": "Debug on Linux", + "type": "gdb", + "request": "launch", + "target": "${workspaceFolder}/bazel-bin/", + "cwd": "${workspaceRoot}", + "valuesFormatting": "parseText" + } ] } diff --git a/examples/common/metrics_foo_library/foo_library.cc b/examples/common/metrics_foo_library/foo_library.cc index 81cb840718..be9867dd78 100644 --- a/examples/common/metrics_foo_library/foo_library.cc +++ b/examples/common/metrics_foo_library/foo_library.cc @@ -24,6 +24,7 @@ #include "opentelemetry/semconv/http_metrics.h" #include "opentelemetry/semconv/incubating/container_metrics.h" #include "opentelemetry/semconv/incubating/system_metrics.h" +#include "opentelemetry/sdk/metrics/view/view_factory.h" namespace metrics_api = opentelemetry::metrics; @@ -112,6 +113,23 @@ void foo_library::histogram_example(const std::string &name) } } +void foo_library::histogram_exp_example(const std::string &name) +{ + std::string histogram_name = name + "_histogram"; + auto provider = metrics_api::Provider::GetMeterProvider(); + opentelemetry::nostd::shared_ptr meter = provider->GetMeter(name, "1.2.0"); + auto histogram_counter = meter->CreateDoubleHistogram(histogram_name, "des", "unit"); + auto context = opentelemetry::context::Context{}; + for (uint32_t i = 0; i < 20; ++i) + { + double val = (rand() % 700) + 1.1; + std::map labels = get_random_attr(); + auto labelkv = opentelemetry::common::KeyValueIterableView{labels}; + histogram_counter->Record(val, labelkv, context); + std::this_thread::sleep_for(std::chrono::milliseconds(250)); + } +} + #if OPENTELEMETRY_ABI_VERSION_NO >= 2 void foo_library::gauge_example(const std::string &name) { diff --git a/examples/common/metrics_foo_library/foo_library.h b/examples/common/metrics_foo_library/foo_library.h index 65997398a5..d157a78f28 100644 --- a/examples/common/metrics_foo_library/foo_library.h +++ b/examples/common/metrics_foo_library/foo_library.h @@ -10,6 +10,7 @@ class foo_library public: static void counter_example(const std::string &name); static void histogram_example(const std::string &name); + static void histogram_exp_example(const std::string &name); static void observable_counter_example(const std::string &name); #if OPENTELEMETRY_ABI_VERSION_NO >= 2 static void gauge_example(const std::string &name); diff --git a/examples/otlp/grpc_metric_main.cc b/examples/otlp/grpc_metric_main.cc index 7b6550edba..030419e535 100644 --- a/examples/otlp/grpc_metric_main.cc +++ b/examples/otlp/grpc_metric_main.cc @@ -1,6 +1,9 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include "grpcpp/grpcpp.h" +#include "opentelemetry/exporters/otlp/otlp_grpc_exporter.h" + #include "opentelemetry/exporters/otlp/otlp_grpc_metric_exporter_factory.h" #include "opentelemetry/metrics/provider.h" #include "opentelemetry/sdk/metrics/aggregation/default_aggregation.h" @@ -11,6 +14,9 @@ #include "opentelemetry/sdk/metrics/meter_provider.h" #include "opentelemetry/sdk/metrics/meter_provider_factory.h" #include "opentelemetry/sdk/metrics/provider.h" +#include "opentelemetry/sdk/metrics/view/instrument_selector_factory.h" +#include "opentelemetry/sdk/metrics/view/view_factory.h" +#include "opentelemetry/sdk/metrics/view/meter_selector_factory.h" #include #include @@ -55,6 +61,61 @@ void InitMetrics() metric_sdk::Provider::SetMeterProvider(provider); } +void InitMetrics(std::string &name) +{ + auto exporter = otlp_exporter::OtlpGrpcMetricExporterFactory::Create(exporter_options); + + std::string version{"1.2.0"}; + std::string schema{"https://opentelemetry.io/schemas/1.2.0"}; + + // Initialize and set the global MeterProvider + metric_sdk::PeriodicExportingMetricReaderOptions reader_options; + reader_options.export_interval_millis = std::chrono::milliseconds(1000); + reader_options.export_timeout_millis = std::chrono::milliseconds(500); + + auto reader = + metric_sdk::PeriodicExportingMetricReaderFactory::Create(std::move(exporter), reader_options); + + auto context = metric_sdk::MeterContextFactory::Create(); + context->AddMetricReader(std::move(reader)); + + auto provider = metric_sdk::MeterProviderFactory::Create(std::move(context)); + + // auto provider = opentelemetry::sdk::metrics::MeterProviderFactory::Create(); + + // std::shared_ptr provider(std::move(u_provider)); + + // histogram view + std::string histogram_name = name + "_histogram"; + std::string unit = "unit"; + + auto histogram_instrument_selector = metric_sdk::InstrumentSelectorFactory::Create( + metric_sdk::InstrumentType::kHistogram, histogram_name, unit); + + auto histogram_meter_selector = metric_sdk::MeterSelectorFactory::Create(name, version, schema); + + auto histogram_aggregation_config = std::unique_ptr( + new metric_sdk::Base2ExponentialHistogramAggregationConfig); + + histogram_aggregation_config->max_scale_ = 3; + + // histogram_aggregation_config->boundaries_ = std::vector{ + // 0.0, 50.0, 100.0, 250.0, 500.0, 750.0, 1000.0, 2500.0, 5000.0, 10000.0, 20000.0}; + + std::shared_ptr aggregation_config( + std::move(histogram_aggregation_config)); + + auto histogram_view = metric_sdk::ViewFactory::Create( + name, "description", unit, metric_sdk::AggregationType::kBase2ExponentialHistogram, aggregation_config); + + provider->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), + std::move(histogram_view)); + + std::shared_ptr api_provider(std::move(provider)); + + metric_sdk::Provider::SetMeterProvider(api_provider); +} + void CleanupMetrics() { std::shared_ptr none; @@ -79,9 +140,15 @@ int main(int argc, char *argv[]) } } // Removing this line will leave the default noop MetricProvider in place. - InitMetrics(); + std::string name{"otlp_grpc_metric_example"}; + InitMetrics(name); + + //InitMetrics(); + + foo_library::histogram_exp_example(name); + if (example_type == "counter") { foo_library::counter_example(name); @@ -102,15 +169,15 @@ int main(int argc, char *argv[]) #endif else { - std::thread counter_example{&foo_library::counter_example, name}; - std::thread observable_counter_example{&foo_library::observable_counter_example, name}; + //std::thread counter_example{&foo_library::counter_example, name}; + // std::thread observable_counter_example{&foo_library::observable_counter_example, name}; std::thread histogram_example{&foo_library::histogram_example, name}; #if OPENTELEMETRY_ABI_VERSION_NO >= 2 std::thread gauge_example{&foo_library::gauge_example, name}; #endif - counter_example.join(); - observable_counter_example.join(); + // counter_example.join(); + // observable_counter_example.join(); histogram_example.join(); #if OPENTELEMETRY_ABI_VERSION_NO >= 2 gauge_example.join(); diff --git a/exporters/otlp/src/otlp_metric_utils.cc b/exporters/otlp/src/otlp_metric_utils.cc index 65f86a4e47..c815b69aae 100644 --- a/exporters/otlp/src/otlp_metric_utils.cc +++ b/exporters/otlp/src/otlp_metric_utils.cc @@ -63,6 +63,11 @@ metric_sdk::AggregationType OtlpMetricUtils::GetAggregationType( { return metric_sdk::AggregationType::kHistogram; } + else if (nostd::holds_alternative( + point_data_with_attributes.point_data)) + { + return metric_sdk::AggregationType::kBase2ExponentialHistogram; + } else if (nostd::holds_alternative( point_data_with_attributes.point_data)) { diff --git a/exporters/prometheus/src/exporter_utils.cc b/exporters/prometheus/src/exporter_utils.cc index c874faa654..7055e0d86c 100644 --- a/exporters/prometheus/src/exporter_utils.cc +++ b/exporters/prometheus/src/exporter_utils.cc @@ -562,6 +562,10 @@ metric_sdk::AggregationType PrometheusExporterUtils::getAggregationType( { return metric_sdk::AggregationType::kHistogram; } + else if (nostd::holds_alternative(point_type)) + { + return metric_sdk::AggregationType::kBase2ExponentialHistogram; + } else if (nostd::holds_alternative(point_type)) { return metric_sdk::AggregationType::kLastValue; diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index 69dc688af4..ce97132c7d 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -83,7 +83,7 @@ Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( { const Base2ExponentialHistogramAggregationConfig default_config; auto ac = static_cast(aggregation_config); - if (!ac) + if (!ac) { ac = &default_config; } @@ -99,7 +99,7 @@ Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( Base2ExponentialHistogramPointData point_data) - : point_data_{std::move(point_data)}, indexer_(point_data.scale_) + : point_data_{std::move(point_data)}, indexer_(point_data.scale_)\ {} void Base2ExponentialHistogramAggregation::Aggregate( @@ -176,7 +176,7 @@ std::unique_ptr Base2ExponentialHistogramAggregation::Merge( auto left = nostd::get(ToPoint()); auto right = nostd::get( (static_cast(delta).ToPoint())); - + auto low_res = left.scale_ < right.scale_ ? left : right; auto high_res = left.scale_ < right.scale_ ? right : left; auto scale_reduction = high_res.scale_ - low_res.scale_; From f246d14bb3c9ca29d203a5f7bc838592cfe234ad Mon Sep 17 00:00:00 2001 From: "Felipe C. Dos Santos" Date: Thu, 3 Apr 2025 17:54:03 +0000 Subject: [PATCH 10/16] Created ConvertExponentialHistogramMetric --- .../exporters/otlp/otlp_metric_utils.h | 3 + exporters/otlp/src/otlp_metric_utils.cc | 60 +++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_metric_utils.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_metric_utils.h index 4f92b8e665..9907f66ce8 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_metric_utils.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_metric_utils.h @@ -55,6 +55,9 @@ class OtlpMetricUtils static void ConvertHistogramMetric(const opentelemetry::sdk::metrics::MetricData &metric_data, proto::metrics::v1::Histogram *const histogram) noexcept; + static void ConvertExponentialHistogramMetric(const opentelemetry::sdk::metrics::MetricData &metric_data, + proto::metrics::v1::ExponentialHistogram *const histogram) noexcept; + static void ConvertGaugeMetric(const opentelemetry::sdk::metrics::MetricData &metric_data, proto::metrics::v1::Gauge *const gauge) noexcept; diff --git a/exporters/otlp/src/otlp_metric_utils.cc b/exporters/otlp/src/otlp_metric_utils.cc index c815b69aae..4d2afcc2e9 100644 --- a/exporters/otlp/src/otlp_metric_utils.cc +++ b/exporters/otlp/src/otlp_metric_utils.cc @@ -182,6 +182,62 @@ void OtlpMetricUtils::ConvertHistogramMetric( } } +void OtlpMetricUtils::ConvertExponentialHistogramMetric( + const metric_sdk::MetricData &metric_data, + proto::metrics::v1::ExponentialHistogram *const histogram) noexcept +{ + histogram->set_aggregation_temporality( + GetProtoAggregationTemporality(metric_data.aggregation_temporality)); + auto start_ts = metric_data.start_ts.time_since_epoch().count(); + auto ts = metric_data.end_ts.time_since_epoch().count(); + for (auto &point_data_with_attributes : metric_data.point_data_attr_) + { + proto::metrics::v1::ExponentialHistogramDataPoint *proto_histogram_point_data = + histogram->add_data_points(); + proto_histogram_point_data->set_start_time_unix_nano(start_ts); + proto_histogram_point_data->set_time_unix_nano(ts); + auto histogram_data = + nostd::get(point_data_with_attributes.point_data); + // sum + proto_histogram_point_data->set_sum(histogram_data.sum_); + proto_histogram_point_data->set_count(histogram_data.count_); + if (histogram_data.record_min_max_) + { + proto_histogram_point_data->set_min(histogram_data.min_); + proto_histogram_point_data->set_max(histogram_data.max_); + } + // negative buckets + if(!histogram_data.negative_buckets_.Empty()) + { + auto negative_buckets = proto_histogram_point_data->mutable_negative(); + + for( auto index = histogram_data.negative_buckets_.StartIndex(); + index <= histogram_data.negative_buckets_.EndIndex(); ++index) + { + negative_buckets->add_bucket_counts(histogram_data.negative_buckets_.Get(index)); + } + } + // positive buckets + if(!histogram_data.positive_buckets_.Empty()) + { + auto positive_buckets = proto_histogram_point_data->mutable_positive(); + + for( auto index = histogram_data.positive_buckets_.StartIndex(); + index <= histogram_data.positive_buckets_.EndIndex(); ++index) + { + positive_buckets->add_bucket_counts(histogram_data.positive_buckets_.Get(index)); + } + } + // attributes + for (auto &kv_attr : point_data_with_attributes.attributes) + { + OtlpPopulateAttributeUtils::PopulateAttribute(proto_histogram_point_data->add_attributes(), + kv_attr.first, kv_attr.second); + } + } +} + + void OtlpMetricUtils::ConvertGaugeMetric(const opentelemetry::sdk::metrics::MetricData &metric_data, proto::metrics::v1::Gauge *const gauge) noexcept { @@ -230,6 +286,10 @@ void OtlpMetricUtils::PopulateInstrumentInfoMetrics( ConvertHistogramMetric(metric_data, metric->mutable_histogram()); break; } + case metric_sdk::AggregationType::kBase2ExponentialHistogram: { + ConvertExponentialHistogramMetric(metric_data, metric->mutable_exponential_histogram()); + break; + } case metric_sdk::AggregationType::kLastValue: { ConvertGaugeMetric(metric_data, metric->mutable_gauge()); break; From 8fe2a5b9afedce0e4864daaad820a3f1d061e6cc Mon Sep 17 00:00:00 2001 From: "Felipe C. Dos Santos" Date: Thu, 3 Apr 2025 20:49:41 +0000 Subject: [PATCH 11/16] ConvertExponentialHistogramMetric: Added the missing conversion fields --- exporters/otlp/src/otlp_metric_utils.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/exporters/otlp/src/otlp_metric_utils.cc b/exporters/otlp/src/otlp_metric_utils.cc index 4d2afcc2e9..d6b8aa3017 100644 --- a/exporters/otlp/src/otlp_metric_utils.cc +++ b/exporters/otlp/src/otlp_metric_utils.cc @@ -210,6 +210,7 @@ void OtlpMetricUtils::ConvertExponentialHistogramMetric( if(!histogram_data.negative_buckets_.Empty()) { auto negative_buckets = proto_histogram_point_data->mutable_negative(); + negative_buckets->set_offset(histogram_data.negative_buckets_.StartIndex()); for( auto index = histogram_data.negative_buckets_.StartIndex(); index <= histogram_data.negative_buckets_.EndIndex(); ++index) @@ -221,6 +222,7 @@ void OtlpMetricUtils::ConvertExponentialHistogramMetric( if(!histogram_data.positive_buckets_.Empty()) { auto positive_buckets = proto_histogram_point_data->mutable_positive(); + positive_buckets->set_offset(histogram_data.positive_buckets_.StartIndex()); for( auto index = histogram_data.positive_buckets_.StartIndex(); index <= histogram_data.positive_buckets_.EndIndex(); ++index) @@ -228,6 +230,11 @@ void OtlpMetricUtils::ConvertExponentialHistogramMetric( positive_buckets->add_bucket_counts(histogram_data.positive_buckets_.Get(index)); } } + proto_histogram_point_data->set_scale(histogram_data.scale_); + proto_histogram_point_data->set_zero_count(histogram_data.zero_count_); + + + // attributes for (auto &kv_attr : point_data_with_attributes.attributes) { From 7be411288213703f77bb17b20dc3b4ab92e4406c Mon Sep 17 00:00:00 2001 From: etmcdonald Date: Thu, 3 Apr 2025 22:30:09 +0000 Subject: [PATCH 12/16] add expo histo switch on grpc metric exmaple --- examples/otlp/grpc_metric_main.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/examples/otlp/grpc_metric_main.cc b/examples/otlp/grpc_metric_main.cc index 030419e535..4ac40e53bb 100644 --- a/examples/otlp/grpc_metric_main.cc +++ b/examples/otlp/grpc_metric_main.cc @@ -139,6 +139,11 @@ int main(int argc, char *argv[]) } } } + std::cout << "Using endpoint: " << exporter_options.endpoint << std::endl; + std::cout << "Using example type: " << example_type << std::endl; + std::cout << "Using cacert path: " << exporter_options.ssl_credentials_cacert_path << std::endl; + std::cout << "Using ssl credentials: " << exporter_options.use_ssl_credentials << std::endl; + // Removing this line will leave the default noop MetricProvider in place. std::string name{"otlp_grpc_metric_example"}; @@ -147,8 +152,6 @@ int main(int argc, char *argv[]) //InitMetrics(); - foo_library::histogram_exp_example(name); - if (example_type == "counter") { foo_library::counter_example(name); @@ -160,6 +163,8 @@ int main(int argc, char *argv[]) else if (example_type == "histogram") { foo_library::histogram_example(name); + } else if (example_type == "exponential_histogram") { + foo_library::histogram_exp_example(name); } #if OPENTELEMETRY_ABI_VERSION_NO >= 2 else if (example_type == "gauge") From 92434ddbd863d69d5dc64b4798e3df8c4fa58107 Mon Sep 17 00:00:00 2001 From: etmcdonald Date: Fri, 4 Apr 2025 04:36:59 +0000 Subject: [PATCH 13/16] add ostream exporter example --- .../common/metrics_foo_library/foo_library.cc | 7 ++-- examples/metrics_simple/metrics_ostream.cc | 35 +++++++++++++++++-- exporters/ostream/src/metric_exporter.cc | 31 ++++++++++++++++ 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/examples/common/metrics_foo_library/foo_library.cc b/examples/common/metrics_foo_library/foo_library.cc index be9867dd78..00f1a398ff 100644 --- a/examples/common/metrics_foo_library/foo_library.cc +++ b/examples/common/metrics_foo_library/foo_library.cc @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -101,7 +102,7 @@ void foo_library::histogram_example(const std::string &name) std::string histogram_name = name + "_histogram"; auto provider = metrics_api::Provider::GetMeterProvider(); opentelemetry::nostd::shared_ptr meter = provider->GetMeter(name, "1.2.0"); - auto histogram_counter = meter->CreateDoubleHistogram(histogram_name, "des", "unit"); + auto histogram_counter = meter->CreateDoubleHistogram(histogram_name, "des", "histogram-unit"); auto context = opentelemetry::context::Context{}; for (uint32_t i = 0; i < 20; ++i) { @@ -115,10 +116,10 @@ void foo_library::histogram_example(const std::string &name) void foo_library::histogram_exp_example(const std::string &name) { - std::string histogram_name = name + "_histogram"; + std::string histogram_name = name + "_exponential_histogram"; auto provider = metrics_api::Provider::GetMeterProvider(); opentelemetry::nostd::shared_ptr meter = provider->GetMeter(name, "1.2.0"); - auto histogram_counter = meter->CreateDoubleHistogram(histogram_name, "des", "unit"); + auto histogram_counter = meter->CreateDoubleHistogram(histogram_name, "des", "histogram-unit"); auto context = opentelemetry::context::Context{}; for (uint32_t i = 0; i < 20; ++i) { diff --git a/examples/metrics_simple/metrics_ostream.cc b/examples/metrics_simple/metrics_ostream.cc index 1d85ff1692..6f05560280 100644 --- a/examples/metrics_simple/metrics_ostream.cc +++ b/examples/metrics_simple/metrics_ostream.cc @@ -17,6 +17,7 @@ #include "opentelemetry/sdk/metrics/instruments.h" #include "opentelemetry/sdk/metrics/meter_provider.h" #include "opentelemetry/sdk/metrics/meter_provider_factory.h" +#include "opentelemetry/sdk/metrics/meter_context_factory.h" #include "opentelemetry/sdk/metrics/metric_reader.h" #include "opentelemetry/sdk/metrics/provider.h" #include "opentelemetry/sdk/metrics/push_metric_exporter.h" @@ -56,9 +57,9 @@ void InitMetrics(const std::string &name) auto reader = metrics_sdk::PeriodicExportingMetricReaderFactory::Create(std::move(exporter), options); - auto provider = opentelemetry::sdk::metrics::MeterProviderFactory::Create(); - - provider->AddMetricReader(std::move(reader)); + auto context = metrics_sdk::MeterContextFactory::Create(); + context->AddMetricReader(std::move(reader)); + auto provider = opentelemetry::sdk::metrics::MeterProviderFactory::Create(std::move(context)); // counter view std::string counter_name = name + "_counter"; @@ -112,6 +113,28 @@ void InitMetrics(const std::string &name) provider->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), std::move(histogram_view)); + // hisogram view with base2 exponential aggregation + std::string histogram_base2_name = name + "_exponential_histogram"; + unit = "histogram-unit"; + auto histogram_base2_instrument_selector = metrics_sdk::InstrumentSelectorFactory::Create( + metrics_sdk::InstrumentType::kHistogram, histogram_base2_name, unit); + auto histogram_base2_meter_selector = metrics_sdk::MeterSelectorFactory::Create(name, version, + schema); + auto histogram_base2_aggregation_config = std::unique_ptr( + new metrics_sdk::Base2ExponentialHistogramAggregationConfig); + histogram_base2_aggregation_config->max_scale_ = 3; + histogram_base2_aggregation_config->record_min_max_ = true; + histogram_base2_aggregation_config->max_buckets_ = 100; + + std::shared_ptr base2_aggregation_config( + std::move(histogram_base2_aggregation_config)); + + auto histogram_base2_view = metrics_sdk::ViewFactory::Create( + name, "description", unit, metrics_sdk::AggregationType::kBase2ExponentialHistogram, + base2_aggregation_config); + + provider->AddView(std::move(histogram_base2_instrument_selector), std::move(histogram_base2_meter_selector), std::move(histogram_base2_view)); + std::shared_ptr api_provider(std::move(provider)); metrics_sdk::Provider::SetMeterProvider(api_provider); @@ -147,6 +170,10 @@ int main(int argc, char **argv) { foo_library::histogram_example(name); } + else if (example_type == "exponential_histogram") + { + foo_library::histogram_exp_example(name); + } #if OPENTELEMETRY_ABI_VERSION_NO >= 2 else if (example_type == "gauge") { @@ -170,6 +197,7 @@ int main(int argc, char **argv) std::thread counter_example{&foo_library::counter_example, name}; std::thread observable_counter_example{&foo_library::observable_counter_example, name}; std::thread histogram_example{&foo_library::histogram_example, name}; + std::thread histogram_exp_example{&foo_library::histogram_exp_example, name}; #if OPENTELEMETRY_ABI_VERSION_NO >= 2 std::thread gauge_example{&foo_library::gauge_example, name}; #endif @@ -181,6 +209,7 @@ int main(int argc, char **argv) counter_example.join(); observable_counter_example.join(); histogram_example.join(); + histogram_exp_example.join(); #if OPENTELEMETRY_ABI_VERSION_NO >= 2 gauge_example.join(); #endif diff --git a/exporters/ostream/src/metric_exporter.cc b/exporters/ostream/src/metric_exporter.cc index 4bdd122492..084286329f 100644 --- a/exporters/ostream/src/metric_exporter.cc +++ b/exporters/ostream/src/metric_exporter.cc @@ -247,6 +247,37 @@ void OStreamMetricExporter::printPointData(const opentelemetry::sdk::metrics::Po sout_ << nostd::get(last_point_data.value_); } } + else if (nostd::holds_alternative(point_data)) + { + auto histogram_point_data = + nostd::get(point_data); + sout_ << "\n type: Base2ExponentialHistogramPointData"; + sout_ << "\n count: " << histogram_point_data.count_; + sout_ << "\n sum: " << histogram_point_data.sum_; + sout_ << "\n zero_count: " << histogram_point_data.zero_count_; + if (histogram_point_data.record_min_max_) + { + sout_ << "\n min: " << histogram_point_data.min_; + sout_ << "\n max: " << histogram_point_data.max_; + } + sout_ << "\n scale: " << histogram_point_data.scale_; + sout_ << "\n positive buckets: "; + if (!histogram_point_data.positive_buckets_.Empty()) + { + for (auto i = histogram_point_data.positive_buckets_.StartIndex(); i <= histogram_point_data.positive_buckets_.EndIndex(); ++i) + { + sout_ << "\n\t" << i << ": " << histogram_point_data.positive_buckets_.Get(i); + } + } + sout_ << "\n negative buckets: "; + if (!histogram_point_data.negative_buckets_.Empty()) + { + for (auto i = histogram_point_data.negative_buckets_.StartIndex(); i <= histogram_point_data.negative_buckets_.EndIndex(); ++i) + { + sout_ << "\n\t" << i << ": " << histogram_point_data.negative_buckets_.Get(i); + } + } + } } void OStreamMetricExporter::printPointAttributes( From c358e5b561a5c5022c959b20603791a143fa96dc Mon Sep 17 00:00:00 2001 From: etmcdonald Date: Fri, 4 Apr 2025 05:39:35 +0000 Subject: [PATCH 14/16] add expo histo aggregation benchmarks at powers of 2 scale --- .../histogram_aggregation_benchmark.cc | 54 ++++++++++++------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/sdk/test/metrics/histogram_aggregation_benchmark.cc b/sdk/test/metrics/histogram_aggregation_benchmark.cc index 7e06f150ab..262fb5a99e 100644 --- a/sdk/test/metrics/histogram_aggregation_benchmark.cc +++ b/sdk/test/metrics/histogram_aggregation_benchmark.cc @@ -97,33 +97,18 @@ void BM_HistogramAggregation(benchmark::State &state) BENCHMARK(BM_HistogramAggregation); -void BM_Base2ExponentialHistogramAggregation(benchmark::State &state) -{ - std::string instrument_unit = "histogram1_unit"; - std::unique_ptr histogram_instrument_selector{ - new InstrumentSelector(InstrumentType::kHistogram, ".*", instrument_unit)}; - std::unique_ptr histogram_meter_selector{ - new MeterSelector("meter1", "version1", "schema1")}; - std::unique_ptr histogram_view{ - new View("base2_expohisto", "description", instrument_unit, AggregationType::kBase2ExponentialHistogram)}; - - std::unique_ptr views{new ViewRegistry()}; - views->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), - std::move(histogram_view)); - HistogramAggregation(state, std::move(views)); -} +// Add this helper function before your benchmark functions -BENCHMARK(BM_Base2ExponentialHistogramAggregation); - -void BM_Base2ExponentialHistogramAggregationZeroScale(benchmark::State &state) -{ +void RunBase2ExponentialHistogramAggregation(benchmark::State &state, int scale) { std::string instrument_unit = "histogram1_unit"; std::unique_ptr histogram_instrument_selector{ new InstrumentSelector(InstrumentType::kHistogram, ".*", instrument_unit)}; std::unique_ptr histogram_meter_selector{ new MeterSelector("meter1", "version1", "schema1")}; + Base2ExponentialHistogramAggregationConfig config; - config.max_scale_ = 0; + config.max_scale_ = scale; + std::unique_ptr histogram_view{ new View("base2_expohisto", "description", instrument_unit, AggregationType::kBase2ExponentialHistogram, std::make_shared(config))}; @@ -131,10 +116,39 @@ void BM_Base2ExponentialHistogramAggregationZeroScale(benchmark::State &state) std::unique_ptr views{new ViewRegistry()}; views->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), std::move(histogram_view)); + HistogramAggregation(state, std::move(views)); } +void BM_Base2ExponentialHistogramAggregationZeroScale(benchmark::State &state) { + RunBase2ExponentialHistogramAggregation(state, 0); +} BENCHMARK(BM_Base2ExponentialHistogramAggregationZeroScale); +void BM_Base2ExponentialHistogramAggregationOneScale(benchmark::State &state) { + RunBase2ExponentialHistogramAggregation(state, 1); +} +BENCHMARK(BM_Base2ExponentialHistogramAggregationOneScale); + +void BM_Base2ExponentialHistogramAggregationTwoScale(benchmark::State &state) { + RunBase2ExponentialHistogramAggregation(state, 2); +} +BENCHMARK(BM_Base2ExponentialHistogramAggregationTwoScale); + +void BM_Base2ExponentialHistogramAggregationFourScale(benchmark::State &state) { + RunBase2ExponentialHistogramAggregation(state, 4); +} +BENCHMARK(BM_Base2ExponentialHistogramAggregationFourScale); + +void BM_Base2ExponentialHistogramAggregationEightScale(benchmark::State &state) { + RunBase2ExponentialHistogramAggregation(state, 8); +} +BENCHMARK(BM_Base2ExponentialHistogramAggregationEightScale); + +void BM_Base2ExponentialHistogramAggregationSixteenScale(benchmark::State &state) { + RunBase2ExponentialHistogramAggregation(state, 16); +} +BENCHMARK(BM_Base2ExponentialHistogramAggregationSixteenScale); + } // namespace BENCHMARK_MAIN(); From 345322bcee2061cc8359d274dc60b713df4f7cf3 Mon Sep 17 00:00:00 2001 From: "Felipe C. Dos Santos" Date: Fri, 4 Apr 2025 12:05:50 +0000 Subject: [PATCH 15/16] Fixed test compile issue andd add record_min_max in exp aggreagation --- .../base2_exponential_histogram_aggregation.h | 5 ++++- .../base2_exponential_histogram_aggregation.cc | 18 +++++++++++++----- sdk/test/metrics/aggregation_test.cc | 6 +++--- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h index a44e402dd7..8a2cbbd4cc 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/base2_exponential_histogram_aggregation.h @@ -21,7 +21,9 @@ class Base2ExponentialHistogramAggregation : public Aggregation { public: Base2ExponentialHistogramAggregation(const AggregationConfig *aggregation_config = nullptr); - Base2ExponentialHistogramAggregation(Base2ExponentialHistogramPointData point_data); + Base2ExponentialHistogramAggregation(const Base2ExponentialHistogramPointData &point_data); + Base2ExponentialHistogramAggregation(Base2ExponentialHistogramPointData &&point_data); + void Aggregate(int64_t value, const PointAttributes &attributes = {}) noexcept override; void Aggregate(double value, const PointAttributes &attributes = {}) noexcept override; @@ -46,6 +48,7 @@ class Base2ExponentialHistogramAggregation : public Aggregation mutable opentelemetry::common::SpinLockMutex lock_; Base2ExponentialHistogramPointData point_data_; Base2ExponentialHistogramIndexer indexer_; + bool record_min_max_ = true; }; } // namespace metrics diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index ce97132c7d..e8995fce7e 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -98,8 +98,12 @@ Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( } Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation( - Base2ExponentialHistogramPointData point_data) - : point_data_{std::move(point_data)}, indexer_(point_data.scale_)\ + const Base2ExponentialHistogramPointData &point_data) + : point_data_{point_data}, indexer_(point_data.scale_), record_min_max_{point_data.record_min_max_} +{} + +Base2ExponentialHistogramAggregation::Base2ExponentialHistogramAggregation(Base2ExponentialHistogramPointData &&point_data) + : point_data_{std::move(point_data)}, indexer_(point_data_.scale_), record_min_max_{point_data_.record_min_max_} {} void Base2ExponentialHistogramAggregation::Aggregate( @@ -115,10 +119,14 @@ void Base2ExponentialHistogramAggregation::Aggregate( { const std::lock_guard locked(lock_); point_data_.sum_ += value; - point_data_.min_ = std::min(point_data_.min_, value); - point_data_.max_ = std::max(point_data_.max_, value); point_data_.count_++; + if (record_min_max_) + { + point_data_.min_ = std::min(point_data_.min_, value); + point_data_.max_ = std::max(point_data_.max_, value); + } + if (value == 0) { point_data_.zero_count_++; @@ -227,7 +235,7 @@ std::unique_ptr Base2ExponentialHistogramAggregation::Diff( auto left = nostd::get(ToPoint()); auto right = nostd::get( (static_cast(next).ToPoint())); - + auto low_res = left.scale_ < right.scale_ ? left : right; auto high_res = left.scale_ < right.scale_ ? right : left; auto scale_reduction = high_res.scale_ - low_res.scale_; diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index a439979c7b..f2721a8199 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -234,7 +234,7 @@ TEST(aggregation, Base2ExponentialHistogramAggregation) scale0_config.max_scale_ = SCALE0; scale0_config.max_buckets_ = MAX_BUCKETS0; scale0_config.record_min_max_ = true; - Base2ExponentialHistogramAggregation scale0_aggr = Base2ExponentialHistogramAggregation(&scale0_config); + Base2ExponentialHistogramAggregation scale0_aggr(&scale0_config); auto point = scale0_aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(point)); auto histo_point = nostd::get(point); @@ -277,12 +277,12 @@ TEST(aggregation, Base2ExponentialHistogramAggregation) EXPECT_EQ(histo_point.max_, 3.5); EXPECT_EQ(histo_point.negative_buckets_.Get(-2), 1); EXPECT_EQ(histo_point.positive_buckets_.Get(1), 2); - + Base2ExponentialHistogramAggregationConfig scale1_config; scale1_config.max_scale_ = 1; scale1_config.max_buckets_ = 14; scale1_config.record_min_max_ = true; - Base2ExponentialHistogramAggregation scale1_aggr = Base2ExponentialHistogramAggregation(&scale1_config); + Base2ExponentialHistogramAggregation scale1_aggr(&scale1_config); scale1_aggr.Aggregate(0.0, {}); scale1_aggr.Aggregate(3.0, {}); From 98c97cdf19d6dc694c73dd0586806e25973b3e3b Mon Sep 17 00:00:00 2001 From: etmcdonald Date: Fri, 4 Apr 2025 16:00:35 +0000 Subject: [PATCH 16/16] Conditional min_max in merge() --- .../aggregation/base2_exponential_histogram_aggregation.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc index e8995fce7e..41b63f10ea 100644 --- a/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/base2_exponential_histogram_aggregation.cc @@ -200,11 +200,14 @@ std::unique_ptr Base2ExponentialHistogramAggregation::Merge( result_value.count_ = low_res.count_ + high_res.count_; result_value.sum_ = low_res.sum_ + high_res.sum_; result_value.zero_count_ = low_res.zero_count_ + high_res.zero_count_; - result_value.min_ = std::min(low_res.min_, high_res.min_); - result_value.max_ = std::max(low_res.max_, high_res.max_); result_value.scale_ = std::min(low_res.scale_, high_res.scale_); result_value.max_buckets_ = low_res.max_buckets_; result_value.record_min_max_ = low_res.record_min_max_ && high_res.record_min_max_; + if (result_value.record_min_max_) + { + result_value.min_ = std::min(low_res.min_, high_res.min_); + result_value.max_ = std::max(low_res.max_, high_res.max_); + } if (!high_res.positive_buckets_.Empty()) { for (int i = high_res.positive_buckets_.StartIndex();