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..42a4b3aa4c115d 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,11 @@ 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_from_event_data + + detector = get_detector_from_event_data(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..72a71075082f1b 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 @@ -139,6 +140,47 @@ 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) + + +def get_detector_from_event_data(event_data: WorkflowEventData) -> Detector: + 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 data", + 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 Detector.DoesNotExist("Detector not found for event data") + + 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 47d930d084acf2..c699c6aa3f3b06 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,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_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): @@ -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, @@ -109,7 +105,6 @@ def trigger_action( has_reappeared=has_reappeared, has_escalated=has_escalated, ) - elif activity_id is not None: event_data = build_workflow_event_data_from_activity( activity_id=activity_id, group_id=group_id @@ -122,6 +117,15 @@ 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}, + sample_rate=1.0, + ) + should_trigger_actions = should_fire_workflow_actions( detector.project.organization, event_data.group.type ) @@ -130,7 +134,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/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, 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/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..e7597a1eee164c 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,41 @@ 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_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") 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 + self.mock_event, self.action, mock_get_detector.return_value ) - def test_trigger_with_failing_handler(self) -> None: + @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") + 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_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") 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, ) 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()