feat: normalize workspace agent session counts into a child table - #27179
feat: normalize workspace agent session counts into a child table#27179EhabY wants to merge 1 commit into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
95ffdd9 to
45df97f
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 4 | Last posted: Round 4, 34 findings (1 P2, 6 P3, 15 Nit, 12 Note), APPROVE. Review Finding inventoryFinding inventory — PR #27179Law analysis
Findings
Author self-review (posted as a PR review comment) — panel disposition
Round logRound 1Netero + Law (infrastructure) then 19-persona panel. Netero: 1 P3, 1 Note (mechanical floor clean). Law: Don't split (advisory). Panel: 1 P2, 4 P3, 11 Nit, 8 Note. Author self-review B1/M2/M3/M4 verified as non-reproducing. Reviewed against 35ade9e..45df97f. Round 2Churn guard: PROCEED. All 23 round-1 findings classified addressed (verified against the tree, not the author's claim). Author fixes claimed at head d0be576; panel verifies on encounter. Notable new code beyond fixes: child table gained a Round 2 panelNetero + 16-persona panel. All 23 R1 fixes verified addressed on encounter. New findings: 2 P3 (CRF-25 server cap test glass, CRF-26 agent cap well-known folding), 4 Note (CRF-24, 27, 28, 32), 3 Nit (CRF-29, 30, 31). 1 dropped (CRF-33). CRF-26 convergence: Hisoka/Mafuuu P3, Meruem/Melody/Razor/Chopper Note; orchestrator verified the agent code lacks the server's known-first protection. Event COMMENT (no P0/P1). Round 3 updateBLOCKED by churn guard. Head fe1ee77 (base a9a1dcc). 7 addressed (CRF-24, 25, 26, 28, 29, 30, 31), 1 acknowledged (CRF-27: re-measured + PR body updated), 1 silent (CRF-32). No panel or Netero this round. CRF-32 (magic_type label documentation) got no author reply and no code change; it was folded into the round-2 review body (metrics.go:50 outside the diff), so it was easy to miss. Posted a BLOCKED review asking the author to fix CRF-32 or explain. The 8 addressed/acknowledged fixes are recorded as author-claimed and will be panel-verified on encounter in the next PROCEED round. Round 4Churn guard: PROCEED. Head 0a4509c (base 6ec45f7). CRF-32 addressed (comment added at both NewCounterVec definitions; author explained the family-valued label is intentional for phase-5 cardinality). Panel runs to verify CRF-32 plus the still-unverified round-3 fixes (CRF-24, 25, 26, 27, 28, 29, 30, 31) on encounter, since round 3 was BLOCKED with no panel. Law not re-run (effective additions +162 since R1). Reviewed against 6ec45f7..0a4509c. Round 4 panelNetero (no findings) + 12-persona panel. All round-3 fixes independently verified genuine and correct on encounter, including the CRF-26 P3 rewrite (known-family types bypass the cap; TestSessionCountsCapped proves it) and the CRF-25 test (now bites when the known-first sort is neutered). New findings: 1 Nit (CRF-35 sessionType/magicType split from the rename), 1 Note (CRF-34 family-label metric assertion only covers the identity case). No P0-P3. PR has converged; posted APPROVE (tool downgrades to COMMENT). No REQUEST_CHANGES to dismiss (all prior rounds were COMMENT). About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Phase 1 of the IDE-session-type normalization, and it is a disciplined one. The idemetadata leaf package gives the agent and server a single app-name vocabulary (pinned against drift by TestFamilyKeysAreCanonical and TestCodersdkUsageAppNamesAreCanonical), the read-query rewrites correctly split into fan-out-tolerant hash joins (DISTINCT/MAX) versus per-row LATERAL pre-aggregation (where connection_count is summed alongside), the batcher's JSONB side-buffer mirrors the established connections_by_proto pattern with a lock-protected positional zip, and the syncmap.LoadOrStore fix is a real correctness fix (the wrapper previously returned the zero value on store, which would have handed getOrCreateConnCounter a nil *atomic.Int64). The old-agent conversion path is the single, well-tested apex. As Killua put it: "The hot paths earn a shrug."
Severity count: 0 P0/P1, 1 P2, 4 P3, 11 Nit, 8 Note.
The P2 is a test gap on the exact code the PR calls its #1 risk: the batcher session-count flush runs green with no assertion on any session-count value. The P3s cluster around the newly-opened name space: an agent-side counter map with no bound (CRF-1), a reconnecting_pty key that the SSH path and the PTY server both write with last-writer-wins (CRF-3), a silent overflow fold with no operator signal (CRF-4), and a "surfaces unchanged" premise that is already false at the usage API and CLI in phase 1, not phase 5 (CRF-5). None are reachable by well-behaved first-party clients today, but this PR's whole point is that arbitrary names start flowing later, and there is no follow-up author to revisit them then.
One thing to flag directly: the author-agent's own review comment posted four findings above Nit (B1 Blocker, M2/M3/M4 Major) and none of them reproduce. B1 claims an INNER->LEFT JOIN change in GetWorkspaceAgentUsageStatsAndLabels, but the base query already used LEFT JOIN latest_agent_stats. M2's minute_buckets overcount cannot happen because the child PK (workspace_agent_stats_id, app_name) guarantees one row per app per stats row. M3's has_connection regression does not occur because the base query already filters to rows with a positive session count before MAX(connection_count), which the new INNER JOIN reproduces. M4's uncapped-fallback claim is wrong because SessionCountsFromProto returns capSessionCounts(counts) for both branches. The independent panel verified each against the base SHA. Worth calibrating the self-review's confidence before those land as blockers on a human.
Process note: the PR description points reviewers at migration 000543_* twice (the NOTE and the file-by-file table), but the shipped files are 000544_*; 000543 is an unrelated chat_status_remove_unused migration. A rebase bumped the number and the description was not updated. Please fix both references so a reviewer grepping by number lands on the right file.
coderd/workspacestats/batcher_internal_test.go:77
P2 [CRF-6] The batcher's session-count flush path executes under test but no session-count value is ever asserted. (Bisky)
DBBatcheris the only caller that runsSessionCountsFromProto, marshals thesessionCountsside-buffer, and depends on the positional zip inInsertWorkspaceAgentStats(the PR's stated #1 risk).randStatssets the deprecated fields, so the child insert does run, but every assertion inTestBatchStatsis a row count. Nothing reads a session count back.
The other suites do not close this: querier_test.go seeds through dbgen (bypasses the batcher) and cli/ssh_test.go/vscodessh_test.go assert against the workspacestatstest.StatsBatcher fake that never inserts or normalizes. A marshal/zip regression (empty slice, off-by-one, wrong buffer, mis-position) on the highest-risk code in the PR ships green. Make a deterministic second flush carry a known session-count map and assert the stored value back.
🤖
🤖 This review was automatically generated with Coder Agents.
|
CRF-6: the batcher test now flushes known session counts for two agents and asserts the stored values round-trip, covering normalization, zero-dropping, and the positional zip. |
daae7b5 to
d0be576
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2. All 23 round-1 findings are addressed, and the panel verified each fix against the tree rather than trusting the resolution. The standouts: CRF-6's batcher test now flushes distinct per-agent maps and asserts they round-trip normalized with zeros dropped (the PR's #1 stated risk finally has a real assertion), and the CRF-20 O(fleet) scan is fixed by construction, not just by comment. The new created_at column is copied positionally from the parent row on every write path (batcher, backfill, dbgen), so the added sc.created_at BRIN filters are redundant-for-correctness and only prune blocks; the windowed results stay byte-identical. Nice work turning a scale note into a structural fix.
Severity count this round: 0 P0/P1, 2 P3, 4 Note, 3 Nit (1 finding dropped on cross-check). Nothing blocks; using COMMENT.
The two P3s are two sides of one guarantee, that well-known session types survive the 64-entry cap. CRF-26 is the live one: the agent-side cap added for CRF-1 does not mirror the server-side cap it cites. The server sorts well-known names first and keeps them; the agent folds by arrival, so once 64 distinct types have ever been seen, a lazily-created ssh or vscode counter can be routed into unknown and its minutes drop out of every canonical read. Two reviewers rated it P3 and four more converged as Note; I verified the agent code lacks the known-first branch. CRF-25 is the same guarantee on the server side, where the code is correct but the preserve-well-known branch is untested (Bisky proved neutering the sort keeps the test green). Fixing both is cheap: exempt the five known-family names from the agent fold, and add a server test with 64 names that sort before a canonical name.
The BRIN design is the right call, but two honesty notes: the PR-body benchmark rows (98.9 ms rollup, 659 ms insights) predate the BRIN by the author's own admission, so they describe code that no longer ships and should be re-measured; and the steady-state prune benefit is unverified, with a plausible correlation-decay path under cascade-delete + vacuum churn (CRF-27). As Hisoka signed off: "When code fights back this well, I don't manufacture a rematch."
agent/agentssh/metrics.go:50
Note [CRF-32] The magic_type Prometheus label now carries the app family, not the raw session type, and nothing at the metric definition says so. (Leorio)
magicTypeMetricLabelwas rewritten [...] toreturn idemetadata.Family(...). Before this PR, a session type the agent didn't recognize landed undermagic_type="unknown"; now every alias folds into its family, soagent_sessions_total{magic_type="vscode"}will start counting Cursor, Windsurf, Positron, VSCodium, Kiro, Devin, and the rest the moment an SSH client sends one of those values.
The metric name stays byte-identical and the canonical four are unchanged in phase 1, so this is latent until phase 5. But the two NewCounterVec definitions declaring []string{"magic_type", "pty"} say nothing about the semantic shift, and an operator's existing dashboard query silently changes shape when family folding goes live. A one-line comment at the metric definitions noting the label is family-valued would prevent the 2 AM surprise.
🤖
🤖 This review was automatically generated with Coder Agents.
d0be576 to
51466be
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3: the fixes are strong. Eight of the nine round-2 findings are handled: the agent cap now protects known-family types and only folds genuinely-unknown overflow with a debug log (CRF-26), the server cap gained a real preservation test with 100 junk names sorting before ssh (CRF-25), the over-cap Warn moved to the normalized pre-cap count (CRF-24), the batcher round-trip test now asserts child.created_at == parent.created_at (CRF-28), the cap constant is unified in idemetadata (CRF-30), the field is renamed and pointer-typed (CRF-31), and CRF-27 was re-measured toggling the BRIN with the PR body updated to state the churn caveat honestly.
One finding is unaddressed with no response, so this round is blocked pending a reply rather than spending another full panel pass:
- CRF-32 (Note,
agent/agentssh/metrics.go:50): themagic_typePrometheus label now returnsidemetadata.Family(...), so it carries the app family rather than the raw session type.agent_sessions_total{magic_type="vscode"}will start aggregating Cursor, Windsurf, Positron, and the rest once phase 5 clients send those values, and an operator's existing dashboard query silently changes shape. The twoNewCounterVecdefinitions declaring[]string{"magic_type", "pty"}say nothing about the shift. This was folded into the round-2 review body (the line sits outside the diff hunks), so it is easy to have missed, which is likely why it went silent.
No code or ticket is strictly required for a Note: a one-line comment at the metric definitions noting the label is family-valued would close it, or reply explaining why it should not be changed (for example, that phase 2 will re-document the metric). A bare non-response is what blocks; any of fix, ticket, or reasoned decline unblocks. Once you respond, the panel will re-engage and verify the eight round-3 fixes on encounter (they are recorded as author-claimed, not yet panel-verified this round).
No inline comments this round; review is blocked on the item above.
🤖 This review was automatically generated with Coder Agents.
fe1ee77 to
0a4509c
Compare
|
CRF-32: the family-valued |
|
/coder-agents-review |
There was a problem hiding this comment.
Round 4, and the PR has converged. Round 3 was blocked on a single silent finding, so eight round-3 fixes had been recorded as author-claimed but never panel-verified. This round the panel verified every one of them against the code and the tests, and they are genuine, not paste. The load-bearing one, CRF-26, is correct: the agent cap now gates the fold behind idemetadata.Family(key) == AppNameUnknown, so lazily-created ssh/vscode counters always keep their own key and only genuinely-unknown types fold past the cap, with a debug log; TestSessionCountsCapped breaks if you drop the guard. CRF-25's server test finally bites (neuter the known-first sort and got["ssh"] drops to 0), CRF-24's over-cap Warn fires on the normalized pre-cap count with agent_id/reported/max, CRF-28's batcher round-trip asserts child.created_at == parent.created_at via a direct SQL join, the cap constant is unified in idemetadata.MaxSessionCountEntries, and CRF-32 (this round's change) documents the family-valued magic_type label at both metric definitions with the cardinality rationale. CRF-27's BRIN was re-measured toggling the index and the PR body now states the prune benefit plus the churn caveat honestly.
All 32 prior findings are resolved or acknowledged. Two minor, non-blocking items are new this round, neither requiring a change to ship:
As Bisky put it: "I went in expecting paste and found the real thing."
Approving. The two inline notes below are optional polish.
agent/agent_test.go:4310
Note [CRF-34] The family-valued magic_type label (this round's CRF-32 change) is only asserted end-to-end for the identity case where family equals the raw type. (Bisky)
magicTypeMetricLabelnow returnsidemetadata.Family(...), so a Cursor or Windsurf session should emitmagic_type="vscode". The only end-to-end metric assertion feeds ansshsession and checksmagic_type="ssh", which passes whether the label carries the family or the raw type; delete the family mapping and this test stays green.
Low value to add, since Family() is unit-tested in TestFamily and magicTypeMetricLabel is a one-line delegator, so the composed behavior is covered indirectly. Worth knowing, not worth blocking. If you touch this area for phase 2, a divergent-case assertion (cursor -> vscode) at the metric boundary would close it.
🤖
🤖 This review was automatically generated with Coder Agents.
|
CRF-34: added |
|
Planning note: template insights and non-canonical app names As of this PR, the rollup ( Decision: rather than adding an interim Residual gap (accepted): custom callers can already send arbitrary names through the workspace usage API / Open question for reviewers: if we can't guarantee phases 3 and 4 land in the same release window, is an interim 🤖 Comment by Coder Agents on behalf of @EhabY. |
33ae017 to
47be094
Compare
Replace the fixed session_count_* columns on workspace_agent_stats with a workspace_agent_session_counts child table keyed by app name, so any IDE can report session counts without schema changes. Agent API v2.11 adds a session_counts map to Stats, deprecating the fixed fields.
47be094 to
9707a7a
Compare
|
Closed in favor of #27952, the same branch squashed to a single commit and rebased on main, since this PR had become too crowded to review. |
1 participant
Adding a new IDE session type (Cursor, Windsurf, ...) currently requires changes to 15+ files across migrations, proto, server, SDK, and UI. This PR replaces the fixed
session_count_*columns on the ephemeralworkspace_agent_statstable with a normalizedworkspace_agent_session_countschild table keyed by app name, and makes the agent report session counts as a dynamic proto map. New session types then flow end-to-end (agent counters → stats pipeline → template usage rollup) with zero schema, proto, or server changes.Phase 1 of the Scalable Approach for Adding New IDE Session Types RFC. External surfaces are intentionally unchanged: all read queries keep their row shapes, so the deployment stats API, Prometheus gauge names, and telemetry snapshots are byte-identical. Later phases add the API map field and family-labeled metrics (2), migrate
connection_logsoff its type enum (3), mergetemplate_usage_stats*_minsintoapp_usage_mins(4), and version-gate the IDE clients (5).NOTE: The migration copies the ephemeral stats buffer (~1 day of rows, bounded by
dbpurge) into the child table, drops the foursession_count_*columns, and recreates the insights covering index without them. There is no reporting gap. The down migration restores the four canonical app names.Key changes:
workspace_agent_session_counts (workspace_agent_stats_id, created_at, app_name, count)withON DELETE CASCADEand no CHECK constraint.created_atis copied from the parent row and carries a BRIN index, so windowed reads prune the child table instead of scanning it. App names are normalized at ingestion byidemetadata.Normalize(): null bytes stripped, truncated to 64 runes, lowercased, and-folded to_(the fold keeps the SDK's legacyreconnecting-ptyspelling aggregating into the canonicalreconnecting_pty). Everything else is preserved, so arbitrary names flow through.Stats.session_countsmap (field 13); fields 8–11 are deprecated and converted server-side byworkspacestats.SessionCountsFromProto, so old agents keep working through the upgrade window.unknown(well-known names always kept, remainder chosen deterministically). This bounds child-table fan-out from malicious or buggy agents.sync.Map; JetBrains keeps its channel-watcher special case via family lookup; agent session metrics are labeled by family to keep cardinality bounded.coderd/idemetadataleaf package: canonical app names,Normalize(), and the family map (cursor/windsurf/... →vscode). Display names are added at the API/UI boundary in phase 2 from the same package.connections_by_proto), exploded into the child table within the same insert statement.coder ssh --usage-appaccept arbitrary app names;codersdk.AllowedAppNamesis removed.Behavior changes live as of this phase: the workspace usage API accepts and stores (normalized) unknown app names instead of returning 400,
coder ssh --usage-apppasses them through instead of coercing tossh, and unknown SSH session types are counted instead of silently dropped. Bundled IDE clients keep sending only the canonical names until the phase-5 gate flips.Benchmarks
Measured on PostgreSQL 13 with 1.73M stats rows (300 agents × 24h @ 15s) and 1.3M child rows; results byte-identical to the old queries in every case. The rollup and insights queries originally used per-row lateral probes and regressed 7.6×/7.7×; rewriting them as direct hash joins (the inclusion filter becomes the join, DISTINCT/MAX aggregates tolerate the fan-out) recovered most of it:
A covering
INCLUDE (count)index was evaluated and rejected: the hash-join plans never touch it, so it would be pure write overhead. Total storage at this scale dropped 697 MB → 666 MB, since the 40% of rows with no session activity now store nothing.BRIN validation (follow-up run at the same scale, different machine, so absolute numbers are not comparable with the table above): toggling the
created_atBRIN, the 1h rollup CTE reads only the window slice of the child table via a bitmap scan (~50k of 1.3M rows) instead of a full seq scan, 163 → 146 ms end-to-end; parent-side aggregation dominates at this scale, and the gap grows as the child table outgrows the window (e.g. the 180-day fresh-deployment buffer). The 24h insights window spans the entire ~1-day table, so its plan and cost are unchanged, and the deployment/Prometheus queries probe the child PK directly and never touch the BRIN. Prune selectivity degrades as vacuum recycles purged pages into new inserts; the floor is the pre-BRIN seq scan, bounded by the ~1-day purge.Risks and mitigations
created_atlets short-window reads prune the child table to their window (measured in the BRIN validation note above); if BRIN selectivity degrades with page churn, the floor is the full child scan, bounded by the ~1-day purge.Add()appends to both buffers unconditionally, so the lengths cannot diverge.unknownaggregation, and the ~1-day purge. Prometheus is unaffected (family labels).Implementation plan and decision log
Goal
Replace the four fixed
session_count_*columns on the ephemeralworkspace_agent_statstable with a normalized child table, make the agent report session counts dynamically via a proto map, and keep every external surface byte-identical (API JSON, Prometheus metric names, telemetry snapshot shape).Non-goals (later phases)
connection_logsENUM to TEXT migration, protoConnection.type_str(Phase 3)template_usage_stats*_minsmerge into JSONB +sftp_minsdrop (Phase 4)FeatureSetgate (Phase 5)Phase 1 invariant: bundled clients send only canonical app names (gated on the Phase 3 release). The usage API and CLI accept arbitrary names as of this phase, but nothing sends them by default, so rollout has no observable behavior change.
Design summary
WHERE session_count_<name> > 0per column, UNION ALL) → drop covering index → drop columns → recreate index with only theconnection_median_latency_msINCLUDE. Down migration re-adds columns, backfills the canonical four, restores the original index. Fixture seeds parent + child rows including a non-canonical name.map<string, int64> session_counts = 13; fields 8–11[deprecated = true], never reserved/removed. Documented as Agent API v2.11 intailnet/proto/version.go. New agents populate the map only.extractMagicSessionType()normalizes (null-strip, truncate, lowercase, hyphen fold) instead of classifying; empty staysssh. Dynamicsync.Mapcounters;SessionCounts()returnsmap[string]int64;JetbrainsChannelWatchertakes the counter from the same store;ReportConnectionand session metric labels map byidemetadata.Family().SessionCountsFromProtois the single old-agent conversion point (and applies normalization plus the 64-entry cap withunknownaggregation), used by the batcher, the reporter activity check, and (viaClearSessionCounts) theExperimentWorkspaceUsagezeroing.postWorkspaceUsagenormalizes and stores any app name, replacing the allowlist 400 and the per-app switch.connectionsByProto; parent + child inserted in one CTE statement (jsonb_array_elementszipped withunnest(@id),jsonb_each_textexplodes each map,> 0filter).LEFT JOIN LATERALper-stats-row pre-aggregation (probing only latest-row subsets); the rollup and insights-by-template CTEs use a direct child join because their aggregates are DISTINCT/MAX (fan-out tolerant) and their inclusion filter is the join itself. Row shapes and output aliases preserved (load-bearing for positional struct conversions in metricscache/prometheusmetrics).UpsertTemplateUsageStatsstill writes the dedicated*_minscolumns until Phase 4.DeleteOldWorkspaceAgentStatsunchanged (cascade).dbgen.WorkspaceAgentStatseeds counts via a variadic map.Decisions (resolved during planning and implementation review)
coderd/idemetadata(importable fromagent/, no heavy deps). Promote to codersdk only if external consumers ever need it.codersdk.AllowedAppNames: removed outright; it only fed the 400 path.UsageAppNameconstants stay.-folded to_at ingestion. The hyphen fold exists to keep the SDK's legacyreconnecting-ptyspelling aggregating intoreconnecting_pty; lowercasing keepsCursor/cursoras one row. No regex sanitizer and no CHECK constraints, per the RFC review thread.unknown, keeping totals accurate while bounding row fan-out.File-by-file
coderd/database/migrations/000545_*.{up,down}.sql+ fixturequeries/workspaceagentstats.sql,queries/insights.sqlqueries.sql.go,models.go,querier.go,dbmock,dbmetrics,dump.sql,agent.pb.gomake genagent/proto/agent.proto,tailnet/proto/version.goagent/agentssh/{agentssh,metrics}.go,agent/agent.gocoderd/idemetadata(new)Normalize(), family mapcoderd/agentapi/stats.go,coderd/workspacestats/{batcher,reporter,sessioncounts}.go,coderd/workspaces.gocodersdk/workspaces.go,cli/ssh.gocoderd/database/dbgen/dbgen.go🤖 This PR was generated by Coder Agents on behalf of @EhabY.