diff --git a/src/sentry/integrations/github/integration.py b/src/sentry/integrations/github/integration.py index 69f0e52bca0edc..cb751ca0986b9d 100644 --- a/src/sentry/integrations/github/integration.py +++ b/src/sentry/integrations/github/integration.py @@ -26,7 +26,7 @@ IntegrationMetadata, IntegrationProvider, ) -from sentry.integrations.github.constants import RATE_LIMITED_MESSAGE +from sentry.integrations.github.constants import ISSUE_LOCKED_ERROR_MESSAGE, RATE_LIMITED_MESSAGE from sentry.integrations.github.tasks.link_all_repos import link_all_repos from sentry.integrations.models.integration import Integration from sentry.integrations.models.organization_integration import OrganizationIntegration @@ -290,6 +290,26 @@ def search_issues(self, query: str | None, **kwargs) -> dict[str, Any]: assert isinstance(resp, dict) return resp + # CommitContextIntegration methods + + def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: str) -> bool: + if api_error.json: + if ISSUE_LOCKED_ERROR_MESSAGE in api_error.json.get("message", ""): + metrics.incr( + metrics_base.format(integration=self.integration_name, key="error"), + tags={"type": "issue_locked_error"}, + ) + return True + + elif RATE_LIMITED_MESSAGE in api_error.json.get("message", ""): + metrics.incr( + metrics_base.format(integration=self.integration_name, key="error"), + tags={"type": "rate_limited_error"}, + ) + return True + + return False + class GitHubIntegrationProvider(IntegrationProvider): key = "github" diff --git a/src/sentry/integrations/github/tasks/open_pr_comment.py b/src/sentry/integrations/github/tasks/open_pr_comment.py index 2b7b38580b3cfa..b9be939ae27a1d 100644 --- a/src/sentry/integrations/github/tasks/open_pr_comment.py +++ b/src/sentry/integrations/github/tasks/open_pr_comment.py @@ -23,7 +23,7 @@ from sentry.constants import EXTENSION_LANGUAGE_MAP, ObjectStatus from sentry.integrations.github.client import GitHubApiClient -from sentry.integrations.github.constants import ISSUE_LOCKED_ERROR_MESSAGE, RATE_LIMITED_MESSAGE +from sentry.integrations.github.constants import RATE_LIMITED_MESSAGE from sentry.integrations.github.tasks.pr_comment import format_comment_url from sentry.integrations.github.tasks.utils import ( GithubAPIErrorType, @@ -624,20 +624,10 @@ def open_pr_comment_workflow(pr_id: int) -> None: language=language, ) except ApiError as e: - if e.json: - if ISSUE_LOCKED_ERROR_MESSAGE in e.json.get("message", ""): - metrics.incr( - OPEN_PR_METRICS_BASE.format(integration="github", key="error"), - tags={"type": "issue_locked_error"}, - ) - return - - elif RATE_LIMITED_MESSAGE in e.json.get("message", ""): - metrics.incr( - OPEN_PR_METRICS_BASE.format(integration="github", key="error"), - tags={"type": "rate_limited_error"}, - ) - return + if installation.on_create_or_update_comment_error( + api_error=e, metrics_base=OPEN_PR_METRICS_BASE + ): + return metrics.incr( OPEN_PR_METRICS_BASE.format(integration="github", key="error"), diff --git a/src/sentry/integrations/github/tasks/pr_comment.py b/src/sentry/integrations/github/tasks/pr_comment.py index 2c7f2910d26227..17fb29d032958b 100644 --- a/src/sentry/integrations/github/tasks/pr_comment.py +++ b/src/sentry/integrations/github/tasks/pr_comment.py @@ -11,7 +11,7 @@ from sentry import features from sentry.constants import ObjectStatus -from sentry.integrations.github.constants import ISSUE_LOCKED_ERROR_MESSAGE, RATE_LIMITED_MESSAGE +from sentry.integrations.github.constants import RATE_LIMITED_MESSAGE from sentry.integrations.github.tasks.utils import PullRequestIssue from sentry.integrations.services.integration import integration_service from sentry.integrations.source_code_management.commit_context import CommitContextIntegration @@ -249,20 +249,10 @@ def github_comment_workflow(pullrequest_id: int, project_id: int): except ApiError as e: cache.delete(cache_key) - if e.json: - if ISSUE_LOCKED_ERROR_MESSAGE in e.json.get("message", ""): - metrics.incr( - MERGED_PR_METRICS_BASE.format(integration="github", key="error"), - tags={"type": "issue_locked_error"}, - ) - return - - elif RATE_LIMITED_MESSAGE in e.json.get("message", ""): - metrics.incr( - MERGED_PR_METRICS_BASE.format(integration="github", key="error"), - tags={"type": "rate_limited_error"}, - ) - return + if installation.on_create_or_update_comment_error( + api_error=e, metrics_base=MERGED_PR_METRICS_BASE + ): + return metrics.incr( MERGED_PR_METRICS_BASE.format(integration="github", key="error"), diff --git a/src/sentry/integrations/github_enterprise/integration.py b/src/sentry/integrations/github_enterprise/integration.py index 68814788b9d556..81188132f51ab2 100644 --- a/src/sentry/integrations/github_enterprise/integration.py +++ b/src/sentry/integrations/github_enterprise/integration.py @@ -232,6 +232,11 @@ def has_repo_access(self, repo: RpcRepository) -> bool: # TODO: define this, used to migrate repositories return False + # CommitContextIntegration methods + + def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: str) -> bool: + raise NotImplementedError + class InstallationForm(forms.Form): url = forms.CharField( diff --git a/src/sentry/integrations/gitlab/integration.py b/src/sentry/integrations/gitlab/integration.py index 902b776704ed6d..b0ea386c927dc8 100644 --- a/src/sentry/integrations/gitlab/integration.py +++ b/src/sentry/integrations/gitlab/integration.py @@ -164,6 +164,11 @@ def extract_source_path_from_source_url(self, repo: Repository, url: str) -> str _, _, source_path = url.partition("/") return source_path + # CommitContextIntegration methods + + def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: str) -> bool: + raise NotImplementedError + # Gitlab only functions def get_group_id(self): diff --git a/src/sentry/integrations/source_code_management/commit_context.py b/src/sentry/integrations/source_code_management/commit_context.py index 1683c2e304b528..f1995e9ea72f67 100644 --- a/src/sentry/integrations/source_code_management/commit_context.py +++ b/src/sentry/integrations/source_code_management/commit_context.py @@ -34,6 +34,7 @@ ) from sentry.models.repository import Repository from sentry.shared_integrations.exceptions import ( + ApiError, ApiInvalidRequestError, ApiRateLimitedError, ApiRetryError, @@ -375,6 +376,15 @@ def create_or_update_comment( extra={"new_comment": pr_comment is None, "pr_key": pr_key, "repo": repo.name}, ) + @abstractmethod + def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: str) -> bool: + """ + Handle errors from the create_or_update_comment method. + + Returns True if the error was handled, False otherwise. + """ + raise NotImplementedError + class CommitContextClient(ABC): base_url: str diff --git a/tests/sentry/integrations/github/tasks/test_open_pr_comment.py b/tests/sentry/integrations/github/tasks/test_open_pr_comment.py index 1a053bf515ceaf..cf31e74e2e3afd 100644 --- a/tests/sentry/integrations/github/tasks/test_open_pr_comment.py +++ b/tests/sentry/integrations/github/tasks/test_open_pr_comment.py @@ -1035,9 +1035,11 @@ def test_comment_workflow_early_return( @patch("sentry.analytics.record") @patch("sentry.integrations.github.tasks.open_pr_comment.metrics") + @patch("sentry.integrations.github.integration.metrics") @responses.activate def test_comment_workflow_api_error( self, + mock_integration_metrics, mock_metrics, mock_analytics, _, @@ -1083,7 +1085,9 @@ def test_comment_workflow_api_error( with pytest.raises(ApiError): open_pr_comment_workflow(self.pr.id) - mock_metrics.incr.assert_called_with("github.open_pr_comment.api_error") + mock_metrics.incr.assert_called_with( + "github.open_pr_comment.error", tags={"type": "api_error"} + ) pr_2 = PullRequest.objects.create( organization_id=self.organization.id, @@ -1093,7 +1097,7 @@ def test_comment_workflow_api_error( # does not raise ApiError for locked issue open_pr_comment_workflow(pr_2.id) - mock_metrics.incr.assert_called_with( + mock_integration_metrics.incr.assert_called_with( "github.open_pr_comment.error", tags={"type": "issue_locked_error"} ) @@ -1106,7 +1110,7 @@ def test_comment_workflow_api_error( # does not raise ApiError for rate limited error open_pr_comment_workflow(pr_3.id) - mock_metrics.incr.assert_called_with( + mock_integration_metrics.incr.assert_called_with( "github.open_pr_comment.error", tags={"type": "rate_limited_error"} ) assert not mock_analytics.called diff --git a/tests/sentry/integrations/github/tasks/test_pr_comment.py b/tests/sentry/integrations/github/tasks/test_pr_comment.py index 54bcee6d5374d0..f2083d75180944 100644 --- a/tests/sentry/integrations/github/tasks/test_pr_comment.py +++ b/tests/sentry/integrations/github/tasks/test_pr_comment.py @@ -492,8 +492,9 @@ def test_comment_workflow_updates_comment(self, mock_metrics, mock_issues): @patch("sentry.integrations.github.tasks.pr_comment.get_top_5_issues_by_count") @patch("sentry.integrations.github.tasks.pr_comment.metrics") + @patch("sentry.integrations.github.integration.metrics") @responses.activate - def test_comment_workflow_api_error(self, mock_metrics, mock_issues): + def test_comment_workflow_api_error(self, mock_integration_metrics, mock_metrics, mock_issues): cache.set(self.cache_key, True, timedelta(minutes=5).total_seconds()) mock_issues.return_value = [ {"group_id": g.id, "event_count": 10} for g in Group.objects.all() @@ -526,8 +527,8 @@ def test_comment_workflow_api_error(self, mock_metrics, mock_issues): with pytest.raises(ApiError): github_comment_workflow(self.pr.id, self.project.id) - assert cache.get(self.cache_key) is None - mock_metrics.incr.assert_called_with("github.pr_comment.api_error") + assert cache.get(self.cache_key) is None + mock_metrics.incr.assert_called_with("github.pr_comment.error", tags={"type": "api_error"}) pr_2 = self.create_pr_issues() cache_key = DEBOUNCE_PR_COMMENT_CACHE_KEY(pr_2.id) @@ -536,7 +537,7 @@ def test_comment_workflow_api_error(self, mock_metrics, mock_issues): # does not raise ApiError for locked issue github_comment_workflow(pr_2.id, self.project.id) assert cache.get(cache_key) is None - mock_metrics.incr.assert_called_with( + mock_integration_metrics.incr.assert_called_with( "github.pr_comment.error", tags={"type": "issue_locked_error"} ) @@ -547,7 +548,7 @@ def test_comment_workflow_api_error(self, mock_metrics, mock_issues): # does not raise ApiError for rate limited error github_comment_workflow(pr_3.id, self.project.id) assert cache.get(cache_key) is None - mock_metrics.incr.assert_called_with( + mock_integration_metrics.incr.assert_called_with( "github.pr_comment.error", tags={"type": "rate_limited_error"} ) diff --git a/tests/sentry/integrations/source_code_management/test_commit_context_slo.py b/tests/sentry/integrations/source_code_management/test_commit_context.py similarity index 97% rename from tests/sentry/integrations/source_code_management/test_commit_context_slo.py rename to tests/sentry/integrations/source_code_management/test_commit_context.py index 255844727eba9b..aa7dea7bf6c79b 100644 --- a/tests/sentry/integrations/source_code_management/test_commit_context_slo.py +++ b/tests/sentry/integrations/source_code_management/test_commit_context.py @@ -9,6 +9,7 @@ ) from sentry.integrations.types import EventLifecycleOutcome from sentry.models.repository import Repository +from sentry.shared_integrations.exceptions import ApiError from sentry.testutils.asserts import assert_failure_metric, assert_halt_metric, assert_slo_metric from sentry.testutils.cases import TestCase from sentry.users.models.identity import Identity @@ -26,8 +27,11 @@ def __init__(self): def get_client(self): return self.client + def on_create_or_update_comment_error(self, api_error: ApiError, metrics_base: str) -> bool: + raise NotImplementedError -class CommitContextIntegrationTest(TestCase): + +class TestCommitContextIntegrationSLO(TestCase): def setUp(self): super().setUp() self.integration = MockCommitContextIntegration()