✨ feat(integration slos): add create_issue parameter to integration SLO metrics (#94140)

Author: iamrajjoshi

src/sentry/integrations/utils/metrics.py

@@ -123,7 +123,10 @@ def add_extras(self, extras: Mapping[str, Any]) -> None:
         self._extra.update(extras)
 
     def record_event(
-        self, outcome: EventLifecycleOutcome, outcome_reason: BaseException | str | None = None
+        self,
+        outcome: EventLifecycleOutcome,
+        outcome_reason: BaseException | str | None = None,
+        create_issue: bool = False,
     ) -> None:
         """Record a starting or halting event.
 
@@ -146,16 +149,26 @@ def record_event(
         }
 
         if isinstance(outcome_reason, BaseException):
-            # (iamrajjoshi): Log the full exception and the repr of the exception
-            # This is an experiment to dogfood Sentry logs
-            #  Logs are paid by bytes, we save money by mking optimizations like this - we should try to dogfood from a similar perspective
-            log_params["exc_info"] = outcome_reason
+            # Capture exception in Sentry if create_issue is True
+            if create_issue:
+                # If the outcome is halted, we want to set the level to warning
+                if outcome == EventLifecycleOutcome.HALTED:
+                    sentry_sdk.set_level("warning")
+
+                event_id = sentry_sdk.capture_exception(
+                    outcome_reason,
+                )
+
+                log_params["extra"]["slo_event_id"] = event_id
+
+            # Add exception summary but don't include full stack trace in logs
+            # TODO(iamrajjoshi): Phase this out once everyone is comfortable with just using the sentry issue
             log_params["extra"]["exception_summary"] = repr(outcome_reason)
         elif isinstance(outcome_reason, str):
             extra["outcome_reason"] = outcome_reason
 
         if outcome == EventLifecycleOutcome.FAILURE:
-            logger.error(key, **log_params)
+            logger.warning(key, **log_params)
         elif outcome == EventLifecycleOutcome.HALTED:
             logger.warning(key, **log_params)
 
@@ -164,14 +177,17 @@ def _report_flow_error(message) -> None:
         logger.error("EventLifecycle flow error: %s", message)
 
     def _terminate(
-        self, new_state: EventLifecycleOutcome, outcome_reason: BaseException | str | None = None
+        self,
+        new_state: EventLifecycleOutcome,
+        outcome_reason: BaseException | str | None = None,
+        create_issue: bool = False,
     ) -> None:
         if self._state is None:
             self._report_flow_error("The lifecycle has not yet been entered")
         if self._state != EventLifecycleOutcome.STARTED:
             self._report_flow_error("The lifecycle has already been exited")
         self._state = new_state
-        self.record_event(new_state, outcome_reason)
+        self.record_event(new_state, outcome_reason, create_issue)
 
     def record_success(self) -> None:
         """Record that the event halted successfully.
@@ -184,14 +200,20 @@ def record_success(self) -> None:
         self._terminate(EventLifecycleOutcome.SUCCESS)
 
     def record_failure(
-        self, failure_reason: BaseException | str | None = None, extra: dict[str, Any] | None = None
+        self,
+        failure_reason: BaseException | str | None = None,
+        extra: dict[str, Any] | None = None,
+        create_issue: bool = True,
     ) -> None:
         """Record that the event halted in failure. Additional data may be passed
         to be logged.
 
         Calling it means that the feature is broken and requires immediate attention.
 
-        An error will be reported to Sentry.
+        An error will be reported to Sentry if create_issue is True (default).
+        The default is True because we want to create an issue for all failures
+        because it will provide a stack trace and help us debug the issue.
+        There needs to be a compelling reason to not create an issue for a failure
 
         There is no need to call this method directly if an exception is raised from
         inside the context. It will be called automatically when exiting the context
@@ -205,20 +227,27 @@ def record_failure(
 
         if extra:
             self._extra.update(extra)
-        self._terminate(EventLifecycleOutcome.FAILURE, failure_reason)
+        self._terminate(EventLifecycleOutcome.FAILURE, failure_reason, create_issue)
 
     def record_halt(
-        self, halt_reason: BaseException | str | None = None, extra: dict[str, Any] | None = None
+        self,
+        halt_reason: BaseException | str | None = None,
+        extra: dict[str, Any] | None = None,
+        create_issue: bool = False,
     ) -> None:
         """Record that the event halted in an ambiguous state.
 
-        It will be logged to GCP but no Sentry error will be reported.
+        It will be logged to GCP but no Sentry error will be reported by default.
+        The default is False because we don't want to create an issue for all halts.
+        However for certain debugging cases, we may want to create an issue.
 
         This method can be called in response to a sufficiently ambiguous exception
         or other error condition, where it may have been caused by a user error or
         other expected condition, but there is some substantial chance that it
         represents a bug.
 
+        Set create_issue=True if you want to create a Sentry issue for this halt.
+
         Such cases usually mean that we want to:
           (1) document the ambiguity;
           (2) monitor it for sudden spikes in frequency; and
@@ -228,7 +257,7 @@ def record_halt(
 
         if extra:
             self._extra.update(extra)
-        self._terminate(EventLifecycleOutcome.HALTED, halt_reason)
+        self._terminate(EventLifecycleOutcome.HALTED, halt_reason, create_issue)
 
     def __enter__(self) -> Self:
         if self._state is not None:
@@ -250,7 +279,8 @@ def __exit__(
 
         if exc_value is not None:
             # We were forced to exit the context by a raised exception.
-            self.record_failure(exc_value)
+            # Default to creating a Sentry issue for unhandled exceptions
+            self.record_failure(exc_value, create_issue=True)
         else:
             # We exited the context without record_success or record_failure being
             # called. Assume success if we were told to do so. Else, log a halt

tests/sentry/api/endpoints/test_group_integration_details.py

@@ -256,7 +256,7 @@ def test_simple_put(self, mock_record_event):
             assert response.status_code == 201
             self.assert_correctly_linked(group, "APP-123", integration, org)
 
-        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None)
+        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False)
 
     @mock.patch.object(ExampleIntegration, "get_issue")
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_halt")
@@ -422,7 +422,7 @@ def test_simple_post(self, mock_record_event):
                 "new": True,
             }
 
