From afd75abd09fe29233e7d1d8bf048249510ebbc56 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Fri, 7 Nov 2025 15:00:08 -0800 Subject: [PATCH 01/12] remove passing in detector to action.trigger --- .../api/endpoints/project_rule_actions.py | 12 +---- .../organization_test_fire_action.py | 12 +---- .../endpoints/utils/test_fire_action.py | 7 +-- src/sentry/workflow_engine/models/action.py | 11 +++-- src/sentry/workflow_engine/tasks/actions.py | 8 ++-- .../handlers/action/test_action_handlers.py | 44 +++++++++++-------- .../workflow_engine/models/test_action.py | 27 +++++++----- 7 files changed, 56 insertions(+), 65 deletions(-) diff --git a/src/sentry/api/endpoints/project_rule_actions.py b/src/sentry/api/endpoints/project_rule_actions.py index 91961bfa734b23..716db6fb33ccb8 100644 --- a/src/sentry/api/endpoints/project_rule_actions.py +++ b/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 diff --git a/src/sentry/workflow_engine/endpoints/organization_test_fire_action.py b/src/sentry/workflow_engine/endpoints/organization_test_fire_action.py index e44eaed666b7f0..38dc8961540be9 100644 --- a/src/sentry/workflow_engine/endpoints/organization_test_fire_action.py +++ b/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) diff --git a/src/sentry/workflow_engine/endpoints/utils/test_fire_action.py b/src/sentry/workflow_engine/endpoints/utils/test_fire_action.py index e47c4d6bf16de8..2b7dd9f4d546b8 100644 --- a/src/sentry/workflow_engine/endpoints/utils/test_fire_action.py +++ b/src/sentry/workflow_engine/endpoints/utils/test_fire_action.py @@ -3,15 +3,13 @@ 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. """ @@ -19,7 +17,6 @@ def test_fire_action( try: action.trigger( event_data=event_data, - detector=detector, ) except Exception as exc: if isinstance(exc, IntegrationFormError): diff --git a/src/sentry/workflow_engine/models/action.py b/src/sentry/workflow_engine/models/action.py index 01f776126b4872..8ad451cbb38ff3 100644 --- a/src/sentry/workflow_engine/models/action.py +++ b/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}, diff --git a/src/sentry/workflow_engine/tasks/actions.py b/src/sentry/workflow_engine/tasks/actions.py index 47d930d084acf2..7451bc3395a6ac 100644 --- a/src/sentry/workflow_engine/tasks/actions.py +++ b/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,6 +75,8 @@ def trigger_action( group_state: GroupState, has_reappeared: bool, has_escalated: bool, + detector_id: int | None = None, + project_id: int | None = None, ) -> None: from sentry.notifications.notification_action.utils import should_fire_workflow_actions @@ -96,7 +97,8 @@ def trigger_action( sample_rate=1.0, ) - project_id = detector.project_id + if project_id is None: + project_id = detector.project_id if event_id is not None: event_data = build_workflow_event_data_from_event( @@ -130,7 +132,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", diff --git a/tests/sentry/workflow_engine/handlers/action/test_action_handlers.py b/tests/sentry/workflow_engine/handlers/action/test_action_handlers.py index 93ff8903b1d444..ba621c18c3099a 100644 --- a/tests/sentry/workflow_engine/handlers/action/test_action_handlers.py +++ b/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", diff --git a/tests/sentry/workflow_engine/models/test_action.py b/tests/sentry/workflow_engine/models/test_action.py index 3bc4eb1f827406..baded6b2bdc90b 100644 --- a/tests/sentry/workflow_engine/models/test_action.py +++ b/tests/sentry/workflow_engine/models/test_action.py @@ -6,7 +6,7 @@ from sentry.services.eventstore.models import GroupEvent from sentry.testutils.cases import TestCase from sentry.utils.registry import NoRegistrationExistsError -from sentry.workflow_engine.models import Action +from sentry.workflow_engine.models import Action, Detector from sentry.workflow_engine.types import ActionHandler, WorkflowEventData @@ -16,7 +16,6 @@ def setUp(self) -> None: self.group = self.create_group() self.mock_event = WorkflowEventData(event=mock_group_event, group=self.group) - self.mock_detector = Mock(name="detector") self.action = Action(type=Action.Type.SLACK) self.config_schema = { "$id": "https://example.com/user-profile.schema.json", @@ -71,35 +70,39 @@ def test_get_handler_unregistered_type(self) -> None: # Verify the registry was queried with the correct action type mock_get.assert_called_once_with(Action.Type.SLACK) - def test_trigger_calls_handler_execute(self) -> None: + @patch("sentry.workflow_engine.processors.detector.get_detector_by_event") + def test_trigger_calls_handler_execute(self, mock_get_detector: MagicMock) -> None: mock_handler = Mock(spec=ActionHandler) + mock_get_detector.return_value = Mock(spec=Detector, type="error") with patch.object(self.action, "get_handler", return_value=mock_handler): - self.action.trigger(self.mock_event, self.mock_detector) + self.action.trigger(self.mock_event) - mock_handler.execute.assert_called_once_with( - self.mock_event, self.action, self.mock_detector - ) + mock_handler.execute.assert_called_once_with(self.mock_event, self.action) - def test_trigger_with_failing_handler(self) -> None: + @patch("sentry.workflow_engine.processors.detector.get_detector_by_event") + def test_trigger_with_failing_handler(self, mock_get_detector: MagicMock) -> None: mock_handler = Mock(spec=ActionHandler) mock_handler.execute.side_effect = Exception("Handler failed") + mock_get_detector.return_value = Mock(spec=Detector, type="error") with patch.object(self.action, "get_handler", return_value=mock_handler): with pytest.raises(Exception, match="Handler failed"): - self.action.trigger(self.mock_event, self.mock_detector) + self.action.trigger(self.mock_event) @patch("sentry.utils.metrics.incr") - def test_trigger_metrics(self, mock_incr: MagicMock) -> None: + @patch("sentry.workflow_engine.processors.detector.get_detector_by_event") + def test_trigger_metrics(self, mock_get_detector: MagicMock, mock_incr: MagicMock) -> None: mock_handler = Mock(spec=ActionHandler) + mock_get_detector.return_value = Mock(spec=Detector, type="error") with patch.object(self.action, "get_handler", return_value=mock_handler): - self.action.trigger(self.mock_event, self.mock_detector) + self.action.trigger(self.mock_event) mock_handler.execute.assert_called_once() mock_incr.assert_called_once_with( "workflow_engine.action.trigger", - tags={"action_type": self.action.type, "detector_type": self.mock_detector.type}, + tags={"action_type": self.action.type, "detector_type": "error"}, sample_rate=1.0, ) From 96eb6556632ff4da225eedd469256f6433e8c9a0 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Mon, 10 Nov 2025 09:31:14 -0800 Subject: [PATCH 02/12] fix tests --- src/sentry/workflow_engine/tasks/actions.py | 26 +++++++++++++------ .../test_organization_test_fire_action.py | 7 ++++- .../workflow_engine/models/test_action.py | 4 ++- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/sentry/workflow_engine/tasks/actions.py b/src/sentry/workflow_engine/tasks/actions.py index 7451bc3395a6ac..7d6aafbe41c3ef 100644 --- a/src/sentry/workflow_engine/tasks/actions.py +++ b/src/sentry/workflow_engine/tasks/actions.py @@ -79,6 +79,10 @@ def trigger_action( project_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 + + if project_id is None and detector_id is None: + raise ValueError("Either project_id or detector_id must be provided") # 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,18 +93,15 @@ 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, - ) - if project_id is None: + # TODO: remove detector usage from this task after we start passing in project_id + detector: Detector | None = None + if detector_id is not None: + detector = Detector.objects.get(id=detector_id) project_id = detector.project_id if event_id is not None: + assert project_id is not None event_data = build_workflow_event_data_from_event( project_id=project_id, event_id=event_id, @@ -124,6 +125,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 ) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py b/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py index 3233e04c634007..0d7b07ec8a37b9 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py +++ b/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: @@ -94,7 +99,7 @@ def test_pagerduty_action( assert mock_send_trigger.call_count == 1 pagerduty_data = mock_send_trigger.call_args.kwargs.get("data") assert pagerduty_data is not None - 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( diff --git a/tests/sentry/workflow_engine/models/test_action.py b/tests/sentry/workflow_engine/models/test_action.py index baded6b2bdc90b..ea3e71a21bd048 100644 --- a/tests/sentry/workflow_engine/models/test_action.py +++ b/tests/sentry/workflow_engine/models/test_action.py @@ -78,7 +78,9 @@ def test_trigger_calls_handler_execute(self, mock_get_detector: MagicMock) -> No with patch.object(self.action, "get_handler", return_value=mock_handler): self.action.trigger(self.mock_event) - mock_handler.execute.assert_called_once_with(self.mock_event, self.action) + mock_handler.execute.assert_called_once_with( + self.mock_event, self.action, mock_get_detector.return_value + ) @patch("sentry.workflow_engine.processors.detector.get_detector_by_event") def test_trigger_with_failing_handler(self, mock_get_detector: MagicMock) -> None: From f935f114be01283b7ddd7052b3bcd3d409e5d740 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Mon, 10 Nov 2025 10:25:51 -0800 Subject: [PATCH 03/12] remove project_id as actions task param --- src/sentry/workflow_engine/tasks/actions.py | 7 ------- src/sentry/workflow_engine/tasks/utils.py | 5 ++--- src/sentry/workflow_engine/tasks/workflows.py | 3 +-- 3 files changed, 3 insertions(+), 12 deletions(-) diff --git a/src/sentry/workflow_engine/tasks/actions.py b/src/sentry/workflow_engine/tasks/actions.py index 7d6aafbe41c3ef..ba11076c3d7423 100644 --- a/src/sentry/workflow_engine/tasks/actions.py +++ b/src/sentry/workflow_engine/tasks/actions.py @@ -76,14 +76,10 @@ def trigger_action( has_reappeared: bool, has_escalated: bool, detector_id: int | None = None, - project_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 - if project_id is None and detector_id is None: - raise ValueError("Either project_id or detector_id must be provided") - # 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): logger.error( @@ -98,12 +94,9 @@ def trigger_action( detector: Detector | None = None if detector_id is not None: detector = Detector.objects.get(id=detector_id) - project_id = detector.project_id if event_id is not None: - assert project_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, diff --git a/src/sentry/workflow_engine/tasks/utils.py b/src/sentry/workflow_engine/tasks/utils.py index 8a0de630535810..82692d188cb20d 100644 --- a/src/sentry/workflow_engine/tasks/utils.py +++ b/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 diff --git a/src/sentry/workflow_engine/tasks/workflows.py b/src/sentry/workflow_engine/tasks/workflows.py index c0fe693f8a264d..0152ea35de0963 100644 --- a/src/sentry/workflow_engine/tasks/workflows.py +++ b/src/sentry/workflow_engine/tasks/workflows.py @@ -93,7 +93,6 @@ 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, @@ -101,6 +100,7 @@ def process_workflows_event( 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, From 72a7ee7a536d9f374a5c80bd237a5bad5398840d Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Mon, 10 Nov 2025 10:29:25 -0800 Subject: [PATCH 04/12] update comment --- src/sentry/workflow_engine/tasks/actions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/workflow_engine/tasks/actions.py b/src/sentry/workflow_engine/tasks/actions.py index ba11076c3d7423..7fe5410ad59fc9 100644 --- a/src/sentry/workflow_engine/tasks/actions.py +++ b/src/sentry/workflow_engine/tasks/actions.py @@ -90,7 +90,7 @@ def trigger_action( action = Action.objects.annotate(workflow_id=Value(workflow_id)).get(id=action_id) - # TODO: remove detector usage from this task after we start passing in 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) From 9fa77bcad8997312cc249b234b4cbcc9581758e2 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Mon, 10 Nov 2025 13:53:14 -0800 Subject: [PATCH 05/12] handle activity notifications requiring detector --- src/sentry/workflow_engine/models/action.py | 5 ++-- .../workflow_engine/processors/detector.py | 17 +++++++++++ src/sentry/workflow_engine/tasks/actions.py | 5 ++-- .../processors/test_detector.py | 28 +++++++++++++++++++ 4 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/sentry/workflow_engine/models/action.py b/src/sentry/workflow_engine/models/action.py index 8ad451cbb38ff3..d34989f926398e 100644 --- a/src/sentry/workflow_engine/models/action.py +++ b/src/sentry/workflow_engine/models/action.py @@ -113,9 +113,10 @@ def get_handler(self) -> builtins.type[ActionHandler]: 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 + from sentry.workflow_engine.processors.detector import get_detector_by_group + + detector = get_detector_by_group(event_data.group) - 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}, diff --git a/src/sentry/workflow_engine/processors/detector.py b/src/sentry/workflow_engine/processors/detector.py index e4bd46f1fe9956..a970628da07b5b 100644 --- a/src/sentry/workflow_engine/processors/detector.py +++ b/src/sentry/workflow_engine/processors/detector.py @@ -139,6 +139,23 @@ def get_detector_by_event(event_data: WorkflowEventData) -> Detector: return detector +def get_detector_by_group(group: Group) -> Detector: + try: + return DetectorGroup.objects.get(group=group).detector + except DetectorGroup.DoesNotExist: + logger.exception( + "DetectorGroup not found for group", + extra={"group_id": group.id}, + ) + pass + + try: + return Detector.objects.get(project_id=group.project_id, type=group.issue_type.slug) + except Detector.DoesNotExist: + # return issue stream detector + return Detector.objects.get(project_id=group.project_id, type=IssueStreamGroupType.slug) + + class _SplitEvents(NamedTuple): events_with_occurrences: list[tuple[GroupEvent, int]] error_events: list[GroupEvent] diff --git a/src/sentry/workflow_engine/tasks/actions.py b/src/sentry/workflow_engine/tasks/actions.py index 7fe5410ad59fc9..3ed3f8b84851b2 100644 --- a/src/sentry/workflow_engine/tasks/actions.py +++ b/src/sentry/workflow_engine/tasks/actions.py @@ -78,7 +78,7 @@ def trigger_action( 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 + from sentry.workflow_engine.processors.detector import get_detector_by_group # 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): @@ -119,7 +119,8 @@ 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) + # Detector is used in action firing for activities for metric alert resolution. + detector = get_detector_by_group(event_data.group) metrics.incr( "workflow_engine.tasks.trigger_action_task_started", diff --git a/tests/sentry/workflow_engine/processors/test_detector.py b/tests/sentry/workflow_engine/processors/test_detector.py index 6e2b25c15a516f..d9985e60f85d34 100644 --- a/tests/sentry/workflow_engine/processors/test_detector.py +++ b/tests/sentry/workflow_engine/processors/test_detector.py @@ -29,6 +29,7 @@ from sentry.workflow_engine.processors.detector import ( associate_new_group_with_detector, get_detector_by_event, + get_detector_by_group, get_detectors_by_groupevents_bulk, process_detectors, ) @@ -1018,6 +1019,33 @@ def test_mixed_occurrences_missing_detectors(self) -> None: mock_metrics.incr.assert_called_with("workflow_engine.detectors.error", amount=1) +class TestGetDetectorByGroup(TestCase): + def setUp(self) -> None: + super().setUp() + self.project = self.create_project() + self.group = self.create_group(project=self.project) + self.detector = self.create_detector(project=self.project, type="metric_issue") + self.error_detector = self.create_detector(project=self.project, type="error") + self.issue_stream_detector = self.create_detector(project=self.project, type="issue_stream") + + def test_uses_detector_group(self) -> None: + DetectorGroup.objects.create(detector=self.detector, group=self.group) + + assert get_detector_by_group(self.group) == self.detector + + def test_error_group(self) -> None: + assert get_detector_by_group(self.group) == self.error_detector + + def test_type_detector(self) -> None: + self.group.update(type=MetricIssue.type_id) + assert get_detector_by_group(self.group) == self.detector + + def test_issue_stream_fallback(self) -> None: + self.group.update(type=PerformanceNPlusOneAPICallsGroupType.type_id) + + assert get_detector_by_group(self.group) == self.issue_stream_detector + + class TestAssociateNewGroupWithDetector(TestCase): def setUp(self) -> None: super().setUp() From 065ac2a4072fbddb9e4f6d2b69d1eaf54252ea43 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Mon, 10 Nov 2025 14:33:27 -0800 Subject: [PATCH 06/12] fix tests --- .../endpoints/test_organization_test_fire_action.py | 5 ++++- tests/sentry/workflow_engine/models/test_action.py | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py b/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py index 0d7b07ec8a37b9..8eb228fb1d1500 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py @@ -99,7 +99,10 @@ def test_pagerduty_action( assert mock_send_trigger.call_count == 1 pagerduty_data = mock_send_trigger.call_args.kwargs.get("data") assert pagerduty_data is not None - assert pagerduty_data["payload"]["summary"].startswith(f"[{self.detector.name}]:") + # test notification has its own type, so it will pick up the issue stream detector + assert pagerduty_data["payload"]["summary"].startswith( + f"[{self.issue_stream_detector.name}]:" + ) @mock.patch.object(NotifyEventAction, "after") @mock.patch( diff --git a/tests/sentry/workflow_engine/models/test_action.py b/tests/sentry/workflow_engine/models/test_action.py index ea3e71a21bd048..071d4ae9915b1a 100644 --- a/tests/sentry/workflow_engine/models/test_action.py +++ b/tests/sentry/workflow_engine/models/test_action.py @@ -70,7 +70,7 @@ def test_get_handler_unregistered_type(self) -> None: # Verify the registry was queried with the correct action type mock_get.assert_called_once_with(Action.Type.SLACK) - @patch("sentry.workflow_engine.processors.detector.get_detector_by_event") + @patch("sentry.workflow_engine.processors.detector.get_detector_by_group") def test_trigger_calls_handler_execute(self, mock_get_detector: MagicMock) -> None: mock_handler = Mock(spec=ActionHandler) mock_get_detector.return_value = Mock(spec=Detector, type="error") @@ -82,7 +82,7 @@ def test_trigger_calls_handler_execute(self, mock_get_detector: MagicMock) -> No self.mock_event, self.action, mock_get_detector.return_value ) - @patch("sentry.workflow_engine.processors.detector.get_detector_by_event") + @patch("sentry.workflow_engine.processors.detector.get_detector_by_group") def test_trigger_with_failing_handler(self, mock_get_detector: MagicMock) -> None: mock_handler = Mock(spec=ActionHandler) mock_handler.execute.side_effect = Exception("Handler failed") @@ -93,7 +93,7 @@ def test_trigger_with_failing_handler(self, mock_get_detector: MagicMock) -> Non self.action.trigger(self.mock_event) @patch("sentry.utils.metrics.incr") - @patch("sentry.workflow_engine.processors.detector.get_detector_by_event") + @patch("sentry.workflow_engine.processors.detector.get_detector_by_group") def test_trigger_metrics(self, mock_get_detector: MagicMock, mock_incr: MagicMock) -> None: mock_handler = Mock(spec=ActionHandler) mock_get_detector.return_value = Mock(spec=Detector, type="error") From 646bc838af576ee56d6827198f5e9363b0abf3d0 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Tue, 11 Nov 2025 15:55:23 -0800 Subject: [PATCH 07/12] fetch detector depending on groupevent or activity --- src/sentry/workflow_engine/tasks/actions.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/sentry/workflow_engine/tasks/actions.py b/src/sentry/workflow_engine/tasks/actions.py index 3ed3f8b84851b2..e224be61652084 100644 --- a/src/sentry/workflow_engine/tasks/actions.py +++ b/src/sentry/workflow_engine/tasks/actions.py @@ -78,7 +78,10 @@ def trigger_action( 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_group + from sentry.workflow_engine.processors.detector import ( + get_detector_by_event, + get_detector_by_group, + ) # 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): @@ -105,11 +108,14 @@ def trigger_action( has_reappeared=has_reappeared, has_escalated=has_escalated, ) - + if not detector: + detector = get_detector_by_event(event_data) elif activity_id is not None: event_data = build_workflow_event_data_from_activity( activity_id=activity_id, group_id=group_id ) + if not detector: + detector = get_detector_by_group(event_data.group) else: # This should never happen, and if it does, need to investigate logger.error( @@ -118,10 +124,6 @@ def trigger_action( ) raise ValueError("Exactly one of event_id or activity_id must be provided") - if detector is None: - # Detector is used in action firing for activities for metric alert resolution. - detector = get_detector_by_group(event_data.group) - metrics.incr( "workflow_engine.tasks.trigger_action_task_started", tags={"action_type": action.type, "detector_type": detector.type}, From 78e552e2ea3df0408a1b41a08d12fb45ebb002bb Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Wed, 12 Nov 2025 16:31:41 -0800 Subject: [PATCH 08/12] get detector from event data --- src/sentry/workflow_engine/models/action.py | 4 ++-- src/sentry/workflow_engine/processors/detector.py | 10 ++++++++++ src/sentry/workflow_engine/tasks/actions.py | 12 ++++-------- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/sentry/workflow_engine/models/action.py b/src/sentry/workflow_engine/models/action.py index d34989f926398e..42a4b3aa4c115d 100644 --- a/src/sentry/workflow_engine/models/action.py +++ b/src/sentry/workflow_engine/models/action.py @@ -113,9 +113,9 @@ def get_handler(self) -> builtins.type[ActionHandler]: return action_handler_registry.get(action_type) def trigger(self, event_data: WorkflowEventData) -> None: - from sentry.workflow_engine.processors.detector import get_detector_by_group + from sentry.workflow_engine.processors.detector import get_detector_from_event_data - detector = get_detector_by_group(event_data.group) + detector = get_detector_from_event_data(event_data) with metrics.timer( "workflow_engine.action.trigger.execution_time", diff --git a/src/sentry/workflow_engine/processors/detector.py b/src/sentry/workflow_engine/processors/detector.py index a970628da07b5b..9d329655337cba 100644 --- a/src/sentry/workflow_engine/processors/detector.py +++ b/src/sentry/workflow_engine/processors/detector.py @@ -17,6 +17,7 @@ from sentry.issues.issue_occurrence import IssueOccurrence from sentry.issues.producer import PayloadType, produce_occurrence_to_kafka from sentry.locks import locks +from sentry.models.activity import Activity from sentry.models.group import Group from sentry.models.project import Project from sentry.services.eventstore.models import GroupEvent @@ -156,6 +157,15 @@ def get_detector_by_group(group: Group) -> Detector: return Detector.objects.get(project_id=group.project_id, type=IssueStreamGroupType.slug) +def get_detector_from_event_data(event_data: WorkflowEventData) -> Detector: + if isinstance(event_data.event, GroupEvent): + return get_detector_by_event(event_data) + elif isinstance(event_data.event, Activity): + return get_detector_by_group(event_data.group) + else: + raise TypeError(f"Cannot determine the detector from {type(event_data.event)}.") + + class _SplitEvents(NamedTuple): events_with_occurrences: list[tuple[GroupEvent, int]] error_events: list[GroupEvent] diff --git a/src/sentry/workflow_engine/tasks/actions.py b/src/sentry/workflow_engine/tasks/actions.py index e224be61652084..c699c6aa3f3b06 100644 --- a/src/sentry/workflow_engine/tasks/actions.py +++ b/src/sentry/workflow_engine/tasks/actions.py @@ -78,10 +78,7 @@ def trigger_action( 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, - get_detector_by_group, - ) + from sentry.workflow_engine.processors.detector import get_detector_from_event_data # 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): @@ -108,14 +105,10 @@ def trigger_action( has_reappeared=has_reappeared, has_escalated=has_escalated, ) - if not detector: - detector = get_detector_by_event(event_data) elif activity_id is not None: event_data = build_workflow_event_data_from_activity( activity_id=activity_id, group_id=group_id ) - if not detector: - detector = get_detector_by_group(event_data.group) else: # This should never happen, and if it does, need to investigate logger.error( @@ -124,6 +117,9 @@ def trigger_action( ) raise ValueError("Exactly one of event_id or activity_id must be provided") + if not detector: + detector = get_detector_from_event_data(event_data) + metrics.incr( "workflow_engine.tasks.trigger_action_task_started", tags={"action_type": action.type, "detector_type": detector.type}, From 3d613a4e6b79fecb1655d20fb6d97bb83fbed9fb Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Wed, 12 Nov 2025 16:33:17 -0800 Subject: [PATCH 09/12] logging --- .../workflow_engine/processors/detector.py | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/sentry/workflow_engine/processors/detector.py b/src/sentry/workflow_engine/processors/detector.py index 9d329655337cba..c30c2e40a38226 100644 --- a/src/sentry/workflow_engine/processors/detector.py +++ b/src/sentry/workflow_engine/processors/detector.py @@ -158,12 +158,26 @@ def get_detector_by_group(group: Group) -> Detector: def get_detector_from_event_data(event_data: WorkflowEventData) -> Detector: - if isinstance(event_data.event, GroupEvent): - return get_detector_by_event(event_data) - elif isinstance(event_data.event, Activity): - return get_detector_by_group(event_data.group) - else: - raise TypeError(f"Cannot determine the detector from {type(event_data.event)}.") + try: + if isinstance(event_data.event, GroupEvent): + return get_detector_by_event(event_data) + elif isinstance(event_data.event, Activity): + return get_detector_by_group(event_data.group) + except Detector.DoesNotExist: + logger.exception( + "Detector not found for event", + extra={ + "type": type(event_data.event), + "id": ( + event_data.event.event_id + if isinstance(event_data.event, GroupEvent) + else event_data.event.id + ), + "group_id": event_data.group.id, + }, + ) + + raise TypeError(f"Cannot determine the detector from {type(event_data.event)}.") class _SplitEvents(NamedTuple): From c197ba30497af479ca39217a014a9f1a430a16c0 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Wed, 12 Nov 2025 16:34:02 -0800 Subject: [PATCH 10/12] log name --- src/sentry/workflow_engine/processors/detector.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sentry/workflow_engine/processors/detector.py b/src/sentry/workflow_engine/processors/detector.py index c30c2e40a38226..72a71075082f1b 100644 --- a/src/sentry/workflow_engine/processors/detector.py +++ b/src/sentry/workflow_engine/processors/detector.py @@ -165,7 +165,7 @@ def get_detector_from_event_data(event_data: WorkflowEventData) -> Detector: return get_detector_by_group(event_data.group) except Detector.DoesNotExist: logger.exception( - "Detector not found for event", + "Detector not found for event data", extra={ "type": type(event_data.event), "id": ( @@ -176,6 +176,7 @@ def get_detector_from_event_data(event_data: WorkflowEventData) -> Detector: "group_id": event_data.group.id, }, ) + raise Detector.DoesNotExist("Detector not found for event data") raise TypeError(f"Cannot determine the detector from {type(event_data.event)}.") From e25609752b3d2664fbe7afed358fa2244a1b0e8d Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Wed, 12 Nov 2025 16:53:59 -0800 Subject: [PATCH 11/12] fix mock --- tests/sentry/workflow_engine/models/test_action.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/sentry/workflow_engine/models/test_action.py b/tests/sentry/workflow_engine/models/test_action.py index 071d4ae9915b1a..e7597a1eee164c 100644 --- a/tests/sentry/workflow_engine/models/test_action.py +++ b/tests/sentry/workflow_engine/models/test_action.py @@ -70,7 +70,7 @@ def test_get_handler_unregistered_type(self) -> None: # Verify the registry was queried with the correct action type mock_get.assert_called_once_with(Action.Type.SLACK) - @patch("sentry.workflow_engine.processors.detector.get_detector_by_group") + @patch("sentry.workflow_engine.processors.detector.get_detector_from_event_data") def test_trigger_calls_handler_execute(self, mock_get_detector: MagicMock) -> None: mock_handler = Mock(spec=ActionHandler) mock_get_detector.return_value = Mock(spec=Detector, type="error") @@ -82,7 +82,7 @@ def test_trigger_calls_handler_execute(self, mock_get_detector: MagicMock) -> No self.mock_event, self.action, mock_get_detector.return_value ) - @patch("sentry.workflow_engine.processors.detector.get_detector_by_group") + @patch("sentry.workflow_engine.processors.detector.get_detector_from_event_data") def test_trigger_with_failing_handler(self, mock_get_detector: MagicMock) -> None: mock_handler = Mock(spec=ActionHandler) mock_handler.execute.side_effect = Exception("Handler failed") @@ -93,7 +93,7 @@ def test_trigger_with_failing_handler(self, mock_get_detector: MagicMock) -> Non self.action.trigger(self.mock_event) @patch("sentry.utils.metrics.incr") - @patch("sentry.workflow_engine.processors.detector.get_detector_by_group") + @patch("sentry.workflow_engine.processors.detector.get_detector_from_event_data") def test_trigger_metrics(self, mock_get_detector: MagicMock, mock_incr: MagicMock) -> None: mock_handler = Mock(spec=ActionHandler) mock_get_detector.return_value = Mock(spec=Detector, type="error") From 4e4b3da76b2a1d18d329d68e3d29a43d7fd4ef8d Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Wed, 12 Nov 2025 17:46:52 -0800 Subject: [PATCH 12/12] fix test --- .../endpoints/test_organization_test_fire_action.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py b/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py index 8eb228fb1d1500..0d7b07ec8a37b9 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_test_fire_action.py @@ -99,10 +99,7 @@ def test_pagerduty_action( assert mock_send_trigger.call_count == 1 pagerduty_data = mock_send_trigger.call_args.kwargs.get("data") assert pagerduty_data is not None - # test notification has its own type, so it will pick up the issue stream detector - assert pagerduty_data["payload"]["summary"].startswith( - f"[{self.issue_stream_detector.name}]:" - ) + assert pagerduty_data["payload"]["summary"].startswith(f"[{self.detector.name}]:") @mock.patch.object(NotifyEventAction, "after") @mock.patch(