Better handling of exception groups

[antonpirker]

Properly handle grouped and chained exceptions. The test case in the linked issue illustrates that some ExceptionGroups have been handled in a wrong way.

Updated some tests, because now that those are handled correctly all the mechanism types except for the root exception are set to "chained" like described in the RFC: https://github.com/getsentry/rfcs/blob/main/text/0079-exception-groups.md#interpretation

Because this will change the grouping of exiting Sentry Issues containing ExceptionGroups, it is safer to release this fix in the next major and make sure that we describe the change in behavior in the changelog. (Note: The grouping in the Ariadne issues will not change because those are not ExceptionGroups and only updating the mechanism.type does not change the grouping)

The main change is the exceptions_from_error function:

sentry-python/sentry_sdk/utils.py

Lines 790 to 890 in 72f3f84

	def exceptions_from_error(
	exc_type, # type: Optional[type]
	exc_value, # type: Optional[BaseException]
	tb, # type: Optional[TracebackType]
	client_options=None, # type: Optional[Dict[str, Any]]
	mechanism=None, # type: Optional[Dict[str, Any]]
	exception_id=0, # type: int
	parent_id=0, # type: int
	source=None, # type: Optional[str]
	full_stack=None, # type: Optional[list[dict[str, Any]]]
	):
	# type: (...) -> Tuple[int, List[Dict[str, Any]]]
	"""
	Converts the given exception information into the Sentry structured "exception" format.
	This will return a list of exceptions (a flattened tree of exceptions) in the
	format of the Exception Interface documentation:
	https://develop.sentry.dev/sdk/data-model/event-payloads/exception/
	This function can handle:
	- simple exceptions
	- chained exceptions (raise .. from ..)
	- exception groups
	"""
	base_exception = single_exception_from_error_tuple(
	exc_type=exc_type,
	exc_value=exc_value,
	tb=tb,
	client_options=client_options,
	mechanism=mechanism,
	exception_id=exception_id,
	parent_id=parent_id,
	source=source,
	full_stack=full_stack,
	)
	exceptions = [base_exception]
	parent_id = exception_id
	exception_id += 1
	causing_exception = None
	exception_source = None
	# Add any causing exceptions, if present.
	should_suppress_context = hasattr(exc_value, "__suppress_context__") and exc_value.__suppress_context__ # type: ignore
	# Note: __suppress_context__ is True if the exception is raised with the `from` keyword.
	if should_suppress_context:
	# Explicitly chained exceptions (Like: raise NewException() from OriginalException())
	# The field `__cause__` is set to OriginalException
	has_explicit_causing_exception = (
	exc_value
	and hasattr(exc_value, "__cause__")
	and exc_value.__cause__ is not None
	)
	if has_explicit_causing_exception:
	exception_source = "__cause__"
	causing_exception = exc_value.__cause__ # type: ignore
	else:
	# Implicitly chained exceptions (when an exception occurs while handling another exception)
	# The field `__context__` is set in the exception that occurs while handling another exception,
	# to the other exception.
	has_implicit_causing_exception = (
	exc_value
	and hasattr(exc_value, "__context__")
	and exc_value.__context__ is not None
	)
	if has_implicit_causing_exception:
	exception_source = "__context__"
	causing_exception = exc_value.__context__ # type: ignore
	if causing_exception:
	(exception_id, child_exceptions) = exceptions_from_error(
	exc_type=type(causing_exception),
	exc_value=causing_exception,
	tb=getattr(causing_exception, "__traceback__", None),
	client_options=client_options,
	mechanism=mechanism,
	exception_id=exception_id,
	parent_id=parent_id,
	source=exception_source,
	full_stack=full_stack,
	)
	exceptions.extend(child_exceptions)
	# Add child exceptions from an ExceptionGroup.
	is_exception_group = exc_value and hasattr(exc_value, "exceptions")
	if is_exception_group:
	for idx, causing_exception in enumerate(exc_value.exceptions): # type: ignore
	(exception_id, child_exceptions) = exceptions_from_error(
	exc_type=type(causing_exception),
	exc_value=causing_exception,
	tb=getattr(causing_exception, "__traceback__", None),
	client_options=client_options,
	mechanism=mechanism,
	exception_id=exception_id,
	parent_id=parent_id,
	source="exceptions[%s]" % idx,
	full_stack=full_stack,
	)
	exceptions.extend(child_exceptions)
	return (exception_id, exceptions)

Fixes #3913

[antonpirker]

(The diff is very hard to read, just check out the whole file to make your live easier)

The function exceptions_from_error handles all the cases, single exceptions, chained exceptions, and exception groups. It walks through the complete tree of exceptions.

(We can in another PR also think about removing walk_exception_chain everywhere and just use exceptions_from_error for everything. But this is outside of the scope of this PR)

[codecov]

❌ 872 Tests Failed:

Tests completed	Failed	Passed	Skipped
21278	872	20406	4507

View the top 3 failed test(s) by shortest run time

 tests.integrations.celery.integration_tests.test_celery_beat_cron_monitoring

Stack Traces | 0s run time

.../hostedtoolcache/Python/3.7.17.../x64/lib/python3.7/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1006: in _gcd_import
    ???
<frozen importlib._bootstrap>:983: in _find_and_load
    ???
<frozen importlib._bootstrap>:953: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:219: in _call_with_frames_removed
    ???
<frozen importlib._bootstrap>:1006: in _gcd_import
    ???
<frozen importlib._bootstrap>:983: in _find_and_load
    ???
<frozen importlib._bootstrap>:967: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:677: in _load_unlocked
    ???
<frozen importlib._bootstrap_external>:728: in exec_module
    ???
<frozen importlib._bootstrap>:219: in _call_with_frames_removed
    ???
.../celery/integration_tests/__init__.py:7: in <module>
    from celery.beat import Scheduler
.tox/py3.7-celery-v5.0.5/lib/python3.7.../site-packages/celery/beat.py:22: in <module>
    from . import __version__, platforms, signals
.tox/py3.7-celery-v5.0.5/lib/python3.7.../site-packages/celery/signals.py:14: in <module>
    from .utils.dispatch import Signal
.tox/py3.7-celery-v5.0.5/lib/python3.7.../celery/utils/__init__.py:16: in <module>
    from .nodenames import nodename, nodesplit, worker_direct
.tox/py3.7-celery-v5.0.5/lib/python3.7.../celery/utils/nodenames.py:6: in <module>
    from kombu.entity import Exchange, Queue
.tox/py3.7-celery-v5.0.5/lib/python3.7.../site-packages/kombu/entity.py:7: in <module>
    from .serialization import prepare_accept_content
.tox/py3.7-celery-v5.0.5/lib/python3.7.../site-packages/kombu/serialization.py:440: in <module>
    for ep, args in entrypoints('kombu.serializers'):  # pragma: no cover
.tox/py3.7-celery-v5.0.5/lib/python3.7.../kombu/utils/compat.py:82: in entrypoints
    for ep in importlib_metadata.entry_points().get(namespace, [])
E   AttributeError: 'EntryPoints' object has no attribute 'get'

 tests.integrations.celery.test_celery

Stack Traces | 0s run time

ImportError while importing test module '.../integrations/celery/test_celery.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
.tox/py3.7-celery-v5.0.5/lib/python3.7.../site-packages/_pytest/python.py:617: in _importtestmodule
    mod = import_path(self.path, mode=importmode, root=self.config.rootpath)
.tox/py3.7-celery-v5.0.5/lib/python3.7.../site-packages/_pytest/pathlib.py:567: in import_path
    importlib.import_module(module_name)
.../hostedtoolcache/Python/3.7.17.../x64/lib/python3.7/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1006: in _gcd_import
    ???
<frozen importlib._bootstrap>:983: in _find_and_load
    ???
<frozen importlib._bootstrap>:967: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:677: in _load_unlocked
    ???
.tox/py3.7-celery-v5.0.5/lib/python3.7.../_pytest/assertion/rewrite.py:186: in exec_module
    exec(co, module.__dict__)
.../integrations/celery/test_celery.py:6: in <module>
    from celery import Celery, VERSION
E   ImportError: cannot import name 'Celery' from 'celery' (.../sentry-python/sentry-python/.tox/py3.7-celery-v5.0.5/lib/python3.7.../site-packages/celery/__init__.py)

 tests.integrations.celery.test_update_celery_task_headers

Stack Traces | 0s run time

.../integrations/celery/beat.py:28: in <module>
    from celery import Task, Celery  # type: ignore
E   ImportError: cannot import name 'Task' from 'celery' (.../sentry-python/sentry-python/.tox/py3.7-celery-v5.0.5/lib/python3.7.../site-packages/celery/__init__.py)

During handling of the above exception, another exception occurred:
.../integrations/celery/test_update_celery_task_headers.py:7: in <module>
    from sentry_sdk.integrations.celery import _update_celery_task_headers
.../integrations/celery/__init__.py:9: in <module>
    from sentry_sdk.integrations.celery.beat import (
.../integrations/celery/beat.py:37: in <module>
    raise DidNotEnable("Celery not installed")
E   sentry_sdk.integrations.DidNotEnable: Celery not installed

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[szokeasaurusrex]

@antonpirker I am still struggling a bit to see how the behavior is supposed to change here. Can you walk me through this on Thursday?

[sentrivana]

special thanks for the comments, very helpful in understanding what's going on

[szokeasaurusrex]

Please review my comments before merging

[szokeasaurusrex]

[optional – can also address in a later PR or wait and see if it is actually a problem in the real world]

Per our offline discussion, please check whether it would be possible to construct the exceptions list (within exceptions_from_error) in the correct order, so that reversing here is unnecessary.

Reason it would be better to construct in the opposite order is that exceptions_from_error on each recursive call is appending all of the exceptions to a list containing the base exception (an O(n) operation). Since we call exceptions_from_error O(n) times, this makes the function potentially O(n^2). If instead, we simply take the array from the recursive call, and then append to that array, I believe we solve this potential problem.

If it is too much effort to address here, let's just merge the PR for now, assuming exception chains are usually small, this won't matter much anyways.

[antonpirker]

Because exception chains are not that long on average in my opinion it is not worth the effort and also the decline in readability of the code to make this change now.