Skip to content

feat(gitlab): track contributor seat on merge request opened#116576

Merged
billyvg merged 4 commits into
masterfrom
tnt-sentry/scm-99-gitlab-mr-seat-tracking
Jun 3, 2026
Merged

feat(gitlab): track contributor seat on merge request opened#116576
billyvg merged 4 commits into
masterfrom
tnt-sentry/scm-99-gitlab-mr-seat-tracking

Conversation

@tnt-sentry

Copy link
Copy Markdown
Contributor

Seeds OrganizationContributors when a GitLab merge request is opened, so seat-based Seer billing works once an org is moved onto organizations:seat-based-seer-enabled. Mirrors the GitHub side, where track_contributor_seat fires from the pull_request opened webhook (src/sentry/integrations/github/webhook.py).

Implemented as a WebhookProcessor registered on MergeEventWebhook so errors are isolated per-processor by the existing _handle loop. Action filtered to "open" only — GitLab's merge_request event also delivers update / close / merge actions that should not seed a contributor.

Gating:

  • organizations:seer-code-review-gitlab — the same cohort flag handle_merge_request_event uses, so seat seeding only happens for orgs that are already opted in to GitLab code review.
  • organizations:seat-based-seer-enabled — checked inside the downstream should_increment_contributor_seat. Required before any row is actually written or a seat is assigned.

This supersedes #116305, which was opened against the pre-#116317 webhook shape and is no longer rebaseable.

Once this lands, the gap documented in src/sentry/seer/code_review/webhooks/merge_request.py's module docstring ("Code review does not fire in production yet: GitLab contributors are never seeded") closes, and the only remaining blocker before billing flips on is the per-org subscription option (SubscriptionOptions.SEER_USER_ENABLED) gating seat-based-seer-enabled.

Refs SCM-99

Adds a webhook processor that seeds OrganizationContributors when a GitLab
merge request is opened, so seat-based Seer billing works once an org is
moved onto organizations:seat-based-seer-enabled. Mirrors the GitHub side
where track_contributor_seat fires from the pull_request opened webhook.

Registered as a WebhookProcessor on MergeEventWebhook so errors are
isolated per-processor by the existing _handle loop. Action filtered to
"open" only — GitLab's merge_request event also delivers update / close /
merge actions that should not seed a contributor.

Gated by organizations:seer-code-review-gitlab — the same cohort flag the
merge_request handler uses, so seat seeding only happens for orgs that are
already opted in to GitLab code review. The downstream
should_increment_contributor_seat additionally requires
organizations:seat-based-seer-enabled before any row is actually written.

Refs SCM-99
@linear-code

linear-code Bot commented Jun 1, 2026

Copy link
Copy Markdown

SCM-99

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 1, 2026
…butor_seat

The webhook processor receives an RpcOrganization, but
track_contributor_seat is typed to take an Organization. Mirror the
pattern used by handle_merge_request_event: fetch the real Organization
from cache via the rpc org's id, then pass that down.
@tnt-sentry tnt-sentry marked this pull request as ready for review June 1, 2026 17:39
@tnt-sentry tnt-sentry requested review from a team as code owners June 1, 2026 17:39
Comment thread src/sentry/integrations/gitlab/webhooks.py
Comment thread src/sentry/seer/code_review/webhooks/seat_tracking.py
Comment thread src/sentry/seer/code_review/webhooks/seat_tracking.py
Addresses two issues called out by Cursor Bugbot on PR #116576:

1. Seat processor runs too late.
   MergeEventWebhook.WEBHOOK_EVENT_PROCESSORS ran handle_merge_request_event
   before track_gitlab_contributor_seat_processor, so the code-review handler's
   preflight billing check hit ORG_CONTRIBUTOR_NOT_FOUND on the first MR open
   from a new contributor even though the same delivery seeded the row
   seconds later. Swap the tuple order so seeding runs first.

