ref(ecosystem): Resubmission of fixes for integration metrics record methods (#80798)

src/sentry/identity/oauth2.py

@@ -322,7 +322,7 @@ class OAuth2CallbackView(PipelineView):
                     "identity.oauth2.ssl-error",
                     extra={"url": self.access_token_url, "verify_ssl": verify_ssl},
                 )
-                lifecycle.record_failure({"failure_reason": "ssl_error"})
+                lifecycle.record_failure("ssl_error")
                 url = self.access_token_url
                 return {
                     "error": "Could not verify SSL certificate",
@@ -331,14 +331,14 @@ class OAuth2CallbackView(PipelineView):
             except ConnectionError:
                 url = self.access_token_url
                 logger.info("identity.oauth2.connection-error", extra={"url": url})
-                lifecycle.record_failure({"failure_reason": "connection_error"})
+                lifecycle.record_failure("connection_error")
                 return {
                     "error": "Could not connect to host or service",
                     "error_description": f"Ensure that {url} is open to connections",
                 }
             except orjson.JSONDecodeError:
                 logger.info("identity.oauth2.json-error", extra={"url": self.access_token_url})
-                lifecycle.record_failure({"failure_reason": "json_error"})
+                lifecycle.record_failure("json_error")
                 return {
                     "error": "Could not decode a JSON Response",
                     "error_description": "We were not able to parse a JSON response, please try again.",
@@ -355,7 +355,7 @@ class OAuth2CallbackView(PipelineView):
             if error:
                 pipeline.logger.info("identity.token-exchange-error", extra={"error": error})
                 lifecycle.record_failure(
-                    {"failure_reason": "token_exchange_error", "msg": ERR_INVALID_STATE}
+                    "token_exchange_error", extra={"failure_info": ERR_INVALID_STATE}
                 )
                 return pipeline.error(ERR_INVALID_STATE)
 
@@ -370,7 +370,7 @@ class OAuth2CallbackView(PipelineView):
                     },
                 )
                 lifecycle.record_failure(
-                    {"failure_reason": "token_exchange_error", "msg": ERR_INVALID_STATE}
+                    "token_exchange_error", extra={"failure_info": ERR_INVALID_STATE}
                 )
                 return pipeline.error(ERR_INVALID_STATE)
 

src/sentry/integrations/bitbucket/integration.py

@@ -269,7 +269,7 @@ class VerifyInstallation(PipelineView):
                     request, BitbucketIntegrationProvider.key
                 )
             except AtlassianConnectValidationError as e:
-                lifecycle.record_failure({"failure_reason": str(e)})
+                lifecycle.record_failure(str(e))
                 return pipeline.error("Unable to verify installation.")
 
             pipeline.bind_state("external_id", integration.external_id)

src/sentry/integrations/bitbucket_server/integration.py

@@ -185,14 +185,12 @@ class OAuthLoginView(PipelineView):
             try:
                 request_token = client.get_request_token()
             except ApiError as error:
-                lifecycle.record_failure({"failure_reason": str(error), "url": config.get("url")})
+                lifecycle.record_failure(str(error), extra={"url": config.get("url")})
                 return pipeline.error(f"Could not fetch a request token from Bitbucket. {error}")
 
             pipeline.bind_state("request_token", request_token)
             if not request_token.get("oauth_token"):
-                lifecycle.record_failure(
-                    {"failure_reason": "missing oauth_token", "url": config.get("url")}
-                )
+                lifecycle.record_failure("missing oauth_token", extra={"url": config.get("url")})
                 return pipeline.error("Missing oauth_token")
 
             authorize_url = client.get_authorize_url(request_token)
@@ -230,7 +228,7 @@ class OAuthCallbackView(PipelineView):
 
                 return pipeline.next_step()
             except ApiError as error:
-                lifecycle.record_failure({"failure_reason": str(error)})
+                lifecycle.record_failure(str(error))
                 return pipeline.error(
                     f"Could not fetch an access token from Bitbucket. {str(error)}"
                 )

src/sentry/integrations/github/integration.py

@@ -450,7 +450,7 @@ class OAuthLoginView(PipelineView):
 
             # At this point, we are past the GitHub "authorize" step
             if request.GET.get("state") != pipeline.signature:
