From fccc108cf57965f49c0f5de8330945469c451b71 Mon Sep 17 00:00:00 2001 From: Ehsan Saei <71217171+esigo@users.noreply.github.com> Date: Mon, 17 Oct 2022 19:58:34 +0200 Subject: [PATCH] fix Histogram crash (#1685) --- examples/metrics_simple/metrics_ostream.cc | 9 +++-- .../metrics/aggregation/aggregation_config.h | 1 - .../metrics/aggregation/default_aggregation.h | 31 ++++++++-------- .../aggregation/histogram_aggregation.h | 6 ++-- .../sdk/metrics/state/async_metric_storage.h | 2 +- .../sdk/metrics/state/sync_metric_storage.h | 7 ++-- .../metrics/state/temporal_metric_storage.h | 4 +-- .../opentelemetry/sdk/metrics/view/view.h | 8 ++--- .../aggregation/histogram_aggregation.cc | 32 ++++++++++------- .../metrics/state/temporal_metric_storage.cc | 36 +++++++++---------- sdk/test/metrics/aggregation_test.cc | 8 ++--- sdk/test/metrics/async_metric_storage_test.cc | 6 ++-- .../sync_metric_storage_counter_test.cc | 6 ++-- .../sync_metric_storage_histogram_test.cc | 6 ++-- 14 files changed, 82 insertions(+), 80 deletions(-) diff --git a/examples/metrics_simple/metrics_ostream.cc b/examples/metrics_simple/metrics_ostream.cc index 9ee1dc1c25..b49c632080 100644 --- a/examples/metrics_simple/metrics_ostream.cc +++ b/examples/metrics_simple/metrics_ostream.cc @@ -73,11 +73,10 @@ void initMetrics(const std::string &name) std::unique_ptr histogram_meter_selector{ new metric_sdk::MeterSelector(name, version, schema)}; std::shared_ptr aggregation_config{ - new opentelemetry::sdk::metrics::HistogramAggregationConfig}; - static_cast *>( - aggregation_config.get()) - ->boundaries_ = - std::list{0.0, 50.0, 100.0, 250.0, 500.0, 750.0, 1000.0, 2500.0, 5000.0, 10000.0}; + new opentelemetry::sdk::metrics::HistogramAggregationConfig}; + static_cast(aggregation_config.get()) + ->boundaries_ = std::list{0.0, 50.0, 100.0, 250.0, 500.0, 750.0, + 1000.0, 2500.0, 5000.0, 10000.0, 20000.0}; std::unique_ptr histogram_view{new metric_sdk::View{ name, "description", metric_sdk::AggregationType::kHistogram, aggregation_config}}; p->AddView(std::move(histogram_instrument_selector), std::move(histogram_meter_selector), diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h index 96e5b2fa37..61bf2716a7 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/aggregation_config.h @@ -16,7 +16,6 @@ class AggregationConfig virtual ~AggregationConfig() = default; }; -template class HistogramAggregationConfig : public AggregationConfig { public: diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h index eb74b2d8dc..73eff4969b 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h @@ -27,7 +27,7 @@ class DefaultAggregation public: static std::unique_ptr CreateAggregation( const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, - const opentelemetry::sdk::metrics::AggregationConfig *aggregation_config) + const AggregationConfig *aggregation_config) { switch (instrument_descriptor.type_) { @@ -40,14 +40,15 @@ class DefaultAggregation : std::move(std::unique_ptr(new DoubleSumAggregation())); break; case InstrumentType::kHistogram: { - return (instrument_descriptor.value_type_ == InstrumentValueType::kLong) - ? std::move(std::unique_ptr(new LongHistogramAggregation( - static_cast< - const opentelemetry::sdk::metrics::HistogramAggregationConfig *>( - aggregation_config)))) - : std::move(std::unique_ptr(new DoubleHistogramAggregation( - static_cast *>(aggregation_config)))); + if (instrument_descriptor.value_type_ == InstrumentValueType::kLong) + { + return (std::unique_ptr(new LongHistogramAggregation(aggregation_config))); + } + else + { + return (std::unique_ptr(new DoubleHistogramAggregation(aggregation_config))); + } + break; } case InstrumentType::kObservableGauge: @@ -60,8 +61,10 @@ class DefaultAggregation }; } - static std::unique_ptr CreateAggregation(AggregationType aggregation_type, - InstrumentDescriptor instrument_descriptor) + static std::unique_ptr CreateAggregation( + AggregationType aggregation_type, + InstrumentDescriptor instrument_descriptor, + const AggregationConfig *aggregation_config = nullptr) { switch (aggregation_type) { @@ -71,11 +74,11 @@ class DefaultAggregation case AggregationType::kHistogram: if (instrument_descriptor.value_type_ == InstrumentValueType::kLong) { - return std::unique_ptr(new LongHistogramAggregation()); + return std::unique_ptr(new LongHistogramAggregation(aggregation_config)); } else { - return std::unique_ptr(new DoubleHistogramAggregation()); + return std::unique_ptr(new DoubleHistogramAggregation(aggregation_config)); } break; case AggregationType::kLastValue: @@ -99,7 +102,7 @@ class DefaultAggregation } break; default: - return DefaultAggregation::CreateAggregation(instrument_descriptor, nullptr); + return DefaultAggregation::CreateAggregation(instrument_descriptor, aggregation_config); } } diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h index c78d02742e..bc5c76ae67 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 #pragma once +#include #ifndef ENABLE_METRICS_PREVIEW # include "opentelemetry/common/spin_lock_mutex.h" # include "opentelemetry/sdk/metrics/aggregation/aggregation.h" @@ -18,7 +19,7 @@ namespace metrics class LongHistogramAggregation : public Aggregation { public: - LongHistogramAggregation(const HistogramAggregationConfig *aggregation_config = nullptr); + LongHistogramAggregation(const AggregationConfig *aggregation_config = nullptr); LongHistogramAggregation(HistogramPointData &&); LongHistogramAggregation(const HistogramPointData &); @@ -48,8 +49,7 @@ class LongHistogramAggregation : public Aggregation class DoubleHistogramAggregation : public Aggregation { public: - DoubleHistogramAggregation( - const HistogramAggregationConfig *aggregation_config = nullptr); + DoubleHistogramAggregation(const AggregationConfig *aggregation_config = nullptr); DoubleHistogramAggregation(HistogramPointData &&); DoubleHistogramAggregation(const HistogramPointData &); diff --git a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h index db9c61741d..8270a32c02 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -28,7 +28,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora AsyncMetricStorage(InstrumentDescriptor instrument_descriptor, const AggregationType aggregation_type, const AttributesProcessor *attributes_processor, - nostd::shared_ptr aggregation_config, + const AggregationConfig *aggregation_config, void *state = nullptr) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, diff --git a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h index 16bac34079..b0c7229864 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -31,7 +31,7 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage const AggregationType aggregation_type, const AttributesProcessor *attributes_processor, nostd::shared_ptr &&exemplar_reservoir, - nostd::shared_ptr aggregation_config) + const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, attributes_hashmap_(new AttributesHashMap()), @@ -40,8 +40,9 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage temporal_metric_storage_(instrument_descriptor, aggregation_config) { - create_default_aggregation_ = [&]() -> std::unique_ptr { - return DefaultAggregation::CreateAggregation(aggregation_type_, instrument_descriptor_); + create_default_aggregation_ = [&, aggregation_config]() -> std::unique_ptr { + return DefaultAggregation::CreateAggregation(aggregation_type_, instrument_descriptor_, + aggregation_config); }; } diff --git a/sdk/include/opentelemetry/sdk/metrics/state/temporal_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/temporal_metric_storage.h index f8a0c36b0f..5cb556ae20 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/temporal_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/temporal_metric_storage.h @@ -26,7 +26,7 @@ class TemporalMetricStorage { public: TemporalMetricStorage(InstrumentDescriptor instrument_descriptor, - nostd::shared_ptr aggregation_config); + const AggregationConfig *aggregation_config); bool buildMetrics(CollectorHandle *collector, nostd::span> collectors, @@ -46,7 +46,7 @@ class TemporalMetricStorage // Lock while building metrics mutable opentelemetry::common::SpinLockMutex lock_; - const nostd::shared_ptr aggregation_config_; + const AggregationConfig *aggregation_config_; }; } // namespace metrics } // namespace sdk diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view.h b/sdk/include/opentelemetry/sdk/metrics/view/view.h index 733e309bb7..7f2ca0e2b2 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -26,7 +26,7 @@ class View View(const std::string &name, const std::string &description = "", AggregationType aggregation_type = AggregationType::kDefault, - std::shared_ptr aggregation_config = std::shared_ptr{}, + std::shared_ptr aggregation_config = nullptr, std::unique_ptr attributes_processor = std::unique_ptr( new opentelemetry::sdk::metrics::DefaultAttributesProcessor())) @@ -45,9 +45,9 @@ class View virtual AggregationType GetAggregationType() const noexcept { return aggregation_type_; } - virtual nostd::shared_ptr GetAggregationConfig() const noexcept + virtual AggregationConfig *GetAggregationConfig() const noexcept { - return aggregation_config_; + return aggregation_config_.get(); } virtual const opentelemetry::sdk::metrics::AttributesProcessor &GetAttributesProcessor() @@ -60,7 +60,7 @@ class View std::string name_; std::string description_; AggregationType aggregation_type_; - nostd::shared_ptr aggregation_config_; + std::shared_ptr aggregation_config_; std::unique_ptr attributes_processor_; }; } // namespace metrics diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index 4dec404d5d..e9ca7d18b0 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -6,6 +6,7 @@ # include # include # include +# include # include "opentelemetry/version.h" # include @@ -15,12 +16,12 @@ namespace sdk namespace metrics { -LongHistogramAggregation::LongHistogramAggregation( - const HistogramAggregationConfig *aggregation_config) +LongHistogramAggregation::LongHistogramAggregation(const AggregationConfig *aggregation_config) { - if (aggregation_config && aggregation_config->boundaries_.size()) + auto ac = static_cast(aggregation_config); + if (ac && ac->boundaries_.size()) { - point_data_.boundaries_ = aggregation_config->boundaries_; + point_data_.boundaries_ = ac->boundaries_; } else { @@ -28,9 +29,9 @@ LongHistogramAggregation::LongHistogramAggregation( 500.0, 750.0, 1000.0, 2500.0, 5000.0, 7500.0, 10000.0}; } - if (aggregation_config) + if (ac) { - record_min_max_ = aggregation_config->record_min_max_; + record_min_max_ = ac->record_min_max_; } point_data_.counts_ = std::vector(point_data_.boundaries_.size() + 1, 0); point_data_.sum_ = 0l; @@ -98,21 +99,21 @@ PointType LongHistogramAggregation::ToPoint() const noexcept return point_data_; } -DoubleHistogramAggregation::DoubleHistogramAggregation( - const HistogramAggregationConfig *aggregation_config) +DoubleHistogramAggregation::DoubleHistogramAggregation(const AggregationConfig *aggregation_config) { - if (aggregation_config && aggregation_config->boundaries_.size()) + auto ac = static_cast(aggregation_config); + if (ac && ac->boundaries_.size()) { - point_data_.boundaries_ = aggregation_config->boundaries_; + point_data_.boundaries_ = ac->boundaries_; } else { 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}; } - if (aggregation_config) + if (ac) { - record_min_max_ = aggregation_config->record_min_max_; + record_min_max_ = ac->record_min_max_; } point_data_.counts_ = std::vector(point_data_.boundaries_.size() + 1, 0); point_data_.sum_ = 0.0; @@ -159,7 +160,12 @@ std::unique_ptr DoubleHistogramAggregation::Merge( auto curr_value = nostd::get(ToPoint()); auto delta_value = nostd::get( (static_cast(delta).ToPoint())); - DoubleHistogramAggregation *aggr = new DoubleHistogramAggregation(); + std::shared_ptr aggregation_config(new HistogramAggregationConfig); + static_cast(aggregation_config.get()) + ->boundaries_ = curr_value.boundaries_; + static_cast(aggregation_config.get()) + ->record_min_max_ = record_min_max_; + DoubleHistogramAggregation *aggr = new DoubleHistogramAggregation(aggregation_config.get()); HistogramMerge(curr_value, delta_value, aggr->point_data_); return std::unique_ptr(aggr); } diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index b8fd38d794..0cb84aa39b 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -17,9 +17,8 @@ namespace sdk namespace metrics { -TemporalMetricStorage::TemporalMetricStorage( - InstrumentDescriptor instrument_descriptor, - nostd::shared_ptr aggregation_config) +TemporalMetricStorage::TemporalMetricStorage(InstrumentDescriptor instrument_descriptor, + const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_config_(aggregation_config) {} @@ -67,7 +66,7 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, else { merged_metrics->Set(attributes, DefaultAggregation::CreateAggregation( - instrument_descriptor_, aggregation_config_.get()) + instrument_descriptor_, aggregation_config_) ->Merge(aggregation)); } return true; @@ -90,20 +89,21 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, if (aggregation_temporarily == AggregationTemporality::kCumulative) { // merge current delta to previous cumulative - last_aggr_hashmap->GetAllEnteries( - [&merged_metrics, this](const MetricAttributes &attributes, Aggregation &aggregation) { - auto agg = merged_metrics->Get(attributes); - if (agg) - { - merged_metrics->Set(attributes, agg->Merge(aggregation)); - } - else - { - auto def_agg = DefaultAggregation::CreateAggregation(instrument_descriptor_, nullptr); - merged_metrics->Set(attributes, def_agg->Merge(aggregation)); - } - return true; - }); + last_aggr_hashmap->GetAllEnteries([&merged_metrics, this](const MetricAttributes &attributes, + Aggregation &aggregation) { + auto agg = merged_metrics->Get(attributes); + if (agg) + { + merged_metrics->Set(attributes, agg->Merge(aggregation)); + } + else + { + auto def_agg = + DefaultAggregation::CreateAggregation(instrument_descriptor_, aggregation_config_); + merged_metrics->Set(attributes, def_agg->Merge(aggregation)); + } + return true; + }); } last_reported_metrics_[collector] = LastReportedMetrics{std::move(merged_metrics), collection_ts}; diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index bc82863803..a41fa65771 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -131,8 +131,8 @@ TEST(Aggregation, LongHistogramAggregation) TEST(Aggregation, LongHistogramAggregationBoundaries) { - nostd::shared_ptr> - aggregation_config{new opentelemetry::sdk::metrics::HistogramAggregationConfig}; + std::shared_ptr aggregation_config{ + new opentelemetry::sdk::metrics::HistogramAggregationConfig}; std::list user_boundaries = {0.0, 50.0, 100.0, 250.0, 500.0, 750.0, 1000.0, 2500.0, 5000.0, 10000.0}; aggregation_config->boundaries_ = user_boundaries; @@ -145,8 +145,8 @@ TEST(Aggregation, LongHistogramAggregationBoundaries) TEST(Aggregation, DoubleHistogramAggregationBoundaries) { - nostd::shared_ptr> - aggregation_config{new opentelemetry::sdk::metrics::HistogramAggregationConfig}; + std::shared_ptr aggregation_config{ + new opentelemetry::sdk::metrics::HistogramAggregationConfig}; std::list user_boundaries = {0.0, 50.0, 100.0, 250.0, 500.0, 750.0, 1000.0, 2500.0, 5000.0, 10000.0}; aggregation_config->boundaries_ = user_boundaries; diff --git a/sdk/test/metrics/async_metric_storage_test.cc b/sdk/test/metrics/async_metric_storage_test.cc index 88be4bb7f2..939dd26f08 100644 --- a/sdk/test/metrics/async_metric_storage_test.cc +++ b/sdk/test/metrics/async_metric_storage_test.cc @@ -68,8 +68,7 @@ TEST_P(WritableMetricStorageTestFixture, TestAggregation) std::unique_ptr default_attributes_processor{ new DefaultAttributesProcessor{}}; opentelemetry::sdk::metrics::AsyncMetricStorage storage( - instr_desc, AggregationType::kSum, default_attributes_processor.get(), - std::shared_ptr{}); + instr_desc, AggregationType::kSum, default_attributes_processor.get(), nullptr); long get_count1 = 20l; long put_count1 = 10l; std::unordered_map measurements1 = { @@ -161,8 +160,7 @@ TEST_P(WritableMetricStorageTestObservableGaugeFixture, TestAggregation) std::unique_ptr default_attributes_processor{ new DefaultAttributesProcessor{}}; opentelemetry::sdk::metrics::AsyncMetricStorage storage( - instr_desc, AggregationType::kLastValue, default_attributes_processor.get(), - std::shared_ptr{}); + instr_desc, AggregationType::kLastValue, default_attributes_processor.get(), nullptr); long freq_cpu0 = 3l; long freq_cpu1 = 5l; std::unordered_map measurements1 = { diff --git a/sdk/test/metrics/sync_metric_storage_counter_test.cc b/sdk/test/metrics/sync_metric_storage_counter_test.cc index de0a7a5ce2..5a41c0ad5e 100644 --- a/sdk/test/metrics/sync_metric_storage_counter_test.cc +++ b/sdk/test/metrics/sync_metric_storage_counter_test.cc @@ -53,8 +53,7 @@ TEST_P(WritableMetricStorageTestFixture, LongSumAggregation) new DefaultAttributesProcessor{}}; opentelemetry::sdk::metrics::SyncMetricStorage storage( instr_desc, AggregationType::kSum, default_attributes_processor.get(), - ExemplarReservoir::GetNoExemplarReservoir(), - std::shared_ptr{}); + ExemplarReservoir::GetNoExemplarReservoir(), nullptr); storage.RecordLong(10l, KeyValueIterableView>(attributes_get), opentelemetry::context::Context{}); @@ -188,8 +187,7 @@ TEST_P(WritableMetricStorageTestFixture, DoubleSumAggregation) new DefaultAttributesProcessor{}}; opentelemetry::sdk::metrics::SyncMetricStorage storage( instr_desc, AggregationType::kSum, default_attributes_processor.get(), - ExemplarReservoir::GetNoExemplarReservoir(), - std::shared_ptr{}); + ExemplarReservoir::GetNoExemplarReservoir(), nullptr); storage.RecordDouble(10.0, KeyValueIterableView>(attributes_get), diff --git a/sdk/test/metrics/sync_metric_storage_histogram_test.cc b/sdk/test/metrics/sync_metric_storage_histogram_test.cc index 8a2d368e64..21042f5460 100644 --- a/sdk/test/metrics/sync_metric_storage_histogram_test.cc +++ b/sdk/test/metrics/sync_metric_storage_histogram_test.cc @@ -54,8 +54,7 @@ TEST_P(WritableMetricStorageHistogramTestFixture, LongHistogram) new DefaultAttributesProcessor{}}; opentelemetry::sdk::metrics::SyncMetricStorage storage( instr_desc, AggregationType::kHistogram, default_attributes_processor.get(), - NoExemplarReservoir::GetNoExemplarReservoir(), - std::shared_ptr{}); + NoExemplarReservoir::GetNoExemplarReservoir(), nullptr); storage.RecordLong(10l, KeyValueIterableView>(attributes_get), opentelemetry::context::Context{}); @@ -189,8 +188,7 @@ TEST_P(WritableMetricStorageHistogramTestFixture, DoubleHistogram) new DefaultAttributesProcessor{}}; opentelemetry::sdk::metrics::SyncMetricStorage storage( instr_desc, AggregationType::kHistogram, default_attributes_processor.get(), - NoExemplarReservoir::GetNoExemplarReservoir(), - std::shared_ptr{}); + NoExemplarReservoir::GetNoExemplarReservoir(), nullptr); storage.RecordDouble(10.0, KeyValueIterableView>(attributes_get),