diff --git a/src/sentry/integrations/api/endpoints/organization_integration_repos.py b/src/sentry/integrations/api/endpoints/organization_integration_repos.py index 58e32275ea57..f633c4655e9d 100644 --- a/src/sentry/integrations/api/endpoints/organization_integration_repos.py +++ b/src/sentry/integrations/api/endpoints/organization_integration_repos.py @@ -16,6 +16,7 @@ from sentry.integrations.api.bases.organization_integrations import ( CellOrganizationIntegrationBaseEndpoint, ) +from sentry.integrations.source_code_management.metrics import SCMIntegrationInteractionType from sentry.integrations.source_code_management.repository import RepositoryIntegration from sentry.models.organization import Organization from sentry.models.repository import Repository @@ -110,20 +111,24 @@ def get( search = request.GET.get("search") accessible_only = request.GET.get("accessibleOnly", "false").lower() == "true" - try: + with install.record_event( + SCMIntegrationInteractionType.GET_REPOSITORY_CHOICES + ).capture() as lifecycle: # This endpoint backs interactive repo pickers (the onboarding # repo selector and the code-mappings path config form), so opt # into parallel pagination for the latency win. Providers that # don't implement it ignore the flag; internal/background callers # of get_repositories keep the serial default. - repositories = install.get_repositories( - search, - accessible_only=accessible_only, - use_cache=accessible_only and bool(search), - parallel=True, - ) - except (IntegrationError, IdentityNotValid) as e: - return self.respond({"detail": str(e)}, status=400) + try: + repositories = install.get_repositories( + search, + accessible_only=accessible_only, + use_cache=accessible_only and bool(search), + parallel=True, + ) + except (IntegrationError, IdentityNotValid) as e: + lifecycle.record_halt(e, create_issue=False) + return self.respond({"detail": str(e)}, status=400) installable_only = request.GET.get("installableOnly", "false").lower() == "true" diff --git a/src/sentry/integrations/github/client.py b/src/sentry/integrations/github/client.py index ffa31a5342eb..1f96b0721327 100644 --- a/src/sentry/integrations/github/client.py +++ b/src/sentry/integrations/github/client.py @@ -39,6 +39,8 @@ from sentry.integrations.source_code_management.metrics import ( SCMIntegrationInteractionEvent, SCMIntegrationInteractionType, + get_current_scm_interaction_type, + normalize_scm_provider_key, ) from sentry.integrations.source_code_management.repo_trees import RepoTreesClient from sentry.integrations.source_code_management.repository import RepositoryClient @@ -107,6 +109,7 @@ # first are fetched concurrently. Bounded to keep the fan-out comfortably under # Github's secondary (concurrent request) rate limits. PAGINATION_MAX_WORKERS = 10 +GET_REPOSITORIES_METRIC = "integrations.source_code_management.get_repositories" JWT_AUTH_ROUTES = ("/app/installations", "access_tokens") @@ -761,29 +764,37 @@ def get_repos( (see ``_get_with_pagination``). Only interactive callers opt in; the background ``link_all_repos`` task keeps the serial default. """ - with SCMIntegrationInteractionEvent( - interaction_type=SCMIntegrationInteractionType.GET_REPOSITORIES, - provider_key=self.integration_name, - integration_id=self.integration.id, - organization_id=self.organization_id, - ).capture() as lifecycle: - try: - return self._get_with_pagination( - "/installation/repositories", - response_key="repositories", - page_number_limit=page_number_limit, - api_request_type=GitHubApiRequestType.GET_REPOSITORIES, - raise_on_page_limit=raise_on_page_limit, - parallel=parallel, - ) - except ApiPaginationTruncated as e: - # Hitting the pagination cap is an expected signal. - # Record as halt so the lifecycle doesn't create a - # Sentry every time this is called. - lifecycle.record_halt(e, create_issue=False) - raise + parent_interaction_type = get_current_scm_interaction_type() + metric_tags = { + "integration_name": normalize_scm_provider_key(self.integration_name), + "parent_interaction_type": ( + str(parent_interaction_type) if parent_interaction_type else "unattributed" + ), + } + try: + repositories = self._get_with_pagination( + "/installation/repositories", + response_key="repositories", + page_number_limit=page_number_limit, + api_request_type=GitHubApiRequestType.GET_REPOSITORIES, + raise_on_page_limit=raise_on_page_limit, + parallel=parallel, + ) + except ApiPaginationTruncated: + metrics.incr(GET_REPOSITORIES_METRIC, tags={**metric_tags, "result": "halted"}) + raise + except Exception: + metrics.incr(GET_REPOSITORIES_METRIC, tags={**metric_tags, "result": "error"}) + raise - def get_repos_cached(self, ttl: int = 300, parallel: bool = False) -> list[CachedRepo]: + metrics.incr(GET_REPOSITORIES_METRIC, tags={**metric_tags, "result": "success"}) + return repositories + + def get_repos_cached( + self, + ttl: int = 300, + parallel: bool = False, + ) -> list[CachedRepo]: """ Return all repos accessible to this installation, cached in Django cache for ``ttl`` seconds. diff --git a/src/sentry/integrations/source_code_management/metrics.py b/src/sentry/integrations/source_code_management/metrics.py index 6ccb852b367c..e2f734881aed 100644 --- a/src/sentry/integrations/source_code_management/metrics.py +++ b/src/sentry/integrations/source_code_management/metrics.py @@ -1,11 +1,16 @@ from collections.abc import Mapping +from contextvars import ContextVar, Token from enum import StrEnum -from typing import Any +from types import TracebackType +from typing import Any, Self from attr import dataclass from sentry.integrations.base import IntegrationDomain -from sentry.integrations.utils.metrics import IntegrationEventLifecycleMetric +from sentry.integrations.utils.metrics import ( + IntegrationEventLifecycle, + IntegrationEventLifecycleMetric, +) from sentry.models.commit import Commit from sentry.models.project import Project from sentry.models.repository import Repository @@ -66,10 +71,56 @@ class SCMIntegrationInteractionType(StrEnum): GET_RATE_LIMIT = "get_rate_limit" # Repo Trees - GET_REPOSITORIES = "get_repositories" GET_REPO_TREE = "get_repo_tree" +_current_scm_interaction_type: ContextVar[SCMIntegrationInteractionType | None] = ContextVar( + "current_scm_interaction_type", default=None +) + + +def get_current_scm_interaction_type() -> SCMIntegrationInteractionType | None: + return _current_scm_interaction_type.get() + + +class SCMIntegrationEventLifecycle(IntegrationEventLifecycle): + def __init__( + self, + payload: IntegrationEventLifecycleMetric, + interaction_type: SCMIntegrationInteractionType, + assume_success: bool, + sample_log_rate: float, + ) -> None: + super().__init__(payload, assume_success, sample_log_rate) + self.interaction_type = interaction_type + self._interaction_token: Token[SCMIntegrationInteractionType | None] | None = None + + def __enter__(self) -> Self: + if self._interaction_token is not None: + raise RuntimeError("SCM integration lifecycle cannot be re-entered") + + self._interaction_token = _current_scm_interaction_type.set(self.interaction_type) + try: + return super().__enter__() + except BaseException: + _current_scm_interaction_type.reset(self._interaction_token) + self._interaction_token = None + raise + + def __exit__( + self, + exc_type: type[BaseException] | None, + exc_value: BaseException | None, + traceback: TracebackType, + ) -> None: + try: + super().__exit__(exc_type, exc_value, traceback) + finally: + if self._interaction_token is not None: + _current_scm_interaction_type.reset(self._interaction_token) + self._interaction_token = None + + class SCMResolutionHaltReason(StrEnum): GROUP_LINK_ALREADY_EXISTS = "group_link_already_exists" ISSUE_ALREADY_RESOLVED = "issue_already_resolved" @@ -98,6 +149,16 @@ def get_interaction_type(self) -> str: def get_integration_id(self) -> int | None: return self.integration_id + def capture( + self, assume_success: bool = True, sample_log_rate: float = 1.0 + ) -> SCMIntegrationEventLifecycle: + return SCMIntegrationEventLifecycle( + self, + self.interaction_type, + assume_success, + sample_log_rate, + ) + def get_extras(self) -> Mapping[str, Any]: return { "organization_id": (self.organization_id if self.organization_id else None), diff --git a/src/sentry/integrations/source_code_management/repo_trees.py b/src/sentry/integrations/source_code_management/repo_trees.py index a2498bc0712f..480df1b88b2a 100644 --- a/src/sentry/integrations/source_code_management/repo_trees.py +++ b/src/sentry/integrations/source_code_management/repo_trees.py @@ -138,7 +138,10 @@ def get_client(self) -> RepoTreesClient: raise NotImplementedError @abstractmethod - def get_repositories(self, query: str | None = None) -> list[RepositoryInfo]: + def get_repositories( + self, + query: str | None = None, + ) -> list[RepositoryInfo]: raise NotImplementedError @property diff --git a/src/sentry/integrations/source_code_management/repository.py b/src/sentry/integrations/source_code_management/repository.py index a1ad02993db0..a8b2cc3967f7 100644 --- a/src/sentry/integrations/source_code_management/repository.py +++ b/src/sentry/integrations/source_code_management/repository.py @@ -119,6 +119,7 @@ def get_repositories( ``parallel`` is a hint that pages after the first may be fetched concurrently. Providers that do not implement parallel pagination may accept and ignore this argument. + """ raise NotImplementedError diff --git a/tests/sentry/integrations/api/endpoints/test_organization_integration_repos.py b/tests/sentry/integrations/api/endpoints/test_organization_integration_repos.py index 2454afd63510..8483defcdbac 100644 --- a/tests/sentry/integrations/api/endpoints/test_organization_integration_repos.py +++ b/tests/sentry/integrations/api/endpoints/test_organization_integration_repos.py @@ -1,5 +1,7 @@ from unittest.mock import MagicMock, patch +from sentry.integrations.types import EventLifecycleOutcome +from sentry.shared_integrations.exceptions import IntegrationError from sentry.testutils.cases import APITestCase @@ -17,6 +19,23 @@ def setUp(self) -> None: f"/api/0/organizations/{self.org.slug}/integrations/{self.integration.id}/repos/" ) + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @patch( + "sentry.integrations.github.integration.GitHubIntegration.get_repositories", + side_effect=IntegrationError("Unable to retrieve repositories"), + ) + def test_expected_error_halts_lifecycle( + self, _get_repositories: MagicMock, record_event: MagicMock + ) -> None: + response = self.client.get(self.path, format="json") + + assert response.status_code == 400 + assert response.data == {"detail": "Unable to retrieve repositories"} + assert [call.args[0] for call in record_event.call_args_list] == [ + EventLifecycleOutcome.STARTED, + EventLifecycleOutcome.HALTED, + ] + @patch( "sentry.integrations.github.integration.GitHubIntegration.get_repositories", return_value=[] ) diff --git a/tests/sentry/integrations/github/tasks/test_link_all_repos.py b/tests/sentry/integrations/github/tasks/test_link_all_repos.py index 36ce4e50d2bd..5c2a271d5f9c 100644 --- a/tests/sentry/integrations/github/tasks/test_link_all_repos.py +++ b/tests/sentry/integrations/github/tasks/test_link_all_repos.py @@ -81,12 +81,10 @@ def test_link_all_repos(self, mock_record: MagicMock, _: MagicMock) -> None: assert repos[0].name == "getsentry/sentry" assert repos[1].name == "getsentry/snuba" - assert len(mock_record.mock_calls) == 4 - start1, start2, end2, end1 = mock_record.mock_calls - assert start1.args[0] == EventLifecycleOutcome.STARTED - assert start2.args[0] == EventLifecycleOutcome.STARTED - assert end2.args[0] == EventLifecycleOutcome.SUCCESS - assert end1.args[0] == EventLifecycleOutcome.SUCCESS + assert len(mock_record.mock_calls) == 2 + start, end = mock_record.mock_calls + assert start.args[0] == EventLifecycleOutcome.STARTED + assert end.args[0] == EventLifecycleOutcome.SUCCESS @responses.activate @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @@ -126,12 +124,10 @@ def test_link_all_repos_api_response_keyerror( assert repos[0].name == "getsentry/snuba" - assert len(mock_record.mock_calls) == 4 - start1, start2, end2, end1 = mock_record.mock_calls - assert start1.args[0] == EventLifecycleOutcome.STARTED - assert start2.args[0] == EventLifecycleOutcome.STARTED - assert end2.args[0] == EventLifecycleOutcome.SUCCESS - assert end1.args[0] == EventLifecycleOutcome.HALTED + assert len(mock_record.mock_calls) == 2 + start, end = mock_record.mock_calls + assert start.args[0] == EventLifecycleOutcome.STARTED + assert end.args[0] == EventLifecycleOutcome.HALTED assert_halt_metric( mock_record, LinkAllReposHaltReason.REPOSITORY_NOT_CREATED.value ) # should be halt because it didn't complete successfully @@ -165,12 +161,10 @@ def test_link_all_repos_api_response_keyerror_single_repo( repos = Repository.objects.all() assert len(repos) == 0 - assert len(mock_record.mock_calls) == 4 - start1, start2, end2, end1 = mock_record.mock_calls - assert start1.args[0] == EventLifecycleOutcome.STARTED - assert start2.args[0] == EventLifecycleOutcome.STARTED - assert end2.args[0] == EventLifecycleOutcome.SUCCESS - assert end1.args[0] == EventLifecycleOutcome.HALTED + assert len(mock_record.mock_calls) == 2 + start, end = mock_record.mock_calls + assert start.args[0] == EventLifecycleOutcome.STARTED + assert end.args[0] == EventLifecycleOutcome.HALTED assert_halt_metric(mock_record, LinkAllReposHaltReason.REPOSITORY_NOT_CREATED.value) @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @@ -213,12 +207,10 @@ def test_link_all_repos_api_error(self, mock_record: MagicMock, _: MagicMock) -> organization_id=self.organization.id, ) - assert len(mock_record.mock_calls) == 4 - start1, start2, end2, end1 = mock_record.mock_calls - assert start1.args[0] == EventLifecycleOutcome.STARTED - assert start2.args[0] == EventLifecycleOutcome.STARTED - assert end2.args[0] == EventLifecycleOutcome.FAILURE - assert end1.args[0] == EventLifecycleOutcome.FAILURE + assert len(mock_record.mock_calls) == 2 + start, end = mock_record.mock_calls + assert start.args[0] == EventLifecycleOutcome.STARTED + assert end.args[0] == EventLifecycleOutcome.FAILURE @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @responses.activate @@ -241,12 +233,10 @@ def test_link_all_repos_api_error_rate_limited( organization_id=self.organization.id, ) - assert len(mock_record.mock_calls) == 4 - start1, start2, end2, end1 = mock_record.mock_calls - assert start1.args[0] == EventLifecycleOutcome.STARTED - assert start2.args[0] == EventLifecycleOutcome.STARTED - assert end2.args[0] == EventLifecycleOutcome.FAILURE - assert end1.args[0] == EventLifecycleOutcome.HALTED + assert len(mock_record.mock_calls) == 2 + start, end = mock_record.mock_calls + assert start.args[0] == EventLifecycleOutcome.STARTED + assert end.args[0] == EventLifecycleOutcome.HALTED assert_halt_metric(mock_record, LinkAllReposHaltReason.RATE_LIMITED.value) @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @@ -265,10 +255,8 @@ def test_link_all_repos_repo_creation_error( organization_id=self.organization.id, ) - assert len(mock_record.mock_calls) == 4 - start1, start2, end2, end1 = mock_record.mock_calls - assert start1.args[0] == EventLifecycleOutcome.STARTED - assert start2.args[0] == EventLifecycleOutcome.STARTED - assert end2.args[0] == EventLifecycleOutcome.SUCCESS - assert end1.args[0] == EventLifecycleOutcome.HALTED + assert len(mock_record.mock_calls) == 2 + start, end = mock_record.mock_calls + assert start.args[0] == EventLifecycleOutcome.STARTED + assert end.args[0] == EventLifecycleOutcome.HALTED assert_halt_metric(mock_record, LinkAllReposHaltReason.REPOSITORY_NOT_CREATED.value) diff --git a/tests/sentry/integrations/github/test_integration.py b/tests/sentry/integrations/github/test_integration.py index 9640a324a743..90151ff7a54f 100644 --- a/tests/sentry/integrations/github/test_integration.py +++ b/tests/sentry/integrations/github/test_integration.py @@ -36,10 +36,11 @@ FileBlameInfo, SourceLineInfo, ) +from sentry.integrations.source_code_management.metrics import SCMIntegrationInteractionType from sentry.integrations.source_code_management.repo_trees import RepoAndBranch, RepoTree from sentry.integrations.types import ExternalProviders from sentry.models.repository import Repository -from sentry.shared_integrations.exceptions import ApiError +from sentry.shared_integrations.exceptions import ApiError, ApiPaginationTruncated from sentry.silo.base import SiloMode from sentry.testutils.cases import APITestCase, IntegrationTestCase from sentry.testutils.helpers import with_feature @@ -551,8 +552,14 @@ def test_get_repositories_all_and_pagination(self) -> None: GitHubIntegration, integration, self.organization.id ) - with patch.object(client.GitHubBaseClient, "page_size", 1): - result = installation.get_repositories(parallel=True) + with ( + patch.object(client.GitHubBaseClient, "page_size", 1), + patch("sentry.integrations.github.client.metrics.incr") as mock_incr, + ): + with installation.record_event( + SCMIntegrationInteractionType.GET_REPOSITORY_CHOICES + ).capture(): + result = installation.get_repositories(parallel=True) assert result == [ { "name": "foo", @@ -573,6 +580,81 @@ def test_get_repositories_all_and_pagination(self) -> None: "default_branch": "master", }, ] + assert ( + mock.call( + client.GET_REPOSITORIES_METRIC, + tags={ + "integration_name": "github", + "parent_interaction_type": "get_repository_choices", + "result": "success", + }, + ) + in mock_incr.mock_calls + ) + + def test_get_repositories_records_error_metric(self) -> None: + self.assert_setup_flow() + integration = Integration.objects.get(provider=self.provider.key) + installation = get_installation_of_type( + GitHubIntegration, integration, self.organization.id + ) + + with ( + patch.object( + client.GitHubBaseClient, + "_get_with_pagination", + side_effect=ApiError("GitHub API error"), + ), + patch("sentry.integrations.github.client.metrics.incr") as mock_incr, + pytest.raises(ApiError), + ): + with installation.record_event( + SCMIntegrationInteractionType.DERIVE_CODEMAPPINGS + ).capture(): + installation.get_repositories() + + assert ( + mock.call( + client.GET_REPOSITORIES_METRIC, + tags={ + "integration_name": "github", + "parent_interaction_type": "derive_codemappings", + "result": "error", + }, + ) + in mock_incr.mock_calls + ) + + def test_get_repositories_records_halted_metric_when_truncated(self) -> None: + self.assert_setup_flow() + integration = Integration.objects.get(provider=self.provider.key) + installation = get_installation_of_type( + GitHubIntegration, integration, self.organization.id + ) + + with ( + patch.object( + client.GitHubBaseClient, + "_get_with_pagination", + side_effect=ApiPaginationTruncated(partial_data=[]), + ), + patch("sentry.integrations.github.client.metrics.incr") as mock_incr, + pytest.raises(ApiPaginationTruncated), + ): + with installation.record_event(SCMIntegrationInteractionType.SYNC_REPOS).capture(): + installation.get_repositories(raise_on_page_limit=True) + + assert ( + mock.call( + client.GET_REPOSITORIES_METRIC, + tags={ + "integration_name": "github", + "parent_interaction_type": "sync_repos", + "result": "halted", + }, + ) + in mock_incr.mock_calls + ) @responses.activate def test_get_repositories_only_first_page(self) -> None: diff --git a/tests/sentry/integrations/source_code_management/test_commit_context.py b/tests/sentry/integrations/source_code_management/test_commit_context.py index 8e3cfbfbe2eb..ee9e215587a4 100644 --- a/tests/sentry/integrations/source_code_management/test_commit_context.py +++ b/tests/sentry/integrations/source_code_management/test_commit_context.py @@ -14,6 +14,7 @@ from sentry.integrations.source_code_management.metrics import ( SCMIntegrationInteractionEvent, SCMIntegrationInteractionType, + get_current_scm_interaction_type, ) from sentry.integrations.types import EventLifecycleOutcome from sentry.models.repository import Repository @@ -47,6 +48,59 @@ def test_scm_interaction_event_normalizes_provider( } +@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") +def test_scm_interaction_context_tracks_nested_lifecycles(_mock_record: MagicMock) -> None: + outer = SCMIntegrationInteractionEvent( + interaction_type=SCMIntegrationInteractionType.DERIVE_CODEMAPPINGS, + provider_key="github", + ) + inner = SCMIntegrationInteractionEvent( + interaction_type=SCMIntegrationInteractionType.GET_REPO_TREE, + provider_key="github", + ) + + assert get_current_scm_interaction_type() is None + with outer.capture(): + assert ( + get_current_scm_interaction_type() == SCMIntegrationInteractionType.DERIVE_CODEMAPPINGS + ) + with inner.capture(): + assert get_current_scm_interaction_type() == SCMIntegrationInteractionType.GET_REPO_TREE + assert ( + get_current_scm_interaction_type() == SCMIntegrationInteractionType.DERIVE_CODEMAPPINGS + ) + assert get_current_scm_interaction_type() is None + + +@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") +def test_scm_interaction_context_resets_after_exception(_mock_record: MagicMock) -> None: + event = SCMIntegrationInteractionEvent( + interaction_type=SCMIntegrationInteractionType.SYNC_REPOS, + provider_key="github", + ) + + with pytest.raises(RuntimeError), event.capture(): + raise RuntimeError + + assert get_current_scm_interaction_type() is None + + +@patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") +def test_scm_interaction_context_rejects_reentry(_mock_record: MagicMock) -> None: + event = SCMIntegrationInteractionEvent( + interaction_type=SCMIntegrationInteractionType.SYNC_REPOS, + provider_key="github", + ) + lifecycle = event.capture() + + with lifecycle: + with pytest.raises(RuntimeError, match="cannot be re-entered"), lifecycle: + pass + assert get_current_scm_interaction_type() == SCMIntegrationInteractionType.SYNC_REPOS + + assert get_current_scm_interaction_type() is None + + class MockCommitContextIntegration(CommitContextIntegration): """Mock implementation for testing"""