ref: abstract on_create_or_update_comment_error method (#89683)

Author: jianyuan

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"

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"),

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"),

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(

src/sentry/integrations/gitlab/integration.py

@@ -160,6 +160,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):

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

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

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"}
         )
 

tests/sentry/integrations/source_code_management/test_commit_context_slo.py renamed to 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()