♻️ chore(slo): SLOs for CommitContextIntegration (#81225)

take 2, added ecosystem slo event lifecycles to the mixin.

Author: iamrajjoshi

Diff for: src/sentry/integrations/source_code_management/commit_context.py

@@ -1,5 +1,5 @@
 from collections.abc import Mapping
-from enum import Enum, StrEnum
+from enum import StrEnum
 from typing import Any
 
 from attr import dataclass
@@ -8,11 +8,14 @@
 from sentry.integrations.models.organization_integration import OrganizationIntegration
 from sentry.integrations.services.integration import RpcOrganizationIntegration
 from sentry.integrations.utils.metrics import IntegrationEventLifecycleMetric
+from sentry.models.commit import Commit
 from sentry.models.organization import Organization
+from sentry.models.project import Project
+from sentry.models.repository import Repository
 from sentry.organizations.services.organization import RpcOrganization
 
 
-class SCMIntegrationInteractionType(Enum):
+class SCMIntegrationInteractionType(StrEnum):
     """
     SCM integration features
     """
@@ -31,15 +34,14 @@ class SCMIntegrationInteractionType(Enum):
     GET = "GET"
 
     # CommitContextIntegration
+    GET_BLAME_FOR_FILES = "GET_BLAME_FOR_FILES"
     CREATE_COMMENT = "CREATE_COMMENT"
     UPDATE_COMMENT = "UPDATE_COMMENT"
+    QUEUE_COMMENT_TASK = "QUEUE_COMMENT_TASK"
 
     # Tasks
     LINK_ALL_REPOS = "LINK_ALL_REPOS"
 
-    def __str__(self) -> str:
-        return self.value.lower()
-
 
 @dataclass
 class SCMIntegrationInteractionEvent(IntegrationEventLifecycleMetric):
@@ -70,6 +72,38 @@ def get_extras(self) -> Mapping[str, Any]:
         }
 
 
+@dataclass
+class CommitContextIntegrationInteractionEvent(SCMIntegrationInteractionEvent):
+    """
+    An instance to be recorded of a CommitContextIntegration feature call.
+    """
+
+    project: Project | None = None
+    commit: Commit | None = None
+    repository: Repository | None = None
+    pull_request_id: int | None = None
+
+    def get_extras(self) -> Mapping[str, Any]:
+        parent_extras = super().get_extras()
+        return {
+            **parent_extras,
+            "project_id": (self.project.id if self.project else None),
+            "commit_id": (self.commit.id if self.commit else None),
+            "repository_id": (self.repository.id if self.repository else None),
+            "pull_request_id": self.pull_request_id,
+        }
+
+
+class CommitContextHaltReason(StrEnum):
+    """Common reasons why a commit context integration may halt without success/failure."""
+
+    PR_BOT_DISABLED = "pr_bot_disabled"
+    INCORRECT_REPO_CONFIG = "incorrect_repo_config"
+    COMMIT_NOT_IN_DEFAULT_BRANCH = "commit_not_in_default_branch"
+    MISSING_PR = "missing_pr"
+    ALREADY_QUEUED = "already_queued"
+
+
 class LinkAllReposHaltReason(StrEnum):
     """
     Common reasons why a link all repos task may halt without success/failure.

Diff for: src/sentry/integrations/source_code_management/metrics.py

@@ -113,6 +113,10 @@ def add_extra(self, name: str, value: Any) -> None:
         """
         self._extra[name] = value
 
+    def add_extras(self, extras: Mapping[str, int | str]) -> None:
+        """Add multiple values to logged "extra" data."""
+        self._extra.update(extras)
+
     def record_event(
         self, outcome: EventLifecycleOutcome, outcome_reason: BaseException | str | None = None
     ) -> None:

Diff for: src/sentry/integrations/utils/metrics.py

