ref(grouping): Move background grouping code to grouping module#62856
Merged
lobsterkatie merged 5 commits intomasterfrom Jan 10, 2024
Merged
ref(grouping): Move background grouping code to grouping module#62856lobsterkatie merged 5 commits intomasterfrom
grouping module#62856lobsterkatie merged 5 commits intomasterfrom
Conversation
Contributor
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: src/sentry/event_manager.py
Did you find this useful? React with a 👍 or 👎 |
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #62856 +/- ##
=======================================
Coverage 81.33% 81.33%
=======================================
Files 5202 5202
Lines 230449 230476 +27
Branches 39824 39830 +6
=======================================
+ Hits 187441 187466 +25
- Misses 37260 37261 +1
- Partials 5748 5749 +1
|
lobsterkatie
force-pushed
the
kmclb-move-background-grouping-code
branch
from
930a1ad to
d9d4265
Compare
lobsterkatie
force-pushed
the
kmclb-move-background-grouping-code
branch
from
d9d4265 to
bb73c69
Compare
wedamija
approved these changes
Jan 10, 2024
trillville
pushed a commit
that referenced
this pull request
Jan 19, 2024
…2856) This moves the code handling background grouping (and the associated tests) into the `grouping` module, as part of a larger refactor. No other code changes made, though I did add a missing test and modify an existing one to spy on rather than mock `_calculate_background_grouping`, both in order to placate codecov.
This was referenced Jan 23, 2024
lobsterkatie
added a commit
that referenced
this pull request
Jan 24, 2024
Background grouping is a safe way for us to measure the performance of a new grouping config - we run the grouping algorithm with that config, time it, and then toss the results, so the change doesn't affect any events' eventual grouping. Currently, there is an option controlling whether to run this test before or after the main hash calculations, presumably because it has the possibility of modifying the event. Or rather, it _had_ the possibility of modifying the event. In #62856, which moved the background grouping functions to the `grouping` module, a change was made so that background grouping is applied to a copy of the event rather than the event itself. It therefore no longer matters whether it happens before or after the main calculations. (It's blocking either way, so that's not a consideration here.) This refactors the code such that background grouping is always run before the main calculations. (I chose before rather than after because that way it's not mixed in with other logic having to do with the main grouping process.) For safety, it doesn't yet remove the option from `defaults.py`, but it does mark it as unused. (The removal will come in a future PR.) In addition, this renames the background grouping function from `run_background_grouping` to `maybe_run_background_grouping` (to reflect the fact that background grouping only runs on a fraction of events) and fixes up the function's docstring.
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
This moves the code handling background grouping (and the associated tests) into the
groupingmodule, as part of a larger refactor.No other code changes made, though I did add a missing test and modify an existing one to spy on rather than mock
_calculate_background_grouping, both in order to placate codecov.