Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions src/sentry/integrations/github/webhook.py
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,10 @@ class PullRequestEventWebhook(GitHubWebhook):
"""https://developer.github.com/v3/activity/events/types/#pullrequestevent"""

EVENT_TYPE = IntegrationWebhookEventType.MERGE_REQUEST
WEBHOOK_EVENT_PROCESSORS = (_handle_pr_webhook_for_autofix_processor,)
WEBHOOK_EVENT_PROCESSORS = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my own mental model, these 2 processors run serially right?

for processor in self.WEBHOOK_EVENT_PROCESSORS:

With a failure in 1 does not block the next, and each processor runs synchronous for most of its logic except for the call to seer in schedule_task which is async?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is correct.

Once the code review processors executes, the work is in the task broker queue and will be handled async.

_handle_pr_webhook_for_autofix_processor,
code_review_handle_webhook_event,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this to the PullRequestEventWebhook class which handles pull_request events.

GithubWebhookType.PULL_REQUEST: PullRequestEventWebhook,

PULL_REQUEST = "pull_request"

)

def _handle(
self,
Expand Down Expand Up @@ -896,8 +899,6 @@ def _handle(
"github.webhook.pull_request.created",
sample_rate=1.0,
tags={
"organization_id": organization.id,
"repository_id": repo.id,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you run the tests with verbosity, you can see that these calls to metrics.incr are failing because these two keys are forbidden due to high cardinality.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajay-sentry @srest2021 heads up ^ in case you were still using that metric to track billing related stuff for GA

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this!!

"is_private": pr_repo_private,
},
)
Expand Down Expand Up @@ -929,10 +930,6 @@ def _handle(
metrics.incr(
"github.webhook.organization_contributor.should_create",
sample_rate=1.0,
tags={
"organization_id": organization.id,
"repository_id": repo.id,
},
)

locked_contributor = None
Expand Down
12 changes: 9 additions & 3 deletions src/sentry/seer/code_review/utils.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from collections.abc import Mapping
from enum import StrEnum
from typing import Any, Literal
from typing import Any

import orjson
from django.conf import settings
Expand All @@ -16,6 +16,11 @@
from sentry.seer.signed_seer_api import make_signed_seer_api_request


class RequestType(StrEnum):
PR_REVIEW = "pr-review"
PR_CLOSED = "pr-closed"


class ClientError(Exception):
"Non-retryable client error from Seer"

Expand Down Expand Up @@ -210,7 +215,7 @@ def transform_webhook_to_codegen_request(
"""
# Determine request_type based on event_type
# For now, we only support pr-review for these webhook types
request_type: Literal["pr-review", "pr-closed"] = "pr-review"
request_type: RequestType = RequestType.PR_REVIEW

# Extract pull request number
# Different event types have PR info in different locations
Expand Down Expand Up @@ -244,9 +249,10 @@ def transform_webhook_to_codegen_request(

trigger_metadata = _get_trigger_metadata(github_event, event_payload)

# XXX: How can we share classes between Sentry and Seer?
# Build CodecovTaskRequest
return {
"request_type": request_type,
"request_type": request_type.value,
"external_owner_id": repo.external_id,
"data": {
"repo": repo_definition,
Expand Down
2 changes: 2 additions & 0 deletions src/sentry/seer/code_review/webhooks/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

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__)

Expand All @@ -25,6 +26,7 @@
EVENT_TYPE_TO_HANDLER: dict[GithubWebhookType, WebhookProcessor] = {
GithubWebhookType.CHECK_RUN: handle_check_run_event,
GithubWebhookType.ISSUE_COMMENT: handle_issue_comment_event,
GithubWebhookType.PULL_REQUEST: handle_pull_request_event,
}


Expand Down
154 changes: 154 additions & 0 deletions src/sentry/seer/code_review/webhooks/pull_request.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
"""
Handler for GitHub pull_request webhook events.
https://docs.github.com/en/webhooks/webhook-events-and-payloads#pull_request
"""

from __future__ import annotations

import enum
import logging
from collections.abc import Mapping
from typing import Any

from sentry import options
from sentry.integrations.github.webhook_types import GithubWebhookType
from sentry.integrations.services.integration import RpcIntegration
from sentry.models.organization import Organization
from sentry.models.repository import Repository
from sentry.models.repositorysettings import CodeReviewTrigger
from sentry.utils import metrics

from ..utils import _get_target_commit_sha

logger = logging.getLogger(__name__)


class ErrorStatus(enum.StrEnum):
MISSING_PULL_REQUEST = "missing_pull_request"
MISSING_ACTION = "missing_action"
UNSUPPORTED_ACTION = "unsupported_action"
DRAFT_PR = "draft_pr"


class Log(enum.StrEnum):
MISSING_PULL_REQUEST = "github.webhook.pull_request.missing-pull-request"
MISSING_ACTION = "github.webhook.pull_request.missing-action"
UNSUPPORTED_ACTION = "github.webhook.pull_request.unsupported-action"
DRAFT_PR = "github.webhook.pull_request.draft-pr"


class Metrics(enum.StrEnum):
ERROR = "seer.code_review.webhook.pull_request.error"
OUTCOME = "seer.code_review.webhook.pull_request.outcome"


class PullRequestAction(enum.StrEnum):
"""
GitHub pull_request webhook actions.
https://docs.github.com/en/webhooks/webhook-events-and-payloads#pull_request
"""

ASSIGNED = "assigned"
AUTO_MERGE_DISABLED = "auto_merge_disabled"
AUTO_MERGE_ENABLED = "auto_merge_enabled"
CLOSED = "closed"
CONVERTED_TO_DRAFT = "converted_to_draft"
DEMILESTONED = "demilestoned"
DEQUEUED = "dequeued"
EDITED = "edited"
ENQUEUED = "enqueued"
LABELED = "labeled"
LOCKED = "locked"
MILESTONED = "milestoned"
OPENED = "opened"
READY_FOR_REVIEW = "ready_for_review"
REOPENED = "reopened"
REVIEW_REQUEST_REMOVED = "review_request_removed"
REVIEW_REQUESTED = "review_requested"
# New commits are pushed to the PR
SYNCHRONIZE = "synchronize"
UNASSIGNED = "unassigned"
UNLABELED = "unlabeled"
UNLOCKED = "unlocked"


WHITELISTED_ACTIONS = {
PullRequestAction.OPENED,
PullRequestAction.READY_FOR_REVIEW,
PullRequestAction.SYNCHRONIZE,
}


def _get_trigger_for_action(action: PullRequestAction) -> CodeReviewTrigger:
match action:
case PullRequestAction.READY_FOR_REVIEW:
return CodeReviewTrigger.ON_READY_FOR_REVIEW
case PullRequestAction.SYNCHRONIZE:
return CodeReviewTrigger.ON_NEW_COMMIT
case _:
raise ValueError(f"Unsupported pull request action: {action}")


def _warn_and_increment_metric(
error_status: ErrorStatus,
extra: Mapping[str, Any],
action: PullRequestAction | str | None = None,
) -> None:
"""
Warn and increment metric for a given error status and action.
"""
logger.warning(Log[error_status.name].value, extra=extra)
tags = {"error_status": error_status.value}
if action:
tags["action"] = action
metrics.incr(Metrics.ERROR.value, tags=tags)


def handle_pull_request_event(
*,
github_event: GithubWebhookType,
event: Mapping[str, Any],
organization: Organization,
repo: Repository,
integration: RpcIntegration | None = None,
**kwargs: Any,
) -> None:
"""
Handle pull_request webhook events for code review.
This handler processes PR events and sends them directly to Seer
"""
extra = {"organization_id": organization.id, "repo": repo.name}
pull_request = event.get("pull_request")
if not pull_request:
_warn_and_increment_metric(ErrorStatus.MISSING_PULL_REQUEST, extra=extra)
return

action_value = event.get("action")
if not action_value or not isinstance(action_value, str):
_warn_and_increment_metric(ErrorStatus.MISSING_ACTION, extra=extra)
return

try:
action = PullRequestAction(action_value)
except ValueError:
_warn_and_increment_metric(ErrorStatus.UNSUPPORTED_ACTION, action=action_value, extra=extra)
return

if action not in WHITELISTED_ACTIONS:
return

if pull_request.get("draft") is True:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later I'm guessing we're going to want to start tracking this metric later too to inform whether we should allow our feature on drafts or not

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain if this is necessary since we check the actions and there's a draft action.

return

if not options.get("github.webhook.pr"):
from .task import schedule_task

schedule_task(
github_event=github_event,
event=event,
organization=organization,
repo=repo,
target_commit_sha=_get_target_commit_sha(github_event, event, repo, integration),
trigger=_get_trigger_for_action(action),
)
6 changes: 4 additions & 2 deletions src/sentry/testutils/helpers/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,19 +138,21 @@ def send_github_webhook_event(

class GitHubWebhookCodeReviewTestCase(GitHubWebhookTestCase):
CODE_REVIEW_FEATURES = {"organizations:gen-ai-features", "organizations:code-review-beta"}
OPTIONS_TO_SET: dict[str, bool] = {}

@contextmanager
def code_review_setup(
self,
features: Collection[str] | Mapping[str, Any] | None = None,
forward_to_overwatch: bool = False,
options: dict[str, bool] | None = None,
) -> Generator[None]:
"""Helper to set up code review test context."""
self.organization.update_option("sentry:enable_pr_review_test_generation", True)
features_to_enable = self.CODE_REVIEW_FEATURES if features is None else features
options_to_set = dict(self.OPTIONS_TO_SET) | (options or {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is nice

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

with (
self.feature(features_to_enable),
self.options({"github.webhook.issue-comment": forward_to_overwatch}),
self.options(options_to_set),
):
yield

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
class IssueCommentEventWebhookTest(GitHubWebhookCodeReviewTestCase):
"""Integration tests for GitHub issue_comment webhook events."""

OPTIONS_TO_SET = {"github.webhook.issue-comment": False}

@pytest.fixture(autouse=True)
def mock_github_api_calls(self) -> Generator[None]:
"""
Expand Down Expand Up @@ -143,7 +145,7 @@ def test_skips_reaction_when_no_comment_id(self, mock_reaction: MagicMock) -> No
@patch("sentry.seer.code_review.webhooks.issue_comment._add_eyes_reaction_to_comment")
def test_skips_processing_when_option_is_true(self, mock_reaction: MagicMock) -> None:
"""Test that when github.webhook.issue-comment option is True (kill switch), no processing occurs."""
with self.code_review_setup(forward_to_overwatch=True), self.tasks():
with self.code_review_setup(options={"github.webhook.issue-comment": True}), self.tasks():
event = self._build_issue_comment_event(f"Please {SENTRY_REVIEW_COMMAND} this PR")
response = self._send_issue_comment_event(event)
assert response.status_code == 204
Expand Down
Loading
Loading