@@ -0,0 +1,118 @@
+from unittest.mock import Mock, patch
+
+from sentry.integrations.source_code_management.commit_context import (
+    CommitContextIntegration,
+    SourceLineInfo,
+)
+from sentry.integrations.types import EventLifecycleOutcome
+from sentry.models.repository import Repository
+from sentry.testutils.cases import TestCase
+from sentry.users.models.identity import Identity
+from tests.sentry.integrations.utils.test_assert_metrics import (
+    assert_failure_metric,
+    assert_halt_metric,
+)
+
+
+class MockCommitContextIntegration(CommitContextIntegration):
+    """Mock implementation for testing"""
+
+    integration_name = "mock_integration"
+
+    def __init__(self):
+        self.client = Mock()
+
+    def get_client(self):
+        return self.client
+
+
+class CommitContextIntegrationTest(TestCase):
+    def setUp(self):
+        super().setUp()
+        self.integration = MockCommitContextIntegration()
+        self.repo = Repository.objects.create(
+            organization_id=self.organization.id,
+            name="example/repo",
+        )
+        self.source_line = SourceLineInfo(
+            lineno=10, path="src/file.py", ref="main", repo=self.repo, code_mapping=Mock()
+        )
+
+    @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
+    def test_get_blame_for_files_success(self, mock_record):
+        """Test successful blame retrieval records correct lifecycle events"""
+        self.integration.client.get_blame_for_files.return_value = []
+
+        result = self.integration.get_blame_for_files([self.source_line], {})
+
+        assert result == []
+        assert len(mock_record.mock_calls) == 2
+
+        start, success = mock_record.mock_calls
+        assert start.args[0] == EventLifecycleOutcome.STARTED
+        assert success.args[0] == EventLifecycleOutcome.SUCCESS
+
+    @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
+    def test_get_blame_for_files_missing_identity(self, mock_record):
+        """Test missing identity records failure"""
+        self.integration.get_client = Mock(side_effect=Identity.DoesNotExist())
+
+        result = self.integration.get_blame_for_files([self.source_line], {})
+
+        assert result == []
+        assert len(mock_record.mock_calls) == 2
+
+        start, failure = mock_record.mock_calls
+        assert start.args[0] == EventLifecycleOutcome.STARTED
+        assert failure.args[0] == EventLifecycleOutcome.FAILURE
+        assert_failure_metric(mock_record, Identity.DoesNotExist())
+
+    @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
+    def test_get_blame_for_files_invalid_identity(self, mock_record):
+        """Test invalid identity records failure"""
+        from sentry.auth.exceptions import IdentityNotValid
+
+        self.integration.client.get_blame_for_files = Mock(side_effect=IdentityNotValid())
+
+        result = self.integration.get_blame_for_files([self.source_line], {})
+
+        assert result == []
+        assert len(mock_record.mock_calls) == 2
+
+        start, failure = mock_record.mock_calls
+        assert start.args[0] == EventLifecycleOutcome.STARTED
+        assert failure.args[0] == EventLifecycleOutcome.FAILURE
+        assert_failure_metric(mock_record, IdentityNotValid())
+
+    @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
+    def test_get_blame_for_files_rate_limited(self, mock_record):
+        """Test rate limited requests record halt"""
+        from sentry.shared_integrations.exceptions import ApiRateLimitedError
+
+        self.integration.client.get_blame_for_files = Mock(
+            side_effect=ApiRateLimitedError(text="Rate limited")
+        )
+
+        result = self.integration.get_blame_for_files([self.source_line], {})
+
+        assert result == []
+        assert len(mock_record.mock_calls) == 2
+
+        start, halt = mock_record.mock_calls
+        assert start.args[0] == EventLifecycleOutcome.STARTED
+        assert halt.args[0] == EventLifecycleOutcome.HALTED
+        assert_halt_metric(mock_record, ApiRateLimitedError(text="Rate limited"))
+
+    @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
+    def test_get_commit_context_all_frames(self, mock_record):
+        """Test get_commit_context_all_frames records correct lifecycle events"""
+        self.integration.client.get_blame_for_files.return_value = []
+
+        result = self.integration.get_commit_context_all_frames([self.source_line], {})
+
+        assert result == []
+        assert len(mock_record.mock_calls) == 2
+
+        start, success = mock_record.mock_calls
+        assert start.args[0] == EventLifecycleOutcome.STARTED
+        assert success.args[0] == EventLifecycleOutcome.SUCCESS

Diff for: tests/sentry/integrations/source_code_management/__init__.py

@@ -6,14 +6,20 @@
 
 
 def assert_halt_metric(mock_record, error_msg):
