feat(code_review): Improve debuggability#106880
Conversation
There was a problem hiding this comment.
We're adding some extra metadata which already exists in GitHub events but were never added to the fixtures.
There was a problem hiding this comment.
The big improvement on this PR versus the previous one is that we add this module to the stronger typing list, thus, issues like SENTRY-5H53 should not happen.
| **kwargs: Additional keyword arguments | ||
| """ | ||
| # The extracted important key values are used for debugging with logs | ||
| extra = extract_github_info(event, github_event=github_event.value) |
There was a problem hiding this comment.
We extract the key values as early as possible in the entry point handler and pass the values to all downstream logging.
There was a problem hiding this comment.
Hey I'm pretty sure this is going to error again unless we move this down to line 56 after the GITHUB_ENTERPRISE check.
This is the same thing I encountered here with the sentry issue here.
The problem is that the type of github_event is wrong and is str in the case of the github enterprise class not enum. Maybe it's better if we fix the type in the github enterprise check here . Yeah either that or move these lines extracting extra to under the ghe check
| repo=repo, | ||
| integration=integration, | ||
| org_code_review_settings=preflight.settings, | ||
| extra=extra, |
There was a problem hiding this comment.
Passing it to the downstream handlers.
| *, | ||
| enqueued_at_str: str, | ||
| github_event: GithubWebhookType, | ||
| github_event: str, |
There was a problem hiding this comment.
When the task gets scheduled, Celery JSON serializes the data and we lose the Enum type.
I'm investigating if there's any tool we could add to prevent issues like this.
There was a problem hiding this comment.
Oh good to know, I can definitely see myself falling into that trap again
|
@sentry review |
| organization_id=organization.id, | ||
| repo=repo, | ||
| pr_number=str(pr_number) if pr_number else None, | ||
| comment_id=str(comment_id), |
There was a problem hiding this comment.
It would be ideal if we could pass extra in here and also the callsite in pull_request.py and log a warning every time we do record_webhook_handler_error. Happy to address this in a followup PR after this gets merged 👍
There was a problem hiding this comment.
Hi @srest2021 I will let you handle since I'm unsure what you have in mind.
suejung-sentry
left a comment
There was a problem hiding this comment.
this makes sense but just a warning to address this or I'm pretty sure it's gonna error again
| *, | ||
| enqueued_at_str: str, | ||
| github_event: GithubWebhookType, | ||
| github_event: str, |
There was a problem hiding this comment.
Oh good to know, I can definitely see myself falling into that trap again
| **kwargs: Additional keyword arguments | ||
| """ | ||
| # The extracted important key values are used for debugging with logs | ||
| extra = extract_github_info(event, github_event=github_event.value) |
There was a problem hiding this comment.
Hey I'm pretty sure this is going to error again unless we move this down to line 56 after the GITHUB_ENTERPRISE check.
This is the same thing I encountered here with the sentry issue here.
The problem is that the type of github_event is wrong and is str in the case of the github enterprise class not enum. Maybe it's better if we fix the type in the github enterprise check here . Yeah either that or move these lines extracting extra to under the ghe check
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| return | ||
|
|
||
| # The extracted important key values are used for debugging with logs | ||
| extra = extract_github_info(event, github_event=github_event.value) |
There was a problem hiding this comment.
Calling .value on string fails for GitHub Enterprise
High Severity
The call to github_event.value at this line assumes github_event is always a GithubWebhookType enum, but GitHub Enterprise webhook handlers pass github_event as a raw string (from request.headers.get("x-github-event")). While there's a GitHub Enterprise check at lines 51-52 that should return early, a reviewer explicitly raised concerns that this will still error. The original bug this PR fixes was exactly AttributeError: 'str' object has no attribute 'value' on this line.
This is a follow-up to #106567 and fixes the issue we found when it was deployed ([SENTRY-5H53](https://sentry.sentry.io/issues/7210274284/)). For this line: `extra = extract_github_info(event, github_event=github_event.value)` we had an `AttributeError: 'str' object has no attribute 'value'`. ## Description This PR adds the `extract_github_info()` function to extract GitHub metadata from webhook events for improved debugging and logging. In the future, we should also add this ability to Seer's code review code path. ## Changes ### New Function: `extract_github_info()` - Extracts GitHub metadata from webhook event payloads - Returns a dictionary with: - `github_owner`: Repository owner/organization name - `github_repo_name`: Repository name - `github_repo_full_name`: Full repository name (owner/repo) - `github_event_url`: URL to the specific event (check_run, pull_request, or comment) ### Integration - Updated `handle_webhook_event()` in `handlers.py` to use `extract_github_info()` - Extracted metadata is passed as `extra` parameter to event handlers - Updated `check_run.py`, `issue_comment.py`, and `pull_request.py` to accept `extra` parameter ### Testing - Added comprehensive unit test suite with 14 test cases - Tests cover all event types (pull_request, check_run, issue_comment) - Tests verify URL precedence logic (comment > check_run > pull_request) - Tests use real GitHub webhook fixtures for better accuracy - Edge cases tested (missing data, empty events, etc.) ### Fixtures - Updated GitHub fixtures to include complete repository metadata - Added missing `repository.name` and `repository.owner.login` fields Fixes [CW-299](https://linear.app/getsentry/issue/CW-299)
This is a follow-up to #106567 and fixes the issue we found when it was deployed (SENTRY-5H53). For this line:
extra = extract_github_info(event, github_event=github_event.value)we had anAttributeError: 'str' object has no attribute 'value'.Description
This PR adds the
extract_github_info()function to extract GitHub metadata from webhook events for improved debugging and logging.In the future, we should also add this ability to Seer's code review code path.
Changes
New Function:
extract_github_info()github_owner: Repository owner/organization namegithub_repo_name: Repository namegithub_repo_full_name: Full repository name (owner/repo)github_event_url: URL to the specific event (check_run, pull_request, or comment)Integration
handle_webhook_event()inhandlers.pyto useextract_github_info()extraparameter to event handlerscheck_run.py,issue_comment.py, andpull_request.pyto acceptextraparameterTesting
Fixtures
repository.nameandrepository.owner.loginfieldsFixes CW-299