From deb58af349a6e9de7ea3173fac8e4b2455a2c238 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Fri, 14 Oct 2022 21:28:32 +0000 Subject: [PATCH 1/8] fix Histogram crash --- examples/metrics_simple/metrics_ostream.cc | 8 ++--- .../metrics/aggregation/aggregation_config.h | 1 - .../metrics/aggregation/default_aggregation.h | 33 +++++++++-------- .../aggregation/histogram_aggregation.h | 6 ++-- .../sdk/metrics/state/async_metric_storage.h | 5 +-- .../sdk/metrics/state/sync_metric_storage.h | 8 +++-- .../metrics/state/temporal_metric_storage.h | 4 +-- .../opentelemetry/sdk/metrics/view/view.h | 6 ++-- .../aggregation/histogram_aggregation.cc | 33 ++++++++++------- .../metrics/state/temporal_metric_storage.cc | 36 +++++++++---------- 10 files changed, 78 insertions(+), 62 deletions(-) diff --git a/examples/metrics_simple/metrics_ostream.cc b/examples/metrics_simple/metrics_ostream.cc index 9ee1dc1c25..5a846a37e3 100644 --- a/examples/metrics_simple/metrics_ostream.cc +++ b/examples/metrics_simple/metrics_ostream.cc @@ -73,11 +73,11 @@ 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()) + 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}; + 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..c1677d8640 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 std::shared_ptr aggregation_config) { switch (instrument_descriptor.type_) { @@ -40,14 +40,17 @@ 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::move( + std::unique_ptr(new LongHistogramAggregation(aggregation_config)))); + } + else + { + return (std::move( + std::unique_ptr(new DoubleHistogramAggregation(aggregation_config)))); + } + break; } case InstrumentType::kObservableGauge: @@ -60,8 +63,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, + std::shared_ptr aggregation_config = nullptr) { switch (aggregation_type) { @@ -71,11 +76,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 +104,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..6bfbb1f0d3 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(std::shared_ptr 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(std::shared_ptr 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..62a1941d74 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, + std::shared_ptr aggregation_config, void *state = nullptr) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, @@ -49,7 +49,8 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora std::lock_guard guard(hashmap_lock_); for (auto &measurement : measurements) { - auto aggr = DefaultAggregation::CreateAggregation(aggregation_type_, instrument_descriptor_); + auto aggr = DefaultAggregation::CreateAggregation( + aggregation_type_, instrument_descriptor_); aggr->Aggregate(measurement.second); auto prev = cumulative_hash_map_->Get(measurement.first); if (prev) 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..be3dfeb06d 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) + std::shared_ptr aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, attributes_hashmap_(new AttributesHashMap()), @@ -40,8 +40,10 @@ 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_type, aggregation_config, + instrument_descriptor]() -> 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..c1da7a73be 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); + std::shared_ptr 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 std::shared_ptr 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..5364d42e16 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,7 +45,7 @@ class View virtual AggregationType GetAggregationType() const noexcept { return aggregation_type_; } - virtual nostd::shared_ptr GetAggregationConfig() const noexcept + virtual std::shared_ptr GetAggregationConfig() const noexcept { return aggregation_config_; } @@ -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..44318332e5 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -1,11 +1,13 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include #ifndef ENABLE_METRICS_PREVIEW -# include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" # include # include +# include # include +# include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" # include "opentelemetry/version.h" # include @@ -16,11 +18,12 @@ namespace metrics { LongHistogramAggregation::LongHistogramAggregation( - const HistogramAggregationConfig *aggregation_config) + std::shared_ptr aggregation_config) { - if (aggregation_config && aggregation_config->boundaries_.size()) + auto ac = std::dynamic_pointer_cast(aggregation_config); + if (ac && ac->boundaries_.size()) { - point_data_.boundaries_ = aggregation_config->boundaries_; + point_data_.boundaries_ = ac->boundaries_; } else { @@ -28,9 +31,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; @@ -99,20 +102,21 @@ PointType LongHistogramAggregation::ToPoint() const noexcept } DoubleHistogramAggregation::DoubleHistogramAggregation( - const HistogramAggregationConfig *aggregation_config) + std::shared_ptr aggregation_config) { - if (aggregation_config && aggregation_config->boundaries_.size()) + auto ac = std::dynamic_pointer_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 +163,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); 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..19ef2fe6b5 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, + std::shared_ptr 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}; From 1f79ad9f3d5ccc569e1b04f2b9d5a98fc88188ab Mon Sep 17 00:00:00 2001 From: Oblivion Date: Fri, 14 Oct 2022 21:31:23 +0000 Subject: [PATCH 2/8] fmt --- examples/metrics_simple/metrics_ostream.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/examples/metrics_simple/metrics_ostream.cc b/examples/metrics_simple/metrics_ostream.cc index 5a846a37e3..b49c632080 100644 --- a/examples/metrics_simple/metrics_ostream.cc +++ b/examples/metrics_simple/metrics_ostream.cc @@ -75,9 +75,8 @@ void initMetrics(const std::string &name) 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, 20000.0}; + ->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), From cd578a511553566b55b9d3835a1cf4242c663945 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Fri, 14 Oct 2022 21:31:50 +0000 Subject: [PATCH 3/8] fmt --- .../opentelemetry/sdk/metrics/state/async_metric_storage.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 62a1941d74..3989502e28 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/async_metric_storage.h @@ -49,8 +49,7 @@ class AsyncMetricStorage : public MetricStorage, public AsyncWritableMetricStora std::lock_guard guard(hashmap_lock_); for (auto &measurement : measurements) { - auto aggr = DefaultAggregation::CreateAggregation( - aggregation_type_, instrument_descriptor_); + auto aggr = DefaultAggregation::CreateAggregation(aggregation_type_, instrument_descriptor_); aggr->Aggregate(measurement.second); auto prev = cumulative_hash_map_->Get(measurement.first); if (prev) From 815056040991de212b5f7ba02cf5568ba29cbb02 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Fri, 14 Oct 2022 21:44:37 +0000 Subject: [PATCH 4/8] fix CI --- sdk/test/metrics/aggregation_test.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sdk/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index bc82863803..ef2cd6fa96 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -131,12 +131,12 @@ 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; - LongHistogramAggregation aggr{aggregation_config.get()}; + LongHistogramAggregation aggr{aggregation_config}; auto data = aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(data)); auto histogram_data = nostd::get(data); @@ -145,12 +145,12 @@ 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; - DoubleHistogramAggregation aggr{aggregation_config.get()}; + DoubleHistogramAggregation aggr{aggregation_config}; auto data = aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(data)); auto histogram_data = nostd::get(data); From 01bcb7199b2fa48a8980561f0f89d8da6387c95f Mon Sep 17 00:00:00 2001 From: Ehsan Saei <71217171+esigo@users.noreply.github.com> Date: Sat, 15 Oct 2022 09:56:51 +0200 Subject: [PATCH 5/8] fix warning --- .../sdk/metrics/aggregation/default_aggregation.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h index c1677d8640..e84b1ad9b4 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h @@ -42,13 +42,11 @@ class DefaultAggregation case InstrumentType::kHistogram: { if (instrument_descriptor.value_type_ == InstrumentValueType::kLong) { - return (std::move( - std::unique_ptr(new LongHistogramAggregation(aggregation_config)))); + return (std::unique_ptr(new LongHistogramAggregation(aggregation_config))); } else { - return (std::move( - std::unique_ptr(new DoubleHistogramAggregation(aggregation_config)))); + return (std::unique_ptr(new DoubleHistogramAggregation(aggregation_config))); } break; From b16d52454412c178503f2868e2761515d059f662 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Sat, 15 Oct 2022 09:16:34 +0000 Subject: [PATCH 6/8] no shared_ptr --- .../sdk/metrics/aggregation/default_aggregation.h | 4 ++-- .../sdk/metrics/aggregation/histogram_aggregation.h | 4 ++-- .../sdk/metrics/state/async_metric_storage.h | 2 +- .../sdk/metrics/state/sync_metric_storage.h | 2 +- .../sdk/metrics/state/temporal_metric_storage.h | 4 ++-- sdk/include/opentelemetry/sdk/metrics/view/view.h | 4 ++-- sdk/src/metrics/aggregation/histogram_aggregation.cc | 12 +++++------- sdk/src/metrics/state/temporal_metric_storage.cc | 2 +- 8 files changed, 16 insertions(+), 18 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/default_aggregation.h index e84b1ad9b4..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 std::shared_ptr aggregation_config) + const AggregationConfig *aggregation_config) { switch (instrument_descriptor.type_) { @@ -64,7 +64,7 @@ class DefaultAggregation static std::unique_ptr CreateAggregation( AggregationType aggregation_type, InstrumentDescriptor instrument_descriptor, - std::shared_ptr aggregation_config = nullptr) + const AggregationConfig *aggregation_config = nullptr) { switch (aggregation_type) { diff --git a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h index 6bfbb1f0d3..bc5c76ae67 100644 --- a/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h +++ b/sdk/include/opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h @@ -19,7 +19,7 @@ namespace metrics class LongHistogramAggregation : public Aggregation { public: - LongHistogramAggregation(std::shared_ptr aggregation_config = nullptr); + LongHistogramAggregation(const AggregationConfig *aggregation_config = nullptr); LongHistogramAggregation(HistogramPointData &&); LongHistogramAggregation(const HistogramPointData &); @@ -49,7 +49,7 @@ class LongHistogramAggregation : public Aggregation class DoubleHistogramAggregation : public Aggregation { public: - DoubleHistogramAggregation(std::shared_ptr 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 3989502e28..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, - std::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 be3dfeb06d..20b37ec6c9 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, - std::shared_ptr aggregation_config) + const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_type_{aggregation_type}, attributes_hashmap_(new AttributesHashMap()), 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 c1da7a73be..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, - std::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 std::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 5364d42e16..7f2ca0e2b2 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view.h @@ -45,9 +45,9 @@ class View virtual AggregationType GetAggregationType() const noexcept { return aggregation_type_; } - virtual std::shared_ptr GetAggregationConfig() const noexcept + virtual AggregationConfig *GetAggregationConfig() const noexcept { - return aggregation_config_; + return aggregation_config_.get(); } virtual const opentelemetry::sdk::metrics::AttributesProcessor &GetAttributesProcessor() diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index 44318332e5..d657cf3984 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -17,10 +17,9 @@ namespace sdk namespace metrics { -LongHistogramAggregation::LongHistogramAggregation( - std::shared_ptr aggregation_config) +LongHistogramAggregation::LongHistogramAggregation(const AggregationConfig *aggregation_config) { - auto ac = std::dynamic_pointer_cast(aggregation_config); + auto ac = static_cast(aggregation_config); if (ac && ac->boundaries_.size()) { point_data_.boundaries_ = ac->boundaries_; @@ -101,10 +100,9 @@ PointType LongHistogramAggregation::ToPoint() const noexcept return point_data_; } -DoubleHistogramAggregation::DoubleHistogramAggregation( - std::shared_ptr aggregation_config) +DoubleHistogramAggregation::DoubleHistogramAggregation(const AggregationConfig *aggregation_config) { - auto ac = std::dynamic_pointer_cast(aggregation_config); + auto ac = static_cast(aggregation_config); if (ac && ac->boundaries_.size()) { point_data_.boundaries_ = ac->boundaries_; @@ -168,7 +166,7 @@ std::unique_ptr DoubleHistogramAggregation::Merge( ->boundaries_ = curr_value.boundaries_; static_cast(aggregation_config.get()) ->record_min_max_ = record_min_max_; - DoubleHistogramAggregation *aggr = new DoubleHistogramAggregation(aggregation_config); + 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 19ef2fe6b5..0cb84aa39b 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -18,7 +18,7 @@ namespace metrics { TemporalMetricStorage::TemporalMetricStorage(InstrumentDescriptor instrument_descriptor, - std::shared_ptr aggregation_config) + const AggregationConfig *aggregation_config) : instrument_descriptor_(instrument_descriptor), aggregation_config_(aggregation_config) {} From 5d96ae8097796f3efca278f39d23b7d39668e577 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Sat, 15 Oct 2022 09:38:22 +0000 Subject: [PATCH 7/8] fix CI --- .../opentelemetry/sdk/metrics/state/sync_metric_storage.h | 5 ++--- sdk/test/metrics/aggregation_test.cc | 4 ++-- sdk/test/metrics/async_metric_storage_test.cc | 6 ++---- sdk/test/metrics/sync_metric_storage_counter_test.cc | 6 ++---- sdk/test/metrics/sync_metric_storage_histogram_test.cc | 6 ++---- 5 files changed, 10 insertions(+), 17 deletions(-) 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 20b37ec6c9..b0c7229864 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h @@ -40,9 +40,8 @@ class SyncMetricStorage : public MetricStorage, public SyncWritableMetricStorage temporal_metric_storage_(instrument_descriptor, aggregation_config) { - create_default_aggregation_ = [aggregation_type, aggregation_config, - instrument_descriptor]() -> 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/test/metrics/aggregation_test.cc b/sdk/test/metrics/aggregation_test.cc index ef2cd6fa96..a41fa65771 100644 --- a/sdk/test/metrics/aggregation_test.cc +++ b/sdk/test/metrics/aggregation_test.cc @@ -136,7 +136,7 @@ TEST(Aggregation, LongHistogramAggregationBoundaries) 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; - LongHistogramAggregation aggr{aggregation_config}; + LongHistogramAggregation aggr{aggregation_config.get()}; auto data = aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(data)); auto histogram_data = nostd::get(data); @@ -150,7 +150,7 @@ TEST(Aggregation, DoubleHistogramAggregationBoundaries) 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; - DoubleHistogramAggregation aggr{aggregation_config}; + DoubleHistogramAggregation aggr{aggregation_config.get()}; auto data = aggr.ToPoint(); ASSERT_TRUE(nostd::holds_alternative(data)); auto histogram_data = nostd::get(data); 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), From f6dee09ddc3c4e2dc3f8aa5457a50bab7ddc6eb3 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Mon, 17 Oct 2022 17:11:33 +0000 Subject: [PATCH 8/8] review comment --- sdk/src/metrics/aggregation/histogram_aggregation.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/sdk/src/metrics/aggregation/histogram_aggregation.cc b/sdk/src/metrics/aggregation/histogram_aggregation.cc index d657cf3984..e9ca7d18b0 100644 --- a/sdk/src/metrics/aggregation/histogram_aggregation.cc +++ b/sdk/src/metrics/aggregation/histogram_aggregation.cc @@ -1,13 +1,12 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -#include #ifndef ENABLE_METRICS_PREVIEW +# include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" # include # include -# include # include -# include "opentelemetry/sdk/metrics/aggregation/histogram_aggregation.h" +# include # include "opentelemetry/version.h" # include