Code review comments (sdk_builder)

marcalff · marcalff · commit eb018dd1d98d · 2025-07-20T00:34:16.000+02:00
diff --git a/examples/yaml/main.cc b/examples/yaml/main.cc
@@ -132,15 +132,15 @@ void InitOtel(const std::string &config_file)
 
   if (sdk != nullptr)
   {
-    sdk->Install();
+    sdk->Init();
   }
 }
 
 void CleanupOtel()
 {
   if (sdk != nullptr)
   {
-    sdk->UnInstall();
+    sdk->Cleanup();
   }
   sdk.reset(nullptr);
 }
diff --git a/functional/configuration/main.cc b/functional/configuration/main.cc
@@ -1,6 +1,9 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
+// NOTE: This file is adapted from example/yaml/main.cc,
+// to execute functional tests driven by shelltest.
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -198,15 +201,15 @@ void InitOtel(const std::string &config_file)
 
   if (sdk != nullptr)
   {
-    sdk->Install();
+    sdk->Init();
   }
 }
 
 void CleanupOtel()
 {
   if (sdk != nullptr)
   {
-    sdk->UnInstall();
+    sdk->Cleanup();
   }
   sdk.reset(nullptr);
 }
diff --git a/sdk/include/opentelemetry/sdk/configuration/configured_sdk.h b/sdk/include/opentelemetry/sdk/configuration/configured_sdk.h
@@ -27,16 +27,28 @@ class ConfiguredSdk
       std::shared_ptr<Registry> registry,
       const std::unique_ptr<opentelemetry::sdk::configuration::Configuration> &model);
 
-  ConfiguredSdk() : resource(opentelemetry::sdk::resource::Resource::GetEmpty()) {}
-
-  void Install();
-  void UnInstall();
-
-  opentelemetry::sdk::resource::Resource resource;
-  std::shared_ptr<opentelemetry::sdk::trace::TracerProvider> tracer_provider;
-  std::shared_ptr<opentelemetry::context::propagation::TextMapPropagator> propagator;
-  std::shared_ptr<opentelemetry::sdk::metrics::MeterProvider> meter_provider;
-  std::shared_ptr<opentelemetry::sdk::logs::LoggerProvider> logger_provider;
+  ConfiguredSdk(
+      opentelemetry::sdk::resource::Resource &&resource,
+      const std::shared_ptr<opentelemetry::sdk::trace::TracerProvider> &tracer_provider,
+      const std::shared_ptr<opentelemetry::context::propagation::TextMapPropagator> &propagator,
+      const std::shared_ptr<opentelemetry::sdk::metrics::MeterProvider> &meter_provider,
+      const std::shared_ptr<opentelemetry::sdk::logs::LoggerProvider> &logger_provider)
+      : resource_(std::move(resource)),
+        tracer_provider_(tracer_provider),
+        propagator_(propagator),
+        meter_provider_(meter_provider),
+        logger_provider_(logger_provider)
+  {}
+
+  void Init();
+  void Cleanup();
+
+private:
+  opentelemetry::sdk::resource::Resource resource_;
+  std::shared_ptr<opentelemetry::sdk::trace::TracerProvider> tracer_provider_;
+  std::shared_ptr<opentelemetry::context::propagation::TextMapPropagator> propagator_;
+  std::shared_ptr<opentelemetry::sdk::metrics::MeterProvider> meter_provider_;
+  std::shared_ptr<opentelemetry::sdk::logs::LoggerProvider> logger_provider_;
 };
 
 }  // namespace configuration
diff --git a/sdk/src/configuration/configured_sdk.cc b/sdk/src/configuration/configured_sdk.cc
@@ -56,53 +56,53 @@ std::unique_ptr<ConfiguredSdk> ConfiguredSdk::Create(
   return sdk;
 }
 
