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

This reverts commit bfaacc5.

Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>

Author: 2 people

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 Workflow
+from sentry.workflow_engine.models import Detector, Workflow
 from sentry.workflow_engine.types import WorkflowEventData
 
 logger = logging.getLogger(__name__)
@@ -161,6 +161,14 @@ 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,
@@ -182,7 +190,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))
+            action_exceptions.extend(test_fire_action(action, event_data, detector))
 
         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
+from sentry.workflow_engine.models import Action, Detector
 from sentry.workflow_engine.types import WorkflowEventData
 
 logger = logging.getLogger(__name__)
@@ -121,6 +121,14 @@ 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(
@@ -135,7 +143,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)
+        exceptions = test_fire_action(action, workflow_event_data, detector)
         if exceptions:
             action_exceptions.extend(exceptions)
 

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

@@ -3,20 +3,23 @@
 import sentry_sdk
 
 from sentry.shared_integrations.exceptions import IntegrationFormError
-from sentry.workflow_engine.models import Action
+from sentry.workflow_engine.models import Action, Detector
 from sentry.workflow_engine.types import WorkflowEventData
 
 logger = logging.getLogger(__name__)
 
 
-def test_fire_action(action: Action, event_data: WorkflowEventData) -> list[str]:
+def test_fire_action(
+    action: Action, event_data: WorkflowEventData, detector: Detector
+) -> 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 ClassVar
+from typing import TYPE_CHECKING, ClassVar
 
 from django.db import models
 from django.db.models import Q
@@ -23,6 +23,10 @@
 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__)
 
 
@@ -112,10 +116,7 @@ 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) -> None:
-        from sentry.workflow_engine.processors.detector import get_detector_by_event
-
-        detector = get_detector_by_event(event_data)
+    def trigger(self, event_data: WorkflowEventData, detector: Detector) -> None:
         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,6 +67,7 @@ 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,
@@ -75,10 +76,8 @@ 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):
@@ -89,14 +88,19 @@ 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)
 
-    # TODO: remove detector usage from this task
-    detector: Detector | None = None
-    if detector_id is not None:
-        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
 
     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,
@@ -118,15 +122,6 @@ 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
     )
@@ -135,7 +130,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)
+            action.trigger(event_data, detector)
     else:
         logger.info(
             "workflow_engine.triggered_actions.dry-run",

src/sentry/workflow_engine/tasks/utils.py

@@ -64,6 +64,7 @@ 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,
@@ -77,14 +78,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,6 +111,7 @@ 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,7 +21,6 @@
 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]
@@ -35,10 +34,6 @@ 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:
@@ -95,7 +90,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(f"[{self.detector.name}]:")
+        assert pagerduty_data["payload"]["summary"].startswith("[Test Detector]:")
 
     @mock.patch.object(NotifyEventAction, "after")
     @mock.patch(

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

@@ -2,16 +2,13 @@
 
 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.notifications.notification_action.test_metric_alert_registry_handlers import (
-    MetricAlertHandlerBase,
-)
+from tests.sentry.workflow_engine.test_base import BaseWorkflowTest
 
 
-class TestNotificationActionHandler(MetricAlertHandlerBase):
+class TestNotificationActionHandler(BaseWorkflowTest):
     def setUp(self) -> None:
         super().setUp()
         self.project = self.create_project()
@@ -20,6 +17,18 @@ 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"
     )
@@ -31,7 +40,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.action.trigger(self.event_data, self.detector)
 
         mock_registry_get.assert_called_once_with(ErrorGroupType.slug)
         mock_handler.handle_workflow_action.assert_called_once_with(
@@ -47,25 +56,10 @@ 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.action.trigger(self.event_data, self.detector)
 
         mock_registry_get.assert_called_once_with(MetricIssue.slug)
         mock_handler.handle_workflow_action.assert_called_once_with(
@@ -78,15 +72,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_detector(
+    def test_execute_unknown_group_type(
         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 we can't find the detector"""
+        """Test that execute does nothing when detector has no group_type"""
 
-        self.action.trigger(self.event_data)
+        self.action.trigger(self.event_data, self.detector)
 
         mock_logger.warning.assert_called_once_with(
             "group_type_notification_registry.get.NoRegistrationExistsError",