-    (event_failures,) = (
+    (event_halts,) = (
         call for call in mock_record.mock_calls if call.args[0] == EventLifecycleOutcome.HALTED
     )
-    assert event_failures.args[1] == error_msg
+    if isinstance(error_msg, Exception):
+        assert isinstance(event_halts.args[1], type(error_msg))
+    else:
+        assert event_halts.args[1] == error_msg
 
 
 def assert_failure_metric(mock_record, error_msg):
     (event_failures,) = (
         call for call in mock_record.mock_calls if call.args[0] == EventLifecycleOutcome.FAILURE
     )
-    assert event_failures.args[1] == error_msg
+    if isinstance(error_msg, Exception):
+        assert isinstance(event_failures.args[1], type(error_msg))
+    else:
+        assert event_failures.args[1] == error_msg

Diff for: tests/sentry/integrations/source_code_management/test_commit_context_slo.py

@@ -17,6 +17,8 @@
     FileBlameInfo,
     SourceLineInfo,
 )
+from sentry.integrations.source_code_management.metrics import CommitContextHaltReason
+from sentry.integrations.types import EventLifecycleOutcome
 from sentry.models.commit import Commit
 from sentry.models.commitauthor import CommitAuthor
 from sentry.models.groupowner import GroupOwner, GroupOwnerType
@@ -36,6 +38,7 @@
 from sentry.testutils.silo import assume_test_silo_mode
 from sentry.testutils.skips import requires_snuba
 from sentry.utils.committers import get_frame_paths
+from tests.sentry.integrations.utils.test_assert_metrics import assert_halt_metric
 
 pytestmark = [requires_snuba]
 
@@ -1183,9 +1186,12 @@ def test_gh_comment_no_repo(self, mock_comment_workflow, mock_get_commit_context
             assert not mock_comment_workflow.called
             assert len(PullRequestCommit.objects.all()) == 0
 
+    @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     @patch("sentry.integrations.github.client.get_jwt", return_value="jwt_token_1")
     @responses.activate
-    def test_gh_comment_debounces(self, get_jwt, mock_comment_workflow, mock_get_commit_context):
+    def test_gh_comment_debounces(
+        self, get_jwt, mock_record, mock_comment_workflow, mock_get_commit_context
+    ):
         mock_get_commit_context.return_value = [self.blame]
         self.add_responses()
         assert not GroupOwner.objects.filter(group=self.event.group).exists()
@@ -1223,10 +1229,18 @@ def test_gh_comment_debounces(self, get_jwt, mock_comment_workflow, mock_get_com
             )
             assert mock_comment_workflow.call_count == 1
 
+        start_1, success_1, start_2, halt_2 = mock_record.mock_calls
+        assert start_1.args[0] == EventLifecycleOutcome.STARTED
+        assert success_1.args[0] == EventLifecycleOutcome.SUCCESS
+        assert start_2.args[0] == EventLifecycleOutcome.STARTED
+        assert halt_2.args[0] == EventLifecycleOutcome.HALTED
+        assert_halt_metric(mock_record, CommitContextHaltReason.ALREADY_QUEUED)
+
+    @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
     @patch("sentry.integrations.github.client.get_jwt", return_value="jwt_token_1")
     @responses.activate
     def test_gh_comment_multiple_comments(
-        self, get_jwt, mock_comment_workflow, mock_get_commit_context
+        self, get_jwt, mock_record, mock_comment_workflow, mock_get_commit_context
     ):
         self.add_responses()
 
@@ -1274,3 +1288,10 @@ def test_gh_comment_multiple_comments(
                 group_id=self.event.group_id,
             )
             assert mock_comment_workflow.call_count == 1
+
+        start_1, success_1, start_2, halt_2 = mock_record.mock_calls
+        assert start_1.args[0] == EventLifecycleOutcome.STARTED
+        assert success_1.args[0] == EventLifecycleOutcome.SUCCESS
+        assert start_2.args[0] == EventLifecycleOutcome.STARTED
+        assert halt_2.args[0] == EventLifecycleOutcome.HALTED
+        assert_halt_metric(mock_record, CommitContextHaltReason.ALREADY_QUEUED)