-                lifecycle.record_failure({"failure_reason": GitHubInstallationError.INVALID_STATE})
+                lifecycle.record_failure(GitHubInstallationError.INVALID_STATE)
                 return error(
                     request,
                     self.active_organization,
@@ -474,7 +474,7 @@ class OAuthLoginView(PipelineView):
                 payload = {}
 
             if "access_token" not in payload:
-                lifecycle.record_failure({"failure_reason": GitHubInstallationError.MISSING_TOKEN})
+                lifecycle.record_failure(GitHubInstallationError.MISSING_TOKEN)
                 return error(
                     request,
                     self.active_organization,
@@ -483,7 +483,7 @@ class OAuthLoginView(PipelineView):
 
             authenticated_user_info = get_user_info(payload["access_token"])
             if "login" not in authenticated_user_info:
-                lifecycle.record_failure({"failure_reason": GitHubInstallationError.MISSING_LOGIN})
+                lifecycle.record_failure(GitHubInstallationError.MISSING_LOGIN)
                 return error(
                     request,
                     self.active_organization,
@@ -525,9 +525,7 @@ class GitHubInstallation(PipelineView):
                 ).exists()
 
             if integration_pending_deletion_exists:
-                lifecycle.record_failure(
-                    {"failure_reason": GitHubInstallationError.PENDING_DELETION}
-                )
+                lifecycle.record_failure(GitHubInstallationError.PENDING_DELETION)
                 return error(
                     request,
                     self.active_organization,
@@ -545,9 +543,7 @@ class GitHubInstallation(PipelineView):
                 return pipeline.next_step()
 
             if installations_exist:
-                lifecycle.record_failure(
-                    {"failure_reason": GitHubInstallationError.INSTALLATION_EXISTS}
-                )
+                lifecycle.record_failure(GitHubInstallationError.INSTALLATION_EXISTS)
                 return error(
                     request,
                     self.active_organization,
@@ -561,9 +557,7 @@ class GitHubInstallation(PipelineView):
                     external_id=installation_id, status=ObjectStatus.ACTIVE
                 )
             except Integration.DoesNotExist:
-                lifecycle.record_failure(
-                    {"failure_reason": GitHubInstallationError.MISSING_INTEGRATION}
-                )
+                lifecycle.record_failure(GitHubInstallationError.MISSING_INTEGRATION)
                 return error(request, self.active_organization)
 
             # Check that the authenticated GitHub user is the same as who installed the app.
@@ -571,7 +565,7 @@ class GitHubInstallation(PipelineView):
                 pipeline.fetch_state("github_authenticated_user")
                 != integration.metadata["sender"]["login"]
             ):
-                lifecycle.record_failure({"failure_reason": GitHubInstallationError.USER_MISMATCH})
+                lifecycle.record_failure(GitHubInstallationError.USER_MISMATCH)
                 return error(
                     request,
                     self.active_organization,

src/sentry/integrations/utils/metrics.py

@@ -114,7 +114,7 @@ class EventLifecycle:
         self._extra[name] = value
 
     def record_event(
-        self, outcome: EventLifecycleOutcome, exc: BaseException | None = None
+        self, outcome: EventLifecycleOutcome, outcome_reason: BaseException | str | None = None
     ) -> None:
         """Record a starting or halting event.
 
@@ -128,28 +128,35 @@ class EventLifecycle:
         sample_rate = 1.0
         metrics.incr(key, tags=tags, sample_rate=sample_rate)
 
+        extra = dict(self._extra)
+        extra.update(tags)
+        log_params: dict[str, Any] = {
+            "extra": extra,
+        }
+
+        if isinstance(outcome_reason, BaseException):
+            log_params["exc_info"] = outcome_reason
+        elif isinstance(outcome_reason, str):
+            extra["outcome_reason"] = outcome_reason
+
         if outcome == EventLifecycleOutcome.FAILURE:
-            extra = dict(self._extra)
-            extra.update(tags)
-            logger.error(key, extra=self._extra, exc_info=exc)
+            logger.error(key, **log_params)
         elif outcome == EventLifecycleOutcome.HALTED:
-            extra = dict(self._extra)
-            extra.update(tags)
-            logger.warning(key, extra=self._extra, exc_info=exc)
+            logger.warning(key, **log_params)
 
     @staticmethod
     def _report_flow_error(message) -> None:
         logger.error("EventLifecycle flow error: %s", message)
 
     def _terminate(
-        self, new_state: EventLifecycleOutcome, exc: BaseException | None = None
+        self, new_state: EventLifecycleOutcome, outcome_reason: BaseException | str | None = None
     ) -> 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, exc)
+        self.record_event(new_state, outcome_reason)
 
     def record_success(self) -> None:
         """Record that the event halted successfully.
@@ -162,7 +169,7 @@ class EventLifecycle:
         self._terminate(EventLifecycleOutcome.SUCCESS)
 
     def record_failure(
-        self, exc: BaseException | None = None, extra: dict[str, Any] | None = None
+        self, failure_reason: BaseException | str | None = None, extra: dict[str, Any] | None = None
     ) -> None:
         """Record that the event halted in failure. Additional data may be passed
         to be logged.
@@ -179,10 +186,10 @@ class EventLifecycle:
 
         if extra:
             self._extra.update(extra)
-        self._terminate(EventLifecycleOutcome.FAILURE, exc)
+        self._terminate(EventLifecycleOutcome.FAILURE, failure_reason)
 
     def record_halt(
-        self, exc: BaseException | None = None, extra: dict[str, Any] | None = None
+        self, halt_reason: BaseException | str | None = None, extra: dict[str, Any] | None = None
     ) -> None:
         """Record that the event halted in an ambiguous state.
 
@@ -200,7 +207,7 @@ class EventLifecycle:
 
         if extra:
             self._extra.update(extra)
-        self._terminate(EventLifecycleOutcome.HALTED, exc)
+        self._terminate(EventLifecycleOutcome.HALTED, halt_reason)
 
     def __enter__(self) -> Self:
         if self._state is not None:

tests/sentry/identity/test_oauth2.py

@@ -44,7 +44,7 @@ class OAuth2CallbackViewTest(TestCase):
         (event_failures,) = (
             call for call in mock_record.mock_calls if call.args[0] == EventLifecycleOutcome.FAILURE
         )
-        assert event_failures.args[1]["failure_reason"] == error_msg
+        assert event_failures.args[1] == error_msg
 
     @responses.activate
     def test_exchange_token_success(

tests/sentry/integrations/bitbucket_server/test_integration.py

@@ -21,7 +21,7 @@ class BitbucketServerIntegrationTest(IntegrationTestCase):
         (event_failures,) = (
             call for call in mock_record.mock_calls if call.args[0] == EventLifecycleOutcome.FAILURE
         )
-        assert event_failures.args[1]["failure_reason"] == error_msg
+        assert event_failures.args[1] == error_msg
 
     def test_config_view(self):
         resp = self.client.get(self.init_path)

tests/sentry/integrations/github/test_integration.py

@@ -116,7 +116,7 @@ class GitHubIntegrationTest(IntegrationTestCase):
         (event_failures,) = (
             call for call in mock_record.mock_calls if call.args[0] == EventLifecycleOutcome.FAILURE
         )
-        assert event_failures.args[1]["failure_reason"] == error_msg
+        assert event_failures.args[1] == error_msg
 
     @pytest.fixture(autouse=True)
     def stub_get_jwt(self):

tests/sentry/integrations/utils/test_lifecycle_metrics.py

@@ -1,5 +1,7 @@
 from unittest import mock
 
+import pytest
+
 from sentry.integrations.base import IntegrationDomain
 from sentry.integrations.types import EventLifecycleOutcome
 from sentry.integrations.utils.metrics import IntegrationEventLifecycleMetric
@@ -7,6 +9,10 @@ from sentry.testutils.cases import TestCase
 from sentry.testutils.silo import no_silo_test
 
 
+class ExampleException(Exception):
+    pass
+
+
 @no_silo_test
 class IntegrationEventLifecycleMetricTest(TestCase):
     class TestLifecycleMetric(IntegrationEventLifecycleMetric):
@@ -73,21 +79,108 @@ class IntegrationEventLifecycleMetricTest(TestCase):
 
     @mock.patch("sentry.integrations.utils.metrics.logger")
     @mock.patch("sentry.integrations.utils.metrics.metrics")
-    def test_recording_failure(self, mock_metrics, mock_logger):
-        class TestException(Exception):
-            pass
+    def test_recording_explicit_halt_with_exception(self, mock_metrics, mock_logger):
+        metric_obj = self.TestLifecycleMetric()
+        with metric_obj.capture() as lifecycle:
+            lifecycle.add_extra("extra", "value")
+            lifecycle.record_halt(ExampleException(""), extra={"even": "more"})
 
+        self._check_metrics_call_args(mock_metrics, "halted")
+        mock_logger.warning.assert_called_once_with(
+            "integrations.slo.halted",
+            extra={
+                "extra": "value",
+                "even": "more",
+                "integration_domain": "messaging",
+                "integration_name": "my_integration",
+                "interaction_type": "my_interaction",
+            },
+            exc_info=mock.ANY,
+        )
+
+    @mock.patch("sentry.integrations.utils.metrics.logger")
+    @mock.patch("sentry.integrations.utils.metrics.metrics")
+    def test_recording_explicit_halt_with_str(self, mock_metrics, mock_logger):
+        metric_obj = self.TestLifecycleMetric()
+        with metric_obj.capture() as lifecycle:
+            lifecycle.add_extra("extra", "value")
+            lifecycle.record_halt("Integration went boom", extra={"even": "more"})
+
+        self._check_metrics_call_args(mock_metrics, "halted")
+        mock_logger.warning.assert_called_once_with(
+            "integrations.slo.halted",
+            extra={
+                "outcome_reason": "Integration went boom",
+                "extra": "value",
+                "even": "more",
+                "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")
+    def test_recording_failure(self, mock_metrics, mock_logger):
         metric_obj = self.TestLifecycleMetric()
-        try:
+        with pytest.raises(ExampleException):
             with metric_obj.capture() as lifecycle:
                 lifecycle.add_extra("extra", "value")
-                raise TestException
-        except TestException:
-            pass
-        else:
-            self.fail()
+                raise ExampleException
+
+        self._check_metrics_call_args(mock_metrics, "failure")
+        mock_logger.error.assert_called_once_with(
+            "integrations.slo.failure",
+            extra={
+                "extra": "value",
+                "integration_domain": "messaging",
+                "integration_name": "my_integration",
+                "interaction_type": "my_interaction",
+            },
+            exc_info=mock.ANY,
+        )
+
+    @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):
+        metric_obj = self.TestLifecycleMetric()
+        with metric_obj.capture() as lifecycle:
+            try:
+                lifecycle.add_extra("extra", "value")
+                raise ExampleException
+            except ExampleException as exc:
+                lifecycle.record_failure(exc, extra={"even": "more"})
+
+        self._check_metrics_call_args(mock_metrics, "failure")
+        mock_logger.error.assert_called_once_with(
+            "integrations.slo.failure",
+            extra={
+                "extra": "value",
+                "even": "more",
+                "integration_domain": "messaging",
+                "integration_name": "my_integration",
+                "interaction_type": "my_interaction",
+            },
+            exc_info=mock.ANY,
+        )
+
+    @mock.patch("sentry.integrations.utils.metrics.logger")
+    @mock.patch("sentry.integrations.utils.metrics.metrics")
+    def test_recording_explicit_failure_with_str(self, mock_metrics, mock_logger):
+        metric_obj = self.TestLifecycleMetric()
+        with metric_obj.capture() as lifecycle:
+            lifecycle.add_extra("extra", "value")
+            lifecycle.record_failure("Integration went boom", extra={"even": "more"})
 
         self._check_metrics_call_args(mock_metrics, "failure")
         mock_logger.error.assert_called_once_with(
-            "integrations.slo.failure", extra={"extra": "value"}, exc_info=mock.ANY
+            "integrations.slo.failure",
+            extra={
+                "outcome_reason": "Integration went boom",
+                "extra": "value",
+                "even": "more",
+                "integration_domain": "messaging",
+                "integration_name": "my_integration",
+                "interaction_type": "my_interaction",
+            },
         )