[SDK] Handle instrumentation object construction failures safely - #4469
[SDK] Handle instrumentation object construction failures safely#446922elix3r wants to merge 7 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.
| << detail << "; returning noop meter."); | ||
| #if OPENTELEMETRY_HAVE_EXCEPTIONS | ||
| } | ||
| catch (...) // NOLINT(bugprone-empty-catch) |
There was a problem hiding this comment.
please catch std::exception here (and the other similar locations). The logging may really only through bad alloc from the string stream but catching the standard exception is safe.
There was a problem hiding this comment.
Done in ff059b9. The logging helpers in MeterProvider, TracerProvider, and LoggerProvider now catch std::exception instead of (...). Logging can still throw (bad_alloc from the string stream is the realistic case), and catching std::exception is enough to keep the noexcept Get* path from escaping.
| LogGetTracerConstructionFailure(ex.what()); | ||
| return noop_tracer_; | ||
| } | ||
| catch (...) |
There was a problem hiding this comment.
Please add a comment here on the rationale for the catch all (that this must catch any user thrown exception in configurators). Please also do the same for logger and meter.
There was a problem hiding this comment.
Added a comment on the catch-all in TracerProvider, LoggerProvider, and MeterProvider in ff059b9. Scope configurators are user callbacks, so they can throw anything, including types that are not std::exception. Get* is noexcept, so we have to catch all of that and return the preallocated noop.
| TEST(MeterProvider, GetMeterReturnsNoopOnConstructionFailure) | ||
| { | ||
| auto should_throw = std::make_shared<bool>(true); | ||
| auto throwing_configurator = |
There was a problem hiding this comment.
Perfect use case. The configurator can be any user provided callback that gets executed when constructing a Meter/Tracer/Logger.
There was a problem hiding this comment.
Glad that landed in the right place. We went through the configurator on purpose so the test hits the same path a real user callback would.
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>
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)