feat(workflow_engine): Split fast and slow condition evaluation #84275

saponifi3d · 2025-01-29T22:53:11Z

Description

This PR splits the fast / slow conditions when we evaluate the condition group. This method will use the condition group logic type to decide when to return slow conditions or if we can short circuit those conditions.

Will Create a follow-up PR for enqueuing the slow conditions to the redis buffer (this was getting large)

Here's the draft of that PR, just need to add tests feat(workflow_engine): Add ability to enqueue slow conditions #84283

codecov · 2025-01-29T23:26:11Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
23711	1	23710	295

View the top 1 failed tests by shortest run time

tests.sentry.uptime.endpoints.test_organization_uptime_stats.OrganizationUptimeCheckIndexEndpointTest::test_too_many_periods

Stack Traces | 4.49s run time

#x1B[1m#x1B[.../uptime/endpoints/test_organization_uptime_stats.py#x1B[0m:99: in test_too_many_periods
    assert response.status_code == 400
#x1B[1m#x1B[31mE   assert 200 == 400#x1B[0m
#x1B[1m#x1B[31mE    +  where 200 = <Response status_code=200, "application/json">.status_code#x1B[0m

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

cathteng

logic changes lgtm but i'd like to clarify what we're actually enqueueing

cathteng · 2025-01-30T00:26:35Z

src/sentry/workflow_engine/processors/workflow.py

+        if remaining_conditions:
+            # If there are remaining conditions for the action filter to evaluate,
+            # then return the list of conditions to enqueue
+            enqueued_conditions.extend(remaining_conditions)


i had planned to enqueue the DataConditionGroup, i think we can still do that here because the remaining_conditions can only be slow conditions, or am i missing something about why it's better to enqueue the conditions themselves? or is this more generic in case remaining_conditions might not only be slow conditions in the future?

on second thought, probably need to rethink the enqueuing logic since i am planning to have the key be {workflow_id}:{group_id}:{dcg_id,...,dcg_id}

yeah, i ended up changing the enqueue method a bit:

sentry/src/sentry/workflow_engine/processors/workflow.py

Lines 32 to 52 in 67d137e

def enqueue_workflow(

workflow: Workflow,

delayed_conditions: list[DataCondition],

event: GroupEvent,

source: WorkflowDataConditionGroupType,

) -> None:

project_id = event.group.project.id

buffer.backend.push_to_sorted_set(key=WORKFLOW_ENGINE_BUFFER_LIST_KEY, value=project_id)

condition_groups = ",".join(

str(condition.condition_group_id) for condition in delayed_conditions

)

value = json.dumps({"event_id": event.event_id, "occurrence_id": event.occurrence_id})

buffer.backend.push_to_hash(

model=Workflow,

filters={"project": project_id},

field=f"{workflow.id}:{event.group.id}:{condition_groups}:{source}",

value=value,

)

Since each condition knows it's condition group, i just made the enqueue method signature a little more ergonomic

yeah this works. will have to refactor my PR using this 😅

out of curiosity, why's that? aren't you just reading from the buffer and that's all the format we chatted about this mornin'

oh jk i need to learn to read

i think i was misled that we are passing the conditions be enqueued when we're still actually enqueuing the
DataConditionGroup, do we need to collect all the conditions in that case?

eh, probably don't need them all, we could probably move that list coercion up and out of this method and just take a list of ids in the method signature, but can figure that out on the other PR :)

cathteng · 2025-01-30T00:37:46Z

src/sentry/workflow_engine/processors/workflow.py

-            triggered_workflows.add(workflow)
+        evaluation, remaining_conditions = workflow.evaluate_trigger_conditions(job)
+        if remaining_conditions:
+            workflows_to_enqueue.add(workflow)


i think we should enqueue the same thing in the buffer as in evaluate_workflow_action filters, but i assume this is happening in a follow up PR

Yep! rather than us trying to create a data structure here and then enqueue a bunch at the end, i ended up just enqueuing directly in the other PR. that made it so we don't need to have an additional query to match the workflows to the data condition groups when enqueuing them.

…filters, because the method takes a list of workflows

the complexity of delayed conditions.

… we don't evaluate up the stack - meaning if we have any slow conditions we can easily enqueue the condition, workflow, etc

…auate

…ll keep all the delayed processing / enqueuing of workflows into a single place.

## Description This PR splits the fast / slow conditions when we evaluate the condition group. This method will use the condition group logic type to decide when to return slow conditions or if we can short circuit those conditions. Will Create a follow-up PR for enqueuing the slow conditions to the redis buffer (this was getting large) - Here's the draft of that PR, just need to add tests #84283

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

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

saponifi3d marked this pull request as ready for review January 29, 2025 23:52

saponifi3d requested a review from a team as a code owner January 29, 2025 23:52

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

saponifi3d force-pushed the jcallender/aci/delayed-processing branch from 33857a2 to 1a1c839 Compare January 30, 2025 00:04

vercel bot deployed to Preview January 30, 2025 00:09 View deployment

cathteng approved these changes Jan 30, 2025

View reviewed changes

saponifi3d added 12 commits January 30, 2025 11:21

add ability to split data conditions by fast / slow

06c5ce8

Rename evaluate_workflow_action_filters to evaluate_workflows_action_…

cbb8005

…filters, because the method takes a list of workflows

rebase into data conditions

3aa2e93

Refactor how we evaluate the grouping logic, this will help when we add

20607fe

the complexity of delayed conditions.

refactor the condition validations, this allows us to pass conditions…

212803d

… we don't evaluate up the stack - meaning if we have any slow conditions we can easily enqueue the condition, workflow, etc

update the action processor to return any remaining conditions to evl…

4299ce1

…auate

update the action filters handler stuff

a18630f

move the processing of action filters to the workflow layer - this wi…

23d9e0b

…ll keep all the delayed processing / enqueuing of workflows into a single place.

fix the logic_result for any, and add unit tests

d70701f

fix some legacy tests with the corrected logic

3f79028

update is_fast_check and change it to is_fast, also it's not a kwarg.

5561b44

fix the logic aruodn remaining conditions / evaluation

df05a43

saponifi3d force-pushed the jcallender/aci/delayed-processing branch from 1a1c839 to df05a43 Compare January 30, 2025 19:21

saponifi3d enabled auto-merge (squash) January 30, 2025 19:22

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

saponifi3d merged commit 93ee4ed into master Jan 30, 2025
48 checks passed

saponifi3d deleted the jcallender/aci/delayed-processing branch January 30, 2025 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(workflow_engine): Split fast and slow condition evaluation #84275

feat(workflow_engine): Split fast and slow condition evaluation #84275

saponifi3d commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading

cathteng left a comment

cathteng Jan 30, 2025

cathteng Jan 30, 2025

saponifi3d Jan 30, 2025

cathteng Jan 30, 2025

saponifi3d Jan 30, 2025

cathteng Jan 30, 2025

cathteng Jan 30, 2025

saponifi3d Jan 30, 2025

cathteng Jan 30, 2025

saponifi3d Jan 30, 2025

	def enqueue_workflow(
	workflow: Workflow,
	delayed_conditions: list[DataCondition],
	event: GroupEvent,
	source: WorkflowDataConditionGroupType,
	) -> None:
	project_id = event.group.project.id

	buffer.backend.push_to_sorted_set(key=WORKFLOW_ENGINE_BUFFER_LIST_KEY, value=project_id)

	condition_groups = ",".join(
	str(condition.condition_group_id) for condition in delayed_conditions
	)

	value = json.dumps({"event_id": event.event_id, "occurrence_id": event.occurrence_id})
	buffer.backend.push_to_hash(
	model=Workflow,
	filters={"project": project_id},
	field=f"{workflow.id}:{event.group.id}:{condition_groups}:{source}",
	value=value,
	)

feat(workflow_engine): Split fast and slow condition evaluation #84275

feat(workflow_engine): Split fast and slow condition evaluation #84275

Conversation

saponifi3d commented Jan 29, 2025 • edited Loading

Description

codecov bot commented Jan 29, 2025 • edited Loading

❌ 1 Tests Failed:

cathteng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saponifi3d commented Jan 29, 2025 •

edited

Loading

codecov bot commented Jan 29, 2025 •

edited

Loading