From a151488d129461d45fc0d932bebe08091037d9c4 Mon Sep 17 00:00:00 2001 From: Giovanni M Guidini Date: Thu, 29 Oct 2026 13:31:20 +0100 Subject: [PATCH] ref(integrations): Separate SCM file check metrics Keep stack-trace and CODEOWNERS feature outcomes on integration SLOs while recording internal file checks with one bounded operational counter. Refs SCM-304 --- .../integrations/cursor_origin/client.py | 2 +- .../integrations/perforce/integration.py | 2 +- .../source_code_management/metrics.py | 1 - .../source_code_management/repository.py | 146 +++++++++++------- .../sentry/integrations/github/test_client.py | 26 ++-- .../sentry/integrations/gitlab/test_client.py | 50 +++--- .../integrations/perforce/test_integration.py | 34 +++- tests/sentry/integrations/vsts/test_client.py | 23 +-- 8 files changed, 174 insertions(+), 110 deletions(-) diff --git a/src/sentry/integrations/cursor_origin/client.py b/src/sentry/integrations/cursor_origin/client.py index 1aa8343fe05e..67473f326e27 100644 --- a/src/sentry/integrations/cursor_origin/client.py +++ b/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( diff --git a/src/sentry/integrations/perforce/integration.py b/src/sentry/integrations/perforce/integration.py index b0d3fae7c32c..6e82222af7fb 100644 --- a/src/sentry/integrations/perforce/integration.py +++ b/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. diff --git a/src/sentry/integrations/source_code_management/metrics.py b/src/sentry/integrations/source_code_management/metrics.py index e2f734881aed..8352407e2a15 100644 --- a/src/sentry/integrations/source_code_management/metrics.py +++ b/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" diff --git a/src/sentry/integrations/source_code_management/repository.py b/src/sentry/integrations/source_code_management/repository.py index a8b2cc3967f7..c65a93a70a72 100644 --- a/src/sentry/integrations/source_code_management/repository.py +++ b/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,7 +383,11 @@ 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) @@ -365,9 +395,13 @@ def get_stacktrace_link( 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) diff --git a/tests/sentry/integrations/github/test_client.py b/tests/sentry/integrations/github/test_client.py index ee171a518986..51508e0159b9 100644 --- a/tests/sentry/integrations/github/test_client.py +++ b/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 diff --git a/tests/sentry/integrations/gitlab/test_client.py b/tests/sentry/integrations/gitlab/test_client.py index 632a424f4a2b..a0081ac67a78 100644 --- a/tests/sentry/integrations/gitlab/test_client.py +++ b/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 diff --git a/tests/sentry/integrations/perforce/test_integration.py b/tests/sentry/integrations/perforce/test_integration.py index 5ac00fac0d7f..1b5e5bcda910 100644 --- a/tests/sentry/integrations/perforce/test_integration.py +++ b/tests/sentry/integrations/perforce/test_integration.py @@ -345,7 +345,7 @@ def test_get_repo_external_id_matches_manual_add(self) -> None: def test_check_file_absolute_depot_path(self, mock_check_file): """Test check_file with absolute depot path (//depot/...)""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - result = self.installation.check_file(self.repo, "//depot/app/services/processor.cpp") + result = self.installation._check_file(self.repo, "//depot/app/services/processor.cpp") assert result is not None assert "//depot/app/services/processor.cpp" in result @@ -353,7 +353,7 @@ def test_check_file_absolute_depot_path(self, mock_check_file): def test_check_file_path_with_depot_name(self, mock_check_file): """Test check_file with path starting with depot name (depot/...)""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - result = self.installation.check_file(self.repo, "depot/app/services/processor.cpp") + result = self.installation._check_file(self.repo, "depot/app/services/processor.cpp") assert result is not None # Should not duplicate depot name in result assert "//depot/app/services/processor.cpp" in result @@ -363,7 +363,7 @@ def test_check_file_path_with_depot_name(self, mock_check_file): def test_check_file_relative_path(self, mock_check_file): """Test check_file with normal relative path (app/...)""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - result = self.installation.check_file(self.repo, "app/services/processor.cpp") + result = self.installation._check_file(self.repo, "app/services/processor.cpp") assert result is not None assert "//depot/app/services/processor.cpp" in result @@ -371,7 +371,7 @@ def test_check_file_relative_path(self, mock_check_file): def test_check_file_with_revision_syntax_absolute(self, mock_check_file): """Test check_file with #revision syntax on absolute path""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - result = self.installation.check_file(self.repo, "//depot/app/services/processor.cpp#1") + result = self.installation._check_file(self.repo, "//depot/app/services/processor.cpp#1") assert result is not None assert "#1" in result @@ -379,7 +379,7 @@ def test_check_file_with_revision_syntax_absolute(self, mock_check_file): def test_check_file_with_revision_syntax_depot_name(self, mock_check_file): """Test check_file with #revision syntax on path with depot name""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - result = self.installation.check_file(self.repo, "depot/app/services/processor.cpp#1") + result = self.installation._check_file(self.repo, "depot/app/services/processor.cpp#1") assert result is not None assert "#1" in result assert "depot/depot" not in result @@ -388,7 +388,7 @@ def test_check_file_with_revision_syntax_depot_name(self, mock_check_file): def test_check_file_with_revision_syntax_relative(self, mock_check_file): """Test check_file with #revision syntax on relative path""" mock_check_file.return_value = {"depotFile": "//depot/app/services/processor.cpp"} - result = self.installation.check_file(self.repo, "app/services/processor.cpp#1") + result = self.installation._check_file(self.repo, "app/services/processor.cpp#1") assert result is not None assert "#1" in result @@ -405,6 +405,28 @@ def test_get_stacktrace_link(self, mock_check_file): assert url == "p4://depot/app/services/processor.cpp#1" assert "#1" in url + @patch( + "sentry.integrations.perforce.client.PerforceClient.check_file", + side_effect=RuntimeError("unexpected error"), + ) + @patch("sentry.integrations.source_code_management.repository.metrics.incr") + def test_get_stacktrace_link_records_file_check_as_unavailable( + self, mock_incr, _mock_check_file + ): + result = self.installation.get_stacktrace_link( + self.repo, "app/services/processor.cpp", "", None + ) + + assert result is None + mock_incr.assert_any_call( + "integrations.source_code_management.check_file", + tags={ + "integration_name": "perforce", + "parent_interaction_type": "get_stacktrace_link", + "result": "unavailable", + }, + ) + @patch("sentry.integrations.perforce.client.PerforceClient.check_file") def test_get_stacktrace_link_changelist_not_url_encoded(self, mock_check_file): """ diff --git a/tests/sentry/integrations/vsts/test_client.py b/tests/sentry/integrations/vsts/test_client.py index 25f783747433..5df46f1bf19d 100644 --- a/tests/sentry/integrations/vsts/test_client.py +++ b/tests/sentry/integrations/vsts/test_client.py @@ -20,7 +20,6 @@ from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized from sentry.silo.base import SiloMode from sentry.silo.util import PROXY_BASE_PATH, PROXY_OI_HEADER, PROXY_PATH, PROXY_SIGNATURE_HEADER -from sentry.testutils.asserts import assert_halt_metric from sentry.testutils.helpers.integrations import get_installation_of_type from sentry.testutils.silo import assume_test_silo_mode, control_silo_test from sentry.users.models.identity import Identity, IdentityProvider @@ -232,9 +231,9 @@ def test_check_file(self) -> None: "sentry.integrations.vsts.client.VstsApiClient.check_file", side_effect=ApiUnauthorized(text="Unauthorized"), ) - @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @mock.patch("sentry.integrations.source_code_management.repository.metrics.incr") def test_check_file_unauthorized( - self, mock_record_event: MagicMock, mock_check_file: MagicMock + self, mock_incr: MagicMock, mock_check_file: MagicMock ) -> None: self.assert_installation() integration, installation = self._get_integration_and_install() @@ -257,7 +256,14 @@ def test_check_file_unauthorized( installation.check_file(repo, path, version) - assert_halt_metric(mock_record_event, ApiUnauthorized("Unauthorized")) + mock_incr.assert_any_call( + "integrations.source_code_management.check_file", + tags={ + "integration_name": "vsts", + "parent_interaction_type": "unattributed", + "result": "unavailable", + }, + ) @responses.activate def test_check_no_file(self) -> None: @@ -390,12 +396,9 @@ def test_get_stacktrace_link_identity_deleted( source_url = installation.get_stacktrace_link(repo, path, "master", version) assert source_url is None - halt = mock_record.mock_calls[-2] - assert halt.args[0] == EventLifecycleOutcome.HALTED - assert ( - halt.args[1].text - == '{"$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}' - ) + start, success = mock_record.mock_calls[-2:] + assert start.args[0] == EventLifecycleOutcome.STARTED + assert success.args[0] == EventLifecycleOutcome.SUCCESS def assert_proxy_request(request, is_proxy=True):