feat(pr-metrics): Persist PR lifecycle fields and use them as emit fallback#116962
Merged
vaind merged 6 commits intoJun 8, 2026
Merged
Conversation
…llback The close/merge metrics row read lifecycle facts (head_commit_sha, closed_at, merged_at, merge_commit_sha) straight from the webhook payload because the CORE-200 columns on PullRequest were never populated. That left no source for a caller that lacks a fresh payload — notably the future judge round-trip, which has the canonical PullRequest but no webhook event. Wire the fields through on every PR webhook: the GitHub handler now persists head_commit_sha, closed_at, merged_at, and a derived lifecycle state (folding GitHub's state + merged flag so a merged PR stores "merged", not an ambiguous "closed"). build_pr_metrics_row() then reads payload-first with the persisted PullRequest field as fallback, and derives merged-vs-closed from the resolved close_action so the payload-less path stays consistent. opened_at stays payload-only and fail-fast: it has no persisted counterpart (date_added is Sentry-side, not the GitHub open time). Refs CORE-228 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
Scope: Backend
build_pr_metrics_row() read the four persisted lifecycle facts payload-first with the PullRequest row as a fallback. Since the webhook upserts the row before the emission processor runs, payload and stored agree on the webhook path — so reading stored is equivalent there, and is the single source of truth on the judge path, which has no payload at all. Read head_commit_sha/closed_at/merged_at/merge_commit_sha straight from the stored row. This matches how organization_id/repository_id/pr_key are already sourced, and it exercises the persistence on the live webhook path instead of only on the not-yet-wired judge path — a persistence bug now surfaces immediately rather than staying masked behind the payload. The payload still supplies the fields with no persisted column: opened_at and the activity counters. Timestamps are emitted via .isoformat() per the analytics-event convention (e.g. OrganizationRemoved.deletion_request_datetime). Refs CORE-228 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The docstring already explains the stored-vs-payload split, so the inline comments on each PrCloseMetricsEvent field were redundant. Drop them and tighten the over-long comments in the webhook handler and tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-persist-pr-metrics-fallback-fields-on-pullrequest
PrCloseMetricsEvent types head_commit_sha and closed_at as required str, but they come from nullable PullRequest columns, so mypy rejected the assignment. A close/merge row must carry both — the webhook always persists them on a terminal event — so narrow with a fail-fast guard rather than loosening the event schema, keeping the original non-null intent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… tests build_pr_metrics_row() now reads lifecycle from the stored PullRequest row, which PullRequestEventWebhook._handle populates before the emission processor runs. These tests call handle_emission() in isolation, so the PR row had no lifecycle and the new fail-fast guard rejected it. Mirror _handle by persisting head_commit_sha/closed_at/merged_at/merge_commit_sha on the row before the call, and trim the now-unused lifecycle keys from the payload fixture. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
giovanni-guidini
approved these changes
Jun 8, 2026
vaind
deleted the
ivandlugos/core-228-persist-pr-metrics-fallback-fields-on-pullrequest
branch
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up from #116842 (CORE-228). That PR's close/merge metrics row read PR lifecycle facts (
head_commit_sha,closed_at,merged_at,merge_commit_sha) straight from the webhook payload, because thePullRequestcolumns added in CORE-200 were never populated. That left no source for a caller that doesn't have a fresh webhook payload — notably the future judge round-trip (CORE-217), which has the canonicalPullRequestbut no originating event.What
Persist the fields —
integrations/github/webhook.pyPullRequestEventWebhook._handlenow writes the lifecycle columns on every PR webhook via the existingupdate_or_create:head_commit_sha←payload["head"]["sha"]closed_at/merged_at← parsed GitHub timestampsstate← derived lifecycle state, folding GitHub'sstate+mergedflag so a merged PR stores"merged"rather than an ambiguous"closed"Source the emit row from the stored row —
pr_metrics/emit.pybuild_pr_metrics_row()readshead_commit_sha,closed_at,merged_at, andmerge_commit_shadirectly from thePullRequestrow. The webhook upserts the row before the emission processor runs, so payload and stored agree on the webhook path — and stored is the single source of truth on the judge path, which has no payload at all. This also matches howorganization_id/repository_id/pr_keyare already sourced, and it exercises the persistence on the live webhook path (a persistence bug surfaces immediately rather than staying masked behind the payload).A close/merge row must carry a
head_commit_shaandclosed_at, sobuild_pr_metrics_row()fails loud if either is null on the stored row (rather than loosening the event schema to optional). The payload still supplies the fields with no persisted column:opened_atand the activity counters (additions,deletions, …,is_assigned).opened_atis read fail-fast too —PullRequest.date_addedis Sentry-side, not the GitHub open time. Timestamps are emitted via.isoformat()per the analytics-event convention.Tests
test_opened/test_closedassert the persisted lifecycle fields onPullRequest.test_emit.pycovers stored-sourced lifecycle, the closed-but-unmerged case, that payload lifecycle is ignored in favor of the stored row, and the fail-loud guards for a missingopened_ator stored lifecycle.Refs CORE-228