From f581d6fbfdc317ecb568e0a80cdaaf70bd137b8d Mon Sep 17 00:00:00 2001 From: Lalit Date: Thu, 13 Oct 2022 17:57:09 -0700 Subject: [PATCH 1/5] fix meter_context forceflush logic --- sdk/src/metrics/meter_context.cc | 25 ++++++++++++--------- sdk/test/metrics/meter_provider_sdk_test.cc | 6 ++++- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/sdk/src/metrics/meter_context.cc b/sdk/src/metrics/meter_context.cc index 8c7abd97e9..68785f2af3 100644 --- a/sdk/src/metrics/meter_context.cc +++ b/sdk/src/metrics/meter_context.cc @@ -67,6 +67,7 @@ void MeterContext::AddMeter(std::shared_ptr meter) bool MeterContext::Shutdown() noexcept { bool result = true; + // Shutdown only once. if (!shutdown_latch_.test_and_set(std::memory_order_acquire)) { @@ -80,6 +81,10 @@ bool MeterContext::Shutdown() noexcept OTEL_INTERNAL_LOG_WARN("[MeterContext::Shutdown] Unable to shutdown all metric readers"); } } + else + { + OTEL_INTERNAL_LOG_WARN("[MeterContext::Shutdown] Shutdown can be invoked only once."); + } return result; } @@ -87,18 +92,16 @@ bool MeterContext::ForceFlush(std::chrono::microseconds timeout) noexcept { // TODO - Implement timeout logic. bool result = true; - if (!shutdown_latch_.test_and_set(std::memory_order_acquire)) + // Simultaneous flush not allowed. + const std::lock_guard locked(forceflush_lock_); + for (auto &collector : collectors_) { - - for (auto &collector : collectors_) - { - bool status = std::static_pointer_cast(collector)->ForceFlush(timeout); - result = result && status; - } - if (!result) - { - OTEL_INTERNAL_LOG_WARN("[MeterContext::ForceFlush] Unable to ForceFlush all metric readers"); - } + bool status = std::static_pointer_cast(collector)->ForceFlush(timeout); + result = result && status; + } + if (!result) + { + OTEL_INTERNAL_LOG_WARN("[MeterContext::ForceFlush] Unable to ForceFlush all metric readers"); } return result; } diff --git a/sdk/test/metrics/meter_provider_sdk_test.cc b/sdk/test/metrics/meter_provider_sdk_test.cc index d21e45f603..eb57dd026b 100644 --- a/sdk/test/metrics/meter_provider_sdk_test.cc +++ b/sdk/test/metrics/meter_provider_sdk_test.cc @@ -81,7 +81,7 @@ TEST(MeterProvider, GetMeter) ASSERT_EQ(m4, m5); ASSERT_NE(m3, m6); - // Should be an sdk::trace::Tracer with the processor attached. + // Should be an sdk::metrics::Meter # ifdef OPENTELEMETRY_RTTI_ENABLED auto sdkMeter1 = dynamic_cast(m1.get()); # else @@ -98,5 +98,9 @@ TEST(MeterProvider, GetMeter) std::unique_ptr meter_selector{new MeterSelector("name1", "version1", "schema1")}; mp1.AddView(std::move(instrument_selector), std::move(meter_selector), std::move(view)); + + // cleanup properly without crash + mp1.ForceFlush(); + mp1.Shutdown(); } #endif From 2d901e4a2302f3bd3201375b4344468143a102a4 Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 14 Oct 2022 15:50:10 -0700 Subject: [PATCH 2/5] call shutdown during MeterProvider destruction --- sdk/include/opentelemetry/sdk/metrics/meter_provider.h | 2 ++ sdk/src/metrics/meter_provider.cc | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/metrics/meter_provider.h b/sdk/include/opentelemetry/sdk/metrics/meter_provider.h index 685f43e747..1a7fea4445 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter_provider.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter_provider.h @@ -85,6 +85,8 @@ class MeterProvider final : public opentelemetry::metrics::MeterProvider */ bool ForceFlush(std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept; + ~MeterProvider(); + private: std::shared_ptr context_; std::mutex lock_; diff --git a/sdk/src/metrics/meter_provider.cc b/sdk/src/metrics/meter_provider.cc index 3a221b5db0..1df3f16c1b 100644 --- a/sdk/src/metrics/meter_provider.cc +++ b/sdk/src/metrics/meter_provider.cc @@ -88,6 +88,15 @@ bool MeterProvider::ForceFlush(std::chrono::microseconds timeout) noexcept return context_->ForceFlush(timeout); } +/** + * Shutdown MeterContext when MeterProvider is destroyed. + * + */ +MeterProvider::~MeterProvider() +{ + context_->Shutdown(); +} + } // namespace metrics } // namespace sdk OPENTELEMETRY_END_NAMESPACE From 94fb5ac1aac01186e7dc70a35cd08023d39d4473 Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 14 Oct 2022 16:36:38 -0700 Subject: [PATCH 3/5] fix --- sdk/src/metrics/meter_provider.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sdk/src/metrics/meter_provider.cc b/sdk/src/metrics/meter_provider.cc index 1df3f16c1b..5077dac8bb 100644 --- a/sdk/src/metrics/meter_provider.cc +++ b/sdk/src/metrics/meter_provider.cc @@ -94,7 +94,10 @@ bool MeterProvider::ForceFlush(std::chrono::microseconds timeout) noexcept */ MeterProvider::~MeterProvider() { - context_->Shutdown(); + if (context_) + { + context_->Shutdown(); + } } } // namespace metrics From e31159d9f09d58b8c24e5efc51a9d79577925bb1 Mon Sep 17 00:00:00 2001 From: Lalit Date: Fri, 14 Oct 2022 23:45:41 -0700 Subject: [PATCH 4/5] fix merge conflict --- sdk/src/metrics/meter_context.cc | 72 +++++++++++++++----------------- 1 file changed, 34 insertions(+), 38 deletions(-) diff --git a/sdk/src/metrics/meter_context.cc b/sdk/src/metrics/meter_context.cc index c689a528c9..398934b520 100644 --- a/sdk/src/metrics/meter_context.cc +++ b/sdk/src/metrics/meter_context.cc @@ -93,56 +93,52 @@ bool MeterContext::ForceFlush(std::chrono::microseconds timeout) noexcept bool result = true; // Simultaneous flush not allowed. const std::lock_guard locked(forceflush_lock_); + // Convert to nanos to prevent overflow + auto timeout_ns = std::chrono::nanoseconds::max(); + if (std::chrono::duration_cast(timeout_ns) > timeout) + { + timeout_ns = std::chrono::duration_cast(timeout); + } + + auto current_time = std::chrono::system_clock::now(); + std::chrono::system_clock::time_point expire_time; + auto overflow_checker = std::chrono::system_clock::time_point::max(); + + // check if the expected expire time doesn't overflow. + if (overflow_checker - current_time > timeout_ns) + { + expire_time = + current_time + std::chrono::duration_cast(timeout_ns); + } + else + { + // overflow happens, reset expire time to max. + expire_time = overflow_checker; + } + for (auto &collector : collectors_) { - // Convert to nanos to prevent overflow - auto timeout_ns = std::chrono::nanoseconds::max(); - if (std::chrono::duration_cast(timeout_ns) > timeout) + if (!std::static_pointer_cast(collector)->ForceFlush( + std::chrono::duration_cast(timeout_ns))) { - timeout_ns = std::chrono::duration_cast(timeout); + result = false; } - auto current_time = std::chrono::system_clock::now(); - std::chrono::system_clock::time_point expire_time; - auto overflow_checker = std::chrono::system_clock::time_point::max(); + current_time = std::chrono::system_clock::now(); - // check if the expected expire time doesn't overflow. - if (overflow_checker - current_time > timeout_ns) + if (expire_time >= current_time) { - expire_time = current_time + - std::chrono::duration_cast(timeout_ns); + timeout_ns = std::chrono::duration_cast(expire_time - current_time); } else { - // overflow happens, reset expire time to max. - expire_time = overflow_checker; - } - - for (auto &collector : collectors_) - { - if (!std::static_pointer_cast(collector)->ForceFlush( - std::chrono::duration_cast(timeout_ns))) - { - result = false; - } - - current_time = std::chrono::system_clock::now(); - - if (expire_time >= current_time) - { - timeout_ns = - std::chrono::duration_cast(expire_time - current_time); - } - else - { - timeout_ns = std::chrono::nanoseconds::zero(); - } - } - if (!result) - { - OTEL_INTERNAL_LOG_WARN("[MeterContext::ForceFlush] Unable to ForceFlush all metric readers"); + timeout_ns = std::chrono::nanoseconds::zero(); } } + if (!result) + { + OTEL_INTERNAL_LOG_WARN("[MeterContext::ForceFlush] Unable to ForceFlush all metric readers"); + } return result; } From 2768cc146d80192e7c7ea655e411af25bafd404c Mon Sep 17 00:00:00 2001 From: Lalit Date: Sat, 15 Oct 2022 00:12:23 -0700 Subject: [PATCH 5/5] fix --- sdk/include/opentelemetry/sdk/metrics/meter_provider.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/meter_provider.h b/sdk/include/opentelemetry/sdk/metrics/meter_provider.h index 1a7fea4445..8fdca2daa8 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter_provider.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter_provider.h @@ -85,7 +85,7 @@ class MeterProvider final : public opentelemetry::metrics::MeterProvider */ bool ForceFlush(std::chrono::microseconds timeout = (std::chrono::microseconds::max)()) noexcept; - ~MeterProvider(); + ~MeterProvider() override; private: std::shared_ptr context_;