2. Open redelivery double-counts num_actions.
   GitLab redelivers merge_request hooks on response timeout, and the endpoint
   dispatches each payload once per installed organization. The action=="open"
   gate is not idempotent against either, so a re-delivered open event would
   call track_contributor_seat again and increment num_actions twice for a
   single MR. Mirror the existing dedup pattern from merge_request.py: short
   Redis TTL key per (org, repo, MR iid), separate namespace from the
   code-review handler's dedup key.

Also rewrite the module docstring to describe what we actually do (action
filter + Redis dedup), how it differs from GitHub's DB-driven `if created:`
gate, and the malformed-payload follow-up tracked on SCM-99.
Comment thread src/sentry/seer/code_review/webhooks/seat_tracking.py
If Organization.objects.get_from_cache raised DoesNotExist, the Redis NX
key was already set, so GitLab redeliveries within the 20s TTL would be
treated as duplicates and skip seeding even though no contributor was ever
written. Resolve the org first; only mark the delivery as seen after we
know we have an org to track for.

The track_contributor_seat reraise path (DB errors, etc.) still leaves the
seen key in place — matches the pattern in merge_request.py and is a small
acceptable trade-off vs. cleanup-on-failure complexity.

Addresses Cursor Bugbot finding on PR #116576.
Comment thread src/sentry/integrations/gitlab/webhooks.py

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit cde14b9. Configure here.

Comment thread src/sentry/seer/code_review/webhooks/seat_tracking.py
@billyvg

billyvg commented Jun 2, 2026

Copy link
Copy Markdown
Member

Reviewed this — design and ordering rationale are solid, and the dedup/ordering docstrings are genuinely helpful. Two things worth addressing before merge (filing the other two findings as SCM follow-ups):

1. Module docstring contradicts the actual write behavior

In seat_tracking.py the docstring says:

The downstream call to should_increment_contributor_seat additionally requires organizations:seat-based-seer-enabled before any row is actually written or a seat is assigned.

But track_contributor_seat (contributor_seats.py) calls OrganizationContributors.objects.get_or_create(...) unconditionally, then checks should_increment_contributor_seat. So the contributor row is written for every org in the seer-code-review-gitlab cohort on every MR-open, regardless of seat-based-seer-enabled — only the action-count increment and seat assignment are gated.

This is almost certainly the intended behavior (seeding the row is exactly what makes preflight stop returning ORG_CONTRIBUTOR_NOT_FOUND), but the docstring — and the PR description, which repeats the claim — misstates the side effect. Could we correct it to something like: "the row is always created for cohort orgs; seat-based-seer-enabled gates only the action increment and seat assignment"? Worth a quick confirm that creating contributor rows for cohort orgs not yet on seat-based-seer-enabled is intended.

4. No end-to-end test that a row is actually seeded

All the unit tests mock track_contributor_seat and call the processor directly, which is great for the action/flag/dedup matrix. But the PR's central claim is "the row exists by the time preflight runs," and nothing exercises the real path through MergeEventWebhook.__call__ → _handle → processor with an unmocked track_contributor_seat asserting the OrganizationContributors row gets created (and that preflight no longer denies with ORG_CONTRIBUTOR_NOT_FOUND). One such test would lock in the integration contract — and would have surfaced the discrepancy in #1. The merge_request.py docstring already nudges toward this ("consider a test that omits [manual seeding] to lock in the intended production behavior").

The documented "known gap" (missing last_commit/author email short-circuits __call__ before _handle, so the author is never seeded, and the subsequent update is filtered out by action == "open") is correctly tracked on SCM-99 — fine to defer.

Two more minor findings (alias/external_identifier actor-source mismatch, and the duplicated _is_duplicate_delivery helper) are being filed as SCM follow-ups rather than blocking here.

@billyvg billyvg merged commit e949124 into master Jun 3, 2026
63 checks passed
@billyvg billyvg deleted the tnt-sentry/scm-99-gitlab-mr-seat-tracking branch June 3, 2026 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants