diff --git a/src/sentry/integrations/jira/integration.py b/src/sentry/integrations/jira/integration.py index 9669982862328c..175dc631e58eeb 100644 --- a/src/sentry/integrations/jira/integration.py +++ b/src/sentry/integrations/jira/integration.py @@ -2,9 +2,10 @@ import logging import re +import sys from collections.abc import Mapping, Sequence from operator import attrgetter -from typing import Any, TypedDict +from typing import Any, NoReturn, TypedDict import sentry_sdk from django.conf import settings @@ -51,6 +52,7 @@ IntegrationFormError, ) from sentry.silo.base import all_silo_function +from sentry.users.models.identity import Identity from sentry.users.models.user import User from sentry.users.services.user import RpcUser from sentry.users.services.user.service import user_service @@ -970,6 +972,31 @@ def create_issue(self, data, **kwargs): # Immediately fetch and return the created issue. return self.get_issue(issue_key) + def raise_error(self, exc: Exception, identity: Identity | None = None) -> NoReturn: + """ + Overrides the base `raise_error` method to treat ApiInvalidRequestErrors + as configuration errors when we don't have error field handling for the + response. + + This is because the majority of Jira errors we receive are external + configuration problems, like required fields missing. + """ + if isinstance(exc, ApiInvalidRequestError): + if exc.json: + error_fields = self.error_fields_from_json(exc.json) + if error_fields is not None: + raise IntegrationFormError(error_fields).with_traceback(sys.exc_info()[2]) + + logger.warning( + "sentry.jira.raise_error.api_invalid_request_error", + extra={ + "exception_type": type(exc).__name__, + "request_body": str(exc.json), + }, + ) + raise IntegrationConfigurationError(exc.text) from exc + super().raise_error(exc, identity=identity) + def sync_assignee_outbound( self, external_issue: ExternalIssue, diff --git a/src/sentry/notifications/notification_action/types.py b/src/sentry/notifications/notification_action/types.py index 4976634e327963..a0c4263f728d5a 100644 --- a/src/sentry/notifications/notification_action/types.py +++ b/src/sentry/notifications/notification_action/types.py @@ -9,6 +9,7 @@ from sentry import features from sentry.constants import ObjectStatus +from sentry.exceptions import InvalidIdentity from sentry.incidents.grouptype import MetricIssueEvidenceData from sentry.incidents.models.incident import TriggerStatus from sentry.incidents.typings.metric_detector import ( @@ -27,9 +28,15 @@ from sentry.notifications.types import TEST_NOTIFICATION_ID from sentry.rules.processing.processor import activate_downstream_actions from sentry.services.eventstore.models import GroupEvent +from sentry.shared_integrations.exceptions import ( + ApiError, + IntegrationConfigurationError, + IntegrationFormError, +) +from sentry.taskworker.retry import RetryError +from sentry.taskworker.workerchild import ProcessingDeadlineExceeded from sentry.types.activity import ActivityType from sentry.types.rules import RuleFuture -from sentry.utils.safe import safe_execute from sentry.workflow_engine.models import Action, AlertRuleWorkflow, Detector from sentry.workflow_engine.types import DetectorPriorityLevel, WorkflowEventData from sentry.workflow_engine.typings.notification_action import ( @@ -41,6 +48,8 @@ logger = logging.getLogger(__name__) +FutureCallback = Callable[[GroupEvent, Sequence[RuleFuture]], Any] + class RuleData(TypedDict): actions: list[dict[str, Any]] @@ -63,6 +72,48 @@ def handle_workflow_action( raise NotImplementedError +EXCEPTION_IGNORE_LIST = (IntegrationFormError, IntegrationConfigurationError, InvalidIdentity) +RETRYABLE_EXCEPTIONS = (ApiError,) + + +def invoke_future_with_error_handling( + event_data: WorkflowEventData, + callback: FutureCallback, + future: Sequence[RuleFuture], +) -> None: + # WorkflowEventData should only ever be a GroupEvent in this context, so we + # narrow the type here to keep mypy happy. + assert isinstance( + event_data.event, GroupEvent + ), f"Expected a GroupEvent, received: {type(event_data.event).__name__}" + try: + callback(event_data.event, future) + except EXCEPTION_IGNORE_LIST: + # no-op on any exceptions in the ignore list. We likely have + # reporting for them in the integration code already. + pass + except ProcessingDeadlineExceeded: + # We need to reraise ProcessingDeadlineExceeded for workflow engine to + # monitor and potentially retry this action. + raise + except RETRYABLE_EXCEPTIONS as e: + raise RetryError from e + except Exception as e: + # This is just a redefinition of the safe_execute util function, as we + # still want to report any unhandled exceptions. + if hasattr(callback, "im_class"): + cls = callback.im_class + else: + cls = callback.__class__ + + func_name = getattr(callback, "__name__", str(callback)) + cls_name = cls.__name__ + local_logger = logging.getLogger(f"sentry.safe_action.{cls_name.lower()}") + + local_logger.exception("%s.process_error", func_name, extra={"exception": e}) + return None + + class BaseIssueAlertHandler(ABC): """ Base class for invoking the legacy issue alert registry. @@ -220,9 +271,7 @@ def get_rule_futures( @staticmethod def execute_futures( event_data: WorkflowEventData, - futures: Collection[ - tuple[Callable[[GroupEvent, Sequence[RuleFuture]], None], list[RuleFuture]] - ], + futures: Collection[tuple[FutureCallback, list[RuleFuture]]], ) -> None: """ This method will execute the futures. @@ -234,7 +283,7 @@ def execute_futures( ) for callback, future in futures: - safe_execute(callback, event_data.event, future) + invoke_future_with_error_handling(event_data, callback, future) @staticmethod def send_test_notification( diff --git a/src/sentry/rules/actions/integrations/create_ticket/utils.py b/src/sentry/rules/actions/integrations/create_ticket/utils.py index 2c13aebfad653e..3d55b03cc601ad 100644 --- a/src/sentry/rules/actions/integrations/create_ticket/utils.py +++ b/src/sentry/rules/actions/integrations/create_ticket/utils.py @@ -18,7 +18,6 @@ from sentry.integrations.services.integration.model import RpcIntegration from sentry.integrations.services.integration.service import integration_service from sentry.models.grouplink import GroupLink -from sentry.notifications.types import TEST_NOTIFICATION_ID from sentry.notifications.utils.links import create_link_to_workflow from sentry.services.eventstore.models import GroupEvent from sentry.shared_integrations.exceptions import ( @@ -187,10 +186,8 @@ def create_issue(event: GroupEvent, futures: Sequence[RuleFuture]) -> None: ) as e: # Most of the time, these aren't explicit failures, they're # some misconfiguration of an issue field - typically Jira. - # We only want to raise if the rule_id is -1 because that means we're testing the action lifecycle.record_halt(e) - if rule_id == TEST_NOTIFICATION_ID: - raise + raise # If we successfully created the issue, we want to create the link else: create_link(integration, installation, event, response) diff --git a/tests/sentry/integrations/jira/test_integration.py b/tests/sentry/integrations/jira/test_integration.py index 680e531b77eea2..4ada11284508b7 100644 --- a/tests/sentry/integrations/jira/test_integration.py +++ b/tests/sentry/integrations/jira/test_integration.py @@ -19,7 +19,11 @@ from sentry.integrations.services.integration import integration_service from sentry.models.grouplink import GroupLink from sentry.models.groupmeta import GroupMeta -from sentry.shared_integrations.exceptions import IntegrationConfigurationError, IntegrationError +from sentry.shared_integrations.exceptions import ( + IntegrationConfigurationError, + IntegrationError, + IntegrationFormError, +) from sentry.silo.base import SiloMode from sentry.testutils.cases import APITestCase, IntegrationTestCase from sentry.testutils.factories import EventType @@ -711,6 +715,59 @@ def test_create_issue(self) -> None: "key": "APP-123", } + @responses.activate + def test_create_issue_with_form_error(self) -> None: + responses.add( + responses.GET, + "https://example.atlassian.net/rest/api/2/issue/createmeta", + body=StubService.get_stub_json("jira", "createmeta_response.json"), + content_type="json", + ) + responses.add( + responses.POST, + "https://example.atlassian.net/rest/api/2/issue", + status=400, + body=json.dumps({"errors": {"issuetype": ["Issue type is required."]}}), + content_type="json", + ) + + installation = self.integration.get_installation(self.organization.id) + with pytest.raises(IntegrationFormError): + installation.create_issue( + { + "title": "example summary", + "description": "example bug report", + "issuetype": "1", + "project": "10000", + } + ) + + @responses.activate + def test_create_issue_with_configuration_error(self) -> None: + responses.add( + responses.GET, + "https://example.atlassian.net/rest/api/2/issue/createmeta", + body=StubService.get_stub_json("jira", "createmeta_response.json"), + content_type="json", + ) + responses.add( + responses.POST, + "https://example.atlassian.net/rest/api/2/issue", + status=400, + body=json.dumps({"error": "Jira had an oopsie"}), + content_type="json", + ) + installation = self.integration.get_installation(self.organization.id) + with pytest.raises(IntegrationConfigurationError): + installation.create_issue( + { + "title": "example summary", + "description": "example bug report", + "issuetype": "1", + "project": "10000", + } + ) + @responses.activate def test_create_issue_labels_and_option(self) -> None: installation = self.integration.get_installation(self.organization.id) diff --git a/tests/sentry/integrations/jira/test_ticket_action.py b/tests/sentry/integrations/jira/test_ticket_action.py index f698ac60cddead..64a273a28046ce 100644 --- a/tests/sentry/integrations/jira/test_ticket_action.py +++ b/tests/sentry/integrations/jira/test_ticket_action.py @@ -13,10 +13,9 @@ from sentry.shared_integrations.exceptions import ( ApiInvalidRequestError, IntegrationConfigurationError, - IntegrationError, IntegrationFormError, ) -from sentry.testutils.asserts import assert_failure_metric, assert_halt_metric +from sentry.testutils.asserts import assert_halt_metric from sentry.testutils.cases import RuleTestCase from sentry.testutils.skips import requires_snuba from sentry.types.rules import RuleFuture @@ -143,18 +142,16 @@ def raise_api_error(*args, **kwargs): rule_object = Rule.objects.get(id=response.data["id"]) event = self.get_event() - with pytest.raises(IntegrationError): - # Trigger its `after`, but with a broken client which should raise - # an ApiInvalidRequestError, which is reraised as an IntegrationError. + with pytest.raises(IntegrationConfigurationError): self.trigger(event, rule_object) assert mock_record_event.call_count == 2 - start, failure = mock_record_event.call_args_list + start, halt = mock_record_event.call_args_list assert start.args == (EventLifecycleOutcome.STARTED,) - assert_failure_metric( + assert_halt_metric( mock_record_event, - IntegrationError("Error Communicating with Jira (HTTP 400): unknown error"), + IntegrationConfigurationError(), ) def test_fails_validation(self) -> None: @@ -211,7 +208,8 @@ def raise_api_error_with_payload(*args, **kwargs): rule_object = Rule.objects.get(id=response.data["id"]) event = self.get_event() - self.trigger(event, rule_object) + with pytest.raises(IntegrationFormError): + self.trigger(event, rule_object) assert mock_record_event.call_count == 2 start, halt = mock_record_event.call_args_list @@ -233,7 +231,8 @@ def test_halts_with_external_api_configuration_error( rule_object = Rule.objects.get(id=response.data["id"]) event = self.get_event() - self.trigger(event, rule_object) + with pytest.raises(IntegrationConfigurationError): + self.trigger(event, rule_object) assert mock_record_event.call_count == 2 start, halt = mock_record_event.call_args_list diff --git a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py index 8544139e1c2d2f..b223e5628471d0 100644 --- a/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py +++ b/tests/sentry/notifications/notification_action/test_issue_alert_registry_handlers.py @@ -250,11 +250,11 @@ def test_create_rule_instance_from_action_no_environment_with_workflow_engine_ui assert rule.status == ObjectStatus.ACTIVE assert rule.source == RuleSource.ISSUE - @mock.patch("sentry.notifications.notification_action.types.safe_execute") + @mock.patch("sentry.notifications.notification_action.types.invoke_future_with_error_handling") @mock.patch("sentry.notifications.notification_action.types.activate_downstream_actions") @mock.patch("uuid.uuid4") def test_invoke_legacy_registry( - self, mock_uuid, mock_activate_downstream_actions, mock_safe_execute + self, mock_uuid, mock_activate_downstream_actions, mock_invoke_future_with_error_handling ): # Test that invoke_legacy_registry correctly processes the action mock_uuid.return_value = uuid.UUID("12345678-1234-5678-1234-567812345678") @@ -272,8 +272,8 @@ def test_invoke_legacy_registry( ) # Verify callback execution - mock_safe_execute.assert_called_once_with( - mock_callback, self.event_data.event, mock_futures + mock_invoke_future_with_error_handling.assert_called_once_with( + self.event_data, mock_callback, mock_futures ) @@ -821,3 +821,140 @@ def test_build_rule_action_blob_sentry_app_no_settings(self) -> None: # Both orgs should have different sentry app installations assert action_1_uuid != action_2_uuid + + +class TestInvokeFutureWithErrorHandling(BaseWorkflowTest): + def setUp(self) -> None: + super().setUp() + self.project = self.create_project() + self.group, self.event, self.group_event = self.create_group_event() + self.event_data = WorkflowEventData( + event=self.group_event, workflow_env=self.environment, group=self.group + ) + + self.mock_callback = mock.Mock() + self.mock_futures = [mock.Mock()] + + def test_happy_path(self): + from sentry.notifications.notification_action.types import invoke_future_with_error_handling + + invoke_future_with_error_handling(self.event_data, self.mock_callback, self.mock_futures) + + self.mock_callback.assert_called_once_with(self.group_event, self.mock_futures) + + def test_invalid_event_data(self): + from sentry.notifications.notification_action.types import invoke_future_with_error_handling + from sentry.workflow_engine.types import WorkflowEventData + + invalid_event_data = WorkflowEventData( + event=mock.Mock(), workflow_env=self.environment, group=self.group + ) + + with pytest.raises(AssertionError) as excinfo: + invoke_future_with_error_handling( + invalid_event_data, self.mock_callback, self.mock_futures + ) + + assert "Expected a GroupEvent" in str(excinfo.value) + + def test_ignores_integration_form_error(self): + from sentry.notifications.notification_action.types import invoke_future_with_error_handling + from sentry.shared_integrations.exceptions import IntegrationFormError + + self.mock_callback.side_effect = IntegrationFormError( + field_errors={"foo": "Test form error"} + ) + + invoke_future_with_error_handling(self.event_data, self.mock_callback, self.mock_futures) + + self.mock_callback.assert_called_once() + + def test_ignores_integration_configuration_error(self): + from sentry.notifications.notification_action.types import invoke_future_with_error_handling + from sentry.shared_integrations.exceptions import IntegrationConfigurationError + + self.mock_callback.side_effect = IntegrationConfigurationError("Test config error") + + invoke_future_with_error_handling(self.event_data, self.mock_callback, self.mock_futures) + + self.mock_callback.assert_called_once() + + def test_reraises_processing_deadline_exceeded(self): + from sentry.notifications.notification_action.types import invoke_future_with_error_handling + from sentry.taskworker.workerchild import ProcessingDeadlineExceeded + + self.mock_callback.side_effect = ProcessingDeadlineExceeded("Deadline exceeded") + + with pytest.raises(ProcessingDeadlineExceeded): + invoke_future_with_error_handling( + self.event_data, self.mock_callback, self.mock_futures + ) + + self.mock_callback.assert_called_once() + + def test_raises_retry_error_for_api_error(self): + from sentry.notifications.notification_action.types import invoke_future_with_error_handling + from sentry.shared_integrations.exceptions import ApiError + from sentry.taskworker.retry import RetryError + + self.mock_callback.side_effect = ApiError("API error", 500) + + with pytest.raises(RetryError) as excinfo: + invoke_future_with_error_handling( + self.event_data, self.mock_callback, self.mock_futures + ) + + assert isinstance(excinfo.value.__cause__, ApiError) + self.mock_callback.assert_called_once() + + @mock.patch("logging.getLogger") + def test_safe_execute_exception_handling(self, mock_get_logger): + from sentry.notifications.notification_action.types import invoke_future_with_error_handling + + mock_localized_logger = mock.Mock() + mock_get_logger.return_value = mock_localized_logger + + test_exception = ValueError("Generic test error") + + class TestCallbackClass: + def __call__(self, event, futures): # noqa: ARG002 + raise test_exception + + @property + def __name__(self): + return "test_callback" + + failing_callback = TestCallbackClass() + + invoke_future_with_error_handling(self.event_data, failing_callback, self.mock_futures) + + mock_get_logger.assert_called_once_with("sentry.safe_action.testcallbackclass") + + mock_localized_logger.exception.assert_called_once_with( + "%s.process_error", "test_callback", extra={"exception": test_exception} + ) + + @mock.patch("logging.getLogger") + def test_generic_exception_with_no_name_attribute(self, mock_get_logger): + from sentry.notifications.notification_action.types import invoke_future_with_error_handling + + mock_localized_logger = mock.Mock() + mock_get_logger.return_value = mock_localized_logger + + test_exception = Exception("Test error") + + class CallableWithoutName: + def __call__(self, event, futures): # noqa: ARG002 + raise test_exception + + callback_without_name = CallableWithoutName() + callback_without_name.__class__.__name__ = "CallbackWithoutName" + + invoke_future_with_error_handling(self.event_data, callback_without_name, self.mock_futures) + + mock_get_logger.assert_called_once_with("sentry.safe_action.callbackwithoutname") + + expected_func_name = str(callback_without_name) + mock_localized_logger.exception.assert_called_once_with( + "%s.process_error", expected_func_name, extra={"exception": test_exception} + )