ref(rules): Fix get_condition_groups#72662
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: src/sentry/rules/processing/delayed_processing.py
Did you find this useful? React with a 👍 or 👎 |
github-actions
Bot
added
the
Scope: Backend
wedamija
left a comment
wedamija
left a comment
There was a problem hiding this comment.
Looks good to me, I removed the fix and ran the tests and verified they failed without the fix
| assert len(rule_fire_histories) == 2 | ||
| assert (self.rule1.id, self.group1.id) in rule_fire_histories | ||
| assert (self.rule1.id, group5.id) in rule_fire_histories | ||
| assert (rule5.id, group5.id) in rule_fire_histories |
There was a problem hiding this comment.
Why doesn't this fire anymore? It should still match this rule I think?
There was a problem hiding this comment.
There were 3 like this that failed after the fix - group5 wasn't enqueued with self.rule1 so I don't think it should have actually fired.
There was a problem hiding this comment.
Right, that makes sense. Damn, so we actually had a test that might have caught this earlier
| assert len(rule_fire_histories) == 2 | ||
| assert (self.rule1.id, self.group1.id) in rule_fire_histories | ||
| assert (self.rule1.id, group5.id) in rule_fire_histories | ||
| assert (rule5.id, group5.id) in rule_fire_histories |
There was a problem hiding this comment.
Right, that makes sense. Damn, so we actually had a test that might have caught this earlier
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #72662 +/- ##
=======================================
Coverage 78.05% 78.06%
=======================================
Files 6597 6597
Lines 294016 294055 +39
Branches 50716 50720 +4
=======================================
+ Hits 229508 229565 +57
+ Misses 58246 58229 -17
+ Partials 6262 6261 -1
|
| else: | ||
| condition_groups[unique_condition] = DataAndGroups( | ||
| condition_data, rules_to_groups[rule.id] | ||
| condition_data, set(rules_to_groups[rule.id]) |
There was a problem hiding this comment.
For anyone reading, just pointing out that this is the fix - we copy the set here to avoid polluting the data
3 participants
Fix
get_condition_groupsto not combine groups from multiple rules - this was causing the wrong rule to be fired for a given group.I also renamed
get_rule_to_slow_conditionstoget_rules_to_slow_conditionssince it's more accurate.