From 336dc82d4b5a5ea4b1cb7a4df325a3aa60ca875c Mon Sep 17 00:00:00 2001 From: glorydavid03023 Date: Tue, 14 Jul 2026 15:24:26 -0500 Subject: [PATCH] fix(observability): add additive audit_events-backed review-activity panels (#3717) Part 1 (#4134) clarified the wording on the snapshot panels. This is part 2: the true additive event-count panels the issue asks for. The six existing stat panels all count rows in `review_targets`, whose `status`/`verdict` are mutable current-state fields overwritten as a PR's disposition changes. `manual`/`commented`/`ignored` are transient for most PRs, so a PR that was held for manual review and then merged inside the same window is counted ONLY under Merged -- never under Manual review. Those panels answer "how many PRs are currently sitting in this state", not "how many times did a PR enter it", and so they structurally under-report for a maintainer who actively drains the queue. That is exactly the reported symptom. `audit_events` is append-only and already records the transitions: `agent.action.hold` when a PR is held for manual review (processors.ts), and `agent.action.${actionClass}` for every executed action (agent-execution.ts). Nothing new needs to be emitted -- the log was already there, and the dashboard simply never queried it. Adds a "Review activity (additive, from audit_events)" row with three stats (manual reviews entered, merges executed, closes executed) and a stacked per-day breakdown, all windowed on `created_at` so a day's count never changes retroactively. Existing panels are untouched; the new row is appended, so no panel shifts position. Scoping to the selected repo goes through `target_key LIKE '#%'` rather than a `repo` column, which `audit_events` does not have. That also means these counts cannot exclude bot-authored PRs the way the review_targets panels do -- stated plainly in the panel descriptions rather than papered over. Tested: 8 assertions pin the panels to audit_events (never review_targets), to `created_at` (never `updated_at`), to the real emitted event types -- including a drift guard that every event_type the panels query is one AGENT_ACTION_CLASSES can actually produce, so renaming an action class cannot silently leave these panels reading zero forever. --- grafana/dashboards/maintainer-reviews.json | 52 +++++++++ ...ost-grafana-additive-review-events.test.ts | 109 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 test/unit/selfhost-grafana-additive-review-events.test.ts diff --git a/grafana/dashboards/maintainer-reviews.json b/grafana/dashboards/maintainer-reviews.json index 97c3fe8a60..42b126b3e0 100644 --- a/grafana/dashboards/maintainer-reviews.json +++ b/grafana/dashboards/maintainer-reviews.json @@ -193,6 +193,58 @@ "fieldConfig": { "defaults": { "color": { "mode": "palette-classic" }, "unit": "short" }, "overrides": [] }, "options": { "legend": { "displayMode": "list", "placement": "right", "showLegend": true, "values": ["value", "percent"] }, "pieType": "donut", "reduceOptions": { "calcs": ["lastNotNull"], "fields": "", "values": true }, "tooltip": { "mode": "single", "sort": "none" } }, "targets": [{ "datasource": { "type": "frser-sqlite-datasource", "uid": "loopover-db" }, "refId": "A", "queryType": "table", "queryText": "SELECT verdict, count(*) AS c FROM review_targets WHERE (submitter NOT LIKE '%[bot]%' OR submitter IS NULL) AND verdict IS NOT NULL AND (${repo:sqlstring} = '__ALL__' OR repo = ${repo:sqlstring}) AND unixepoch(updated_at) >= ${__from:date:seconds} AND unixepoch(updated_at) < ${__to:date:seconds} GROUP BY verdict ORDER BY c DESC", "rawQueryText": "SELECT verdict, count(*) AS c FROM review_targets WHERE (submitter NOT LIKE '%[bot]%' OR submitter IS NULL) AND verdict IS NOT NULL AND (${repo:sqlstring} = '__ALL__' OR repo = ${repo:sqlstring}) AND unixepoch(updated_at) >= ${__from:date:seconds} AND unixepoch(updated_at) < ${__to:date:seconds} GROUP BY verdict ORDER BY c DESC" }] + }, + { + "type": "row", + "id": 15, + "title": "Review activity (additive, from audit_events)", + "collapsed": false, + "gridPos": { "h": 1, "w": 24, "x": 0, "y": 34 }, + "panels": [] + }, + { + "type": "stat", + "id": 16, + "title": "Manual reviews entered", + "description": "ADDITIVE lifetime tally: how many times a PR was HELD for manual review in this window, counted from the append-only audit_events log (agent.action.hold) rather than from review_targets' mutable current status. The 'Manual review' stat above is a snapshot -- it silently drops any PR you later merged or closed -- so a PR that entered manual review and was resolved inside the same window is missing there but counted here. This is the number that matches how much manual review actually happened (#3717). Caveat: audit_events records the action the agent took, not the PR's submitter, so bot-authored PRs cannot be filtered out of this count the way they are above.", + "datasource": { "type": "frser-sqlite-datasource", "uid": "loopover-db" }, + "gridPos": { "h": 4, "w": 8, "x": 0, "y": 35 }, + "fieldConfig": { "defaults": { "color": { "mode": "fixed", "fixedColor": "orange" }, "unit": "short" }, "overrides": [] }, + "options": { "colorMode": "background", "graphMode": "none", "reduceOptions": { "calcs": ["lastNotNull"], "fields": "", "values": false }, "textMode": "value" }, + "targets": [{ "datasource": { "type": "frser-sqlite-datasource", "uid": "loopover-db" }, "refId": "A", "queryType": "table", "queryText": "SELECT count(*) AS n FROM audit_events WHERE event_type = 'agent.action.hold' AND outcome = 'completed' AND (${repo:sqlstring} = '__ALL__' OR target_key LIKE ${repo:sqlstring} || '#%') AND unixepoch(created_at) >= ${__from:date:seconds} AND unixepoch(created_at) < ${__to:date:seconds}", "rawQueryText": "SELECT count(*) AS n FROM audit_events WHERE event_type = 'agent.action.hold' AND outcome = 'completed' AND (${repo:sqlstring} = '__ALL__' OR target_key LIKE ${repo:sqlstring} || '#%') AND unixepoch(created_at) >= ${__from:date:seconds} AND unixepoch(created_at) < ${__to:date:seconds}" }] + }, + { + "type": "stat", + "id": 17, + "title": "Merges executed", + "description": "ADDITIVE: merge actions the agent executed in this window (agent.action.merge), from the append-only audit_events log. Each merge is counted once, at the moment it happened (#3717).", + "datasource": { "type": "frser-sqlite-datasource", "uid": "loopover-db" }, + "gridPos": { "h": 4, "w": 8, "x": 8, "y": 35 }, + "fieldConfig": { "defaults": { "color": { "mode": "fixed", "fixedColor": "green" }, "unit": "short" }, "overrides": [] }, + "options": { "colorMode": "background", "graphMode": "none", "reduceOptions": { "calcs": ["lastNotNull"], "fields": "", "values": false }, "textMode": "value" }, + "targets": [{ "datasource": { "type": "frser-sqlite-datasource", "uid": "loopover-db" }, "refId": "A", "queryType": "table", "queryText": "SELECT count(*) AS n FROM audit_events WHERE event_type = 'agent.action.merge' AND outcome = 'completed' AND (${repo:sqlstring} = '__ALL__' OR target_key LIKE ${repo:sqlstring} || '#%') AND unixepoch(created_at) >= ${__from:date:seconds} AND unixepoch(created_at) < ${__to:date:seconds}", "rawQueryText": "SELECT count(*) AS n FROM audit_events WHERE event_type = 'agent.action.merge' AND outcome = 'completed' AND (${repo:sqlstring} = '__ALL__' OR target_key LIKE ${repo:sqlstring} || '#%') AND unixepoch(created_at) >= ${__from:date:seconds} AND unixepoch(created_at) < ${__to:date:seconds}" }] + }, + { + "type": "stat", + "id": 18, + "title": "Closes executed", + "description": "ADDITIVE: close actions the agent executed in this window (agent.action.close), from the append-only audit_events log. Each close is counted once, at the moment it happened (#3717).", + "datasource": { "type": "frser-sqlite-datasource", "uid": "loopover-db" }, + "gridPos": { "h": 4, "w": 8, "x": 16, "y": 35 }, + "fieldConfig": { "defaults": { "color": { "mode": "fixed", "fixedColor": "red" }, "unit": "short" }, "overrides": [] }, + "options": { "colorMode": "background", "graphMode": "none", "reduceOptions": { "calcs": ["lastNotNull"], "fields": "", "values": false }, "textMode": "value" }, + "targets": [{ "datasource": { "type": "frser-sqlite-datasource", "uid": "loopover-db" }, "refId": "A", "queryType": "table", "queryText": "SELECT count(*) AS n FROM audit_events WHERE event_type = 'agent.action.close' AND outcome = 'completed' AND (${repo:sqlstring} = '__ALL__' OR target_key LIKE ${repo:sqlstring} || '#%') AND unixepoch(created_at) >= ${__from:date:seconds} AND unixepoch(created_at) < ${__to:date:seconds}", "rawQueryText": "SELECT count(*) AS n FROM audit_events WHERE event_type = 'agent.action.close' AND outcome = 'completed' AND (${repo:sqlstring} = '__ALL__' OR target_key LIKE ${repo:sqlstring} || '#%') AND unixepoch(created_at) >= ${__from:date:seconds} AND unixepoch(created_at) < ${__to:date:seconds}" }] + }, + { + "type": "timeseries", + "id": 19, + "title": "Review actions per day (additive)", + "description": "ADDITIVE: daily count of each agent action recorded in audit_events -- hold (manual review), merge, close, and review (advisory comment). Because audit_events is append-only, a day's bar never changes retroactively when a PR's status later moves on -- exactly what the mutable review_targets-backed panels above cannot show (#3717).", + "datasource": { "type": "frser-sqlite-datasource", "uid": "loopover-db" }, + "gridPos": { "h": 8, "w": 24, "x": 0, "y": 39 }, + "fieldConfig": { "defaults": { "custom": { "drawStyle": "bars", "fillOpacity": 60, "stacking": { "mode": "normal", "group": "A" } }, "unit": "short" }, "overrides": [] }, + "options": { "legend": { "displayMode": "list", "placement": "bottom", "showLegend": true }, "tooltip": { "mode": "multi", "sort": "none" } }, + "targets": [{ "datasource": { "type": "frser-sqlite-datasource", "uid": "loopover-db" }, "refId": "A", "queryType": "time series", "queryText": "SELECT unixepoch(date(created_at)) * 1000 AS time, event_type AS metric, count(*) AS actions FROM audit_events WHERE event_type IN ('agent.action.hold', 'agent.action.merge', 'agent.action.close', 'agent.action.review') AND outcome = 'completed' AND (${repo:sqlstring} = '__ALL__' OR target_key LIKE ${repo:sqlstring} || '#%') AND unixepoch(created_at) >= ${__from:date:seconds} AND unixepoch(created_at) < ${__to:date:seconds} GROUP BY date(created_at), event_type ORDER BY time", "rawQueryText": "SELECT unixepoch(date(created_at)) * 1000 AS time, event_type AS metric, count(*) AS actions FROM audit_events WHERE event_type IN ('agent.action.hold', 'agent.action.merge', 'agent.action.close', 'agent.action.review') AND outcome = 'completed' AND (${repo:sqlstring} = '__ALL__' OR target_key LIKE ${repo:sqlstring} || '#%') AND unixepoch(created_at) >= ${__from:date:seconds} AND unixepoch(created_at) < ${__to:date:seconds} GROUP BY date(created_at), event_type ORDER BY time", "timeColumns": ["time"] }] } ] } diff --git a/test/unit/selfhost-grafana-additive-review-events.test.ts b/test/unit/selfhost-grafana-additive-review-events.test.ts new file mode 100644 index 0000000000..ab151884aa --- /dev/null +++ b/test/unit/selfhost-grafana-additive-review-events.test.ts @@ -0,0 +1,109 @@ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; + +import { AGENT_ACTION_CLASSES } from "../../src/settings/autonomy"; + +type Target = { queryText?: string; rawQueryText?: string; queryType?: string; timeColumns?: string[] }; +type Panel = { id: number; type: string; title: string; description?: string; targets?: Target[] }; + +const dashboard = JSON.parse(readFileSync(join(process.cwd(), "grafana/dashboards/maintainer-reviews.json"), "utf8")) as { + panels: Panel[]; +}; + +const panel = (id: number): Panel => { + const found = dashboard.panels.find((candidate) => candidate.id === id); + expect(found, `panel ${id} is missing`).toBeDefined(); + return found!; +}; + +const sqlOf = (id: number): string => panel(id).targets?.[0]?.queryText ?? ""; + +// The additive panels added by #3717 part 2. Part 1 (#4134) only clarified the existing snapshot panels' wording. +const ADDITIVE_STAT_PANELS = [ + { id: 16, title: "Manual reviews entered", eventType: "agent.action.hold" }, + { id: 17, title: "Merges executed", eventType: "agent.action.merge" }, + { id: 18, title: "Closes executed", eventType: "agent.action.close" }, +]; + +describe("maintainer-reviews dashboard: additive review-event panels (#3717)", () => { + it("adds the additive panels without disturbing the existing snapshot panels", () => { + const ids = dashboard.panels.map((p) => p.id); + expect(new Set(ids).size).toBe(ids.length); // ids stay unique + // The original review_targets-backed stat panels are untouched and still present. + for (const id of [2, 3, 4, 5, 6, 7]) expect(sqlOf(id)).toContain("FROM review_targets"); + }); + + it("counts from the append-only audit_events log, NOT the mutable review_targets snapshot", () => { + // This is the whole point of the issue: review_targets.status/.verdict are current-state fields overwritten + // as a PR's disposition changes, so a PR that passed through manual review and was then merged inside the + // same window vanishes from the manual count. audit_events rows are never rewritten. + for (const { id } of [...ADDITIVE_STAT_PANELS, { id: 19 }]) { + const sql = sqlOf(id); + expect(sql).toContain("FROM audit_events"); + expect(sql).not.toContain("review_targets"); + // Window on created_at (when the event happened), never updated_at (which is what makes the old panels a snapshot). + expect(sql).toContain("unixepoch(created_at) >="); + expect(sql).not.toContain("updated_at"); + } + }); + + it("keys each additive stat on the real event_type the agent actually emits", () => { + for (const { id, title, eventType } of ADDITIVE_STAT_PANELS) { + expect(panel(id).title).toBe(title); + expect(sqlOf(id)).toContain(`event_type = '${eventType}'`); + expect(sqlOf(id)).toContain("outcome = 'completed'"); + } + }); + + it("DRIFT GUARD: every event_type the panels query is one the code can actually emit", () => { + // agent-execution.ts builds `agent.action.${actionClass}` from AGENT_ACTION_CLASSES, so a class renamed there + // must not silently leave these panels reading zero forever. `hold` is emitted directly by processors.ts + // (the manual-review disposition) rather than via an action class, so it is allowed alongside them. + const queried = new Set(); + for (const p of dashboard.panels.filter((candidate) => [16, 17, 18, 19].includes(candidate.id))) { + for (const match of (p.targets?.[0]?.queryText ?? "").matchAll(/'(agent\.action\.[a-z_]+)'/g)) queried.add(match[1]!); + } + expect(queried.size).toBeGreaterThan(0); + const emittable = new Set(["agent.action.hold", ...AGENT_ACTION_CLASSES.map((cls) => `agent.action.${cls}`)]); + for (const eventType of queried) expect(emittable).toContain(eventType); + }); + + it("scopes to the selected repo via the audit_events target_key, since it has no repo column", () => { + // audit_events.target_key is `${repoFullName}#${pullNumber}` (processors.ts), so the repo filter is a prefix + // match rather than the `repo = ...` equality the review_targets panels use. + for (const { id } of ADDITIVE_STAT_PANELS) { + expect(sqlOf(id)).toContain("target_key LIKE ${repo:sqlstring} || '#%'"); + expect(sqlOf(id)).toContain("${repo:sqlstring} = '__ALL__'"); + } + }); + + it("renders the per-day breakdown as a real time series", () => { + const timeseries = panel(19); + expect(timeseries.type).toBe("timeseries"); + const target = timeseries.targets?.[0]; + expect(target?.queryType).toBe("time series"); + expect(target?.timeColumns).toEqual(["time"]); + // Grafana's SQLite datasource wants epoch MILLIseconds for the time column. + expect(target?.queryText).toContain("unixepoch(date(created_at)) * 1000 AS time"); + expect(target?.queryText).toContain("GROUP BY date(created_at), event_type"); + }); + + it("explains in each panel description why the number differs from the snapshot above it", () => { + for (const { id } of [...ADDITIVE_STAT_PANELS, { id: 19 }]) { + const description = panel(id).description ?? ""; + expect(description).toMatch(/additive/i); + expect(description).toContain("#3717"); + } + // The manual-review panel is the one the maintainer actually reported as under-reporting, so it must say so. + expect(panel(16).description).toContain("audit_events"); + expect(panel(16).description).toMatch(/snapshot/i); + }); + + it("keeps both queryText and rawQueryText in sync, as the datasource requires", () => { + for (const id of [16, 17, 18, 19]) { + const target = panel(id).targets?.[0]; + expect(target?.rawQueryText).toBe(target?.queryText); + } + }); +});