-            mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None)
+            mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False)
 
     @mock.patch.object(ExampleIntegration, "create_issue")
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_halt")

tests/sentry/integrations/jira/test_installed.py

@@ -121,7 +121,7 @@ def test_with_shared_secret(self, mock_set_tag: MagicMock, mock_record_event):
 
         mock_set_tag.assert_any_call("integration_id", integration.id)
         assert integration.status == ObjectStatus.ACTIVE
-        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None)
+        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False)
 
     @patch("sentry_sdk.set_tag")
     @responses.activate

tests/sentry/integrations/jira/test_ticket_action.py

@@ -123,7 +123,7 @@ def test_ticket_rules(self, mock_record_event):
 
             # assert new ticket NOT created in DB
             assert ExternalIssue.objects.count() == external_issue_count
-            mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None)
+            mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False)
 
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     @mock.patch.object(MockJira, "create_issue")

tests/sentry/integrations/tasks/test_sync_assignee_outbound.py

@@ -54,7 +54,7 @@ def test_syncs_outbound_assignee(self, mock_sync_assignee, mock_record_event):
         assert isinstance(user_arg, RpcUser)
         assert user_arg.id == self.user.id
 
-        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None)
+        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False)
 
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_failure")
     @mock.patch.object(ExampleIntegration, "sync_assignee_outbound")

tests/sentry/integrations/tasks/test_sync_status_outbound.py

@@ -52,7 +52,7 @@ def test_successful_outbound_sync(self, mock_sync_status, mock_record_event):
 
         sync_status_outbound(self.group.id, external_issue_id=external_issue.id)
         mock_sync_status.assert_called_once_with(external_issue, False, self.group.project_id)