-void ConfiguredSdk::Install()
+void ConfiguredSdk::Init()
 {
-  if (propagator)
+  if (propagator_)
   {
-    opentelemetry::context::propagation::GlobalTextMapPropagator::SetGlobalPropagator(propagator);
+    opentelemetry::context::propagation::GlobalTextMapPropagator::SetGlobalPropagator(propagator_);
   }
 
-  if (tracer_provider)
+  if (tracer_provider_)
   {
-    std::shared_ptr<opentelemetry::trace::TracerProvider> api_tracer_provider = tracer_provider;
+    std::shared_ptr<opentelemetry::trace::TracerProvider> api_tracer_provider = tracer_provider_;
     opentelemetry::trace::Provider::SetTracerProvider(api_tracer_provider);
   }
 
-  if (meter_provider)
+  if (meter_provider_)
   {
-    std::shared_ptr<opentelemetry::metrics::MeterProvider> api_meter_provider = meter_provider;
+    std::shared_ptr<opentelemetry::metrics::MeterProvider> api_meter_provider = meter_provider_;
     opentelemetry::metrics::Provider::SetMeterProvider(api_meter_provider);
   }
 
-  if (logger_provider)
+  if (logger_provider_)
   {
-    std::shared_ptr<opentelemetry::logs::LoggerProvider> api_logger_provider = logger_provider;
+    std::shared_ptr<opentelemetry::logs::LoggerProvider> api_logger_provider = logger_provider_;
     opentelemetry::logs::Provider::SetLoggerProvider(api_logger_provider);
   }
 }
 
