feat(grouping): Exception Groups #48653
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattjohnsonpint
requested review from
mitsuhiko,
armenzg,
untitaker,
wedamija,
barkbarkimashark and
a team
github-actions
bot
added
the
Scope: Backend
This was referenced May 5, 2023
untitaker
requested changes
May 8, 2023
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #48653 +/- ##
==========================================
+ Coverage 78.47% 80.99% +2.52%
==========================================
Files 4816 4817 +1
Lines 202076 202142 +66
Branches 11369 11369
==========================================
+ Hits 158579 163728 +5149
+ Misses 43243 38160 -5083
Partials 254 254
|
untitaker
approved these changes
May 22, 2023
volokluev
pushed a commit
that referenced
this pull request
May 30, 2023
Implements issue grouping and titling changes required for processing exception groups per [Sentry RFC 79](https://github.com/getsentry/rfcs/blob/main/text/0079-exception-groups.md#sentry-issue-grouping). Part of #37716 A few notes: - The issue titling requirements were implemented by dropping a `main_exception_id` on the event data in the exception grouping logic, and then later using it in the `ErrorEvent.extract_metadata` function. If present, it uses the corresponding exception for metadata including the title. This is working well, but just wanted to check if this is safe, or if there's a better way. - In the RFC, I had indicated that disparate top-level exceptions would be grouped under their immediate parent exception, which might not necessarily be the root exception. This turned out to be difficult to implement, and didn't add much value. Instead, I always use the root exception. This seems to work well enough. I included a comment at the appropriate place in the code, in case it comes up in the future. - I only modified the `NewStyle` strategy. I'm not sure if `Legacy` or other strategies should be updated as well? - I fixed a related bug in `exception.py`, which was that the first exception was being used to create default issue tags instead of the last. This should be done regardless of exception groups, as it corrects the `handled` and `mechanism` event tags such that they are for the outermost (latest) exception. Tests are updated to match this change as well.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements issue grouping and titling changes required for processing exception groups per Sentry RFC 79.
Part of #37716
A few notes:
The issue titling requirements were implemented by dropping a
main_exception_idon the event data in the exception grouping logic, and then later using it in theErrorEvent.extract_metadatafunction. If present, it uses the corresponding exception for metadata including the title. This is working well, but just wanted to check if this is safe, or if there's a better way.In the RFC, I had indicated that disparate top-level exceptions would be grouped under their immediate parent exception, which might not necessarily be the root exception. This turned out to be difficult to implement, and didn't add much value. Instead, I always use the root exception. This seems to work well enough. I included a comment at the appropriate place in the code, in case it comes up in the future.
I only modified the
NewStylestrategy. I'm not sure ifLegacyor other strategies should be updated as well?I fixed a related bug in
exception.py, which was that the first exception was being used to create default issue tags instead of the last. This should be done regardless of exception groups, as it corrects thehandledandmechanismevent tags such that they are for the outermost (latest) exception. Tests are updated to match this change as well.