-        mock_record_event.assert_any_call(EventLifecycleOutcome.SUCCESS, None)
+        mock_record_event.assert_any_call(EventLifecycleOutcome.SUCCESS, None, False)
 
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     @mock.patch.object(ExampleIntegration, "sync_status_outbound")
@@ -69,7 +69,7 @@ def test_should_not_sync(self, mock_should_sync, mock_sync_status, mock_record_e
         assert mock_record_event.call_count == 2
         start, success = mock_record_event.mock_calls
         assert start.args == (EventLifecycleOutcome.STARTED,)
-        assert success.args == (EventLifecycleOutcome.SUCCESS, None)
+        assert success.args == (EventLifecycleOutcome.SUCCESS, None, False)
 
     @mock.patch.object(ExampleIntegration, "sync_status_outbound")
     def test_missing_external_issue(self, mock_sync_status):

tests/sentry/integrations/utils/test_lifecycle_metrics.py

@@ -66,6 +66,7 @@ def test_recording_success(self, mock_metrics, mock_logger):
             pass
         self._check_metrics_call_args(mock_metrics, "success")
         mock_logger.error.assert_not_called()
+        mock_logger.warning.assert_not_called()
 
     @mock.patch("sentry.integrations.utils.metrics.logger")
     @mock.patch("sentry.integrations.utils.metrics.metrics")
@@ -74,7 +75,14 @@ def test_recording_halt(self, mock_metrics, mock_logger):
         with metric_obj.capture(assume_success=False):
             pass
         self._check_metrics_call_args(mock_metrics, "halted")
-        mock_logger.error.assert_not_called()
+        mock_logger.warning.assert_called_once_with(
+            "integrations.slo.halted",
+            extra={
+                "integration_domain": "messaging",
+                "integration_name": "my_integration",
+                "interaction_type": "my_interaction",
+            },
+        )
 
     @mock.patch("sentry.integrations.utils.metrics.logger")
     @mock.patch("sentry.integrations.utils.metrics.metrics")
@@ -95,7 +103,6 @@ def test_recording_explicit_halt_with_exception(self, mock_metrics, mock_logger)
                 "interaction_type": "my_interaction",
                 "exception_summary": repr(ExampleException("")),
             },
-            exc_info=mock.ANY,
         )
 
     @mock.patch("sentry.integrations.utils.metrics.logger")
@@ -119,31 +126,40 @@ def test_recording_explicit_halt_with_str(self, mock_metrics, mock_logger):
             },
         )
 
+    @mock.patch("sentry.integrations.utils.metrics.sentry_sdk")
     @mock.patch("sentry.integrations.utils.metrics.logger")
     @mock.patch("sentry.integrations.utils.metrics.metrics")
-    def test_recording_failure(self, mock_metrics, mock_logger):
+    def test_recording_failure(self, mock_metrics, mock_logger, mock_sentry_sdk):
+        mock_sentry_sdk.capture_exception.return_value = "test-event-id"
+
         metric_obj = self.TestLifecycleMetric()
         with pytest.raises(ExampleException):
             with metric_obj.capture() as lifecycle:
                 lifecycle.add_extra("extra", "value")
                 raise ExampleException()
 
         self._check_metrics_call_args(mock_metrics, "failure")