-void ConfiguredSdk::UnInstall()
+void ConfiguredSdk::Cleanup()
 {
-  if (propagator)
+  if (propagator_)
   {
     std::shared_ptr<opentelemetry::context::propagation::TextMapPropagator> none;
     opentelemetry::context::propagation::GlobalTextMapPropagator::SetGlobalPropagator(none);
   }
 
-  if (tracer_provider)
+  if (tracer_provider_)
   {
     std::shared_ptr<opentelemetry::trace::TracerProvider> none;
     opentelemetry::trace::Provider::SetTracerProvider(none);
   }
 
-  if (meter_provider)
+  if (meter_provider_)
   {
     std::shared_ptr<opentelemetry::metrics::MeterProvider> none;
     opentelemetry::metrics::Provider::SetMeterProvider(none);
   }
 
-  if (logger_provider)
+  if (logger_provider_)
   {
     std::shared_ptr<opentelemetry::logs::LoggerProvider> none;
     opentelemetry::logs::Provider::SetLoggerProvider(none);
diff --git a/sdk/src/configuration/sdk_builder.cc b/sdk/src/configuration/sdk_builder.cc
@@ -1717,11 +1717,16 @@ void SdkBuilder::SetResource(
 std::unique_ptr<ConfiguredSdk> SdkBuilder::CreateConfiguredSdk(
     const std::unique_ptr<opentelemetry::sdk::configuration::Configuration> &model) const
 {
-  auto sdk = std::make_unique<ConfiguredSdk>();
+  std::unique_ptr<ConfiguredSdk> sdk;
+  auto resource = opentelemetry::sdk::resource::Resource::GetEmpty();
+  std::shared_ptr<opentelemetry::sdk::trace::TracerProvider> tracer_provider;
+  std::shared_ptr<opentelemetry::context::propagation::TextMapPropagator> propagator;
+  std::shared_ptr<opentelemetry::sdk::metrics::MeterProvider> meter_provider;
+  std::shared_ptr<opentelemetry::sdk::logs::LoggerProvider> logger_provider;
 
   if (!model->disabled)
   {
-    SetResource(sdk->resource, model->resource);
+    SetResource(resource, model->resource);
 
     if (model->attribute_limits)
     {
@@ -1732,25 +1737,27 @@ std::unique_ptr<ConfiguredSdk> SdkBuilder::CreateConfiguredSdk(
 
     if (model->tracer_provider)
     {
-      sdk->tracer_provider = CreateTracerProvider(model->tracer_provider, sdk->resource);
+      tracer_provider = CreateTracerProvider(model->tracer_provider, resource);
     }
 
     if (model->propagator)
     {
-      sdk->propagator = CreatePropagator(model->propagator);
+      propagator = CreatePropagator(model->propagator);
     }
 
     if (model->meter_provider)
     {
-      sdk->meter_provider = CreateMeterProvider(model->meter_provider, sdk->resource);
+      meter_provider = CreateMeterProvider(model->meter_provider, resource);
     }
 
     if (model->logger_provider)
     {
-      sdk->logger_provider = CreateLoggerProvider(model->logger_provider, sdk->resource);
+      logger_provider = CreateLoggerProvider(model->logger_provider, resource);
     }
   }
 
+  sdk = std::make_unique<ConfiguredSdk>(std::move(resource), tracer_provider, propagator,
+                                        meter_provider, logger_provider);
   return sdk;
 }
 

Original file line number	Diff line number	Diff line change
`@@ -132,15 +132,15 @@ void InitOtel(const std::string &config_file)`
`132`	`132`
`133`	`133`	`if (sdk != nullptr)`
`134`	`134`	`{`
`135`		`- sdk->Install();`
	`135`	`+ sdk->Init();`
`136`	`136`	`}`
`137`	`137`	`}`
`138`	`138`
`139`	`139`	`void CleanupOtel()`
`140`	`140`	`{`
`141`	`141`	`if (sdk != nullptr)`
`142`	`142`	`{`
`143`		`- sdk->UnInstall();`
	`143`	`+ sdk->Cleanup();`
`144`	`144`	`}`
`145`	`145`	`sdk.reset(nullptr);`
`146`	`146`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,9 @@`
`1`	`1`	`// Copyright The OpenTelemetry Authors`
`2`	`2`	`// SPDX-License-Identifier: Apache-2.0`
`3`	`3`
	`4`	`+// NOTE: This file is adapted from example/yaml/main.cc,`
	`5`	`+// to execute functional tests driven by shelltest.`
	`6`	`+`
`4`	`7`	`#include <stdio.h>`
`5`	`8`	`#include <stdlib.h>`
`6`	`9`	`#include <string.h>`
`@@ -198,15 +201,15 @@ void InitOtel(const std::string &config_file)`
`198`	`201`
`199`	`202`	`if (sdk != nullptr)`
`200`	`203`	`{`
`201`		`- sdk->Install();`
	`204`	`+ sdk->Init();`
`202`	`205`	`}`
`203`	`206`	`}`
`204`	`207`
`205`	`208`	`void CleanupOtel()`
`206`	`209`	`{`
`207`	`210`	`if (sdk != nullptr)`
`208`	`211`	`{`
`209`		`- sdk->UnInstall();`
	`212`	`+ sdk->Cleanup();`
`210`	`213`	`}`
`211`	`214`	`sdk.reset(nullptr);`
`212`	`215`	`}`
Original file line number	Diff line number	Diff line change
`@@ -56,53 +56,53 @@ std::unique_ptr<ConfiguredSdk> ConfiguredSdk::Create(`
`56`	`56`	`return sdk;`
`57`	`57`	`}`
`58`	`58`
`59`		`-void ConfiguredSdk::Install()`
	`59`	`+void ConfiguredSdk::Init()`
`60`	`60`	`{`
`61`		`- if (propagator)`
	`61`	`+ if (propagator_)`
`62`	`62`	`{`
`63`		`- opentelemetry::context::propagation::GlobalTextMapPropagator::SetGlobalPropagator(propagator);`
	`63`	`+ opentelemetry::context::propagation::GlobalTextMapPropagator::SetGlobalPropagator(propagator_);`
`64`	`64`	`}`
`65`	`65`
`66`		`- if (tracer_provider)`
	`66`	`+ if (tracer_provider_)`
`67`	`67`	`{`
`68`		`- std::shared_ptr<opentelemetry::trace::TracerProvider> api_tracer_provider = tracer_provider;`
	`68`	`+ std::shared_ptr<opentelemetry::trace::TracerProvider> api_tracer_provider = tracer_provider_;`
`69`	`69`	`opentelemetry::trace::Provider::SetTracerProvider(api_tracer_provider);`
`70`	`70`	`}`
`71`	`71`
`72`		`- if (meter_provider)`
	`72`	`+ if (meter_provider_)`
`73`	`73`	`{`
`74`		`- std::shared_ptr<opentelemetry::metrics::MeterProvider> api_meter_provider = meter_provider;`
	`74`	`+ std::shared_ptr<opentelemetry::metrics::MeterProvider> api_meter_provider = meter_provider_;`
`75`	`75`	`opentelemetry::metrics::Provider::SetMeterProvider(api_meter_provider);`
`76`	`76`	`}`
`77`	`77`
`78`		`- if (logger_provider)`
	`78`	`+ if (logger_provider_)`
`79`	`79`	`{`
`80`		`- std::shared_ptr<opentelemetry::logs::LoggerProvider> api_logger_provider = logger_provider;`
	`80`	`+ std::shared_ptr<opentelemetry::logs::LoggerProvider> api_logger_provider = logger_provider_;`
`81`	`81`	`opentelemetry::logs::Provider::SetLoggerProvider(api_logger_provider);`
`82`	`82`	`}`
`83`	`83`	`}`
`84`	`84`
`85`		`-void ConfiguredSdk::UnInstall()`
	`85`	`+void ConfiguredSdk::Cleanup()`
`86`	`86`	`{`
`87`		`- if (propagator)`
	`87`	`+ if (propagator_)`
`88`	`88`	`{`
`89`	`89`	`std::shared_ptr<opentelemetry::context::propagation::TextMapPropagator> none;`
`90`	`90`	`opentelemetry::context::propagation::GlobalTextMapPropagator::SetGlobalPropagator(none);`
`91`	`91`	`}`
`92`	`92`
`93`		`- if (tracer_provider)`
	`93`	`+ if (tracer_provider_)`
`94`	`94`	`{`
`95`	`95`	`std::shared_ptr<opentelemetry::trace::TracerProvider> none;`
`96`	`96`	`opentelemetry::trace::Provider::SetTracerProvider(none);`
`97`	`97`	`}`
`98`	`98`
`99`		`- if (meter_provider)`
	`99`	`+ if (meter_provider_)`
`100`	`100`	`{`
`101`	`101`	`std::shared_ptr<opentelemetry::metrics::MeterProvider> none;`
`102`	`102`	`opentelemetry::metrics::Provider::SetMeterProvider(none);`
`103`	`103`	`}`
`104`	`104`
`105`		`- if (logger_provider)`
	`105`	`+ if (logger_provider_)`
`106`	`106`	`{`
`107`	`107`	`std::shared_ptr<opentelemetry::logs::LoggerProvider> none;`
`108`	`108`	`opentelemetry::logs::Provider::SetLoggerProvider(none);`
Original file line number	Diff line number	Diff line change
`@@ -1717,11 +1717,16 @@ void SdkBuilder::SetResource(`
`1717`	`1717`	`std::unique_ptr<ConfiguredSdk> SdkBuilder::CreateConfiguredSdk(`
`1718`	`1718`	`const std::unique_ptr<opentelemetry::sdk::configuration::Configuration> &model) const`
`1719`	`1719`	`{`
`1720`		`- auto sdk = std::make_unique<ConfiguredSdk>();`
	`1720`	`+ std::unique_ptr<ConfiguredSdk> sdk;`
	`1721`	`+ auto resource = opentelemetry::sdk::resource::Resource::GetEmpty();`
	`1722`	`+ std::shared_ptr<opentelemetry::sdk::trace::TracerProvider> tracer_provider;`
	`1723`	`+ std::shared_ptr<opentelemetry::context::propagation::TextMapPropagator> propagator;`
	`1724`	`+ std::shared_ptr<opentelemetry::sdk::metrics::MeterProvider> meter_provider;`
	`1725`	`+ std::shared_ptr<opentelemetry::sdk::logs::LoggerProvider> logger_provider;`
`1721`	`1726`
`1722`	`1727`	`if (!model->disabled)`
`1723`	`1728`	`{`
`1724`		`- SetResource(sdk->resource, model->resource);`
	`1729`	`+ SetResource(resource, model->resource);`
`1725`	`1730`
`1726`	`1731`	`if (model->attribute_limits)`
`1727`	`1732`	`{`
`@@ -1732,25 +1737,27 @@ std::unique_ptr<ConfiguredSdk> SdkBuilder::CreateConfiguredSdk(`
`1732`	`1737`
`1733`	`1738`	`if (model->tracer_provider)`
`1734`	`1739`	`{`
`1735`		`- sdk->tracer_provider = CreateTracerProvider(model->tracer_provider, sdk->resource);`
	`1740`	`+ tracer_provider = CreateTracerProvider(model->tracer_provider, resource);`
`1736`	`1741`	`}`
`1737`	`1742`
`1738`	`1743`	`if (model->propagator)`
`1739`	`1744`	`{`
`1740`		`- sdk->propagator = CreatePropagator(model->propagator);`
	`1745`	`+ propagator = CreatePropagator(model->propagator);`
`1741`	`1746`	`}`
`1742`	`1747`
`1743`	`1748`	`if (model->meter_provider)`
`1744`	`1749`	`{`
`1745`		`- sdk->meter_provider = CreateMeterProvider(model->meter_provider, sdk->resource);`
	`1750`	`+ meter_provider = CreateMeterProvider(model->meter_provider, resource);`
`1746`	`1751`	`}`
`1747`	`1752`
`1748`	`1753`	`if (model->logger_provider)`
`1749`	`1754`	`{`
`1750`		`- sdk->logger_provider = CreateLoggerProvider(model->logger_provider, sdk->resource);`
	`1755`	`+ logger_provider = CreateLoggerProvider(model->logger_provider, resource);`
`1751`	`1756`	`}`
`1752`	`1757`	`}`
`1753`	`1758`
	`1759`	`+ sdk = std::make_unique<ConfiguredSdk>(std::move(resource), tracer_provider, propagator,`
	`1760`	`+ meter_provider, logger_provider);`
`1754`	`1761`	`return sdk;`
`1755`	`1762`	`}`
`1756`	`1763`