ref(aci): remove passing in detector to action.trigger (#103000)

Author: cathteng

src/sentry/api/endpoints/project_rule_actions.py

@@ -25,7 +25,7 @@
 from sentry.workflow_engine.migration_helpers.rule_action import (
     translate_rule_data_actions_to_notification_actions,
 )
-from sentry.workflow_engine.models import Detector, Workflow
+from sentry.workflow_engine.models import Workflow
 from sentry.workflow_engine.types import WorkflowEventData
 
 logger = logging.getLogger(__name__)
@@ -161,14 +161,6 @@ def execute_future_on_test_event_workflow_engine(
             organization=rule.project.organization,
         )
 
-        detector = Detector(
-            id=TEST_NOTIFICATION_ID,
-            project=rule.project,
-            name=rule.label,
-            enabled=True,
-            type=ErrorGroupType.slug,
-        )
-
         event_data = WorkflowEventData(
             event=test_event,
             group=test_event.group,
@@ -190,7 +182,7 @@ def execute_future_on_test_event_workflow_engine(
                 action_exceptions.append(f"An unexpected error occurred. Error ID: '{error_id}'")
                 continue
 
-            action_exceptions.extend(test_fire_action(action, event_data, detector))
+            action_exceptions.extend(test_fire_action(action, event_data))
 
         status = None
         data = None

src/sentry/workflow_engine/endpoints/organization_test_fire_action.py

@@ -23,7 +23,7 @@
 )
 from sentry.workflow_engine.endpoints.utils.test_fire_action import test_fire_action
 from sentry.workflow_engine.endpoints.validators.base.action import BaseActionValidator
-from sentry.workflow_engine.models import Action, Detector
+from sentry.workflow_engine.models import Action
 from sentry.workflow_engine.types import WorkflowEventData
 
 logger = logging.getLogger(__name__)
@@ -121,14 +121,6 @@ def test_fire_actions(actions: list[dict[str, Any]], project: Project):
         group=test_event.group,
     )
 
-    detector = Detector(
-        id=TEST_NOTIFICATION_ID,
-        project=project,
-        name="Test Detector",
-        enabled=True,
-        type="error",
-    )
-
     for action_data in actions:
         # Create a temporary Action object (not saved to database)
         action = Action(
@@ -143,7 +135,7 @@ def test_fire_actions(actions: list[dict[str, Any]], project: Project):
         setattr(action, "workflow_id", workflow_id)
 
         # Test fire the action and collect any exceptions
-        exceptions = test_fire_action(action, workflow_event_data, detector)
+        exceptions = test_fire_action(action, workflow_event_data)
         if exceptions:
             action_exceptions.extend(exceptions)
 

src/sentry/workflow_engine/endpoints/utils/test_fire_action.py

@@ -3,23 +3,20 @@
 import sentry_sdk
 
 from sentry.shared_integrations.exceptions import IntegrationFormError
-from sentry.workflow_engine.models import Action, Detector
+from sentry.workflow_engine.models import Action
 from sentry.workflow_engine.types import WorkflowEventData
 
 logger = logging.getLogger(__name__)
 
 
-def test_fire_action(
-    action: Action, event_data: WorkflowEventData, detector: Detector
-) -> list[str]:
+def test_fire_action(action: Action, event_data: WorkflowEventData) -> list[str]:
     """
     This function will fire an action and return a list of exceptions that occurred.
     """
     action_exceptions = []
     try:
         action.trigger(
             event_data=event_data,
-            detector=detector,
         )
     except Exception as exc:
         if isinstance(exc, IntegrationFormError):

src/sentry/workflow_engine/models/action.py

@@ -3,7 +3,7 @@
 import builtins
 import logging
 from enum import StrEnum
-from typing import TYPE_CHECKING, ClassVar
+from typing import ClassVar
 
 from django.db import models
 from django.db.models import Q
@@ -23,10 +23,6 @@
 from sentry.workflow_engine.registry import action_handler_registry
 from sentry.workflow_engine.types import ActionHandler, WorkflowEventData
 
-if TYPE_CHECKING:
-    from sentry.workflow_engine.models import Detector
-
-
 logger = logging.getLogger(__name__)
 
 
@@ -116,7 +112,10 @@ def get_handler(self) -> builtins.type[ActionHandler]:
         action_type = Action.Type(self.type)
         return action_handler_registry.get(action_type)
 
-    def trigger(self, event_data: WorkflowEventData, detector: Detector) -> None:
+    def trigger(self, event_data: WorkflowEventData) -> None:
+        from sentry.workflow_engine.processors.detector import get_detector_by_event
+
+        detector = get_detector_by_event(event_data)
         with metrics.timer(
             "workflow_engine.action.trigger.execution_time",
             tags={"action_type": self.type, "detector_type": detector.type},

src/sentry/workflow_engine/tasks/actions.py

@@ -67,7 +67,6 @@ def build_trigger_action_task_params(
 @retry(timeouts=True, raise_on_no_retries=False, ignore_and_capture=Action.DoesNotExist)
 def trigger_action(
     action_id: int,
-    detector_id: int,
     workflow_id: int,
     event_id: str | None,
     activity_id: int | None,
@@ -76,8 +75,10 @@ def trigger_action(
     group_state: GroupState,
     has_reappeared: bool,
     has_escalated: bool,
+    detector_id: int | None = None,
 ) -> None:
     from sentry.notifications.notification_action.utils import should_fire_workflow_actions
+    from sentry.workflow_engine.processors.detector import get_detector_by_event
 
     # XOR check to ensure exactly one of event_id or activity_id is provided
     if (event_id is not None) == (activity_id is not None):
@@ -88,19 +89,14 @@ def trigger_action(
         raise ValueError("Exactly one of event_id or activity_id must be provided")
 
     action = Action.objects.annotate(workflow_id=Value(workflow_id)).get(id=action_id)
-    detector = Detector.objects.get(id=detector_id)
 
-    metrics.incr(
-        "workflow_engine.tasks.trigger_action_task_started",
-        tags={"action_type": action.type, "detector_type": detector.type},
-        sample_rate=1.0,
-    )
-
-    project_id = detector.project_id
+    # TODO: remove detector usage from this task
+    detector: Detector | None = None
+    if detector_id is not None:
+        detector = Detector.objects.get(id=detector_id)
 
     if event_id is not None:
         event_data = build_workflow_event_data_from_event(
-            project_id=project_id,
             event_id=event_id,
             group_id=group_id,
             workflow_id=workflow_id,
@@ -122,6 +118,15 @@ def trigger_action(
         )
         raise ValueError("Exactly one of event_id or activity_id must be provided")
 
+    if detector is None:
+        detector = get_detector_by_event(event_data)
+
+    metrics.incr(
+        "workflow_engine.tasks.trigger_action_task_started",
+        tags={"action_type": action.type, "detector_type": detector.type},
+        sample_rate=1.0,
+    )
+
     should_trigger_actions = should_fire_workflow_actions(
         detector.project.organization, event_data.group.type
     )
@@ -130,7 +135,7 @@ def trigger_action(
         # Set up a timeout grouping context because we want to make sure any Sentry timeout reporting
         # in this scope is grouped properly.
         with timeout_grouping_context(action.type):
-            action.trigger(event_data, detector)
+            action.trigger(event_data)
     else:
         logger.info(
             "workflow_engine.triggered_actions.dry-run",

src/sentry/workflow_engine/tasks/utils.py

@@ -64,7 +64,6 @@ def __init__(self, event_id: str, project_id: int):
 
 @scopedstats.timer()
 def build_workflow_event_data_from_event(
-    project_id: int,
     event_id: str,
     group_id: int,
     workflow_id: int | None = None,
@@ -78,14 +77,14 @@ def build_workflow_event_data_from_event(
     This method handles all the database fetching and object construction logic.
     Raises EventNotFoundError if the event is not found.
     """
-
+    group = Group.objects.get_from_cache(id=group_id)
+    project_id = group.project_id
     event = fetch_event(event_id, project_id)
     if event is None:
         raise EventNotFoundError(event_id, project_id)
 
     occurrence = IssueOccurrence.fetch(occurrence_id, project_id) if occurrence_id else None
 
-    group = Group.objects.get_from_cache(id=group_id)
     group_event = GroupEvent.from_event(event, group)
     group_event.occurrence = occurrence
 

src/sentry/workflow_engine/tasks/workflows.py

@@ -93,14 +93,14 @@ def process_workflow_activity(activity_id: int, group_id: int, detector_id: int)
     on_silent=DataConditionGroup.DoesNotExist,
 )
 def process_workflows_event(
-    project_id: int,
     event_id: str,
     group_id: int,
     occurrence_id: str | None,
     group_state: GroupState,
     has_reappeared: bool,
     has_escalated: bool,
     start_timestamp_seconds: float | None = None,
+    project_id: int | None = None,
     **kwargs: dict[str, Any],
 ) -> None:
     from sentry.workflow_engine.buffer.batch_client import DelayedWorkflowClient
@@ -111,7 +111,6 @@ def process_workflows_event(
     with recorder.record():
         try:
             event_data = build_workflow_event_data_from_event(
-                project_id=project_id,
                 event_id=event_id,
                 group_id=group_id,
                 occurrence_id=occurrence_id,

tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py

@@ -21,6 +21,7 @@
 from sentry.testutils.silo import assume_test_silo_mode
 from sentry.testutils.skips import requires_snuba
 from sentry.workflow_engine.models import Action
+from sentry.workflow_engine.typings.grouptype import IssueStreamGroupType
 from tests.sentry.workflow_engine.test_base import BaseWorkflowTest
 
 pytestmark = [requires_snuba]
@@ -34,6 +35,10 @@ def setUp(self) -> None:
         super().setUp()
         self.login_as(self.user)
         self.project = self.create_project(organization=self.organization)
+        self.detector = self.create_detector(project=self.project)
+        self.issue_stream_detector = self.create_detector(
+            project=self.project, type=IssueStreamGroupType.slug
+        )
         self.workflow = self.create_workflow()
 
     def setup_pd_service(self) -> PagerDutyServiceDict:
@@ -90,7 +95,7 @@ def test_pagerduty_action(
         assert response.status_code == 200
         assert mock_send_trigger.call_count == 1
         pagerduty_data = mock_send_trigger.call_args.kwargs.get("data")
-        assert pagerduty_data["payload"]["summary"].startswith("[Test Detector]:")
+        assert pagerduty_data["payload"]["summary"].startswith(f"[{self.detector.name}]:")
 
     @mock.patch.object(NotifyEventAction, "after")
     @mock.patch(

tests/sentry/workflow_engine/handlers/action/test_action_handlers.py

@@ -2,13 +2,16 @@
 
 from sentry.grouping.grouptype import ErrorGroupType
 from sentry.incidents.grouptype import MetricIssue
+from sentry.types.group import PriorityLevel
 from sentry.utils.registry import NoRegistrationExistsError
 from sentry.workflow_engine.models import Action
 from sentry.workflow_engine.types import WorkflowEventData
-from tests.sentry.workflow_engine.test_base import BaseWorkflowTest
+from tests.sentry.notifications.notification_action.test_metric_alert_registry_handlers import (
+    MetricAlertHandlerBase,
+)
 
 
-class TestNotificationActionHandler(BaseWorkflowTest):
+class TestNotificationActionHandler(MetricAlertHandlerBase):
     def setUp(self) -> None:
         super().setUp()
         self.project = self.create_project()
@@ -17,18 +20,6 @@ def setUp(self) -> None:
         self.group, self.event, self.group_event = self.create_group_event()
         self.event_data = WorkflowEventData(event=self.group_event, group=self.group)
 
-    @mock.patch("sentry.notifications.notification_action.utils.execute_via_issue_alert_handler")
-    def test_execute_without_group_type(
-        self, mock_execute_via_issue_alert_handler: mock.MagicMock
-    ) -> None:
-        """Test that execute does nothing when detector has no group_type"""
-        self.detector.type = ""
-        self.action.trigger(self.event_data, self.detector)
-
-        mock_execute_via_issue_alert_handler.assert_called_once_with(
-            self.event_data, self.action, self.detector
-        )
-
     @mock.patch(
         "sentry.notifications.notification_action.registry.group_type_notification_registry.get"
     )
@@ -40,7 +31,7 @@ def test_execute_error_group_type(self, mock_registry_get: mock.MagicMock) -> No
         mock_handler = mock.Mock()
         mock_registry_get.return_value = mock_handler
 
-        self.action.trigger(self.event_data, self.detector)
+        self.action.trigger(self.event_data)
 
         mock_registry_get.assert_called_once_with(ErrorGroupType.slug)
         mock_handler.handle_workflow_action.assert_called_once_with(
@@ -56,10 +47,25 @@ def test_execute_metric_alert_type(self, mock_registry_get: mock.MagicMock) -> N
         self.detector.config = {"threshold_period": 1, "detection_type": "static"}
         self.detector.save()
 
+        self.group.type = MetricIssue.type_id
+        self.group.save()
+
+        group, _, group_event = self.create_group_event(
+            group_type_id=MetricIssue.type_id,
+            occurrence=self.create_issue_occurrence(
+                priority=PriorityLevel.HIGH.value,
+                level="error",
+                evidence_data={
+                    "detector_id": self.detector.id,
+                },
+            ),
+        )
+        self.event_data = WorkflowEventData(event=group_event, group=group)
+
         mock_handler = mock.Mock()
         mock_registry_get.return_value = mock_handler
 
-        self.action.trigger(self.event_data, self.detector)
+        self.action.trigger(self.event_data)
 
         mock_registry_get.assert_called_once_with(MetricIssue.slug)
         mock_handler.handle_workflow_action.assert_called_once_with(
@@ -72,15 +78,15 @@ def test_execute_metric_alert_type(self, mock_registry_get: mock.MagicMock) -> N
         side_effect=NoRegistrationExistsError,
     )
     @mock.patch("sentry.notifications.notification_action.utils.logger")
-    def test_execute_unknown_group_type(
+    def test_execute_unknown_detector(
         self,
         mock_logger: mock.MagicMock,
         mock_registry_get: mock.MagicMock,
         mock_execute_via_issue_alert_handler: mock.MagicMock,
     ) -> None:
-        """Test that execute does nothing when detector has no group_type"""
+        """Test that execute does nothing when we can't find the detector"""
 
-        self.action.trigger(self.event_data, self.detector)
+        self.action.trigger(self.event_data)
 
         mock_logger.warning.assert_called_once_with(
             "group_type_notification_registry.get.NoRegistrationExistsError",