Repository navigation
Conversation
Signed-off-by: SOUMYAJIT GHOSH <23051387@kiit.ac.in>
Pull request dashboard statusWaiting on reviewers · refreshed 2026-10-02 16:08 UTC Review the latest changes. Status above doesn't look right?
|
chrikrah
left a comment
There was a problem hiding this comment.
I read this against the issue and ran the repro on main first, so the notes below are from the code
rather than from the description. The shape looks right to me: guarding every callback rather than
only on_end covers the case the issue's own suggested diff misses, where a ConcurrentMultiSpanProcessor
is installed at the top level and the exception escapes from future.result().
Two things I could not find a test for, both behaviour this pull request changes:
force_flush. A processor that raises now setsall_flushed = Falseand the method returnsFalse
instead of propagating. The other four callbacks each got a test; this one did not, and it is the
only one of the five with a return value a caller acts on.- The submit side of
_submit_and_await. Ifself._executor.submitraises, the processor is dropped
fromfuturesand the call continues. That is reachable at interpreter exit, where
concurrent.futures.threadshuts its executor down in its ownatexithook before
TracerProvider.shutdownruns, andsubmitthen raisesRuntimeError: cannot schedule new futures after shutdown.
One question rather than a request. force_flush reports failure through its return value, but
shutdown() returns nothing, so a processor that fails to shut down is now invisible to the caller.
Given the issue is about failures that happen quietly, is that asymmetry deliberate?
One thing worth stating because a reviewer might otherwise ask: the new tests sit on
MultiSpanProcessorTestBase, so both TestSynchronousMultiSpanProcessor and
TestConcurrentMultiSpanProcessor inherit them. That matters here, because the two classes fail
differently before the fix. The synchronous loop stops at the first raiser, so later processors never
run, while the concurrent one submits every future before awaiting any and only loses the results
after the first failure.
|
Thank you @chrikrah for the detailed and thorough review! I have addressed each of the points in commit
|
|
Both land. Checked out The one that fails is
|
8c3371e to
d4f28a6
Compare
Signed-off-by: SOUMYAJIT GHOSH <23051387@kiit.ac.in>
Fixes open-telemetry#5624 Per the OpenTelemetry specification: 1. OnEnd(Span) and OnStart(Span) MUST NOT throw an exception. 2. OpenTelemetry implementations MUST NOT throw unhandled exceptions at runtime. Previously, SynchronousMultiSpanProcessor and ConcurrentMultiSpanProcessor did not catch exceptions raised by underlying SpanProcessor instances during on_start, _on_ending, on_end, shutdown, and force_flush. An unhandled exception in on_end during context manager exit (__exit__) would escape and replace or suppress real application exceptions, or crash the application during normal execution. Additionally, a failure in an earlier processor would prevent subsequent processors from being invoked. Wrap underlying processor calls in try-except blocks, logging exceptions with logger.exception so that application execution is never disrupted and all registered processors are reliably called. Signed-off-by: Soumyajit Ghosh <jobsoumyajit6124@gmail.com> Signed-off-by: SOUMYAJIT GHOSH <23051387@kiit.ac.in>
Signed-off-by: SOUMYAJIT GHOSH <23051387@kiit.ac.in>
…ilure - Cover force_flush exception handling in MultiSpanProcessorTestBase ensuring it returns False without propagating the exception. - Cover ConcurrentMultiSpanProcessor submission failure in _submit_and_await and force_flush when ThreadPoolExecutor is shut down. - Set all_flushed = False in ConcurrentMultiSpanProcessor.force_flush if submit raises. Signed-off-by: Soumyajit Ghosh <jobsoumyajit6124@gmail.com>
d4f28a6 to
2c8cd34
Compare
|
Gentle check-in on this PR — reviewer @chrikrah verified that the tests in commit |
|
Some real-world impact data in support of this fix, from testing GenAI instrumentations against a span processor that raises: from opentelemetry.sdk.trace import TracerProvider, SpanProcessor
class BuggyProcessor(SpanProcessor):
def on_start(self, span, parent_context=None):
raise RuntimeError("PROCESSOR_FAULT") # or raise in on_end
provider = TracerProvider()
provider.add_span_processor(BuggyProcessor())
# then instrument the OpenAI SDK with this provider and make a chat completion callResult for an ordinary
So with the current SDK, a bug in any single span processor turns every traced LLM call into a failure for instrumentations that rely on the SDK not throwing, as the spec allows them to. Catching processor exceptions in |
|
Thank you @armanunix for providing this concrete real-world impact data. This directly demonstrates why the SDK MultiSpanProcessor must catch and log unhandled exceptions during on_start and on_end, as permitted by the specification, rather than allowing processor faults to crash caller workflows in GenAI and HTTP instrumentations. The implementation and unit tests in this PR are verified and ready for maintainer review. |
Description
Fixes #5624
Per the OpenTelemetry specification:
Span.End()API, therefore it should not block or throw an exception."Tracer.StartSpanAPI, therefore it should not block or throw an exception."Previously,
SynchronousMultiSpanProcessorandConcurrentMultiSpanProcessordid not catch exceptions raised by underlyingSpanProcessorinstances duringon_start,_on_ending,on_end,shutdown, andforce_flush.When a span processor raised in
on_endduring context manager exit (Span.__exit__), the exception escaped and replaced or suppressed the application's actual exception. Furthermore, a failure in an earlier processor in the list prevented subsequent processors from being called.This change wraps calls to underlying processors in
try...except Exception:blocks and logs failures vialogger.exception(...), ensuring that:Type of change
How Has This Been Tested?
test_on_end_exception_does_not_raisetoMultiSpanProcessorTestBase(runs against both Synchronous and Concurrent processors).test_on_start_exception_does_not_raisetoMultiSpanProcessorTestBase.test_on_ending_exception_does_not_raisetoMultiSpanProcessorTestBase.test_shutdown_exception_does_not_raisetoMultiSpanProcessorTestBase.test_on_end_exception_does_not_replace_application_exceptionverifying that application exceptions insidewith tracer.start_as_current_span(...)are properly propagated and never replaced.opentelemetry-sdk/tests/trace/test_span_processor.pypass.ruff checkpasses with 0 errors.Does This PR Require a Contrib Repo Change?
Checklist: