From fd581bf79cc7bafd2689343d8d50d2282cb6e616 Mon Sep 17 00:00:00 2001 From: Marc Alff Date: Fri, 7 Jul 2023 10:54:51 +0200 Subject: [PATCH 1/3] Fixes #2203 --- examples/grpc/tracer_common.h | 4 ++-- examples/http/tracer_common.h | 4 ++-- ext/test/w3c_tracecontext_test/main.cc | 7 ++++--- .../opentelemetry/sdk/logs/logger_context.h | 6 +++--- .../opentelemetry/sdk/logs/logger_provider.h | 6 +++--- .../sdk/logs/logger_provider_factory.h | 2 +- .../opentelemetry/sdk/trace/tracer_provider.h | 4 ++-- .../sdk/trace/tracer_provider_factory.h | 2 +- sdk/src/logs/logger_provider.cc | 14 ++++++-------- sdk/src/logs/logger_provider_factory.cc | 6 ++++-- sdk/src/trace/tracer_provider.cc | 4 ++-- sdk/src/trace/tracer_provider_factory.cc | 6 ++++-- sdk/test/logs/logger_provider_sdk_test.cc | 6 ++++-- sdk/test/trace/tracer_provider_test.cc | 16 ++++++++++------ 14 files changed, 48 insertions(+), 39 deletions(-) diff --git a/examples/grpc/tracer_common.h b/examples/grpc/tracer_common.h index 347e7a8184..e26f6bd2c6 100644 --- a/examples/grpc/tracer_common.h +++ b/examples/grpc/tracer_common.h @@ -80,10 +80,10 @@ void InitTracer() std::vector> processors; processors.push_back(std::move(processor)); // Default is an always-on sampler. - std::shared_ptr context = + std::unique_ptr context = opentelemetry::sdk::trace::TracerContextFactory::Create(std::move(processors)); std::shared_ptr provider = - opentelemetry::sdk::trace::TracerProviderFactory::Create(context); + opentelemetry::sdk::trace::TracerProviderFactory::Create(std::move(context)); // Set the global trace provider opentelemetry::trace::Provider::SetTracerProvider(provider); diff --git a/examples/http/tracer_common.h b/examples/http/tracer_common.h index 7d9f1ca571..8b61e7bec3 100644 --- a/examples/http/tracer_common.h +++ b/examples/http/tracer_common.h @@ -70,10 +70,10 @@ void InitTracer() std::vector> processors; processors.push_back(std::move(processor)); // Default is an always-on sampler. - std::shared_ptr context = + std::unique_ptr context = opentelemetry::sdk::trace::TracerContextFactory::Create(std::move(processors)); std::shared_ptr provider = - opentelemetry::sdk::trace::TracerProviderFactory::Create(context); + opentelemetry::sdk::trace::TracerProviderFactory::Create(std::move(context)); // Set the global trace provider opentelemetry::trace::Provider::SetTracerProvider(provider); diff --git a/ext/test/w3c_tracecontext_test/main.cc b/ext/test/w3c_tracecontext_test/main.cc index 92807dd8ad..349626e219 100644 --- a/ext/test/w3c_tracecontext_test/main.cc +++ b/ext/test/w3c_tracecontext_test/main.cc @@ -55,9 +55,10 @@ void initTracer() new trace_sdk::SimpleSpanProcessor(std::move(exporter))); std::vector> processors; processors.push_back(std::move(processor)); - auto context = std::make_shared(std::move(processors)); - auto provider = - nostd::shared_ptr(new trace_sdk::TracerProvider(context)); + auto context = std::unique_ptr( + new trace_sdk::TracerContext(std::move(processors))); + auto provider = nostd::shared_ptr( + new trace_sdk::TracerProvider(std::move(context))); // Set the global trace provider trace_api::Provider::SetTracerProvider(provider); } diff --git a/sdk/include/opentelemetry/sdk/logs/logger_context.h b/sdk/include/opentelemetry/sdk/logs/logger_context.h index 0b2de9bc15..196ecd545f 100644 --- a/sdk/include/opentelemetry/sdk/logs/logger_context.h +++ b/sdk/include/opentelemetry/sdk/logs/logger_context.h @@ -9,6 +9,7 @@ # include # include +# include "opentelemetry/sdk/logs/processor.h" # include "opentelemetry/sdk/resource/resource.h" # include "opentelemetry/version.h" @@ -17,7 +18,6 @@ namespace sdk { namespace logs { -class LogRecordProcessor; /** * A class which stores the LoggerContext context. @@ -40,10 +40,10 @@ class LoggerContext opentelemetry::sdk::resource::Resource::Create({})) noexcept; /** - * Attaches a log processor to list of configured processors to this tracer context. + * Attaches a log processor to list of configured processors to this logger context. * Processor once attached can't be removed. * @param processor The new log processor for this tracer. This must not be - * a nullptr. Ownership is given to the `TracerContext`. + * a nullptr. Ownership is given to the `LoggerContext`. * * Note: This method is not thread safe. */ diff --git a/sdk/include/opentelemetry/sdk/logs/logger_provider.h b/sdk/include/opentelemetry/sdk/logs/logger_provider.h index 3c9211cc36..01dc241470 100644 --- a/sdk/include/opentelemetry/sdk/logs/logger_provider.h +++ b/sdk/include/opentelemetry/sdk/logs/logger_provider.h @@ -56,9 +56,9 @@ class LoggerProvider final : public opentelemetry::logs::LoggerProvider /** * Initialize a new logger provider with a specified context - * @param context The shared logger configuration/pipeline for this provider. + * @param context The owned logger configuration/pipeline for this provider. */ - explicit LoggerProvider(std::shared_ptr context) noexcept; + explicit LoggerProvider(std::unique_ptr context) noexcept; ~LoggerProvider() override; @@ -106,7 +106,7 @@ class LoggerProvider final : public opentelemetry::logs::LoggerProvider private: // order of declaration is important here - loggers should destroy only after context. std::vector> loggers_; - std::shared_ptr context_; + std::shared_ptr context_; std::mutex lock_; }; } // namespace logs diff --git a/sdk/include/opentelemetry/sdk/logs/logger_provider_factory.h b/sdk/include/opentelemetry/sdk/logs/logger_provider_factory.h index bbd715dc89..5d74d7918d 100644 --- a/sdk/include/opentelemetry/sdk/logs/logger_provider_factory.h +++ b/sdk/include/opentelemetry/sdk/logs/logger_provider_factory.h @@ -64,7 +64,7 @@ class OPENTELEMETRY_EXPORT LoggerProviderFactory * Create a LoggerProvider. */ static std::unique_ptr Create( - std::shared_ptr context); + std::unique_ptr context); }; } // namespace logs diff --git a/sdk/include/opentelemetry/sdk/trace/tracer_provider.h b/sdk/include/opentelemetry/sdk/trace/tracer_provider.h index e15f686898..092db1eaf8 100644 --- a/sdk/include/opentelemetry/sdk/trace/tracer_provider.h +++ b/sdk/include/opentelemetry/sdk/trace/tracer_provider.h @@ -57,9 +57,9 @@ class TracerProvider final : public opentelemetry::trace::TracerProvider /** * Initialize a new tracer provider with a specified context - * @param context The shared tracer configuration/pipeline for this provider. + * @param context The owned tracer configuration/pipeline for this provider. */ - explicit TracerProvider(std::shared_ptr context) noexcept; + explicit TracerProvider(std::unique_ptr context) noexcept; ~TracerProvider() override; diff --git a/sdk/include/opentelemetry/sdk/trace/tracer_provider_factory.h b/sdk/include/opentelemetry/sdk/trace/tracer_provider_factory.h index 93bd1085ac..e9dfa82edb 100644 --- a/sdk/include/opentelemetry/sdk/trace/tracer_provider_factory.h +++ b/sdk/include/opentelemetry/sdk/trace/tracer_provider_factory.h @@ -78,7 +78,7 @@ class OPENTELEMETRY_EXPORT TracerProviderFactory /* Create with a tracer context. */ static std::unique_ptr Create( - std::shared_ptr context); + std::unique_ptr context); }; } // namespace trace diff --git a/sdk/src/logs/logger_provider.cc b/sdk/src/logs/logger_provider.cc index 1e1b83c7de..638ece3802 100644 --- a/sdk/src/logs/logger_provider.cc +++ b/sdk/src/logs/logger_provider.cc @@ -24,29 +24,27 @@ LoggerProvider::LoggerProvider(std::unique_ptr &&processor, { std::vector> processors; processors.emplace_back(std::move(processor)); - context_ = std::make_shared(std::move(processors), std::move(resource)); + context_ = std::make_shared(std::move(processors), std::move(resource)); OTEL_INTERNAL_LOG_DEBUG("[LoggerProvider] LoggerProvider created."); } LoggerProvider::LoggerProvider(std::vector> &&processors, opentelemetry::sdk::resource::Resource resource) noexcept - : context_{ - std::make_shared(std::move(processors), std::move(resource))} + : context_{std::make_shared(std::move(processors), std::move(resource))} {} LoggerProvider::LoggerProvider() noexcept - : context_{std::make_shared( - std::vector>{})} + : context_{std::make_shared(std::vector>{})} {} -LoggerProvider::LoggerProvider(std::shared_ptr context) noexcept - : context_{context} +LoggerProvider::LoggerProvider(std::unique_ptr context) noexcept + : context_(std::move(context)) {} LoggerProvider::~LoggerProvider() { // Logger hold the shared pointer to the context. So we can not use destructor of LoggerContext to - // Shutdown and flush all pending recordables when we hasve more than one loggers.These + // Shutdown and flush all pending recordables when we have more than one loggers. These // recordables may use the raw pointer of instrumentation_scope_ in Logger if (context_) { diff --git a/sdk/src/logs/logger_provider_factory.cc b/sdk/src/logs/logger_provider_factory.cc index 3177b9180e..167d6cbf65 100644 --- a/sdk/src/logs/logger_provider_factory.cc +++ b/sdk/src/logs/logger_provider_factory.cc @@ -4,6 +4,7 @@ #ifdef ENABLE_LOGS_PREVIEW # include "opentelemetry/sdk/logs/logger_provider_factory.h" +# include "opentelemetry/sdk/logs/logger_context.h" # include "opentelemetry/sdk/logs/logger_provider.h" # include "opentelemetry/sdk/resource/resource.h" @@ -46,9 +47,10 @@ std::unique_ptr LoggerProviderFactory::Crea } std::unique_ptr LoggerProviderFactory::Create( - std::shared_ptr context) + std::unique_ptr context) { - std::unique_ptr provider(new LoggerProvider(context)); + std::unique_ptr provider( + new LoggerProvider(std::move(context))); return provider; } diff --git a/sdk/src/trace/tracer_provider.cc b/sdk/src/trace/tracer_provider.cc index 7ac66ed6db..13d56f9966 100644 --- a/sdk/src/trace/tracer_provider.cc +++ b/sdk/src/trace/tracer_provider.cc @@ -16,8 +16,8 @@ namespace trace namespace resource = opentelemetry::sdk::resource; namespace trace_api = opentelemetry::trace; -TracerProvider::TracerProvider(std::shared_ptr context) noexcept - : context_{context} +TracerProvider::TracerProvider(std::unique_ptr context) noexcept + : context_(std::move(context)) { OTEL_INTERNAL_LOG_DEBUG("[TracerProvider] TracerProvider created."); } diff --git a/sdk/src/trace/tracer_provider_factory.cc b/sdk/src/trace/tracer_provider_factory.cc index 158ae949ea..c9b02a13ad 100644 --- a/sdk/src/trace/tracer_provider_factory.cc +++ b/sdk/src/trace/tracer_provider_factory.cc @@ -5,6 +5,7 @@ #include "opentelemetry/sdk/trace/processor.h" #include "opentelemetry/sdk/trace/random_id_generator_factory.h" #include "opentelemetry/sdk/trace/samplers/always_on_factory.h" +#include "opentelemetry/sdk/trace/tracer_context.h" #include "opentelemetry/sdk/trace/tracer_provider.h" namespace trace_api = opentelemetry::trace; @@ -87,9 +88,10 @@ std::unique_ptr TracerProviderFactory::Cre } std::unique_ptr TracerProviderFactory::Create( - std::shared_ptr context) + std::unique_ptr context) { - std::unique_ptr provider(new trace_sdk::TracerProvider(context)); + std::unique_ptr provider( + new trace_sdk::TracerProvider(std::move(context))); return provider; } diff --git a/sdk/test/logs/logger_provider_sdk_test.cc b/sdk/test/logs/logger_provider_sdk_test.cc index e9bc897745..c6b8dec372 100644 --- a/sdk/test/logs/logger_provider_sdk_test.cc +++ b/sdk/test/logs/logger_provider_sdk_test.cc @@ -170,7 +170,8 @@ TEST(LoggerProviderSDK, Shutdown) std::vector> processors; processors.push_back(std::move(processor)); - LoggerProvider lp(std::make_shared(std::move(processors))); + std::unique_ptr context(new LoggerContext(std::move(processors))); + LoggerProvider lp(std::move(context)); EXPECT_TRUE(lp.Shutdown()); EXPECT_TRUE(processor_ptr->IsShutdown()); @@ -185,7 +186,8 @@ TEST(LoggerProviderSDK, ForceFlush) std::vector> processors; processors.push_back(std::move(processor)); - LoggerProvider lp(std::make_shared(std::move(processors))); + std::unique_ptr context(new LoggerContext(std::move(processors))); + LoggerProvider lp(std::move(context)); EXPECT_TRUE(lp.ForceFlush()); } diff --git a/sdk/test/trace/tracer_provider_test.cc b/sdk/test/trace/tracer_provider_test.cc index 641fc72c54..364efc0208 100644 --- a/sdk/test/trace/tracer_provider_test.cc +++ b/sdk/test/trace/tracer_provider_test.cc @@ -20,7 +20,9 @@ TEST(TracerProvider, GetTracer) std::unique_ptr processor(new SimpleSpanProcessor(nullptr)); std::vector> processors; processors.push_back(std::move(processor)); - TracerProvider tp1(std::make_shared(std::move(processors), Resource::Create({}))); + std::unique_ptr context1( + new TracerContext(std::move(processors), Resource::Create({}))); + TracerProvider tp1(std::move(context1)); auto t1 = tp1.GetTracer("test"); auto t2 = tp1.GetTracer("test"); auto t3 = tp1.GetTracer("different", "1.0.0"); @@ -49,10 +51,11 @@ TEST(TracerProvider, GetTracer) std::unique_ptr processor2(new SimpleSpanProcessor(nullptr)); std::vector> processors2; processors2.push_back(std::move(processor2)); - TracerProvider tp2( - std::make_shared(std::move(processors2), Resource::Create({}), - std::unique_ptr(new AlwaysOffSampler()), - std::unique_ptr(new RandomIdGenerator))); + std::unique_ptr context2( + new TracerContext(std::move(processors2), Resource::Create({}), + std::unique_ptr(new AlwaysOffSampler()), + std::unique_ptr(new RandomIdGenerator))); + TracerProvider tp2(std::move(context2)); #ifdef OPENTELEMETRY_RTTI_ENABLED auto sdkTracer2 = dynamic_cast(tp2.GetTracer("test").get()); #else @@ -81,7 +84,8 @@ TEST(TracerProvider, Shutdown) std::vector> processors; processors.push_back(std::move(processor)); - TracerProvider tp1(std::make_shared(std::move(processors))); + std::unique_ptr context1(new TracerContext(std::move(processors))); + TracerProvider tp1(std::move(context1)); EXPECT_TRUE(tp1.Shutdown()); From 4fc10de43543e923e7d3429b80062a82a3f83b53 Mon Sep 17 00:00:00 2001 From: Marc Alff Date: Fri, 7 Jul 2023 11:02:10 +0200 Subject: [PATCH 2/3] Add CHANGELOG --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83814e8c59..7275955f74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ Increment the: * [SDK] MeterProvider should own MeterContext, not share it [#2218](https://github.com/open-telemetry/opentelemetry-cpp/pull/2218) +* [SDK] TracerProvider should own TracerContext, not share it + [#2221](https://github.com/open-telemetry/opentelemetry-cpp/pull/2221) + Important changes: * [REMOVAL] Remove the jaeger exporter @@ -37,6 +40,14 @@ Important changes: `MeterContext`, instead of a `shared_ptr`. * Please adjust SDK configuration code accordingly. +* [SDK] TracerProvider should own TracerContext, not share it + [#2221](https://github.com/open-telemetry/opentelemetry-cpp/pull/2221) + * The `TracerProvider` constructor now takes a `unique_ptr` on + `TracerContext`, instead of a `shared_ptr`. + * The `LoggerProvider` constructor now takes a `unique_ptr` on + `LoggerContext`, instead of a `shared_ptr`. + * Please adjust SDK configuration code accordingly. + ## [1.9.1] 2023-05-26 * [DEPRECATION] Drop C++11 support From 9b2ad7f7bb86a3c8210dfa08ff2d4ea16bd46eeb Mon Sep 17 00:00:00 2001 From: Marc Alff Date: Sat, 8 Jul 2023 15:04:27 +0200 Subject: [PATCH 3/3] Document breaking change in CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7275955f74..4849ece1ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,7 @@ Increment the: * [SDK] TracerProvider should own TracerContext, not share it [#2221](https://github.com/open-telemetry/opentelemetry-cpp/pull/2221) -Important changes: +Breaking changes: * [REMOVAL] Remove the jaeger exporter [#2031](https://github.com/open-telemetry/opentelemetry-cpp/pull/2031)