ref(rules): refactor delayed processing batching logic to prepare for workflows #83670

cathteng · 2025-01-17T19:46:35Z

Workflow engine will be using the same logic as delayed processing for rules to process slow conditions. This PR refactors the shared bit, the batching logic, to prepare for adding processing for workflows.

There are two differences the two kinds of delayed processing:

The exact task for processing
The information to get the hash from the buffer

To encapsulate the two differences, I've added a registry. Depending on which delayed processing we are doing, we fetch a handler that includes the information above and we use it in batching the task.

sentry-io · 2025-01-17T19:46:47Z

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: src/sentry/rules/processing/delayed_processing.py

Function	Unhandled Issue
`apply_delayed`	Rule.DoesNotExist: Rule matching query does not exist. sentry.rules.processing.delayed_proc... `Event Count:` 1

_{Did you find this useful? React with a 👍 or 👎}

cathteng · 2025-01-17T19:50:49Z

src/sentry/rules/processing/buffer_processing.py

+    # with metrics.timer("delayed_workflow.process_all_conditions.duration"):
+    #     process_project_ids(fetch_time, WORKFLOW_ENGINE_PROJECT_ID_BUFFER_LIST_KEY, "delayed_workflow")


example of how this will be done for workflow engine

codecov · 2025-01-17T19:51:13Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
324	1	323	10

View the top 1 failed tests by shortest run time

::tests.sentry.buffer.test_redis

Stack Traces | 0s run time

#x1B[31mImportError while importing test module '.../sentry/buffer/test_redis.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
#x1B[1m#x1B[.../hostedtoolcache/Python/3.13.1.../x64/lib/python3.13/importlib/__init__.py#x1B[0m:88: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
#x1B[1m#x1B[.../sentry/buffer/test_redis.py#x1B[0m:21: in <module>
    from sentry.rules.processing.buffer_processing import process_delayed_alert_conditions
#x1B[1m#x1B[31mE   ImportError: cannot import name 'process_delayed_alert_conditions' from 'sentry.rules.processing.buffer_processing' (.../rules/processing/buffer_processing.py)#x1B[0m#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

cathteng · 2025-01-17T21:10:32Z

src/sentry/rules/processing/buffer_processing.py

+    try:
+        processing_info = delayed_processing_registry.get(processing_type)(project_id)
+    except NoRegistrationExistsError:
+        logger.exception(log_format.format(processing_type, "no_registration"))
+        return
+
+    hash_args = processing_info.hash_args
+    task = processing_info.processing_task
+    filters: dict[str, models.Model | str | int] = asdict(hash_args.filters)


