ref(integrations): Separate SCM file check metrics

src/sentry/integrations/cursor_origin/client.py

@@ -501,7 +501,7 @@ def get_contents(
 
     def check_file(self, repo: Repository, path: str, version: str | None) -> object | None:
         # Origin has no HEAD route for contents, so this is a GET whose body we discard.
-        # Errors are left for RepositoryIntegration.check_file, which classifies them.
+        # Errors are left for RepositoryIntegration._check_file, which classifies them.
         return self.get_contents(repo.name, path, ref=version)
 
     def get_file(

src/sentry/integrations/perforce/integration.py

@@ -240,7 +240,7 @@ def source_url_matches(self, url: str) -> bool:
 
         return False
 
-    def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None:
+    def _check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None:
         """
         Check if a file exists in the Perforce depot and return the URL.
 

src/sentry/integrations/source_code_management/metrics.py

@@ -29,7 +29,6 @@ class SCMIntegrationInteractionType(StrEnum):
     # RepositoryIntegration
     GET_STACKTRACE_LINK = "get_stacktrace_link"
     GET_CODEOWNER_FILE = "get_codeowner_file"
-    CHECK_FILE = "check_file"
 
     # SourceCodeIssueIntegration (SCM only)
     GET_REPOSITORY_CHOICES = "get_repository_choices"

src/sentry/integrations/source_code_management/repository.py

@@ -15,6 +15,8 @@
 from sentry.integrations.source_code_management.metrics import (
     SCMIntegrationInteractionEvent,
     SCMIntegrationInteractionType,
+    get_current_scm_interaction_type,
+    normalize_scm_provider_key,
 )
 from sentry.integrations.types import IntegrationProviderSlug
 from sentry.models.repository import Repository
@@ -32,6 +34,9 @@
     UnsupportedResponseType,
 )
 from sentry.users.models.identity import Identity
+from sentry.utils import metrics
+
+CHECK_FILE_METRIC = "integrations.source_code_management.check_file"
 
 HaltReason = Literal[
     "configuration_error",
@@ -252,7 +257,12 @@ def record_event(self, event: SCMIntegrationInteractionType) -> SCMIntegrationIn
             integration_id=self.org_integration.integration_id,
         )
 
-    def check_file(self, repo: Repository, filepath: str, branch: str | None = None) -> str | None:
+    def _check_file(
+        self,
+        repo: Repository,
+        filepath: str,
+        branch: str | None = None,
+    ) -> str | None:
         """
         Calls the client's `check_file` method to see if the file exists.
         Returns the link to the file if it exists, otherwise return `None`.
@@ -265,59 +275,75 @@ def check_file(self, repo: Repository, filepath: str, branch: str | None = None)
         filepath: file from the stacktrace (string)
         branch: commitsha or default_branch (string)
         """
-        with self.record_event(SCMIntegrationInteractionType.CHECK_FILE).capture() as lifecycle:
-            filepath = filepath.lstrip("/")
-            try:
-                client = self.get_client()
-            except IntegrationConfigurationError:
-                # This is likely due to access being revoked by the user, or
-                # some other misconfiguration on the integration's side.
+        filepath = filepath.lstrip("/")
+        try:
+            client = self.get_client()
+        except IntegrationConfigurationError:
+            # This is likely due to access being revoked by the user, or
+            # some other misconfiguration on the integration's side.
+            return None
+        except (Identity.DoesNotExist, IntegrationError):
+            sentry_sdk.capture_exception()
+            return None
+        try:
+            response = client.check_file(repo, filepath, branch)
+            if not response:
                 return None
-            except (Identity.DoesNotExist, IntegrationError):
-                sentry_sdk.capture_exception()
+        except IdentityNotValid:
+            return None
+        except ApiRetryError:
+            # Ignore retry errors for self-hosted GitLab.
+            if (
+                self.integration_name == IntegrationProviderSlug.GITLAB.value
+                and client.base_url != GITLAB_CLOUD_BASE_URL
+            ):
                 return None
-            try:
-                response = client.check_file(repo, filepath, branch)
-                if not response:
-                    return None
-            except IdentityNotValid:
+            raise
+        except ApiUnauthorized:
+            return None
+        except (ApiForbiddenError, IntegrationConfigurationError):
+            # Re-raise since 403 errors need to be propagated to the user.
+            raise
+        except ApiError as e:
+            if e.code in (404, 400):
                 return None
-            except ApiRetryError as e:
-                # Ignore retry errors for GitLab
-                # TODO(ecosystem): Remove this once we have a better way to handle this
-                if (
-                    self.integration_name == IntegrationProviderSlug.GITLAB.value
-                    and client.base_url != GITLAB_CLOUD_BASE_URL
-                ):
-                    lifecycle.record_halt(e)
-                    return None
-                else:
-                    raise
-            except ApiUnauthorized as e:
-                lifecycle.record_halt(e)
+            # TODO(ecosystem): Remove this once we have a better way to handle this
+            # It involves decomposing this logic.
+            if (
+                e.json
+                and e.json.get("typeKey") == "AadUserStateException"
+                and self.integration_name == IntegrationProviderSlug.AZURE_DEVOPS.value
+            ):
                 return None
-            except (ApiForbiddenError, IntegrationConfigurationError) as e:
-                lifecycle.record_halt(e)
-                # Need to re-raise since 403 errors will be returned to user via get_link
-                raise
-            except ApiError as e:
-                if e.code in (404, 400):
-                    lifecycle.record_halt(e)
-                    return None
-                # TODO(ecosystem): Remove this once we have a better way to handle this
-                # It involves decomposing this logic
-                #  {"$id":"1","innerException":null,"message":"According to Microsoft Entra, your Identity xxx is currently Disabled within the following Microsoft Entra tenant: xxx. Please contact your Microsoft Entra administrator to resolve this.","typeName":"Microsoft.TeamFoundation.Framework.Server.AadUserStateException, Microsoft.TeamFoundation.Framework.Server","typeKey":"AadUserStateException","errorCode":0,"eventId":3000}"
-                elif (
-                    e.json
-                    and e.json.get("typeKey") == "AadUserStateException"
-                    and self.integration_name == IntegrationProviderSlug.AZURE_DEVOPS.value
-                ):
-                    lifecycle.record_halt(e)
-                    return None
-                else:
-                    raise
-
-            return self.format_source_url(repo, filepath, branch)
+            raise
+
+        return self.format_source_url(repo, filepath, branch)
+
+    def check_file(
+        self,
+        repo: Repository,
+        filepath: str,
+        branch: str | None = None,
+    ) -> str | None:
+        parent_interaction_type = get_current_scm_interaction_type() or "unattributed"
+        metric_tags = {
+            "integration_name": normalize_scm_provider_key(self.integration_name),
+            "parent_interaction_type": str(parent_interaction_type),
+        }
+        try:
+            source_url = self._check_file(repo, filepath, branch)
+        except Exception:
+            metrics.incr(CHECK_FILE_METRIC, tags={**metric_tags, "result": "error"})
+            raise
+
+        metrics.incr(
+            CHECK_FILE_METRIC,
+            tags={
+                **metric_tags,
+                "result": "file_found" if source_url is not None else "unavailable",
+            },
+        )
+        return source_url
 
     def encode_source_url(self, url: str) -> str:
         """Percent-encode a source URL's path, such as square brackets in a filepath."""
@@ -357,17 +383,25 @@ def get_stacktrace_link(
                 if version:
                     scope.set_tag("stacktrace_link.tried_version", True)
                     scope.set_attribute("stacktrace_link.tried_version", True)
-                    source_url = self.check_file(repo, filepath, version)
+                    source_url = self.check_file(
+                        repo,
+                        filepath,
+                        version,
+                    )
                     if source_url:
                         scope.set_tag("stacktrace_link.used_version", True)
                         scope.set_attribute("stacktrace_link.used_version", True)
                         return self.encode_source_url(source_url)
 
                 scope.set_tag("stacktrace_link.used_version", False)
                 scope.set_attribute("stacktrace_link.used_version", False)
-                source_url = self.check_file(repo, filepath, default)
+                source_url = self.check_file(
+                    repo,
+                    filepath,
+                    default,
+                )
             except (ApiForbiddenError, IntegrationConfigurationError) as e:
-                # Similar to the `check_file` implementation, we need to re-raise
+                # Similar to the `_check_file` implementation, we need to re-raise
                 # for 403 errors as these need to be propagated to the user.
                 lifecycle.record_halt(e)
                 raise
@@ -402,7 +436,11 @@ def get_codeowner_file(
                 raise NotImplementedError("Implement self.codeowners_locations to use this method.")
 
             for filepath in self.codeowners_locations:
-                html_url = self.check_file(repo, filepath, ref)
+                html_url = self.check_file(
+                    repo,
+                    filepath,
+                    ref,
+                )
                 if html_url:
                     try:
                         contents = self.get_client().get_file(repo, filepath, ref, codeowners=True)

tests/sentry/integrations/github/test_client.py

@@ -273,8 +273,9 @@ def test_check_no_file(self, get_jwt) -> None:
 
     @mock.patch("sentry.integrations.github.client.get_jwt", return_value="jwt_token_1")
     @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event")
+    @mock.patch("sentry.integrations.source_code_management.repository.metrics.incr")
     @responses.activate
-    def test_get_stacktrace_link(self, mock_record, get_jwt) -> None:
+    def test_get_stacktrace_link(self, mock_incr, mock_record, get_jwt) -> None:
         path = "/src/sentry/integrations/github/client.py"
         version = "master"
         url = "https://api.github.com/repos/{}/contents/{}?ref={}".format(
@@ -292,14 +293,17 @@ def test_get_stacktrace_link(self, mock_record, get_jwt) -> None:
             source_url
             == "https://github.com/Test-Organization/foo/blob/master/src/sentry/integrations/github/client.py"
         )
-        assert (
-            len(mock_record.mock_calls) == 4
-        )  # get_stacktrace_link calls check_file, which also has metrics
-        start1, start2, halt1, halt2 = mock_record.mock_calls
-        assert start1.args[0] == EventLifecycleOutcome.STARTED
-        assert start2.args[0] == EventLifecycleOutcome.STARTED  # check_file
-        assert halt1.args[0] == EventLifecycleOutcome.SUCCESS  # check_file
-        assert halt2.args[0] == EventLifecycleOutcome.SUCCESS
+        assert len(mock_record.mock_calls) == 2
+        assert mock_record.mock_calls[0].args[0] == EventLifecycleOutcome.STARTED
+        assert mock_record.mock_calls[1].args[0] == EventLifecycleOutcome.SUCCESS
+        mock_incr.assert_any_call(
+            "integrations.source_code_management.check_file",
+            tags={
+                "integration_name": "github",
+                "parent_interaction_type": "get_stacktrace_link",
+                "result": "file_found",
+            },
+        )
 
     @mock.patch("sentry.integrations.github.client.get_jwt", return_value="jwt_token_1")
     @responses.activate
@@ -702,9 +706,7 @@ def test_get_codeowner_file(self, mock_record, mock_jwt, mock_check_file) -> Non
             responses.calls[0].request.headers["Content-Type"] == "application/raw; charset=utf-8"
         )
         assert result == GITHUB_CODEOWNERS
-        assert (
-            len(mock_record.mock_calls) == 2
-        )  # check_file is mocked in this test, so there will be no metrics logged for it
+        assert len(mock_record.mock_calls) == 2
         assert mock_record.mock_calls[0].args[0] == EventLifecycleOutcome.STARTED
         assert mock_record.mock_calls[1].args[0] == EventLifecycleOutcome.SUCCESS
 

tests/sentry/integrations/gitlab/test_client.py

@@ -461,19 +461,24 @@ def test_get_stacktrace_link(self, mock_record: mock.MagicMock) -> None:
             json={"text": 200},
         )
 
-        source_url = self.installation.get_stacktrace_link(self.repo, path, "master", ref)
+        with mock.patch.object(
+            self.installation,
+            "check_file",
+            wraps=self.installation.check_file,
+        ) as mock_check_file:
+            source_url = self.installation.get_stacktrace_link(self.repo, path, "master", ref)
         assert (
             source_url
             == "https://example.gitlab.com/example-repo/blob/537f2e94fbc489b2564ca3d6a5f0bd9afa38c3c3/src/file.py"
         )
-        assert (
-            len(mock_record.mock_calls) == 4
-        )  # get_stacktrace_link calls check_file, which also has metrics
-        start1, start2, halt1, halt2 = mock_record.mock_calls
-        assert start1.args[0] == EventLifecycleOutcome.STARTED
-        assert start2.args[0] == EventLifecycleOutcome.STARTED  # check_file
-        assert halt1.args[0] == EventLifecycleOutcome.SUCCESS  # check_file
-        assert halt2.args[0] == EventLifecycleOutcome.SUCCESS
+        mock_check_file.assert_called_once_with(
+            self.repo,
+            path,
+            ref,
+        )
+        assert len(mock_record.mock_calls) == 2
+        assert mock_record.mock_calls[0].args[0] == EventLifecycleOutcome.STARTED
+        assert mock_record.mock_calls[1].args[0] == EventLifecycleOutcome.SUCCESS
 
     @responses.activate
     @mock.patch(
@@ -494,14 +499,9 @@ def test_get_stacktrace_link_retry_error(
 
         source_url = self.installation.get_stacktrace_link(self.repo, path, "master", None)
         assert source_url is None
-        assert (
-            len(mock_record.mock_calls) == 4
-        )  # get_stacktrace_link calls check_file, which also has metrics
-        start1, start2, halt1, halt2 = mock_record.mock_calls
-        assert start1.args[0] == EventLifecycleOutcome.STARTED
-        assert start2.args[0] == EventLifecycleOutcome.STARTED  # check_file
-        assert halt1.args[0] == EventLifecycleOutcome.HALTED  # check_file
-        assert halt2.args[0] == EventLifecycleOutcome.SUCCESS
+        assert len(mock_record.mock_calls) == 2
+        assert mock_record.mock_calls[0].args[0] == EventLifecycleOutcome.STARTED
+        assert mock_record.mock_calls[1].args[0] == EventLifecycleOutcome.SUCCESS
 
     @responses.activate
     @mock.patch("requests.sessions.Session.send")
@@ -521,14 +521,9 @@ def test_get_stacktrace_link_restricted_ip_address(
         with pytest.raises(ApiHostError):
             self.installation.get_stacktrace_link(self.repo, path, "master", None)
 
-        assert (
-            len(mock_record.mock_calls) == 4
-        )  # get_stacktrace_link calls check_file, which also has metrics
-        start1, start2, halt1, halt2 = mock_record.mock_calls
-        assert start1.args[0] == EventLifecycleOutcome.STARTED
-        assert start2.args[0] == EventLifecycleOutcome.STARTED  # check_file
-        assert halt1.args[0] == EventLifecycleOutcome.HALTED  # check_file
-        assert halt2.args[0] == EventLifecycleOutcome.HALTED
+        assert len(mock_record.mock_calls) == 2
+        assert mock_record.mock_calls[0].args[0] == EventLifecycleOutcome.STARTED
+        assert mock_record.mock_calls[1].args[0] == EventLifecycleOutcome.HALTED
 
     @mock.patch(
         "sentry.integrations.gitlab.integration.GitlabIntegration.check_file",
@@ -553,6 +548,11 @@ def test_get_codeowner_file(
         )
 
         assert result == GITLAB_CODEOWNERS
+        mock_check_file.assert_called_once_with(
+            self.config.project_repository.repository,
+            "CODEOWNERS",
+            self.config.default_branch,
+        )
 
         assert (
             len(mock_record.mock_calls) == 2