feat: add Prometheus metrics for AI Governance cost control - #27490
Conversation
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
f9a901e to
1c7af26
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 15 findings (2 P2, 6 P3, 4 Nit, 3 Note), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none yet) Round logRound 2Churn guard PROCEED: CRF-1 author fixed (350c639 added the four cost_control series to scripts/metricsdocgen/metrics and docs/admin/integrations/prometheus.md), CRF-2 deferred to AIGOV-527 (no code change, thread resolved). Author also filed AIGOV-561 for the broader ai-gateway metricsdocgen gap. Netero round-2 found new P3 CRF-3 (collector cold-start blind window). P3-only, mechanical floor clean, panel runs (first panel round). Reviewed against 60c20be..350c639. Panel: 17 reviewers (5 always-on, Gon+Leorio once-per-PR, 8 specialists, Meruem+Kite wildcards). New findings CRF-4..CRF-15. Cross-check resolutions verified against code:
Round 1Netero-only first pass. 1 P2, 1 P3. Reviewed against 6f2011a..1c7af26. Netero decision gate: P2 present, panel deferred until mechanical findings addressed. Orchestrator verified both findings against head: metrics.go is in scanner skipPaths and the four cost_control series appear in neither scripts/metricsdocgen/metrics (102 existing coder_ai_gateway lines, 0 cost_control) nor docs/admin/integrations/prometheus.md; aicostcontrol.sql:252-300 confirmed the inline user_highest_group+effective CTE duplication under TODO(AIGOV-527). 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.
First-pass review only. These are mechanical findings from Netero (conventions, coverage, dead code, duplication, build). The full review panel has not yet reviewed this PR; it will review after these findings are addressed. Netero traced callers of every new symbol and ran the new unit tests.
What the change does well: metric recording is nil-safe at every call site, cardinality is reasoned about per collector, and test density is high (60.7%) with real assertions against a live Prometheus registry and real SQL against a real DB rather than tautological checks. The dbauthz AsSystemRestricted use is justified and scoped, and query naming follows the PerX aggregation convention.
Severity count: 1 P2, 1 P3.
The P2 is a documentation gap that the code's own skip-list comment predicts: a governance/cost-control feature should not ship metrics operators cannot discover in the generated Prometheus docs. The P3 is a drift trap already tracked by TODO(AIGOV-527); it is not blocking but no test pins the query to the Go resolver, so a second budget policy would silently diverge the gauge from enforcement.
In Netero's words: "Their metrics must be maintained in the static metrics file instead" is the skip-list's own instruction, and this PR added the skip without the maintenance.
🤖 This review was automatically generated with Coder Agents.
| // eliminate the need for this skip list. | ||
| var skipPaths = []string{ | ||
| "coderd/aibridged/metrics.go", | ||
| "coderd/aibridgedserver/metrics.go", |
There was a problem hiding this comment.
P2 [CRF-1] Adding coderd/aibridgedserver/metrics.go to skipPaths excludes its four new metrics from scanning, but they were never added to the static metrics file, so they will be absent from the generated Prometheus docs. (Netero)
The skipPaths comment states: "Their metrics must be maintained in the static metrics file instead." The static file scripts/metricsdocgen/metrics holds every other coder_ai_gateway_ series (102 existing lines; 0 cost_control), and docs/admin/integrations/prometheus.md also has 0. The PR touches no file under docs/ or scripts/metricsdocgen/metrics. The four new series (coder_ai_gateway_cost_control_blocked_requests_total, _blocked_users, _unpriced_requests_total, _enforcement_duration_seconds) end up undocumented for a feature whose whole point is operator-facing observability. Fix: add the four series to scripts/metricsdocgen/metrics and regenerate prometheus.md.
🤖
There was a problem hiding this comment.
This is actually a problem with the current ai-gateway metrics not being properly documented. Created this follow-up issue: https://linear.app/codercom/issue/AIGOV-561/add-ai-gateway-metrics-to-metricsdocgen
There was a problem hiding this comment.
Verified fixed in 350c639. All four coder_ai_gateway_cost_control_* series are present in scripts/metricsdocgen/metrics and docs/admin/integrations/prometheus.md, and coderd/aibridgedserver/metrics.go is in the scanner skipPaths, matching the hand-maintenance requirement. Thanks for filing AIGOV-561 for the broader ai-gateway doc gap.
🤖
1c7af26 to
350c639
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
First panel review (round 2). Round 1's mechanical findings are resolved: CRF-1 (metrics missing from generated docs) is verified fixed in 350c639 (all four cost_control series now in scripts/metricsdocgen/metrics and docs/admin/integrations/prometheus.md, with the follow-up AIGOV-561 filed for the broader ai-gateway doc gap), and CRF-2 (effective-group resolution duplication) is deferred to AIGOV-527. Nice.
What this does well: the gauge query is a faithful twin of the enforcement path (same >= threshold, override-wins precedence, deleted-org filter, and period-window normalization), so blocked_users and blocked_requests_total count the same population enforcement blocks; recording is nil-safe at every site; and the tests assert real collected values against a live registry, including Reset clearing stale series across rounds. Knuckle put it well: "This mirroring is the hard, boring kind of correct."
Severity count: 1 P2, 5 P3, 4 Nit, 3 Note. No P0/P1 after verification, so this is a COMMENT, not a request for changes.
One transparency note on the panel: Killua raised a P1 that the client-controlled model label on unpriced_requests_total could OOM the control plane. I investigated and downgraded it to a Note (CRF-7). The increment only fires after RecordTokenUsage, which the aibridge library calls only once it has parsed a usage block from an accepted provider response, so arbitrary/garbage model strings are rejected upstream and never mint series; and the model label is the established aibridge convention (baseLabels on 8 sibling metrics, InterceptionCount already labels even failed interceptions by model), so this counter is a strict subset that adds no new cardinality class. The real, in-scope points survive as CRF-7: the counter accrues series even when Prometheus is disabled (only the collector is gated), the "up to 15" comment understates the model domain, and provider alone would answer this counter's question.
The P2 (CRF-6) is worth deciding now because a metric name is immutable API: unpriced_requests_total counts token-usage records, not requests, and streaming inflates it by a variable multiplier. The P3s cluster on two real seams: the blocked_requests_total test never checks the group_id label this PR exists to add (CRF-4), and the blocked_users gauge is refreshed with a non-atomic Reset()+Set() loop that a scrape can photograph mid-rebuild (CRF-5). Ryosuke and Meruem both note a single refactor (a pull-based custom Collector) would resolve CRF-5 and the cold-start blind window (CRF-3) together.
For the record, two reviewer premises did not survive verification and were dropped: the "operator-chosen provider" cardinality angle (intc.Provider is p.Type(), bounded to anthropic/openai/copilot, so the help text is accurate).
🤖 This review was automatically generated with Coder Agents.
|
|
||
| // Reset clears groups that dropped to zero since the last cycle so their | ||
| // stale series do not linger. | ||
| m.BlockedUsers.Reset() |
There was a problem hiding this comment.
P3 [CRF-5] updateBlockedUsers calls BlockedUsers.Reset() then repopulates in a loop, which is not atomic against a concurrent Prometheus scrape. (Takumi P3, Komugi P3, Ryosuke P3; Hisoka/Knuckle/Kite Note)
MetricVec.Reset() and each WithLabelValues().Set() take the vec lock independently, while Collect takes only its own RLock. A scrape landing after Reset() but before the loop finishes snapshots zero series or a partial subset, so blocked_users reads absent/short for groups that are still over budget. The window is microseconds against a 5m refresh and ~15s scrapes, and it self-heals next scrape, which is why this is P3 and not higher, but it produces spurious drop-to-zero samples on a governance gauge and can flap instantaneous alerts. Ryosuke's fix, which Meruem seconds: turn BlockedUsers into a custom prometheus.Collector backed by a snapshot updated under a mutex; Collect emits it in one pass, so no scrape can observe a partial state. That single restructuring also removes the cold-start window (CRF-3) and the manual Reset() bookkeeping.
🤖
There was a problem hiding this comment.
The gap between the Reset() and repopulating the gauge is only microseconds against a 5m refresh, and the next scrape corrects it, so I'd rather keep the simpler code for now. If we find a scrape landing in that window is producing wrong blocked_users values that trip operator alerts, we can switch to a custom Collector that builds the values under a lock and emits them in one pass.
| Name: "blocked_users", | ||
| Help: "The number of users currently over their AI budget.", | ||
| }, []string{"group_id"}), | ||
| // Pessimistic cardinality: 3 providers, 5 models = up to 15. |
There was a problem hiding this comment.
Note [CRF-7] The model label on unpriced_requests_total ranges over the client-influenced unpriced tail, and the "Pessimistic cardinality: 3 providers, 5 models = up to 15" comment understates it. (Killua P1 downgraded, Mafu-san P2, Chopper P3; Kurapika/Hisoka/Pariston/Mafuuu Note)
The label fires precisely on the sql.ErrNoRows branch, i.e. models not in the price table: dated snapshots, preview models, and fine-tune IDs. Each distinct value mints a permanent series (counter, no Reset, no eviction), and the counter is created whenever BridgeConfig.Enabled, so series accrue even when Prometheus.Enable is false (only the collector is gated). Why this is a Note and not the P1 Killua first rated it: the increment requires a RecordTokenUsage call, which the aibridge library issues only after parsing a usage block from an accepted provider response, so garbage model strings are rejected upstream and cannot be minted at will; and the model label is the established aibridge convention (baseLabels on 8 sibling metrics, and InterceptionCount already labels even failed interceptions by model), so this counter is a strict subset that adds no new worst-case cardinality. Two actionable options if you want a hard bound here specifically: drop the model label (provider alone answers "are we failing to price a provider's traffic") or bucket unknowns into a single value. At minimum, fix the comment so it does not tell the next maintainer the label is bounded to 5.
🤖
There was a problem hiding this comment.
model ranges over unpriced models (unknown to the price table), so cardinality is unbounded in principle but gated by accepted provider responses. This is consistent with the model label on other aigateway metrics.
350c639 to
e93e806
Compare
| Help: "The number of AI requests blocked because the initiator's budget was exceeded.", | ||
| }, []string{"group_id"}), | ||
| // Pessimistic cardinality: one series per group with an over-budget user. | ||
| BlockedUsers: promauto.With(reg).NewGaugeVec(prometheus.GaugeOpts{ |
There was a problem hiding this comment.
This metric is included in the RFC (link), but populating it requires a potentially heavy SQL query since this is deployment-wide. Its cost scales with the number of users who have a budget (so with the member size of budgeted groups), and with longer budget periods since we scan more daily-spend rows. Since it's a gauge, we have to refresh it on a schedule (currently every 5 minutes in a goroutine).
So I think this comes down to: is this metric useful enough to operators to justify that recurring cost?
There was a problem hiding this comment.
If it's just called every five minutes then I think it's probably fine.
e93e806 to
35e2dd2
Compare
2 participants
Description
Adds Prometheus metrics for AI budget cost control, emitted by the aibridged server under the
cost_controlsubsystem (full names are prefixedcoder_ai_gateway_).blocked_requests_total(counter) — labels:group_idblocked_users(gauge) — labels:group_idunpriced_requests_total(counter) — labels:provider,modelenforcement_duration_seconds(histogram) — labels:outcomeChanges
GetOverBudgetUsersPerGroupquery (plus dbauthz/dbmetrics/dbmock wiring) to count over-budget users per effective group.blocked_usersgauge on an interval, started only when Prometheus is enabled.Metricsthrough the aibridged server, coderd API,cli/server.go, and the enterprise AI gateway handler; recording is nil-safe when metrics are unset.Closes https://linear.app/codercom/issue/AIGOV-296/add-prometheus-metrics-for-cost-control
Note
Initially generated by Claude Opus 4.7, modified and reviewed by @ssncferreira