From 4cf41c4d6cf9a27c29ee6b49fb1ae56dea0b93d0 Mon Sep 17 00:00:00 2001 From: Ehsan Saei <71217171+esigo@users.noreply.github.com> Date: Thu, 11 Aug 2022 16:56:39 +0200 Subject: [PATCH] Fix metrics context circular reference (#1535) --- sdk/include/opentelemetry/sdk/metrics/meter.h | 4 +- .../sdk/metrics/state/metric_collector.h | 5 +- sdk/src/metrics/meter.cc | 33 ++++- sdk/src/metrics/meter_context.cc | 3 +- sdk/src/metrics/state/metric_collector.cc | 13 +- sdk/src/metrics/state/observable_registry.cc | 7 + sdk/src/metrics/sync_instruments.cc | 128 +++++++++++++++++- sdk/test/metrics/metric_reader_test.cc | 2 +- 8 files changed, 171 insertions(+), 24 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/meter.h b/sdk/include/opentelemetry/sdk/metrics/meter.h index 332c849ee1..c6bce29205 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter.h @@ -30,7 +30,7 @@ class Meter final : public opentelemetry::metrics::Meter public: /** Construct a new Meter with the given pipeline. */ explicit Meter( - std::shared_ptr meter_context, + std::weak_ptr meter_context, std::unique_ptr scope = opentelemetry::sdk::instrumentationscope::InstrumentationScope::Create("")) noexcept; @@ -111,7 +111,7 @@ class Meter final : public opentelemetry::metrics::Meter // order of declaration is important here - instrumentation scope should destroy after // meter-context. std::unique_ptr scope_; - std::shared_ptr meter_context_; + std::weak_ptr meter_context_; // Mapping between instrument-name and Aggregation Storage. std::unordered_map> storage_registry_; std::unique_ptr RegisterSyncMetricStorage( diff --git a/sdk/include/opentelemetry/sdk/metrics/state/metric_collector.h b/sdk/include/opentelemetry/sdk/metrics/state/metric_collector.h index bac0d9966b..8134edea8a 100644 --- a/sdk/include/opentelemetry/sdk/metrics/state/metric_collector.h +++ b/sdk/include/opentelemetry/sdk/metrics/state/metric_collector.h @@ -31,8 +31,7 @@ class CollectorHandle class MetricCollector : public MetricProducer, public CollectorHandle { public: - MetricCollector(std::shared_ptr &&context, - std::unique_ptr metric_reader); + MetricCollector(MeterContext *context, std::unique_ptr metric_reader); AggregationTemporality GetAggregationTemporality( InstrumentType instrument_type) noexcept override; @@ -50,7 +49,7 @@ class MetricCollector : public MetricProducer, public CollectorHandle bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept; private: - std::shared_ptr meter_context_; + MeterContext *meter_context_; std::shared_ptr metric_reader_; }; } // namespace metrics diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index c8d5901382..3f6b1aa4a1 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -24,7 +24,7 @@ namespace metrics = opentelemetry::metrics; namespace nostd = opentelemetry::nostd; Meter::Meter( - std::shared_ptr meter_context, + std::weak_ptr meter_context, std::unique_ptr instrumentation_scope) noexcept : scope_{std::move(instrumentation_scope)}, meter_context_{meter_context} {} @@ -204,7 +204,14 @@ const sdk::instrumentationscope::InstrumentationScope *Meter::GetInstrumentation std::unique_ptr Meter::RegisterSyncMetricStorage( InstrumentDescriptor &instrument_descriptor) { - auto view_registry = meter_context_->GetViewRegistry(); + auto ctx = meter_context_.lock(); + if (!ctx) + { + OTEL_INTERNAL_LOG_ERROR("[Meter::RegisterMetricStorage] - Error during finding matching views." + << "The metric context is invalid"); + return nullptr; + } + auto view_registry = ctx->GetViewRegistry(); std::unique_ptr storages(new SyncMultiMetricStorage()); auto success = view_registry->FindViews( @@ -240,7 +247,15 @@ std::unique_ptr Meter::RegisterSyncMetricStorage( std::unique_ptr Meter::RegisterAsyncMetricStorage( InstrumentDescriptor &instrument_descriptor) { - auto view_registry = meter_context_->GetViewRegistry(); + auto ctx = meter_context_.lock(); + if (!ctx) + { + OTEL_INTERNAL_LOG_ERROR( + "[Meter::RegisterAsyncMetricStorage] - Error during finding matching views." + << "The metric context is invalid"); + return nullptr; + } + auto view_registry = ctx->GetViewRegistry(); std::unique_ptr storages(new AsyncMultiMetricStorage()); auto success = view_registry->FindViews( instrument_descriptor, *GetInstrumentationScope(), @@ -276,11 +291,17 @@ std::vector Meter::Collect(CollectorHandle *collector, { std::vector metric_data_list; + auto ctx = meter_context_.lock(); + if (!ctx) + { + OTEL_INTERNAL_LOG_ERROR("[Meter::Collect] - Error during collection." + << "The metric context is invalid"); + return std::vector{}; + } for (auto &metric_storage : storage_registry_) { - metric_storage.second->Collect(collector, meter_context_->GetCollectors(), - meter_context_->GetSDKStartTime(), collect_ts, - [&metric_data_list](MetricData metric_data) { + metric_storage.second->Collect(collector, ctx->GetCollectors(), ctx->GetSDKStartTime(), + collect_ts, [&metric_data_list](MetricData metric_data) { metric_data_list.push_back(metric_data); return true; }); diff --git a/sdk/src/metrics/meter_context.cc b/sdk/src/metrics/meter_context.cc index 73721e324d..346b1dce29 100644 --- a/sdk/src/metrics/meter_context.cc +++ b/sdk/src/metrics/meter_context.cc @@ -46,8 +46,7 @@ opentelemetry::common::SystemTimestamp MeterContext::GetSDKStartTime() noexcept void MeterContext::AddMetricReader(std::unique_ptr reader) noexcept { - auto collector = - std::shared_ptr{new MetricCollector(shared_from_this(), std::move(reader))}; + auto collector = std::shared_ptr{new MetricCollector(this, std::move(reader))}; collectors_.push_back(collector); } diff --git a/sdk/src/metrics/state/metric_collector.cc b/sdk/src/metrics/state/metric_collector.cc index 95a9567d13..97f29a8fbe 100644 --- a/sdk/src/metrics/state/metric_collector.cc +++ b/sdk/src/metrics/state/metric_collector.cc @@ -16,10 +16,9 @@ namespace sdk namespace metrics { -MetricCollector::MetricCollector( - std::shared_ptr &&context, - std::unique_ptr metric_reader) - : meter_context_{std::move(context)}, metric_reader_{std::move(metric_reader)} +MetricCollector::MetricCollector(opentelemetry::sdk::metrics::MeterContext *context, + std::unique_ptr metric_reader) + : meter_context_{context}, metric_reader_{std::move(metric_reader)} { metric_reader_->SetMetricProducer(this); } @@ -33,6 +32,12 @@ AggregationTemporality MetricCollector::GetAggregationTemporality( bool MetricCollector::Collect( nostd::function_ref callback) noexcept { + if (!meter_context_) + { + OTEL_INTERNAL_LOG_ERROR("[MetricCollector::Collect] - Error during collecting." + << "The metric context is invalid"); + return false; + } ResourceMetrics resource_metrics; for (auto &meter : meter_context_->GetMeters()) { diff --git a/sdk/src/metrics/state/observable_registry.cc b/sdk/src/metrics/state/observable_registry.cc index 7e12d4a190..0d6d770f87 100644 --- a/sdk/src/metrics/state/observable_registry.cc +++ b/sdk/src/metrics/state/observable_registry.cc @@ -7,6 +7,7 @@ # include "opentelemetry/sdk/metrics/async_instruments.h" # include "opentelemetry/sdk/metrics/observer_result.h" # include "opentelemetry/sdk/metrics/state/metric_storage.h" +# include "opentelemetry/sdk_config.h" OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk @@ -51,6 +52,12 @@ void ObservableRegistry::Observe(opentelemetry::common::SystemTimestamp collecti auto storage = static_cast(callback_wrap->instrument) ->GetMetricStorage(); + if (!storage) + { + OTEL_INTERNAL_LOG_ERROR("[ObservableRegistry::Observe] - Error during observe." + << "The metric storage is invalid"); + return; + } if (value_type == InstrumentValueType::kDouble) { nostd::shared_ptr> ob_res( diff --git a/sdk/src/metrics/sync_instruments.cc b/sdk/src/metrics/sync_instruments.cc index 61e30248c3..5db5d02251 100644 --- a/sdk/src/metrics/sync_instruments.cc +++ b/sdk/src/metrics/sync_instruments.cc @@ -16,11 +16,22 @@ namespace metrics LongCounter::LongCounter(InstrumentDescriptor instrument_descriptor, std::unique_ptr storage) : Synchronous(instrument_descriptor, std::move(storage)) -{} +{ + if (!storage_) + { + OTEL_INTERNAL_LOG_ERROR("[LongCounter::LongCounter] - Error during constructing LongCounter." + << "The metric storage is invalid" + << "No value will be added"); + } +} void LongCounter::Add(long value, const opentelemetry::common::KeyValueIterable &attributes) noexcept { + if (!storage_) + { + return; + } auto context = opentelemetry::context::Context{}; return storage_->RecordLong(value, attributes, context); } @@ -29,29 +40,53 @@ void LongCounter::Add(long value, const opentelemetry::common::KeyValueIterable &attributes, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } return storage_->RecordLong(value, attributes, context); } void LongCounter::Add(long value) noexcept { auto context = opentelemetry::context::Context{}; + if (!storage_) + { + return; + } return storage_->RecordLong(value, context); } void LongCounter::Add(long value, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } return storage_->RecordLong(value, context); } DoubleCounter::DoubleCounter(InstrumentDescriptor instrument_descriptor, std::unique_ptr storage) : Synchronous(instrument_descriptor, std::move(storage)) -{} +{ + if (!storage_) + { + OTEL_INTERNAL_LOG_ERROR( + "[DoubleCounter::DoubleCounter] - Error during constructing DoubleCounter." + << "The metric storage is invalid" + << "No value will be added"); + } +} void DoubleCounter::Add(double value, const opentelemetry::common::KeyValueIterable &attributes) noexcept { auto context = opentelemetry::context::Context{}; + if (!storage_) + { + return; + } return storage_->RecordDouble(value, attributes, context); } @@ -59,29 +94,53 @@ void DoubleCounter::Add(double value, const opentelemetry::common::KeyValueIterable &attributes, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } return storage_->RecordDouble(value, attributes, context); } void DoubleCounter::Add(double value) noexcept { auto context = opentelemetry::context::Context{}; + if (!storage_) + { + return; + } return storage_->RecordDouble(value, context); } void DoubleCounter::Add(double value, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } return storage_->RecordDouble(value, context); } LongUpDownCounter::LongUpDownCounter(InstrumentDescriptor instrument_descriptor, std::unique_ptr storage) : Synchronous(instrument_descriptor, std::move(storage)) -{} +{ + if (!storage_) + { + OTEL_INTERNAL_LOG_ERROR( + "[LongUpDownCounter::LongUpDownCounter] - Error during constructing LongUpDownCounter." + << "The metric storage is invalid" + << "No value will be added"); + } +} void LongUpDownCounter::Add(long value, const opentelemetry::common::KeyValueIterable &attributes) noexcept { auto context = opentelemetry::context::Context{}; + if (!storage_) + { + return; + } return storage_->RecordLong(value, attributes, context); } @@ -89,24 +148,45 @@ void LongUpDownCounter::Add(long value, const opentelemetry::common::KeyValueIterable &attributes, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } return storage_->RecordLong(value, attributes, context); } void LongUpDownCounter::Add(long value) noexcept { auto context = opentelemetry::context::Context{}; + if (!storage_) + { + return; + } return storage_->RecordLong(value, context); } void LongUpDownCounter::Add(long value, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } return storage_->RecordLong(value, context); } DoubleUpDownCounter::DoubleUpDownCounter(InstrumentDescriptor instrument_descriptor, std::unique_ptr storage) : Synchronous(instrument_descriptor, std::move(storage)) -{} +{ + if (!storage_) + { + OTEL_INTERNAL_LOG_ERROR( + "[DoubleUpDownCounter::DoubleUpDownCounter] - Error during constructing " + "DoubleUpDownCounter." + << "The metric storage is invalid" + << "No value will be added"); + } +} void DoubleUpDownCounter::Add(double value, const opentelemetry::common::KeyValueIterable &attributes) noexcept @@ -119,24 +199,44 @@ void DoubleUpDownCounter::Add(double value, const opentelemetry::common::KeyValueIterable &attributes, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } return storage_->RecordDouble(value, attributes, context); } void DoubleUpDownCounter::Add(double value) noexcept { + if (!storage_) + { + return; + } auto context = opentelemetry::context::Context{}; return storage_->RecordDouble(value, context); } void DoubleUpDownCounter::Add(double value, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } return storage_->RecordDouble(value, context); } LongHistogram::LongHistogram(InstrumentDescriptor instrument_descriptor, std::unique_ptr storage) : Synchronous(instrument_descriptor, std::move(storage)) -{} +{ + if (!storage_) + { + OTEL_INTERNAL_LOG_ERROR( + "[LongHistogram::LongHistogram] - Error during constructing LongHistogram." + << "The metric storage is invalid" + << "No value will be added"); + } +} void LongHistogram::Record(long value, const opentelemetry::common::KeyValueIterable &attributes, @@ -167,12 +267,24 @@ void LongHistogram::Record(long value, const opentelemetry::context::Context &co DoubleHistogram::DoubleHistogram(InstrumentDescriptor instrument_descriptor, std::unique_ptr storage) : Synchronous(instrument_descriptor, std::move(storage)) -{} +{ + if (!storage_) + { + OTEL_INTERNAL_LOG_ERROR( + "[DoubleHistogram::DoubleHistogram] - Error during constructing DoubleHistogram." + << "The metric storage is invalid" + << "No value will be added"); + } +} void DoubleHistogram::Record(double value, const opentelemetry::common::KeyValueIterable &attributes, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } if (value < 0 || std::isnan(value) || std::isinf(value)) { OTEL_INTERNAL_LOG_WARN( @@ -186,6 +298,10 @@ void DoubleHistogram::Record(double value, void DoubleHistogram::Record(double value, const opentelemetry::context::Context &context) noexcept { + if (!storage_) + { + return; + } if (value < 0 || std::isnan(value) || std::isinf(value)) { OTEL_INTERNAL_LOG_WARN( diff --git a/sdk/test/metrics/metric_reader_test.cc b/sdk/test/metrics/metric_reader_test.cc index 82fca653f6..9f15aa1cbe 100644 --- a/sdk/test/metrics/metric_reader_test.cc +++ b/sdk/test/metrics/metric_reader_test.cc @@ -38,7 +38,7 @@ TEST(MetricReaderTest, BasicTests) std::unique_ptr metric_reader2(new MockMetricReader()); std::shared_ptr meter_context2(new MeterContext()); std::shared_ptr metric_producer{ - new MetricCollector(std::move(meter_context2), std::move(metric_reader2))}; + new MetricCollector(meter_context2.get(), std::move(metric_reader2))}; EXPECT_NO_THROW(metric_producer->Collect([](ResourceMetrics &metric_data) { return true; })); } #endif