refactor to have a consistent return type from the evaluation

Author: saponifi3d

src/sentry/workflow_engine/processors/workflow.py

@@ -9,7 +9,6 @@
 from django.db.models import Q
 
 from sentry import features
-from sentry.db.models.manager.base_query_set import BaseQuerySet
 from sentry.models.activity import Activity
 from sentry.models.environment import Environment
 from sentry.services.eventstore.models import GroupEvent
@@ -34,7 +33,7 @@
 )
 from sentry.workflow_engine.processors.detector import get_detector_by_event
 from sentry.workflow_engine.processors.workflow_fire_history import create_workflow_fire_histories
-from sentry.workflow_engine.types import WorkflowEventData, WorkflowNotProcessable
+from sentry.workflow_engine.types import WorkflowEvaluation, WorkflowEventData
 from sentry.workflow_engine.utils import log_context, scopedstats
 from sentry.workflow_engine.utils.metrics import metrics_incr
 
@@ -465,7 +464,7 @@ def process_workflows(
     event_data: WorkflowEventData,
     event_start_time: datetime,
     detector: Detector | None = None,
-) -> tuple[set[Workflow], BaseQuerySet[Action]] | WorkflowNotProcessable:
+) -> WorkflowEvaluation:
     """
     This method will get the detector based on the event, and then gather the associated workflows.
     Next, it will evaluate the "when" (or trigger) conditions for each workflow, if the conditions are met,
@@ -479,7 +478,7 @@ def process_workflows(
         fire_actions,
     )
 
-    # This dictionary stores the data for WorkflowNotProcessable
+    # This dictionary stores the data for WorkflowEvaluation
     # and is eventually unpacked to pass into the frozen dataclass
     # TODO -- should this be a workflow event context data instead?
     extra_data: dict[str, Any] = {
@@ -504,7 +503,8 @@ def process_workflows(
             )
         )
     except Detector.DoesNotExist:
-        return WorkflowNotProcessable(
+        return WorkflowEvaluation(
+            tainted=True,
             message="No Detectors associated with the issue were found",
             **extra_data,
         )
@@ -523,7 +523,8 @@ def process_workflows(
             )
         )
     except Environment.DoesNotExist:
-        return WorkflowNotProcessable(
+        return WorkflowEvaluation(
+            tainted=True,
             message="Environment for event not found",
             **extra_data,
         )
@@ -535,7 +536,8 @@ def process_workflows(
     extra_data["workflows"] = workflows
 
     if not workflows:
-        return WorkflowNotProcessable(
+        return WorkflowEvaluation(
+            tainted=True,
             message="No workflows are associated with the detector in the event",
             **extra_data,
         )
@@ -547,7 +549,8 @@ def process_workflows(
     extra_data["triggered_workflows"] = triggered_workflows
 
     if not triggered_workflows and not queue_items_by_workflow_id:
-        return WorkflowNotProcessable(
+        return WorkflowEvaluation(
+            tainted=True,
             message="No items were triggered or queued for slow evaluation",
             **extra_data,
         )
@@ -570,7 +573,8 @@ def process_workflows(
     }
 
     if not actions:
-        return WorkflowNotProcessable(
+        return WorkflowEvaluation(
+            tainted=True,
             message="No actions to evaluate; filtered or not triggered",
             **extra_data,
         )
@@ -586,18 +590,18 @@ def process_workflows(
     )
 
     fire_actions(actions, detector, event_data)
-    return triggered_workflows, actions
+    return WorkflowEvaluation(tainted=False, **extra_data)
 
 
 def process_workflows_with_logs(
     batch_client: DelayedWorkflowClient,
     event_data: WorkflowEventData,
     event_start_time: datetime,
     detector: Detector | None = None,
-) -> None:
+) -> WorkflowEvaluation:
     """
     This method is used to create improved logging for `process_workflows`,
-    where we can capture the `WorkflowNotProcessable` results, and log them.
+    where we can capture the `WorkflowEvaluation` results, and log them.
 
     This should create a log for each issue, so we can determine why a workflow
     did or did not trigger.
