[CONFIGURATION] Provider builder interfaces - #4426
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4426 +/- ##
==========================================
+ Coverage 82.61% 82.65% +0.04%
==========================================
Files 511 514 +3
Lines 20132 20161 +29
==========================================
+ Hits 16631 16662 +31
+ Misses 3501 3499 -2
🚀 New features to boost your workflow:
|
abfeb2d to
b6f9e2f
Compare
| * @param meter_configurator Provides access to a function that computes the MeterConfig for | ||
| * Meters provided by this MeterProvider. | ||
| */ | ||
| MeterProvider( |
There was a problem hiding this comment.
This three arg constructor takes views, resource, and a configurator but also provides defaults for all three making the constructor default-constructible. This is generally okay, however on Windows DLL builds any TU that only includes the header and does not instantiate the MeterProvider still requires linking to the metrics library, when it is not necessary in order to avoid link errors:
error LNK2019: unresolved external symbol MeterConfig::Default(void)
referenced in function MeterProvider::`default constructor closure'(void) (??_F...)
error LNK2019: unresolved external symbol MeterProvider::MeterProvider(unique_ptr<ViewRegistry>, ...)
referenced in function MeterProvider::`default constructor closure'(void) (??_F...)
In configured_sdk.cc the Install method sets the global provider singletons from the configured SDK types and this requires a shared_ptr upcast to the API types. This can be done at compile time with only including the header files for the SDK providers and not require a link-time dependency on the SDK signal libraries.
This change to add a default constructor and make the three arg constructor explicit resolves the issue and Windows DLL builds can support ConfiguredSdk::Install without requiring configuration_core to link to opentelemetry_metrics
dbarker
changed the title
| /** | ||
| * Configuration context for building a logger provider. | ||
| */ | ||
| struct LoggerProviderBuilderContext |
There was a problem hiding this comment.
The rationale for adding these builder context structs is to support future args being passed from the root configuration node into the provider builders without requiring a change to the builder interface virtual functions.
| struct MeterProviderBuilderContext | ||
| { | ||
| /** Registry is required and must not be null. */ | ||
| const Registry *registry{nullptr}; |
There was a problem hiding this comment.
Passing in the registry allows the provider builders to take over the role of building signal specific SDK components. This means those SdkBuilder::Create* methods that are signal specific can be moved to the signal builders library (where the concrete signal provider builder will live) and the dependency between the signal library and SdkBuilder can be removed.
| return sdk; | ||
| } | ||
|
|
||
| void ConfiguredSdk::Install() |
There was a problem hiding this comment.
The Install method is a nice addition to the ConfiguredSdk interface and is an otel-cpp extension of the spec. We can continue to support it and break the ConfiguredSdk's dependency on signal libraries by forward declaring the SDK providers in the ConfiguredSdk header, then only including the provider headers here in this .cc.
The compiler just needs to see the declaration of the providers for the upcasts below and no link to the signal libraries are needed for this.
The only catch is with the MeterProvider (it has a default-constructible constructor with all defaulted arguments that the Windows DLL build requires a link to the metrics library for), but this can be resolved with the changes here https://github.com/open-telemetry/opentelemetry-cpp/pull/4426/changes#r3792380516.
| TracerProviderBuilder &operator=(const TracerProviderBuilder &other) = default; | ||
| virtual ~TracerProviderBuilder() = default; | ||
|
|
||
| virtual std::shared_ptr<opentelemetry::sdk::trace::TracerProvider> Build( |
There was a problem hiding this comment.
Using shared_ptr for the return type allows the SDK provider to be forward declared and the SdkBuilder (which will get the provider builder from the registry and call Build when creating the ConfiguredSdk object) to not require a link to the signal libraries (metrics, trace, logs).
1 participant
Contributes to #4352 (Step three from #4352 (comment))
This PR creates
{Tracer,Logger,Meter}Providerbuilder interfaces and adds config Registry slots for them.These builder interfaces are required for two main reasons:
configuration_coreto be a pure configuration library with no signal library dependencies.These builder interfaces are driven by the configuration specification for the Config SDK Create method. The spec requires the create method return SDK provider types. Currently they intentionally do not support building any provider derived from the OTel C++ API, however the builders may be extended in the future to support this (through added virtual functions) if the spec changes.
Changes
MeterProviderand make the three arg constructor explicit but not default constructable. This is required to allowsdk/metrics/meter_provider.hto be included byconfigured_sdk.ccto support the upcast to API shared ptrs without creating a link time dependency.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes