Skip to content

Better handling of exception groups #4117

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.

Sign up for GitHub

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

Closed
wants to merge 12 commits into from
Closed

Better handling of exception groups #4117

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cc8e329
Better handling of exception groups
antonpirker Mar 4, 2025
1c2c5c1
Updated some tests
antonpirker Mar 4, 2025
e5d4f56
trigger ci
antonpirker Mar 5, 2025
8c957c6
Updated some tests
antonpirker Mar 5, 2025
8062aa3
Updated ariadne tests
antonpirker Mar 5, 2025
5c45019
Updated strawberry tests
antonpirker Mar 5, 2025
d1b06c6
Merge branch 'master' into antonpirker/fix/exception_groups
antonpirker Mar 19, 2025
f3791ec
Removed useless if
antonpirker Mar 19, 2025
781925a
Make it easier to undertand
antonpirker Mar 19, 2025
5a3a3ac
Give missing parent to recursive calls
antonpirker Mar 19, 2025
e36d43e
Cleanup
antonpirker Mar 19, 2025
72f3f84
Merge branch 'master' into antonpirker/fix/exception_groups
antonpirker Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 11 additions & 29 deletions sentry_sdk/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,8 @@ def single_exception_from_error_tuple(
is_root_exception = exception_id == 0
if not is_root_exception and parent_id is not None:
exception_value["mechanism"]["parent_id"] = parent_id
exception_value["mechanism"]["type"] = "chained"
if exception_id is not None and exception_id > 0:
exception_value["mechanism"]["type"] = "chained"

if is_root_exception and "type" not in exception_value["mechanism"]:
exception_value["mechanism"]["type"] = "generic"
Expand Down Expand Up @@ -897,36 +898,17 @@ def exceptions_from_error_tuple(
# type: (...) -> List[Dict[str, Any]]
exc_type, exc_value, tb = exc_info

is_exception_group = BaseExceptionGroup is not None and isinstance(
exc_value, BaseExceptionGroup
_, exceptions = exceptions_from_error(
Copy link
Member

@szokeasaurusrex szokeasaurusrex Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] does the exceptions_from_error function already internally handle the non-exception group case?

From naively looking at the diff, it seems that the code for handling exceptions which are not exception groups has been deleted, so I am just wondering where this logic is handled now.

Copy link
Contributor Author

@antonpirker antonpirker Mar 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now updated the code to make it easier to grasp what is happening. (The diff is very hard to read, just checkout the file)

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)

exc_type=exc_type,
exc_value=exc_value,
tb=tb,
client_options=client_options,
mechanism=mechanism,
exception_id=0,
parent_id=0,
full_stack=full_stack,
)

if is_exception_group:
(_, exceptions) = exceptions_from_error(
exc_type=exc_type,
exc_value=exc_value,
tb=tb,
client_options=client_options,
mechanism=mechanism,
exception_id=0,
parent_id=0,
full_stack=full_stack,
)

else:
exceptions = []
for exc_type, exc_value, tb in walk_exception_chain(exc_info):
exceptions.append(
single_exception_from_error_tuple(
exc_type=exc_type,
exc_value=exc_value,
tb=tb,
client_options=client_options,
mechanism=mechanism,
full_stack=full_stack,
)
)

exceptions.reverse()

return exceptions
Expand Down
14 changes: 10 additions & 4 deletions tests/integrations/ariadne/test_ariadne.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ def test_capture_request_and_response_if_send_pii_is_on_async(
assert len(events) == 1

(event,) = events
assert event["exception"]["values"][0]["mechanism"]["type"] == "ariadne"
assert event["exception"]["values"][0]["mechanism"]["type"] == "chained"
assert event["exception"]["values"][-1]["mechanism"]["type"] == "ariadne"
assert event["contexts"]["response"] == {
"data": {
"data": {"error": None},
Expand Down Expand Up @@ -111,7 +112,9 @@ def graphql_server():
assert len(events) == 1

(event,) = events
assert event["exception"]["values"][0]["mechanism"]["type"] == "ariadne"
assert event["exception"]["values"][0]["mechanism"]["type"] == "chained"
assert event["exception"]["values"][-1]["mechanism"]["type"] == "ariadne"

assert event["contexts"]["response"] == {
"data": {
"data": {"error": None},
Expand Down Expand Up @@ -152,7 +155,9 @@ def test_do_not_capture_request_and_response_if_send_pii_is_off_async(
assert len(events) == 1

(event,) = events
assert event["exception"]["values"][0]["mechanism"]["type"] == "ariadne"
assert event["exception"]["values"][0]["mechanism"]["type"] == "chained"
assert event["exception"]["values"][-1]["mechanism"]["type"] == "ariadne"

assert "data" not in event["request"]
assert "response" not in event["contexts"]

Expand Down Expand Up @@ -182,7 +187,8 @@ def graphql_server():
assert len(events) == 1

(event,) = events
assert event["exception"]["values"][0]["mechanism"]["type"] == "ariadne"
assert event["exception"]["values"][0]["mechanism"]["type"] == "chained"
assert event["exception"]["values"][-1]["mechanism"]["type"] == "ariadne"
assert "data" not in event["request"]
assert "response" not in event["contexts"]

Expand Down
6 changes: 4 additions & 2 deletions tests/integrations/strawberry/test_strawberry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,8 @@ def test_capture_request_if_available_and_send_pii_is_on(

(error_event,) = events

assert error_event["exception"]["values"][0]["mechanism"]["type"] == "strawberry"
assert error_event["exception"]["values"][0]["mechanism"]["type"] == "chained"
assert error_event["exception"]["values"][-1]["mechanism"]["type"] == "strawberry"
assert error_event["request"]["api_target"] == "graphql"
assert error_event["request"]["data"] == {
"query": query,
Expand Down Expand Up @@ -258,7 +259,8 @@ def test_do_not_capture_request_if_send_pii_is_off(
assert len(events) == 1

(error_event,) = events
assert error_event["exception"]["values"][0]["mechanism"]["type"] == "strawberry"
assert error_event["exception"]["values"][0]["mechanism"]["type"] == "chained"
assert error_event["exception"]["values"][-1]["mechanism"]["type"] == "strawberry"
assert "data" not in error_event["request"]
assert "response" not in error_event["contexts"]

Expand Down
6 changes: 5 additions & 1 deletion tests/new_scopes_compat/test_new_scopes_compat_event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ def create_expected_error_event(trx, span):
"exception": {
"values": [
{
"mechanism": {"type": "generic", "handled": True},
"mechanism": {
"exception_id": 0,
"type": "generic",
"handled": True,
},
"module": None,
"type": "ValueError",
"value": "This is a test exception",
Expand Down
13 changes: 11 additions & 2 deletions tests/test_exceptiongroup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,10 @@ def test_exception_chain_cause():
{
"mechanism": {
"handled": False,
"type": "test_suite",
"type": "chained",
"exception_id": 1,
"parent_id": 0,
"source": "__cause__",
},
"module": None,
"type": "TypeError",
Expand All @@ -227,6 +230,7 @@ def test_exception_chain_cause():
"mechanism": {
"handled": False,
"type": "test_suite",
"exception_id": 0,
},
"module": None,
"type": "ValueError",
Expand Down Expand Up @@ -257,7 +261,10 @@ def test_exception_chain_context():
{
"mechanism": {
"handled": False,
"type": "test_suite",
"type": "chained",
"exception_id": 1,
"parent_id": 0,
"source": "__context__",
},
"module": None,
"type": "TypeError",
Expand All @@ -267,6 +274,7 @@ def test_exception_chain_context():
"mechanism": {
"handled": False,
"type": "test_suite",
"exception_id": 0,
},
"module": None,
"type": "ValueError",
Expand Down Expand Up @@ -297,6 +305,7 @@ def test_simple_exception():
"mechanism": {
"handled": False,
"type": "test_suite",
"exception_id": 0,
},
"module": None,
"type": "ValueError",
Expand Down
Loading