-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(code_review): Fix and more debugging #105996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import logging | ||
| from collections.abc import Mapping | ||
| from typing import TYPE_CHECKING, Any | ||
| from collections.abc import Callable, Mapping | ||
| from typing import Any | ||
|
|
||
| from sentry.integrations.github.webhook_types import GithubWebhookType | ||
| from sentry.integrations.services.integration import RpcIntegration | ||
|
|
@@ -11,20 +11,14 @@ | |
| from sentry.models.repository import Repository | ||
|
|
||
| from ..preflight import CodeReviewPreflightService | ||
|
|
||
| if TYPE_CHECKING: | ||
| from sentry.integrations.github.webhook import WebhookProcessor | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will explain below. |
||
|
|
||
| from .check_run import handle_check_run_event | ||
| from .issue_comment import handle_issue_comment_event | ||
| from .pull_request import handle_pull_request_event | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| METRICS_PREFIX = "seer.code_review.webhook" | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have anymore metrics in this module. |
||
|
|
||
|
|
||
| EVENT_TYPE_TO_HANDLER: dict[GithubWebhookType, WebhookProcessor] = { | ||
| EVENT_TYPE_TO_HANDLER: dict[GithubWebhookType, Callable[..., None]] = { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a more sensible signature. The only function that needs to follow |
||
| GithubWebhookType.CHECK_RUN: handle_check_run_event, | ||
| GithubWebhookType.ISSUE_COMMENT: handle_issue_comment_event, | ||
| GithubWebhookType.PULL_REQUEST: handle_pull_request_event, | ||
|
|
@@ -75,11 +69,23 @@ def handle_webhook_event( | |
| # TODO: add metric | ||
| return | ||
|
|
||
| repository = event.get("repository", {}) | ||
| if repository: | ||
| logger.info("repository: %s", repository) | ||
| github_org = event.get("repository", {}).get("owner", {}).get("login") | ||
| if github_org: | ||
| logger.info("github_org: %s", github_org) | ||
| else: | ||
| logger.info("github_org not found") | ||
| from .config import get_direct_to_seer_gh_orgs | ||
|
|
||
| gh_orgs_to_only_send_to_seer = get_direct_to_seer_gh_orgs() | ||
| logger.info("gh_orgs_to_only_send_to_seer: %s", gh_orgs_to_only_send_to_seer) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Debugging purposes. We will remove it. |
||
| handler( | ||
| github_event=github_event, | ||
| event=event, | ||
| organization=organization, | ||
| github_org=github_org, | ||
| repo=repo, | ||
| integration=integration, | ||
| **kwargs, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed anymore. |
||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,7 @@ def _add_eyes_reaction_to_comment( | |
| metrics.incr( | ||
| Metrics.ERROR.value, | ||
| tags={"error_status": ErrorStatus.MISSING_INTEGRATION.value}, | ||
| sample_rate=1.0, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to have certainty of events happening while debugging.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I missed this. I'll add it to my other metrics pr |
||
| ) | ||
| logger.warning( | ||
| Log.MISSING_INTEGRATION.value, | ||
|
|
@@ -72,11 +73,13 @@ def _add_eyes_reaction_to_comment( | |
| metrics.incr( | ||
| Metrics.OUTCOME.value, | ||
| tags={"status": "reaction_added"}, | ||
| sample_rate=1.0, | ||
| ) | ||
| except Exception: | ||
| metrics.incr( | ||
| Metrics.ERROR.value, | ||
| tags={"error_status": ErrorStatus.REACTION_FAILED.value}, | ||
| sample_rate=1.0, | ||
| ) | ||
| logger.exception( | ||
| Log.REACTION_FAILED.value, | ||
|
|
@@ -89,6 +92,7 @@ def handle_issue_comment_event( | |
| github_event: GithubWebhookType, | ||
| event: Mapping[str, Any], | ||
| organization: Organization, | ||
| github_org: str, | ||
| repo: Repository, | ||
| integration: RpcIntegration | None = None, | ||
| **kwargs: Any, | ||
|
|
@@ -103,7 +107,6 @@ def handle_issue_comment_event( | |
| if not is_pr_review_command(comment_body or ""): | ||
| return | ||
|
|
||
| github_org = event.get("repository", {}).get("owner", {}).get("login") | ||
| if github_org in get_direct_to_seer_gh_orgs(): | ||
| if comment_id: | ||
| _add_eyes_reaction_to_comment(integration, organization, repo, str(comment_id)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,14 +101,15 @@ def _warn_and_increment_metric( | |
| tags = {"error_status": error_status.value} | ||
| if action: | ||
| tags["action"] = action | ||
| metrics.incr(Metrics.ERROR.value, tags=tags) | ||
| metrics.incr(Metrics.ERROR.value, tags=tags, sample_rate=1.0) | ||
|
|
||
|
|
||
| def handle_pull_request_event( | ||
| *, | ||
| github_event: GithubWebhookType, | ||
| event: Mapping[str, Any], | ||
| organization: Organization, | ||
| github_org: str, | ||
| repo: Repository, | ||
| integration: RpcIntegration | None = None, | ||
| **kwargs: Any, | ||
|
|
@@ -139,10 +140,8 @@ def handle_pull_request_event( | |
| return | ||
|
|
||
| if pull_request.get("draft") is True: | ||
| _warn_and_increment_metric(ErrorStatus.DRAFT_PR, action=action_value, extra=extra) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This unnecessarily shows up in the logs. |
||
| return | ||
|
|
||
| github_org = event.get("repository", {}).get("owner", {}).get("login") | ||
| if github_org in get_direct_to_seer_gh_orgs(): | ||
| from .task import schedule_task | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,7 @@ def schedule_task( | |
| metrics.incr( | ||
| f"{METRICS_PREFIX}.{github_event.value}.skipped", | ||
| tags={"reason": "failed_to_transform", "github_event": github_event.value}, | ||
| sample_rate=1.0, | ||
| ) | ||
| return | ||
|
|
||
|
|
@@ -71,6 +72,7 @@ def schedule_task( | |
| metrics.incr( | ||
| f"{METRICS_PREFIX}.{github_event.value}.enqueued", | ||
| tags={"status": "success", "github_event": github_event.value}, | ||
| sample_rate=1.0, | ||
| ) | ||
|
|
||
|
|
||
|
|
@@ -100,13 +102,21 @@ def process_github_webhook_event( | |
| should_record_latency = True | ||
| option_key = get_webhook_option_key(github_event) | ||
|
|
||
| # Check if repo owner is in the whitelist (always send to Seer for these orgs) | ||
| # Otherwise, check option key to see if Overwatch should handle this | ||
| repo_owner = event_payload.get("data", {}).get("repo", {}).get("owner") | ||
| if repo_owner not in get_direct_to_seer_gh_orgs(): | ||
| # If option is True, Overwatch handles this - skip Seer processing | ||
| if option_key and options.get(option_key): | ||
| return | ||
| # Skip this check for CHECK_RUN events (always go to Seer) | ||
| if github_event != GithubWebhookType.CHECK_RUN: | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bypassing system is only for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, though I'm confused when the regression occurred because the last spot I updated it here would have retained the logic for everyone except our 2 test orgs if I did that correctly. I feel like this additional spot checking the feature flag may just be able to remove completely since we're doing the checks upstream already? Anyway we can just leave it in since it will be temporary |
||
| # Check if repo owner is in the whitelist (always send to Seer for these orgs) | ||
| # Otherwise, check option key to see if Overwatch should handle this | ||
| repo_owner = event_payload.get("data", {}).get("repo", {}).get("owner") | ||
| logger.info("payload: %s", event_payload) | ||
| if repo_owner: | ||
| logger.info("repo_owner: %s", repo_owner) | ||
| else: | ||
| logger.info("repo_owner not found") | ||
| logger.info("get_direct_to_seer_gh_orgs: %s", get_direct_to_seer_gh_orgs()) | ||
| if repo_owner not in get_direct_to_seer_gh_orgs(): | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that once we're migrated we will remove this bypassing system. |
||
| # If option is True, Overwatch handles this - skip Seer processing | ||
| if option_key and options.get(option_key): | ||
| return | ||
|
|
||
| try: | ||
| path = get_seer_endpoint_for_event(github_event).value | ||
|
|
@@ -121,7 +131,7 @@ def process_github_webhook_event( | |
| raise | ||
| finally: | ||
| if status != "success": | ||
| metrics.incr(f"{PREFIX}.error", tags={"error_status": status}) | ||
| metrics.incr(f"{PREFIX}.error", tags={"error_status": status}, sample_rate=1.0) | ||
| if should_record_latency: | ||
| record_latency(status, enqueued_at_str) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,3 +150,25 @@ def test_check_run_skips_when_hide_ai_features_enabled( | |
| CHECK_RUN_REREQUESTED_ACTION_EVENT_EXAMPLE, | ||
| ) | ||
| mock_make_seer_request.assert_not_called() | ||
|
|
||
| @patch("sentry.seer.code_review.webhooks.task.get_direct_to_seer_gh_orgs") | ||
| @patch("sentry.seer.code_review.webhooks.task.make_seer_request") | ||
| def test_check_run_bypasses_org_whitelist_check( | ||
| self, mock_make_seer_request: MagicMock, mock_get_orgs: MagicMock | ||
| ) -> None: | ||
| """Test that CHECK_RUN events go to Seer even when org is not whitelisted. | ||
|
|
||
| This verifies the bug fix: CHECK_RUN events should bypass the org whitelist | ||
| check because they are user-initiated reruns from GitHub UI. | ||
| """ | ||
| mock_get_orgs.return_value = [] | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if the re-run comes from a non-whitelisted org it will work. |
||
|
|
||
| with self.code_review_setup(), self.tasks(): | ||
| self._send_webhook_event( | ||
| GithubWebhookType.CHECK_RUN, | ||
| CHECK_RUN_REREQUESTED_ACTION_EVENT_EXAMPLE, | ||
| ) | ||
|
|
||
| mock_make_seer_request.assert_called_once() | ||
| call_kwargs = mock_make_seer_request.call_args[1] | ||
| assert call_kwargs["path"] == "/v1/automation/codegen/pr-review/rerun" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really use
organization.