@@ -608,7 +612,7 @@ def process_workflows_with_logs(
 
     evaluation = process_workflows(batch_client, event_data, event_start_time, detector)
 
-    if isinstance(evaluation, WorkflowNotProcessable):
+    if isinstance(evaluation, WorkflowEvaluation):
         # Attempted to evaluate, but no actions were triggered.
 
         if evaluation.triggered_workflows is None:
@@ -621,8 +625,6 @@ def process_workflows_with_logs(
             logger.info("process_workflows.evaluation.workflow.triggered", extra=asdict(evaluation))
     else:
         # Workflow was fully processed, and actions were triggered
-        workflows, actions = evaluation
-        logger.info(
-            "process_workflows.evaluation.actions.triggered",
-            extra={"workflows": workflows, "actions": actions},
-        )
+        logger.info("process_workflows.evaluation.actions.triggered", extra=asdict(evaluation))
+
+    return evaluation

src/sentry/workflow_engine/types.py

@@ -73,9 +73,22 @@ class WorkflowEventData:
 
 
 @dataclass(frozen=True)
-class WorkflowNotProcessable:
-    group_event: GroupEvent
-    message: str
+class WorkflowEvaluation:
+    """
+    The result of when a workflow is evaluated,
+    the only required property is the `tainted` field,
+    this is used to determine if the evaluation was
+    successfully executed or not.
+
+    The rest of the data is meant to include debug information
+    so we can easily determine why a workflow was or was not triggered.
+    """
+
+    tainted: bool
+
+    group_event: GroupEvent | None = None
+    message: str | None = None
+
     actions: list[Action] | None = None
     workflows: list[Workflow] | None = None
     triggered_actions: list[Action] | None = None

tests/sentry/workflow_engine/processors/test_workflow.py

@@ -39,7 +39,7 @@
     process_workflows,
 )
 from sentry.workflow_engine.tasks.workflows import process_workflows_event
-from sentry.workflow_engine.types import WorkflowEventData, WorkflowNotProcessable
+from sentry.workflow_engine.types import WorkflowEventData
 from tests.sentry.workflow_engine.test_base import BaseWorkflowTest
 
 FROZEN_TIME = before_now(days=1).replace(hour=1, minute=30, second=0, microsecond=0)
@@ -89,12 +89,10 @@ def test_skips_disabled_workflows(self) -> None:
         )
 
         result = process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
-        assert isinstance(result, WorkflowNotProcessable)
         assert result.triggered_workflows == {self.error_workflow}
 
     def test_error_event(self) -> None:
         result = process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
-        assert isinstance(result, WorkflowNotProcessable)
         assert result.triggered_workflows == {self.error_workflow}
 
     @patch("sentry.workflow_engine.processors.action.fire_actions")
@@ -163,9 +161,9 @@ def test_populate_workflow_env_for_filters(self, mock_filter: MagicMock) -> None
         assert self.event_data.group_state
         self.event_data.group_state["is_new"] = True
 
-        process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
-
+        result = process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
         mock_filter.assert_called_with({workflow_filters}, self.event_data)
+        assert result.tainted is False
 
     def test_same_environment_only(self) -> None:
         env = self.create_environment(project=self.project)
@@ -211,7 +209,6 @@ def test_same_environment_only(self) -> None:
         )
 
         result = process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
-        assert isinstance(result, WorkflowNotProcessable)
         assert result.triggered_workflows == {self.error_workflow, matching_env_workflow}
         assert result.message == "No actions to evaluate; filtered or not triggered"
 
@@ -220,7 +217,6 @@ def test_issue_occurrence_event(self) -> None:
         self.group_event.occurrence = issue_occurrence
 
         result = process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
-        assert isinstance(result, WorkflowNotProcessable)
         assert result.triggered_workflows == {self.workflow}
 
     def test_regressed_event(self) -> None:
@@ -239,7 +235,7 @@ def test_regressed_event(self) -> None:
         )
 
         result = process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
-        assert isinstance(result, WorkflowNotProcessable)
+        assert result.tainted is True
         assert result.triggered_workflows == {self.error_workflow, workflow}
         assert result.message == "No actions to evaluate; filtered or not triggered"
 
@@ -249,7 +245,6 @@ def test_no_detector(self, mock_logger: MagicMock, mock_incr: MagicMock) -> None
         self.group_event.occurrence = self.build_occurrence(evidence_data={})
 
         result = process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
-        assert isinstance(result, WorkflowNotProcessable)
         assert result.message == "No Detectors associated with the issue were found"
 
         mock_incr.assert_called_once_with("workflow_engine.detectors.error")
@@ -269,7 +264,6 @@ def test_no_environment(self, mock_logger: MagicMock, mock_incr: MagicMock) -> N
         cache.clear()
         result = process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
 
-        assert isinstance(result, WorkflowNotProcessable)
         assert not result.triggered_workflows
         assert result.message == "Environment for event not found"
 
@@ -349,9 +343,8 @@ def test_defaults_to_error_workflows(self) -> None:
 
         result = process_workflows(self.batch_client, self.event_data, FROZEN_TIME)
 
-        assert isinstance(result, WorkflowNotProcessable)
+        assert result.tainted is True
         assert result.triggered_workflows == {self.error_workflow}
-
         assert result.triggered_actions is not None
         assert len(result.triggered_actions) == 0