this is new, depending on the processing_type (basically if we're processing rules or workflows) we'll have different args to pass when getting things from the buffer or pushing to it, and we'll have a different task to kick off to query snuba to evaluate slow conditions.

the rest of the logic below just uses this information and is the same as before.

could we give models.Model | str | int type a name to help illustrate the different args for this?

saponifi3d

overall i feel like this approach is great, i think it's mostly making sure we're making this generic in a nicely composable way. i think the biggest callout is that i think we should take a look at how we're registering to the FLUSH event in the buffer. right now i think it's a little split between two worlds, being generic and being specific. if the desire is to be highly generic, then i think we should rename some stuff or maybe move some code around. if teh desire is to be specific; we could change the buffer registry to have multiple handlers for a single event.

src/sentry/rules/processing/buffer_processing.py

cathteng · 2025-01-28T18:30:59Z

@saponifi3d i made it more generic

saponifi3d

i think this approach looks great, just the nits / little cleanups 🎉

src/sentry/rules/processing/buffer_processing.py

saponifi3d · 2025-01-29T17:43:18Z

src/sentry/rules/processing/buffer_processing.py

+    try:
+        processing_info = delayed_processing_registry.get(processing_type)(project_id)
+    except NoRegistrationExistsError:
+        logger.exception(log_format.format(processing_type, "no_registration"))
+        return
+
+    hash_args = processing_info.hash_args
+    task = processing_info.processing_task
+    filters: dict[str, models.Model | str | int] = asdict(hash_args.filters)


could we give models.Model | str | int type a name to help illustrate the different args for this?

src/sentry/rules/processing/buffer_processing.py

saponifi3d

🎉 looks great, thanks for all the clean up.

saponifi3d · 2025-01-30T18:40:00Z

src/sentry/rules/processing/buffer_processing.py

+    should_emit_logs = options.get("delayed_processing.emit_logs")
+
+    for processing_type, handler in delayed_processing_registry.registrations.items():
+        with metrics.timer(f"{processing_type}.process_all_conditions.duration"):


should we have a consistent prefix on the metrics for delayed_processing?

possibly, i can clean this up in a follow up

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

cathteng commented Jan 17, 2025

View reviewed changes

vercel bot deployed to Preview January 17, 2025 21:00 View deployment

cathteng commented Jan 17, 2025

View reviewed changes

cathteng marked this pull request as ready for review January 21, 2025 18:03

cathteng requested a review from a team as a code owner January 21, 2025 18:03

cathteng requested a review from a team January 21, 2025 18:03

cathteng force-pushed the cathy/aci/delayed-processing-setup branch from 2a7b002 to a8e8717 Compare January 21, 2025 22:59

vercel bot deployed to Preview January 21, 2025 23:05 View deployment

saponifi3d reviewed Jan 22, 2025

View reviewed changes

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

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

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

cathteng added 3 commits January 28, 2025 10:30

refactor delayed processing batching logic to prepare for workflows

32c46ee

fix tests

250ba3c

generalize the logic

e98e919

cathteng force-pushed the cathy/aci/delayed-processing-setup branch from a8e8717 to e98e919 Compare January 28, 2025 18:30

cathteng requested review from saponifi3d and a team January 28, 2025 18:30

vercel bot deployed to Preview January 28, 2025 18:35 View deployment

fix test

314a4dc

vercel bot deployed to Preview January 28, 2025 20:25 View deployment

cathteng requested a review from a team January 29, 2025 17:08

saponifi3d reviewed Jan 29, 2025

View reviewed changes

address nits

5612d3e

cathteng requested review from saponifi3d and a team January 29, 2025 19:28

vercel bot deployed to Preview January 29, 2025 19:29 View deployment

saponifi3d approved these changes Jan 30, 2025

View reviewed changes

cathteng merged commit e05d7bc into master Jan 30, 2025
49 checks passed

cathteng deleted the cathy/aci/delayed-processing-setup branch January 30, 2025 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(rules): refactor delayed processing batching logic to prepare for workflows #83670

ref(rules): refactor delayed processing batching logic to prepare for workflows #83670

cathteng commented Jan 17, 2025 •

edited

Loading

sentry-io bot commented Jan 17, 2025

cathteng Jan 17, 2025

codecov bot commented Jan 17, 2025 •

edited

Loading

cathteng Jan 17, 2025

saponifi3d Jan 29, 2025

saponifi3d left a comment

cathteng commented Jan 28, 2025

saponifi3d left a comment

saponifi3d Jan 29, 2025

saponifi3d left a comment

saponifi3d Jan 30, 2025

cathteng Jan 30, 2025

		# with metrics.timer("delayed_workflow.process_all_conditions.duration"):
		# process_project_ids(fetch_time, WORKFLOW_ENGINE_PROJECT_ID_BUFFER_LIST_KEY, "delayed_workflow")

ref(rules): refactor delayed processing batching logic to prepare for workflows #83670

ref(rules): refactor delayed processing batching logic to prepare for workflows #83670

Conversation

cathteng commented Jan 17, 2025 • edited Loading

sentry-io bot commented Jan 17, 2025

🔍 Existing Issues For Review

cathteng Jan 17, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 17, 2025 • edited Loading

❌ 1 Tests Failed:

cathteng Jan 17, 2025

Choose a reason for hiding this comment

saponifi3d Jan 29, 2025

Choose a reason for hiding this comment

saponifi3d left a comment

Choose a reason for hiding this comment

cathteng commented Jan 28, 2025

saponifi3d left a comment

Choose a reason for hiding this comment

saponifi3d Jan 29, 2025

Choose a reason for hiding this comment

saponifi3d left a comment

Choose a reason for hiding this comment

saponifi3d Jan 30, 2025

Choose a reason for hiding this comment

cathteng Jan 30, 2025

Choose a reason for hiding this comment

cathteng commented Jan 17, 2025 •

edited

Loading

codecov bot commented Jan 17, 2025 •

edited

Loading