ref(rules): Create apply_delayed processor #69167

ceorourke · 2024-04-17T23:14:10Z

Adds a processor for rules with "slow" conditions that are evaluated up to a minute after the event comes in. This PR only goes as far as getting the rules that we need to fire, the actual firing will happen in a follow up PR.

Addresses part of https://github.com/getsentry/team-core-product-foundations/issues/242

codecov · 2024-04-17T23:50:11Z

Codecov Report

Attention: Patch coverage is 88.28125% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 79.65%. Comparing base (781586e) to head (6fa2e8c).
Report is 2 commits behind head on master.

❗ Current head 6fa2e8c differs from pull request most recent head 1ad7747. Consider uploading reports for the commit 1ad7747 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #69167       +/-   ##
===========================================
+ Coverage   53.51%   79.65%   +26.14%     
===========================================
  Files        6291     6479      +188     
  Lines      276821   287758    +10937     
  Branches    47739    49596     +1857     
===========================================
+ Hits       148137   229212    +81075     
+ Misses     128196    58109    -70087     
+ Partials      488      437       -51

Files	Coverage Δ
src/sentry/rules/processing/processor.py	`91.97% <78.57%> (+70.01%)`	⬆️
src/sentry/rules/processing/delayed_processing.py	`90.55% <89.47%> (ø)`

... and 2853 files with indirect coverage changes

src/sentry/rules/processing/delayed_processing.py

ceorourke · 2024-04-19T22:36:45Z

src/sentry/rules/processing/delayed_processing.py

+
+@instrumented_task(
+    name="sentry.delayed_processing.tasks.apply_delayed",
+    queue="dynamicsampling",


Is it ok to use this queue? If not, how can we choose/create a new/different one?

I'm not sure the impact of this 😬 -- cc @wedamija

We should either specify no queue, or make a new one, not use an existing unrelated one. I always forget what's needed when creating a new queue, ask in ops if we need to do anything to have this queue be assigned workers.

By specifying no queue do you mean just not pass it for now?

Yep, you can just not pass queue at all and it'll go to default

src/sentry/rules/processing/delayed_processing.py

tests/sentry/rules/processing/test_delayed_processing.py

Remove a metric that was put in [4 years ago](#69167 (comment)) to track snuba query usage that we no longer need.

saponifi3d

LGTM, just the question about the task queue for Dan. Since we're not executing the task / using the queue, we shouldn't need to worry about it as a blocker.

tests/sentry/rules/processing/test_delayed_processing.py

saponifi3d · 2024-04-24T21:03:30Z

src/sentry/rules/processing/delayed_processing.py

+    time_limit=60,  # 1 minute
+    silo_mode=SiloMode.REGION,
+)
+def apply_delayed(project: Project, buffer: RedisBuffer) -> DefaultDict[Rule, set[int]] | None:


this function is sooooo much nicer now 🎉 thanks for all the cleanup.

saponifi3d · 2024-04-24T21:04:43Z

src/sentry/rules/processing/delayed_processing.py

+
+@instrumented_task(
+    name="sentry.delayed_processing.tasks.apply_delayed",
+    queue="dynamicsampling",


I'm not sure the impact of this 😬 -- cc @wedamija

saponifi3d · 2024-04-24T21:17:55Z

src/sentry/rules/processing/processor.py

+    return rule_type
+
+
+def split_conditions_and_filters(rule_condition_list):


🎉 this is a great idea to pull it out and make this a composable function.

tests/sentry/rules/processing/test_delayed_processing.py

saponifi3d · 2024-04-24T21:36:26Z

tests/sentry/rules/processing/test_delayed_processing.py

+        assert self.rule1 in rules
+        assert diff_env_rule in rules
+
+    def test_apply_delayed_two_rules_one_fires(self):


i really like how you have each of these these structured 🎉 it's really clear the cases you're testing for and the intended result.

wedamija

This is looking really good, great job on untangling all of this and structuring it to be easy to follow.

Feel free to merge as is and address my feedback in a separate pr

wedamija · 2024-04-24T21:43:00Z

src/sentry/rules/processing/delayed_processing.py

+
+@instrumented_task(
+    name="sentry.delayed_processing.tasks.apply_delayed",
+    queue="dynamicsampling",


We should either specify no queue, or make a new one, not use an existing unrelated one. I always forget what's needed when creating a new queue, ask in ops if we need to do anything to have this queue be assigned workers.

src/sentry/rules/conditions/event_frequency.py

src/sentry/rules/processing/delayed_processing.py

wedamija · 2024-04-24T22:18:10Z

src/sentry/rules/processing/delayed_processing.py

+        for slow_condition in slow_conditions:
+            if slow_condition:
+                condition_id = slow_condition.get("id")
+                condition_interval = slow_condition.get("interval")
+                target_value = int(str(slow_condition.get("value")))
+                for condition_data, results in condition_group_results.items():
+                    if (
+                        alert_rule.environment_id == condition_data.environment_id
+                        and condition_id == condition_data.cls_id
+                        and condition_interval == condition_data.interval
+                    ):


We need to take into account whether the rule is ANY or ALL here. When ALL, we should only add to rules_to_fire if all slow conditions pass. This logic works fine as is for ANY.

Might be easier as two pass - so iterate through all the rules and conditions, and find out which ones actually fired. Then for each of those rules, add the groups

👀 I think this I'll address in a separate PR since it'll need more test coverage

src/sentry/rules/processing/delayed_processing.py

wedamija · 2024-04-24T22:25:09Z

src/sentry/rules/processing/delayed_processing.py

+    time_limit=60,  # 1 minute
+    silo_mode=SiloMode.REGION,
+)
+def apply_delayed(project: Project, buffer: RedisBuffer) -> DefaultDict[Rule, set[int]] | None:


Why do we have a return value here? Just for tests?

Yeah this is just temporary for now since it's not actually firing the rules yet and makes it easier to test. Once they fire I can check RuleFireHistory.

Fwiw I think it's fine to return here for testability, it doesn't cause problems with the task or anything

src/sentry/rules/processing/delayed_processing.py

Remove a metric that was put in [4 years ago](#69167 (comment)) to track snuba query usage that we no longer need.

Follow up to comments on #69167 to not pass the Redis buffer to `apply_delayed`, and also a minor comment to rename a function.

Another follow up to #69167, specifically the [comment about handling any and all action matches](#69167 (comment)) to check if the rule is for "any" or "all" conditions and only add to the list to be fired if it matches accordingly.

Follow up to #69167 to actually fire the rules Closes getsentry/team-core-product-foundations#242

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 17, 2024

vercel bot deployed to Preview April 17, 2024 23:15 View deployment

ceorourke commented Apr 17, 2024

View reviewed changes

src/sentry/rules/processing/delayed_processing.py Outdated Show resolved Hide resolved

ceorourke commented Apr 18, 2024

View reviewed changes

src/sentry/rules/processing/delayed_processing.py Outdated Show resolved Hide resolved

ceorourke commented Apr 18, 2024

View reviewed changes

src/sentry/rules/processing/delayed_processing.py Outdated Show resolved Hide resolved

ceorourke commented Apr 18, 2024

View reviewed changes

src/sentry/rules/processing/delayed_processing.py Show resolved Hide resolved

vercel bot deployed to Preview April 18, 2024 23:03 View deployment

vercel bot deployed to Preview April 18, 2024 23:43 View deployment

vercel bot deployed to Preview April 19, 2024 01:09 View deployment

vercel bot deployed to Preview April 19, 2024 22:18 View deployment

ceorourke commented Apr 19, 2024

View reviewed changes

src/sentry/rules/processing/delayed_processing.py Outdated Show resolved Hide resolved

ceorourke commented Apr 19, 2024

View reviewed changes

vercel bot deployed to Preview April 19, 2024 22:36 View deployment

ceorourke commented Apr 19, 2024

View reviewed changes

src/sentry/rules/processing/delayed_processing.py Show resolved Hide resolved

ceorourke commented Apr 19, 2024

View reviewed changes

tests/sentry/rules/processing/test_delayed_processing.py Outdated Show resolved Hide resolved

ceorourke mentioned this pull request Apr 19, 2024

ref(rules): Remove event frequency query metric #69369

Merged

ceorourke added a commit that referenced this pull request Apr 22, 2024

ref(rules): Remove event frequency query metric (#69369)

94d3870

Remove a metric that was put in [4 years ago](#69167 (comment)) to track snuba query usage that we no longer need.

ceorourke force-pushed the ceorourke/apply_delayed_rules branch from 4fc44b6 to 088b9c7 Compare April 22, 2024 17:05

vercel bot deployed to Preview April 22, 2024 17:08 View deployment

ceorourke mentioned this pull request Apr 22, 2024

ref(rules): Refactor get_rate to be used for bulk queries #69446

Merged

ceorourke force-pushed the ceorourke/apply_delayed_rules branch from 088b9c7 to 6fa2e8c Compare April 23, 2024 18:36

vercel bot deployed to Preview April 23, 2024 18:38 View deployment

ceorourke added 7 commits April 23, 2024 12:11

seiji's initial stuff

fe40b1b

start writing tests and fix up existing code

156ccd5

some typing

62fee64

a sad attempt at making progress

da49c29

an attempt

1e22c7d

it's a str -_-

afc1759

put into functions, update test setup

7e99b90

vercel bot deployed to Preview April 24, 2024 18:00 View deployment

more typing

0cec32b

vercel bot deployed to Preview April 24, 2024 18:49 View deployment

ceorourke marked this pull request as ready for review April 24, 2024 18:52

ceorourke requested a review from saponifi3d April 24, 2024 18:53

ceorourke added 2 commits April 24, 2024 14:14

don't recreate a function we already have

40db0ea

rm ghost

8853de2

vercel bot deployed to Preview April 24, 2024 21:18 View deployment

saponifi3d approved these changes Apr 24, 2024

View reviewed changes

use create_event_frequency_condition

4580a2f

vercel bot deployed to Preview April 24, 2024 21:54 View deployment

wedamija reviewed Apr 24, 2024

View reviewed changes

wedamija approved these changes Apr 24, 2024

View reviewed changes

small pr comments

2556635

vercel bot deployed to Preview April 24, 2024 23:14 View deployment

wedamija reviewed Apr 24, 2024

View reviewed changes

src/sentry/rules/processing/delayed_processing.py Outdated Show resolved Hide resolved

dont need this anymore

0e160e7

vercel bot deployed to Preview April 24, 2024 23:24 View deployment

MichaelSun48 pushed a commit that referenced this pull request Apr 25, 2024

ref(rules): Remove event frequency query metric (#69369)

cee21db

Remove a metric that was put in [4 years ago](#69167 (comment)) to track snuba query usage that we no longer need.

ceorourke merged commit 2bdf2a7 into master Apr 25, 2024

ceorourke deleted the ceorourke/apply_delayed_rules branch April 25, 2024 16:36

ceorourke mentioned this pull request Apr 25, 2024

ref(rules): Don't pass buffer to apply_delayed #69700

Merged

ceorourke added a commit that referenced this pull request Apr 26, 2024

ref(rules): Don't pass buffer to apply_delayed (#69700)

d3680f3

Follow up to comments on #69167 to not pass the Redis buffer to `apply_delayed`, and also a minor comment to rename a function.

This was referenced Apr 26, 2024

ref(rules): Handle action match in delayed rule processor #69797

Merged

ref(rules): Fire delayed rules #69830

Merged

schew2381 mentioned this pull request Apr 30, 2024

feat(issue-alerts): Group unique conditions #68517

Closed

schew2381 assigned ceorourke Apr 30, 2024

ceorourke added a commit that referenced this pull request May 2, 2024

ref(rules): Fire delayed rules (#69830)

6a38c60

Follow up to #69167 to actually fire the rules Closes getsentry/team-core-product-foundations#242

github-actions bot locked and limited conversation to collaborators May 16, 2024

		return rule_type


		def split_conditions_and_filters(rule_condition_list):

Uh oh!

ref(rules): Create apply_delayed processor #69167

ref(rules): Create apply_delayed processor #69167

Uh oh!

Conversation

ceorourke commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wedamija Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

saponifi3d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wedamija left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ceorourke commented Apr 17, 2024 •

edited

Loading

codecov bot commented Apr 17, 2024 •

edited

Loading

wedamija Apr 24, 2024 •

edited

Loading