Skip to content

fix(observability): clarify manual/commented/ignored panel semantics on the Reviews & PRs dashboard #3717

Description

@JSONbored

Context

Part of #1819.

Reported symptom: the "Manual review" (21), "Commented (advisory)" (4), and "Ignored" (0) stat panels on the "Gittensory — Reviews & PRs (maintainer)" dashboard read far lower than the maintainer's own felt sense of activity ("I've definitely had more than 21 manual reviews... I've also commented on more than 4 total").

Root cause, confirmed by directly reading grafana/dashboards/maintainer-reviews.json (panels id 2-7): all six top-row stat panels — including PRs tracked and Merged/Closed, not just the three in question — share the exact same query shape against the local review_targets table:

SELECT count(*) AS manual FROM review_targets
WHERE (status='manual' OR verdict='manual')
AND unixepoch(updated_at) >= ${__from:date:seconds}
AND unixepoch(updated_at) < ${__to:date:seconds}

This counts rows whose last update timestamp falls inside the dashboard's currently-selected time window (the reported screenshot had the picker on "Last 2 days" — the dashboard's own default is now-90d to now).

review_targets.status/.verdict are mutable, current-state fields, not an append-only log — confirmed by the schema comment in migrations/0050_review_targets.sql and the terminal-status set in src/review/ops.ts (merged, closed, commented, manual, ignored, error). Each row holds exactly one current status, overwritten as a PR's disposition changes.

The combination is the bug-shaped part: manual/commented/ignored are inherently transient states for a meaningful share of PRs — most manually-held PRs eventually get merged or closed by the maintainer. A PR that was held for manual review and then resolved within the same time window shows up only under Merged/Closed, never under Manual, because the row's updated_at reflects its latest transition, not the fact that it passed through manual earlier in the window. So these three panels measure "how many PRs are currently sitting in this state as of their last update in-window" — not "how many times a PR entered this state" — and will structurally under-report real activity for anyone who actively resolves their manual-review queue, which is exactly the reported symptom.

An append-only audit_events ledger already exists (migrations/0008_private_beta_auth.sql: event_type/outcome/created_at, insert-only) and is already used pervasively in src/queue/processors.ts (20+ recordAuditEvent call sites) for review-disposition-adjacent events — the natural candidate source for a true event-count metric, but it is not yet confirmed whether an existing event_type cleanly captures "PR disposition became manual/commented/ignored", or whether a new emission point needs adding at whichever call site actually sets review_targets.status/.verdict to one of these values.

Requirements

  • Confirm the hypothesis with real data before changing anything: find a PR known to have passed through manual and later merged within a short window, and confirm it disappears from the Manual panel once merged (or refute the hypothesis if this isn't what happens).
  • Ship two things, not one:
    1. Immediate UX clarity fix (cheap, ship regardless of part 2): add a panel description/tooltip to the three affected stat panels making the current semantics honest — e.g. "PRs whose most recent update in this window is 'manual'; a PR later merged/closed in the same window shows under Merged/Closed instead." Do this even if part 2 turns out to be more involved than expected.
    2. A true event-count metric: new panel(s) — additive, not a replacement — that count disposition events (not current state) over the window, sourced from audit_events if a suitable event_type already exists, or from a newly-added emission point at the actual disposition call site if not.
  • Do not silently change the existing panels' query semantics. "Manual review: 21" read as "21 PRs currently need my attention" is a legitimate and arguably more useful reading than a lifetime tally — keep that view intact and add the event-count view alongside it, unless the investigation concludes otherwise.

Deliverables

  • A short written finding (issue comment or PR description) confirming or refuting the undercounting hypothesis with real data.
  • Updated grafana/dashboards/maintainer-reviews.json: clarified descriptions/tooltips on the three existing panels, plus new event-count panel(s) once a suitable event source is confirmed or added.
  • If a new audit_events emission point is needed: wired in at the exact call site(s) with a clear, consistent event_type naming convention (matching the existing dotted style, e.g. github_app.gate_overridden).

Expected outcome

A maintainer can tell, at a glance, both (a) how many PRs currently sit in manual-review/commented/ignored status — the existing panels, now honestly labeled — and (b) how many times they were actually pulled into manual review, left an advisory comment, or had a PR ignored over a given period, matching their own felt sense of activity rather than a point-in-time snapshot.

Non-goals

Changing the underlying review_targets state machine. Retroactively backfilling event-counts for history that predates any new audit-event emission point, unless a suitable existing audit_events history already covers it cheaply — investigate first, backfill only if it's close to free.

Metadata

Metadata

Assignees

Labels

maintainer-onlyOwner-only work — yields no Gittensor points.

Projects

Status
Todo

Relationships

None yet

Development

No branches or pull requests

Issue actions