Repository navigation
fix(sdk): catch and log unhandled exceptions in MultiSpanProcessor #5626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2565d2d
542c380
a517a73
f186a47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| `opentelemetry-sdk`: catch and log unhandled exceptions in MultiSpanProcessor |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -178,7 +178,10 @@ def on_start( | |
| parent_context: context_api.Context | None = None, | ||
| ) -> None: | ||
| for sp in self._span_processors: | ||
| sp.on_start(span, parent_context=parent_context) | ||
| try: | ||
| sp.on_start(span, parent_context=parent_context) | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| logger.exception("Exception while calling SpanProcessor.on_start.") | ||
|
|
||
| def _on_ending(self, span: "Span") -> None: | ||
| for sp in self._span_processors: | ||
|
|
@@ -190,12 +193,18 @@ def _on_ending(self, span: "Span") -> None: | |
|
|
||
| def on_end(self, span: "ReadableSpan") -> None: | ||
| for sp in self._span_processors: | ||
| sp.on_end(span) | ||
| try: | ||
| sp.on_end(span) | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| logger.exception("Exception while calling SpanProcessor.on_end.") | ||
|
|
||
| def shutdown(self) -> None: | ||
| """Sequentially shuts down all underlying span processors.""" | ||
| for sp in self._span_processors: | ||
| sp.shutdown() | ||
| try: | ||
| sp.shutdown() | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| logger.exception("Exception while calling SpanProcessor.shutdown.") | ||
|
|
||
| def force_flush(self, timeout_millis: int = 30000) -> bool: | ||
| """Sequentially calls force_flush on all underlying | ||
|
|
@@ -218,7 +227,11 @@ def force_flush(self, timeout_millis: int = 30000) -> bool: | |
| if current_time_ns >= deadline_ns: | ||
| return False | ||
|
|
||
| if sp.force_flush((deadline_ns - current_time_ns) // 1000000) is False: | ||
| try: | ||
| if sp.force_flush((deadline_ns - current_time_ns) // 1000000) is False: | ||
| all_flushed = False | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| logger.exception("Exception while calling SpanProcessor.force_flush.") | ||
| all_flushed = False | ||
|
|
||
| return all_flushed | ||
|
|
@@ -275,10 +288,16 @@ def _submit_and_await( | |
| ): | ||
| futures = [] | ||
| for sp in self._span_processors: | ||
| future = self._executor.submit(func(sp), *args, **kwargs) | ||
| futures.append(future) | ||
| try: | ||
| future = self._executor.submit(func(sp), *args, **kwargs) | ||
| futures.append(future) | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| logger.exception("Exception while submitting task to SpanProcessor executor.") | ||
| for future in futures: | ||
| future.result() | ||
| try: | ||
| future.result() | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| logger.exception("Exception while executing SpanProcessor task.") | ||
|
|
||
| def on_start( | ||
| self, | ||
|
|
@@ -316,21 +335,30 @@ def force_flush(self, timeout_millis: int = 30000) -> bool: | |
| True if all span processors flushed their spans within the given | ||
| timeout, False otherwise. | ||
| """ | ||
| all_flushed = True | ||
| futures = [] | ||
| for sp in self._span_processors: | ||
| future = self._executor.submit(sp.force_flush, timeout_millis) | ||
| futures.append(future) | ||
| try: | ||
| future = self._executor.submit(sp.force_flush, timeout_millis) | ||
| futures.append(future) | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exception potentially raised from the span processor won't be raised here, it'll be raised on the result below. Likewise the call to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed @herin049. The span processor exception is raised on future.result() in the result evaluation loop below, not at submit(). The try/except around submit() was originally added to guard against executor shutdown at interpreter exit (RuntimeError: cannot schedule new futures after shutdown). Rebased cleanly on upstream main, updated force_flush to inspect completed futures on timeout, and all 48 unit tests pass cleanly. |
||
| logger.exception("Exception while submitting task to SpanProcessor executor.") | ||
| all_flushed = False | ||
|
|
||
| timeout_sec = timeout_millis / 1e3 | ||
| done_futures, not_done_futures = concurrent.futures.wait(futures, timeout_sec) | ||
| if not_done_futures: | ||
| return False | ||
| all_flushed = False | ||
|
|
||
| for future in done_futures: | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved in commit f186a47: removed early return on non-empty not_done_futures, marked all_flushed = False, and continue processing done_futures so completed futures are inspected and any exceptions are logged. Added regression test test_force_flush_timeout_still_inspects_completed_failures. |
||
| if future.result() is False: | ||
| return False | ||
| try: | ||
| if future.result() is False: | ||
| all_flushed = False | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| logger.exception("Exception while executing SpanProcessor force_flush.") | ||
| all_flushed = False | ||
|
|
||
| return True | ||
| return all_flushed | ||
|
|
||
|
|
||
| class EventBase(abc.ABC): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue here.