[SDK] Handle instrumentation object construction failures safely - #4469
[SDK] Handle instrumentation object construction failures safely#446922elix3r wants to merge 9 commits into
Conversation
Remove noexcept from SDK TracerProvider, LoggerProvider, MeterProvider, Tracer, Logger, and Meter constructors so initialization-time allocation failures can propagate. Keep GetTracer/GetLogger/GetMeter noexcept and return a pre-allocated noop object if constructing a new instrumentation object fails, without caching the failed attempt. Fixes open-telemetry#4361 Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4469 +/- ##
==========================================
- Coverage 83.12% 83.08% -0.03%
==========================================
Files 519 519
Lines 20256 20312 +56
==========================================
+ Hits 16835 16875 +40
- Misses 3421 3437 +16
🚀 New features to boost your workflow:
|
Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
|
@dbarker Latest commits are up (format/IWYU follow-up + merge from main). CI is waiting on workflow approval again. |
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenTelemetry C++ SDK to safely handle failures during instrumentation object construction while preserving noexcept guarantees for runtime GetTracer/GetLogger/GetMeter calls by returning a pre-allocated noop fallback when construction fails (with exception handling compiled in only when exceptions are enabled).
Changes:
- Remove
noexceptfrom SDK provider and instrumentation constructors so initialization failures can propagate to the caller. - Keep provider
Get*methodsnoexcept, adding guarded try/catch to log construction failures and return a pre-allocated noop object without poisoning caches. - Add unit tests for constructor exception specs and deterministic fault injection/recovery behavior (skipped when exceptions are disabled).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sdk/test/trace/tracer_provider_test.cc | Adds static_asserts for constructor exception specs and tests noop fallback + recovery for GetTracer. |
| sdk/test/metrics/meter_provider_sdk_test.cc | Adds static_asserts and tests noop fallback + recovery for GetMeter. |
| sdk/test/logs/logger_provider_sdk_test.cc | Adds static_asserts and tests noop fallback + recovery for GetLogger; introduces a new test processor. |
| sdk/src/trace/tracer.cc | Removes noexcept from Tracer constructor implementation. |
| sdk/src/trace/tracer_provider.cc | Adds pre-allocated noop tracer fallback and exception-guarded construction in GetTracer. |
| sdk/src/metrics/meter.cc | Removes noexcept from Meter constructor implementation. |
| sdk/src/metrics/meter_provider.cc | Adds pre-allocated noop meter fallback and exception-guarded construction in GetMeter. |
| sdk/src/logs/logger.cc | Removes noexcept from Logger constructor implementation. |
| sdk/src/logs/logger_provider.cc | Adds pre-allocated noop logger fallback and exception-guarded construction in GetLogger. |
| sdk/include/opentelemetry/sdk/trace/tracer.h | Removes noexcept from Tracer constructor declaration. |
| sdk/include/opentelemetry/sdk/trace/tracer_provider.h | Removes noexcept from TracerProvider constructors; adds stored noop tracer member. |
| sdk/include/opentelemetry/sdk/metrics/meter.h | Removes noexcept from Meter constructor declaration. |
| sdk/include/opentelemetry/sdk/metrics/meter_provider.h | Removes noexcept from MeterProvider constructors; adds stored noop meter member. |
| sdk/include/opentelemetry/sdk/logs/logger.h | Removes noexcept from Logger constructor declaration. |
| sdk/include/opentelemetry/sdk/logs/logger_provider.h | Removes noexcept from LoggerProvider constructors; adds stored noop logger member. |
| CHANGELOG.md | Documents the SDK behavior change for constructor exception specs and noop fallback behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
Restore CountingProcessor ownership of the OnEmit record via std::move. Add the includes IWYU asked for, and mark empty logging catches so clang-tidy does not count them against the unique-warning limit. Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
…er-construction-4361 Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
|
@dbarker IWYU and clang-tidy should be clean now. CountingProcessor uses std::move again, and the branch is merged onto latest main. Ready for another look when you have a minute. |
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the PR! Please see some feedback below.
Logging can throw std::bad_alloc from the string stream. Catch std::exception in the GetTracer/GetLogger/GetMeter logging helpers instead of (...). Document why the construction catch-all remains: user scope configurators can throw any exception type. Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
Add the includes IWYU reported for the construction-fallback change and tests. Also cover the catch-all path by injecting a non-std exception from a user scope configurator. Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
…er-construction-4361 Signed-off-by: elix3r <157088510+22elix3r@users.noreply.github.com>
|
@dbarker Follow-up after ff059b9:
Locally: |
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the PR! Please see feedback below. I'm requesting a change to ensure that repeated calls to Get<Tracer/Meter/Logger> will not continue exercising the exceptional path after the first occurrence.
| *should_throw = false; | ||
| auto recovered = provider.GetTracer("throwing-scope"); | ||
| ASSERT_NE(recovered, nullptr); | ||
| EXPECT_NE(recovered, failed); |
There was a problem hiding this comment.
A change in this implementation is needed to ensure that repeated calls to GetTracer with a given instrumentation scope will not continue to exercise the exceptional path.
The rationale is that error handling and immediate recovery isn't possible for callers since the GetTracer method doesn't propagate any usable error signal. We must also expect users to call GetTracer in the hot path for span creation/recording (although this is not recommended if performance is important). We can assume that repeated calls to GetTracer will continue to fail after the first occurrence.
Looking at the examples highlights a common pattern that exercises the worst case where a failure in GetTracer will continue to repeat the exceptional path. This would mean an exception is thrown, caught, and logged in the hot path for span recording.
See: examples/common/foo_library/foo_library.cc#L30
auto span = trace::Provider::GetTracerProvider()->GetTracer("foo_library")->StartSpan("f1");There are a few options to consider in addressing this:
- Do not support a recovery path
- Once an exception is thrown and handled in GetTracer then the TracerProvider is assumed to be non functional and any later calls to GetTracer should fail immediately and return a noop tracer.
- Support recovery on configuration change
- The spec requires that if configuration updates are supported then any configuration update must apply to all previously retrieved tracers.
- We could take this to mean that if GetTracer ever fails and returns a noop tracer, that tracer should later be recovered and become functional if UpdateTracerConfigurator is called with a configurator that resolves the issue. This could make sense for the throwing configurator case (replace the bad configurator), but does not address the bad_alloc case or others.
Let's consider option 2 out of scope for this PR due to complexity and since it cannot address all cases where GetTracer fails.
Please consider option 1 and not support a recovery path once GetTracer fails.
Fixes #4361
Changes
Provider construction happens during SDK initialization. Runtime
GetTracer/GetLogger/GetMetercalls must not throw. This change follows the post-SIG direction from @dbarker:noexceptfrom the SDK constructors forTracerProvider,LoggerProvider,MeterProvider,Tracer,Logger, andMeter. Allocation or initialization failures during provider construction can now propagate so the caller (often the configuration library) can handle them.GetTracer,GetLogger, andGetMeternoexcept. If constructing a previously unseen tracer/logger/meter fails, the provider catches the exception (when exceptions are enabled), logs viaOTEL_INTERNAL_LOG_ERRORwithout letting logging escape, and returns a valid noop API object.new NoopTracer/NoopLogger/NoopMeterstored as anostd::shared_ptr). Runtime failure handling does not allocate a fresh noop object.Get*call can retry and return a normal SDK object once the failure is gone. Existing cached objects continue to be returned unchanged.shared_ptrcontrol-block allocation, cache insertion, and metricsAddMeter.Get*signatures are unchanged. Try/catch is compiled only whenOPENTELEMETRY_HAVE_EXCEPTIONSis set.Tests added
noexceptstatic_asserts for each signal.ScopeConfiguratormatcher that throws for a named scope.Get*returns a valid noop-compatible object, using it produces no telemetry, the cache is not poisoned, cached objects are unchanged, and a later call recovers when the injected failure is removed.Validation
tools/format.shwas not run:clang-formatis not installed in this environment.CHANGELOG.mdupdated for non-trivial changesGet*contracts unchanged)