Revert "ref(ecosystem): Allows strings for integration SLO failure and halt metrics (#80756)"

This reverts commit c06aeabe95e37e406f24c4075db3fcc6d2fb5cae.

Co-authored-by: GabeVillalobos <5643012+GabeVillalobos@users.noreply.github.com>

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("ssl_error")
+                lifecycle.record_failure({"failure_reason": "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("connection_error")
+                lifecycle.record_failure({"failure_reason": "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("json_error")
+                lifecycle.record_failure({"failure_reason": "json_error"})
                 return {
                     "error": "Could not decode a JSON Response",
                     "error_description": "We were not able to parse a JSON response, please try again.",
@@ -354,7 +354,9 @@ class OAuth2CallbackView(PipelineView):
 
             if error:
                 pipeline.logger.info("identity.token-exchange-error", extra={"error": error})
-                lifecycle.record_failure("token_exchange_error", extra={"msg": ERR_INVALID_STATE})
+                lifecycle.record_failure(
+                    {"failure_reason": "token_exchange_error", "msg": ERR_INVALID_STATE}
+                )
                 return pipeline.error(ERR_INVALID_STATE)
 
             if state != pipeline.fetch_state("state"):
@@ -367,7 +369,9 @@ class OAuth2CallbackView(PipelineView):
                         "code": code,
                     },
                 )
-                lifecycle.record_failure("token_exchange_error", extra={"msg": ERR_INVALID_STATE})
+                lifecycle.record_failure(
+                    {"failure_reason": "token_exchange_error", "msg": ERR_INVALID_STATE}
+                )
                 return pipeline.error(ERR_INVALID_STATE)
 
         # separate lifecycle event inside exchange_token

src/sentry/integrations/bitbucket/integration.py

@@ -269,7 +269,7 @@ class VerifyInstallation(PipelineView):
                     request, BitbucketIntegrationProvider.key
                 )
             except AtlassianConnectValidationError as e:
-                lifecycle.record_failure(str(e))
+                lifecycle.record_failure({"failure_reason": 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,12 +185,14 @@ class OAuthLoginView(PipelineView):
             try:
                 request_token = client.get_request_token()
             except ApiError as error:
-                lifecycle.record_failure(str(error), extra={"url": config.get("url")})
+                lifecycle.record_failure({"failure_reason": str(error), "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("missing oauth_token", extra={"url": config.get("url")})
+                lifecycle.record_failure(
+                    {"failure_reason": "missing oauth_token", "url": config.get("url")}
+                )
                 return pipeline.error("Missing oauth_token")
 
             authorize_url = client.get_authorize_url(request_token)
@@ -228,7 +230,7 @@ class OAuthCallbackView(PipelineView):
 
                 return pipeline.next_step()
             except ApiError as error:
-                lifecycle.record_failure(str(error))
+                lifecycle.record_failure({"failure_reason": 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(GitHubInstallationError.INVALID_STATE)
+                lifecycle.record_failure({"failure_reason": 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(GitHubInstallationError.MISSING_TOKEN)
+                lifecycle.record_failure({"failure_reason": 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(GitHubInstallationError.MISSING_LOGIN)
+                lifecycle.record_failure({"failure_reason": GitHubInstallationError.MISSING_LOGIN})
                 return error(
                     request,
                     self.active_organization,
@@ -525,7 +525,9 @@ class GitHubInstallation(PipelineView):
                 ).exists()
 
             if integration_pending_deletion_exists:
-                lifecycle.record_failure(GitHubInstallationError.PENDING_DELETION)
+                lifecycle.record_failure(
+                    {"failure_reason": GitHubInstallationError.PENDING_DELETION}
+                )
                 return error(
                     request,
                     self.active_organization,
@@ -543,7 +545,9 @@ class GitHubInstallation(PipelineView):
                 return pipeline.next_step()
 
             if installations_exist:
-                lifecycle.record_failure(GitHubInstallationError.INSTALLATION_EXISTS)
+                lifecycle.record_failure(
+                    {"failure_reason": GitHubInstallationError.INSTALLATION_EXISTS}
+                )
                 return error(
                     request,
                     self.active_organization,
@@ -557,7 +561,9 @@ class GitHubInstallation(PipelineView):
                     external_id=installation_id, status=ObjectStatus.ACTIVE
                 )
             except Integration.DoesNotExist:
-                lifecycle.record_failure(GitHubInstallationError.MISSING_INTEGRATION)
+                lifecycle.record_failure(
+                    {"failure_reason": GitHubInstallationError.MISSING_INTEGRATION}
+                )
                 return error(request, self.active_organization)
 
             # Check that the authenticated GitHub user is the same as who installed the app.
@@ -565,7 +571,7 @@ class GitHubInstallation(PipelineView):
                 pipeline.fetch_state("github_authenticated_user")
                 != integration.metadata["sender"]["login"]
             ):
-                lifecycle.record_failure(GitHubInstallationError.USER_MISMATCH)
+                lifecycle.record_failure({"failure_reason": 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, outcome_reason: BaseException | str | None = None
+        self, outcome: EventLifecycleOutcome, exc: BaseException | None = None
     ) -> None:
         """Record a starting or halting event.
 
@@ -128,35 +128,28 @@ 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:
-            logger.error(key, **log_params)
+            extra = dict(self._extra)
+            extra.update(tags)
+            logger.error(key, extra=self._extra, exc_info=exc)
         elif outcome == EventLifecycleOutcome.HALTED:
-            logger.warning(key, **log_params)
+            extra = dict(self._extra)
+            extra.update(tags)
+            logger.warning(key, extra=self._extra, exc_info=exc)
 
     @staticmethod
     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, exc: BaseException | 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, outcome_reason)
+        self.record_event(new_state, exc)
 
     def record_success(self) -> None:
         """Record that the event halted successfully.
@@ -169,7 +162,7 @@ class EventLifecycle:
         self._terminate(EventLifecycleOutcome.SUCCESS)
 
     def record_failure(
-        self, failure_reason: BaseException | str | None = None, extra: dict[str, Any] | None = None
+        self, exc: BaseException | None = None, extra: dict[str, Any] | None = None
     ) -> None:
         """Record that the event halted in failure. Additional data may be passed
         to be logged.
@@ -186,10 +179,10 @@ class EventLifecycle:
 
         if extra:
             self._extra.update(extra)
-        self._terminate(EventLifecycleOutcome.FAILURE, failure_reason)
+        self._terminate(EventLifecycleOutcome.FAILURE, exc)
 
     def record_halt(
-        self, halt_reason: BaseException | str | None = None, extra: dict[str, Any] | None = None
+        self, exc: BaseException | None = None, extra: dict[str, Any] | None = None
     ) -> None:
         """Record that the event halted in an ambiguous state.
 
@@ -207,7 +200,7 @@ class EventLifecycle:
 
         if extra:
             self._extra.update(extra)
-        self._terminate(EventLifecycleOutcome.HALTED, halt_reason)
+        self._terminate(EventLifecycleOutcome.HALTED, exc)
 
     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] == error_msg
+        assert event_failures.args[1]["failure_reason"] == 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] == error_msg
+        assert event_failures.args[1]["failure_reason"] == 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] == error_msg
+        assert event_failures.args[1]["failure_reason"] == error_msg
 
     @pytest.fixture(autouse=True)
     def stub_get_jwt(self):

tests/sentry/integrations/utils/test_lifecycle_metrics.py

@@ -1,7 +1,5 @@
 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
@@ -9,10 +7,6 @@ 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):
@@ -77,110 +71,23 @@ class IntegrationEventLifecycleMetricTest(TestCase):
         self._check_metrics_call_args(mock_metrics, "halted")
         mock_logger.error.assert_not_called()
 
-    @mock.patch("sentry.integrations.utils.metrics.logger")
-    @mock.patch("sentry.integrations.utils.metrics.metrics")
-    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()
-        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(
-            "integrations.slo.failure",
-            extra={
-                "extra": "value",
-                "integration_domain": "messaging",
-                "integration_name": "my_integration",
-                "interaction_type": "my_interaction",
-            },
-            exc_info=mock.ANY,
-        )
+        class TestException(Exception):
+            pass
 
-    @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:
+        try:
+            with metric_obj.capture() as lifecycle:
                 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"})
+                raise TestException
+        except TestException:
+            pass
+        else:
+            self.fail()
 
         self._check_metrics_call_args(mock_metrics, "failure")
         mock_logger.error.assert_called_once_with(
-            "integrations.slo.failure",
-            extra={
-                "outcome_reason": "Integration went boom",
-                "extra": "value",
-                "even": "more",
-                "integration_domain": "messaging",
-                "integration_name": "my_integration",
-                "interaction_type": "my_interaction",
-            },
+            "integrations.slo.failure", extra={"extra": "value"}, exc_info=mock.ANY
         )