-        mock_logger.error.assert_called_once_with(
+        mock_sentry_sdk.capture_exception.assert_called_once()
+        mock_logger.warning.assert_called_once_with(
             "integrations.slo.failure",
             extra={
                 "extra": "value",
                 "integration_domain": "messaging",
                 "integration_name": "my_integration",
                 "interaction_type": "my_interaction",
                 "exception_summary": repr(ExampleException()),
+                "slo_event_id": "test-event-id",
             },
-            exc_info=mock.ANY,
         )
 
+    @mock.patch("sentry.integrations.utils.metrics.sentry_sdk")
     @mock.patch("sentry.integrations.utils.metrics.logger")
     @mock.patch("sentry.integrations.utils.metrics.metrics")
-    def test_recording_explicit_failure_with_exception(self, mock_metrics, mock_logger):
+    def test_recording_explicit_failure_with_exception(
+        self, mock_metrics, mock_logger, mock_sentry_sdk
+    ):
+        mock_sentry_sdk.capture_exception.return_value = "test-event-id"
+
         metric_obj = self.TestLifecycleMetric()
         with metric_obj.capture() as lifecycle:
             try:
@@ -153,7 +169,8 @@ def test_recording_explicit_failure_with_exception(self, mock_metrics, mock_logg
                 lifecycle.record_failure(exc, extra={"even": "more"})
 
         self._check_metrics_call_args(mock_metrics, "failure")
-        mock_logger.error.assert_called_once_with(
+        mock_sentry_sdk.capture_exception.assert_called_once()
+        mock_logger.warning.assert_called_once_with(
             "integrations.slo.failure",
             extra={
                 "extra": "value",
@@ -162,8 +179,8 @@ def test_recording_explicit_failure_with_exception(self, mock_metrics, mock_logg
                 "integration_name": "my_integration",
                 "interaction_type": "my_interaction",
                 "exception_summary": repr(ExampleException()),
+                "slo_event_id": "test-event-id",
             },
-            exc_info=mock.ANY,
         )
 
     @mock.patch("sentry.integrations.utils.metrics.logger")
@@ -175,7 +192,7 @@ def test_recording_explicit_failure_with_str(self, mock_metrics, mock_logger):
             lifecycle.record_failure("Integration went boom", extra={"even": "more"})
 
         self._check_metrics_call_args(mock_metrics, "failure")
-        mock_logger.error.assert_called_once_with(
+        mock_logger.warning.assert_called_once_with(
             "integrations.slo.failure",
             extra={
                 "outcome_reason": "Integration went boom",
@@ -186,3 +203,56 @@ def test_recording_explicit_failure_with_str(self, mock_metrics, mock_logger):
                 "interaction_type": "my_interaction",
             },
         )
+
+    @mock.patch("sentry.integrations.utils.metrics.sentry_sdk")
+    @mock.patch("sentry.integrations.utils.metrics.logger")
+    @mock.patch("sentry.integrations.utils.metrics.metrics")
+    def test_recording_halt_with_create_issue_true(
+        self, mock_metrics, mock_logger, mock_sentry_sdk
+    ):
+        """
+        Test that halt can create Sentry issues when create_issue=True
+        """
+        mock_sentry_sdk.capture_exception.return_value = "test-event-id"
+
+        metric_obj = self.TestLifecycleMetric()
+        with metric_obj.capture() as lifecycle:
+            lifecycle.add_extra("extra", "value")
+            lifecycle.record_halt(ExampleException("test"), create_issue=True)
+
+        self._check_metrics_call_args(mock_metrics, "halted")
+        mock_sentry_sdk.capture_exception.assert_called_once()
+        mock_logger.warning.assert_called_once_with(
+            "integrations.slo.halted",
+            extra={
+                "extra": "value",
+                "integration_domain": "messaging",
+                "integration_name": "my_integration",
+                "interaction_type": "my_interaction",
+                "exception_summary": repr(ExampleException("test")),
+                "slo_event_id": "test-event-id",
+            },
+        )
+
+    @mock.patch("sentry.integrations.utils.metrics.logger")
+    @mock.patch("sentry.integrations.utils.metrics.metrics")
+    def test_recording_failure_with_create_issue_false(self, mock_metrics, mock_logger):
+        """
+        Test that failure can skip creating Sentry issues when create_issue=False
+        """
+        metric_obj = self.TestLifecycleMetric()
+        with metric_obj.capture() as lifecycle:
+            lifecycle.add_extra("extra", "value")
+            lifecycle.record_failure(ExampleException("test"), create_issue=False)
+
+        self._check_metrics_call_args(mock_metrics, "failure")
+        mock_logger.warning.assert_called_once_with(
+            "integrations.slo.failure",
+            extra={
+                "extra": "value",
+                "integration_domain": "messaging",
+                "integration_name": "my_integration",
+                "interaction_type": "my_interaction",
+                "exception_summary": repr(ExampleException("test")),
+            },
+        )

tests/sentry/integrations/utils/test_sync.py

@@ -76,7 +76,7 @@ def test_unassign(self, mock_record_event):
         )
 
         assert self.group.get_assignee() is None
-        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None)
+        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False)
 
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     def test_assignment(self, mock_record_event):
@@ -99,7 +99,7 @@ def test_assignment(self, mock_record_event):
         assert updated_assignee is not None
         assert updated_assignee.id == self.test_user.id
         assert updated_assignee.email == "test@example.com"
-        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None)
+        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False)
 
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     def test_assign_with_multiple_groups(self, mock_record_event):
@@ -149,7 +149,7 @@ def test_assign_with_multiple_groups(self, mock_record_event):
             assert assignee.id == self.test_user.id
             assert assignee.email == "test@example.com"
 
-        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None)
+        mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False)
 
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_halt")
     def test_assign_with_no_user_found(self, mock_record_halt):
@@ -208,7 +208,7 @@ def raise_exception(*args, **kwargs):
         updated_assignee = self.group.get_assignee()
         assert updated_assignee is None
 
-        mock_record_failure.assert_called_once_with(mock.ANY)
+        mock_record_failure.assert_called_once_with(mock.ANY, create_issue=True)
 
         exception_param = mock_record_failure.call_args_list[0].args[0]