[SDK] Support TracerConfigurator updates#4065
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4065 +/- ##
==========================================
- Coverage 82.01% 81.95% -0.05%
==========================================
Files 385 385
Lines 16031 16089 +58
==========================================
+ Hits 13146 13184 +38
- Misses 2885 2905 +20
🚀 New features to boost your workflow:
|
| #if OPENTELEMETRY_ABI_VERSION_NO < 2 | ||
| std::atomic<bool> is_enabled_; | ||
| #endif |
There was a problem hiding this comment.
First, the check on ABI_VERSION can be removed, this is in the SDK.
Second, the initialization MUST use {value}, see PR #2244:
[SDK] Valgrind errors on std::atomic variables (#2244)
There was a problem hiding this comment.
First, the check on ABI_VERSION can be removed, this is in the SDK.
Thanks. This sdk::trace::Tracer::is_enabled_ atomic was added for only ABIv1 since the API Tracer::enabled_ exists only for ABIv2. I think we should keep it guarded here in the SDK Tracer since it is redundant with the API Tracer boolean in ABIv2. When the project moves to ABIv2 as the default and ABIv1 is deprecated this SDK Tracer::is_enabled_ atomic should be removed. Does that sound reasonable?
Second, the initialization MUST use {value}
Good catch. I'll default initialize it in the header. CI is may not be warning on this since it is set in the constructor member initializer list.
| #if OPENTELEMETRY_ABI_VERSION_NO >= 2 | ||
| UpdateEnabled(enabled); | ||
| #else | ||
| is_enabled_.store(enabled, std::memory_order_relaxed); | ||
| #endif |
There was a problem hiding this comment.
Ok, so UpdateEnabled() is a method that exists only in the API, starting with ABI version 2.
How about, in ABI version 1, defining the very same method with the same implementation, only in the SDK, instead of implementing a different attribute name (is_enabled) and type (std::atomic) ?
This code then becomes:
UpdateEnabled(enabled);
in all cases.
Likewise, define method Enabled() and member enabled_ in the SDK in ABI v1.
Basically, the pattern is the opposite of ForceFlushWithMicroseconds() which was demoted from the API to the SDK.
Here Enabled() can be in the SDK in ABI v1, promoted to the API in ABI v2.
There was a problem hiding this comment.
Good idea. I'll try this way.
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
See suggestion for Enabled().
| // The example simulates a debugging workflow where only the tracer configuration is changed at | ||
| // runtime through the provider: | ||
| // | ||
| // Stage 1 – Start with all tracing disabled (low-overhead production default). | ||
| // Stage 2 – An issue is reported. Enable only the user library traces to get an initial signal. | ||
| // Stage 3 – The issue involves external libraries too. Enable all traces for full visibility. | ||
| // Stage 4 – Investigation complete. Disable all tracing again. |
There was a problem hiding this comment.
Looks very nice.
Could you paste the test output in a comment, for reviewers ?
3 participants
Add support for updating the TracerConfigurator at runtime and provide an example.
The TracerProvider configuration includes the TracerConfigurator. SDKs may support updating the TracerConfigurator and resulting TracerConfig objects at runtime provided it is applied to previously created tracers. This feature allows dynamic enabling of tracers to turn tracing on or off without restarting the application.
https://opentelemetry.io/docs/specs/otel/trace/sdk/#configuration
Changes
TracerProvider::UpdateTracerConfiguratormethod and testsTracer::tracer_config_that will support dynamic updates including growth beyond the enabled bool.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes