✨ feat: add workflow_id to WorkflowEventData#90959
Conversation
github-actions
Bot
added
the
Scope: Backend
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| else: | ||
| logger.error( | ||
| "Action %s has no associated DataConditionGroupAction", | ||
| action.id, | ||
| extra={"action_id": action.id}, | ||
| ) |
There was a problem hiding this comment.
So this seems like an invariant we'd want to potentially uphold. Does it make sense to continue executing the rest of this function when this occurs, or should we raise an exception and bail due to the data inconsistency this likely signifies?
There was a problem hiding this comment.
hmm, thinking about this. i know this is a hot path so whatever we end up selecting needs to be resilient
saponifi3d
left a comment
saponifi3d
left a comment
There was a problem hiding this comment.
I feel like this is probably not happing in quite the right spot of the workflow engine processing, and that's where the complexity in this method is coming from.
i think it'd be a bit cleaner if we handled this when we trigger the action in process workflows or have the action.trigger do the lookup so it's better encapsulated whenever any action is triggered.
|
|
||
| result_list: set[tuple[Action, int]] = set() | ||
| for action in filtered_actions: | ||
| dcg_action = action.dataconditiongroupaction_set.first() |
There was a problem hiding this comment.
we shouldn't make an assumption about there only being 1 dcga per action, can we just handle this in a loop instead?
| if dcg_action: | ||
| workflow_id = filtered_action_groups[dcg_action.condition_group] | ||
| result_list.add((action, workflow_id)) | ||
| else: |
There was a problem hiding this comment.
is this possible given the filter on dcga on at the beginning of the method?
| filtered_action_groups: set[DataConditionGroup], event_data: WorkflowEventData | ||
| ) -> BaseQuerySet[Action]: | ||
| filtered_action_groups: dict[DataConditionGroup, int], event_data: WorkflowEventData | ||
| ) -> set[tuple[Action, int]]: |
There was a problem hiding this comment.
🤔 not sure i agree with this method signature change - it seems like we're just grabbing the workflow id that we probably already have when we're invoking this method (we would need the workflow to get the list of dcgs to filter on).
Is there somewhere else we should be adding the workflow id instead? could we keep it super simple and get the id from process workflows when we're triggering actions? (or handle this in action.trigger to find the corresponding workflow so it's more generic)
|
closed in favor of https://github.com/getsentry/sentry/pull/91031/files |
3 participants
This PR adds the corresponding
workflow_idto theWorkflowEventDatadataclass.This allows us to use that
workflow_idin our notificationsThe main change is rather than returning a set of
Action, we return a set oftuple[Action, int], where the int is theworkflow_id