-
Notifications
You must be signed in to change notification settings - Fork 558
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.
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #4117 +/- ##
==========================================
- Coverage 79.58% 79.58% -0.01%
==========================================
Files 141 141
Lines 15723 15721 -2
Branches 2673 2672 -1
==========================================
- Hits 12513 12511 -2
+ Misses 2367 2365 -2
- Partials 843 845 +2
|
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.
@antonpirker I think I am generally lacking context in this part of the codebase, so I am struggling to understand how exactly this PR changes the exception grouping behavior.
A walkthrough or technical explanation of how the changes work might help. Although, I suppose it is not urgent, since we will wait for a major release, anyways.
|
||
is_exception_group = BaseExceptionGroup is not None and isinstance( | ||
exc_value, BaseExceptionGroup | ||
_, exceptions = exceptions_from_error( |
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.
[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.
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.
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)
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.
I took a look too and have the same questions as Daniel.
OT: we can change the base of this to potel-base
so that it gets released with the next major.
I am closing this in favor of this PR that will be merged into potel-base: |
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)Fixes #3913