New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(aci): enforce workflow frequency when firing actions #83160

Open

cathteng wants to merge 3 commits into master from cathy/aci/enforce-workflow-frequency

+140 −3

Member

cathteng commented Jan 9, 2025 •

edited

Loading

When firing actions, utilize the new ActionGroupStatus table to check if that action should be firing, given how long ago it last fired for the group and the frequency configured in workflow.config (default 30 min).

This involves some Django annotations and hopping through quite a few lookup tables to go from Action to Workflow 😓

github-actions bot added the Scope: Backend label

vercel bot deployed to Preview

January 9, 2025 16:27

View deployment

vercel bot deployed to Preview

January 9, 2025 16:44

View deployment

cathteng commented

View reviewed changes

src/sentry/workflow_engine/processors/action.py

+                  # TODO(cathy): only reinforce workflow frequency for certain issue types
+                  now = timezone.now()
+                  statuses = ActionGroupStatus.objects.filter(group=group, action__in=actions)

Member Author

cathteng Jan 9, 2025

debating whether to add select_related here, i think it would make the annotation more performant? but is it going across too many FKs and/or there won't be too many objects here?

.select_related("action__dataconditiongroupaction__condition_group__workflowdataconditiongroup__workflow__config")

Contributor

saponifi3d Jan 10, 2025

i think given that we only need the 1 field, and how the number of joins that it will be pretty even on the performance. in general, select_related is more performant unless the tables are huge and you want multiple properties.

cathteng requested a review from a team

January 9, 2025 17:49

cathteng commented

View reviewed changes

src/sentry/workflow_engine/processors/action.py

Comment on lines +50 to +71

+                  ActionGroupStatus.objects.filter(action__in=actions_to_include, group=group).update(
+                      date_updated=now
+                  )
+                  ActionGroupStatus.objects.bulk_create(
+                      [
+                          ActionGroupStatus(action=action, group=group, date_updated=now)
+                          for action in actions_without_statuses
+                      ],
+                      batch_size=1000,
+                  )

Member Author

cathteng Jan 9, 2025

i think bulk updating here would be better than in each action individually

saponifi3d reviewed

View reviewed changes

Contributor

saponifi3d left a comment

generally lgtm, i think my biggest question is if we could utilize the base query set being passed into the filter_recently_fired_workflow_actions method at the end to get the filtered actions instead?

src/sentry/workflow_engine/processors/action.py Outdated

Comment on lines 25 to 39

+                  statuses = statuses.annotate(
+                      frequency=Cast(
+                          Coalesce(
+                              KeyTextTransform(
+                                  "frequency",
+                                  F(
+                                      "action__dataconditiongroupaction__condition_group__workflowdataconditiongroup__workflow__config"
+                                  ),
+                              ),
+                              Value("30"),  # default 30
+                          ),
+                          output_field=IntegerField(),
+                      ),
+                      frequency_seconds=ExpressionWrapper(
+                          F("frequency") * timedelta(minutes=1),  # convert to timedelta
+                          output_field=DurationField(),
+                      ),
+                      difference=ExpressionWrapper(
+                          Value(now) - F("date_updated"), output_field=DurationField()
+                      ),  # how long ago the action fired
+                  )

Contributor

saponifi3d Jan 10, 2025

mind decomposing this a little? it could be nice to just set a few vars for each of the kwargs.

Maybe something along these lines:

    workflow_config_path = F("action__dataconditiongroupaction__condition_group__workflowdataconditiongroup__workflow__config")
    
    check_frequency_minutes = Cast(
        Coalesce(
            KeyTextTransform("frequency", workflow_config_path),
            Value("30")
        ),
        output_field=IntegerField()
    )
    
    check_interval = ExpressionWrapper(
        F("frequency") * timedelta(minutes=1),
        output_field=DurationField()
    )
    
    time_since_last_update = ExpressionWrapper(
        Value(current_time) - F("date_updated"),
        output_field=DurationField()
    )
    
    return statuses.annotate(
        frequency=check_frequency_minutes,
        frequency_seconds=check_interval,
        difference=time_since_last_update
    )

🤔 should this be it's own method?

Member Author

cathteng Jan 13, 2025

i moved it into its own method 👍

src/sentry/workflow_engine/processors/action.py

+                  # TODO(cathy): only reinforce workflow frequency for certain issue types
+                  now = timezone.now()
+                  statuses = ActionGroupStatus.objects.filter(group=group, action__in=actions)

Contributor

saponifi3d Jan 10, 2025

i think given that we only need the 1 field, and how the number of joins that it will be pretty even on the performance. in general, select_related is more performant unless the tables are huge and you want multiple properties.

src/sentry/workflow_engine/processors/action.py Outdated

+                  )
+                  actions_without_statuses_ids = {action.id for action in actions_without_statuses}
+                  filtered_actions = Action.objects.filter(

Contributor

saponifi3d Jan 10, 2025

do we need to make another query to the action table or could we apply filters to the action query set that's being passed into this method?

Member Author

cathteng Jan 13, 2025

updated!

cathteng added 3 commits

January 13, 2025 11:46


          enforce workflow frequency when firing actions

d0ec0d3


          fix typing

d6d1d80


          improve organization

faffbbc

cathteng force-pushed the cathy/aci/enforce-workflow-frequency branch from cbeb55d to faffbbc Compare

January 13, 2025 19:46

cathteng requested review from a team and saponifi3d

January 13, 2025 19:47

vercel bot deployed to Preview

January 13, 2025 19:53

View deployment

codecov bot commented Jan 13, 2025 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #83160      +/-   ##
==========================================
- Coverage   74.90%   66.61%   -8.30%     
==========================================
  Files        9481     9481              
  Lines      537568   537565       -3     
  Branches    21174    21174              
==========================================
- Hits       402681   358085   -44596     
- Misses     134531   176033   +41502     
- Partials      356     3447    +3091

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels