Fix: Plot a scoped agent's own latency on the usage pane - #1296
Conversation
Under an agent scope the usage pane's latency metric always said "latency is not available per agent" (rossoctl#1294). The pane narrows the group=agent series client-side, and that narrowing zeroes latency: a bucket's mean covers every agent that shared it. Since a704825 the server keeps a ring per recognised agent and answers agent=<name> from it, latency included. The pane now makes that read too, when scoped to one agent with no session, and copies only the latency onto its narrowed buckets, matched by bucket time. Counts, cost, the whole-window residual note and the window's units still come from the narrowing, since the agent= answer carries neither of the last two. The latency is not taken from a server that ignored agent= (it answered for every agent), nor from one that applied it by narrowing (v0.8.x: requests, no samples). There, and under Other or within a session, the pane still says latency is unavailable, and now names which. Fixes rossoctl#1294 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 6 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review follow-up on rossoctl#1296. - The two reads must cover the same buckets. The server's fold groups minutes from the window's oldest, so at 1h and 6h every bucket's time moves each minute, and a minute turning between the reads left no bucket time in common: the graft copied nothing, still reported success, and the pane said "no latency samples in this window" until the next poll. The reads are now compared bucket for bucket; a mismatch reads both again once, and is reported as a latency error if the window moves again. - A failed latency read no longer takes down the pane. It travels apart from the window read's error and is shown only on the latency metric, as a failed read rather than as latency the proxy does not keep. Each read has its own 5s timeout. - README: the server keeps per-agent latency only for a recognised agent across all sessions; "both say so" no longer fits. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ed Snible <snible@us.ibm.com>
esnible
left a comment
esnible
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at e6e8adc. The new commit fixes all three issues from the first pass:
- Window moving between the two reads:
graftAgentLatencynow requires both reads to cover identical bucket times. When they don't, it reportserrUsageWindowMoved, andfetchUsagereads both again, once. - A failed latency read blanking the pane: the failure now goes on
latencyErrand shows only on the latency metric, and each read gets its ownusageReadTimeout. - README: the paragraph now gives the right reason.
What I checked:
- The
cmd/agentop/tuisuite passes locally, including the three new tests.go vetandgofmt -lare clean. - Making
sameBucketTimescompare only lengths fails both new reread tests, so they guard the fix. - The per-agent rings are allocated at the same
NumBucketswith the same window clamp, and an absent ring still emits zeroed buckets at the right times. So the strict match fails only when a minute really did turn.
One nit inline; otherwise this is ready.
Author: esnible (MEMBER — maintainer)
Areas reviewed: Go (agentop TUI, core/cost/usage), Docs
Agent/IDE config (.claude/.vscode): none
Commits: 2 commits, all signed-off: yes
CI status: pending on e6e8adc at review time
| // them not. The server's fold groups minutes from the window's oldest, so at the 1h and 6h | ||
| // resolutions every bucket's time moves each minute and none would line up. That is |
There was a problem hiding this comment.
nit: 1h and 6h are the windows; their resolutions are 5m and 30m. Maybe "so at the 1h and 6h windows every bucket's time moves each minute".
rubambiza
added
the
ready-for-ai-review
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Grafts the scoped agent's own latency onto the narrowed usage buckets from a second agent= read, refusing answers that are not the agent's own and rereading both once when the window moves between the reads; a failed latency read stays off the other metrics. The graft, both refusals, the reread paths, and the unavailable-note wordings are all covered by tests — one nit inline, nothing blocking.
- nit —
cmd/agentop/tui/usage_pane.go:244: "at the 1h and 6h resolutions" — 1h and 6h are the windows; their resolutions are 5m and 30m.
Reviewed by clawgenti using the github-pr-review skill
| // come from. | ||
| // | ||
| // THE TWO READS MUST COVER THE SAME BUCKETS, and a minute turning between them is enough to make | ||
| // them not. The server's fold groups minutes from the window's oldest, so at the 1h and 6h |
There was a problem hiding this comment.
nit: "at the 1h and 6h resolutions" — 1h and 6h are the windows; their resolutions are 5m and 30m. Perhaps "so at the 1h and 6h windows every bucket's time moves each minute", as the PR description has it.
mrsabath
left a comment
mrsabath
left a comment
There was a problem hiding this comment.
A real fix for #1294: a scoped agent's latency was reported unavailable because ScopeToAgent zeroes it, when the server does keep per-agent response times in its own ring. The fix reads them from a second agent= call and grafts only the latency.
The CI picture, because the check list looks alarming
Eight checks show fail with 19m–64m durations. None is a real failure. Every one is cancelled with an empty steps array, an empty runner_name, and started_at == created_at, and the run annotation reads "The job was not acquired by Runner of type hosted even after multiple attempts". GitHub never assigned a hosted runner — the long durations are pure queue wait, then reaping. The incident hit both ubuntu-latest and macos-latest across runs 37366607521 and 37366608488, and it spread to three more jobs while I was reviewing (Go CI (authbridge cortex), Shell Script Lint, proxy-init iptables rules were pending at the start and have since been reaped the same way).
What matters: the two jobs that cover this PR's code both passed — Go CI (authbridge agentop) (2m35s) and Go CI (core) (1m19s) — as did every job that got a runner at all (both CodeQL, Trivy, Bandit, Dependency Review, Action Pinning). This PR touches only cmd/agentop/** and core/cost/usage/scope.go; nothing it changes is in scope for Hadolint, YAML Lint, Shell Script Lint or proxy-init in the first place. gh run rerun --failed on both run IDs should clear it.
What I verified
The echo guard is load-bearing, and the whole chain checks out. graftAgentLatency refuses when own.Agent != scope. That is not defensive boilerplate: GetUsageWindowForAgent's own doc notes that a server predating agent= ignores it and leaves Snapshot.Agent empty, so without the check an old proxy would hand back every agent's latency to be plotted under one agent's name — exactly the failure the narrowing exists to prevent. On the server side /v1/usage?agent= routes through sessionapi.ringSnapshot → usage.AgentSnapshot (scope.go:179), and all three producers (AgentSnapshot, NarrowToAgent, the ledger path) set Snapshot.Agent. So the guard passes on every real path and fails only in the old-server case it was written for.
sameBucketTimes is correct for a subtle reason. It compares with .Equal rather than == because two times decoded from the same non-local offset carry distinct *time.Location pointers and compare unequal under == however equal the instants are. The fixture deliberately writes one instant two ways (2026-09-29T10:00:00Z against 2026-09-29T06:00:00-04:00) to prove the graft matches on the instant rather than the representation. Weakening sameBucketTimes to compare only lengths fails both reread tests, which is the right negative control.
The reread is bounded and correctly prioritised. On errUsageWindowMoved it rereads both once, and keeps the second pass only if again.err == nil — so a latency problem cannot take down the counts and cost the first read already answered. Each read gets its own usageReadTimeout, so a slow window read cannot spend the latency read's budget.
The three unavailable-reason branches earn their keep. Separating Other / within-a-session / old-proxy matters because the remedies differ — the first two are views to leave, the third is a proxy to upgrade — and a failed read gets its own line rather than being miscategorised as "not available per agent". Eight new tests cover the graft, both refusals, both reread paths, each wording, and the failure isolation. No t.Skip, no secrets, no new dependencies.
One nit inline, the same one @clawgenti and your own review already flagged.
A correction to my own process, for the record: I initially suspected the new AgentSnapshot reference in scope.go's comment was stale, because my first fetch of that file did not surface the symbol. It exists, fifteen lines below the comment that names it. The reference is accurate.
Areas reviewed: Go (agentop TUI, core/cost/usage), docs, tests, CI failure triage, security
Commits: 2, both signed off, imperative subjects at 56/72 chars
CI: 8 checks failed to hosted-runner starvation with zero steps executed; the agentop and core Go jobs covering this code passed
One disclosure on method: I did not run the suite — no clone of this repo on my side — so the execution evidence is the passing Go CI (authbridge agentop) job plus your reported local run. I verified the logic, the client/server agent= chain, and the CI failure classification directly against source and the Actions API at e6e8adc.
Nothing blocking. LGTM.
| // come from. | ||
| // | ||
| // THE TWO READS MUST COVER THE SAME BUCKETS, and a minute turning between them is enough to make | ||
| // them not. The server's fold groups minutes from the window's oldest, so at the 1h and 6h |
There was a problem hiding this comment.
Nit, and not an original one — @clawgenti flagged it and so did your own review, so this is just the third vote for fixing it while you are in here.
"at the 1h and 6h resolutions": 1h and 6h are windows, not resolutions. usageState.window() returns (window, resolution) as two distinct values from the usageWindows table, so the two are explicitly different concepts in this very file.
The reasoning the sentence carries is right and worth keeping — it is the justification for why a strict bucket-time match is necessary rather than paranoid, since at those windows the fold regroups from the window's oldest minute and every bucket time shifts each minute. Only the noun is wrong. "at the 1h and 6h windows" would do it.
4 participants
Fixes #1294
Problem
When one agent is selected (
A), the usage pane's latency metric always shows "latency is not available per agent". The pane narrows thegroup=agentseries client-side, and that narrowing (narrowBucketsToAgent) zeroes latency, because a bucket's mean covers every agent that shared it.That stopped being necessary with a704825. The aggregator now keeps a ring per recognised agent, and
AgentSnapshotanswersagent=<name>(with no session) from that ring, latency included. The spend drawer already usesagent=; the usage pane was never switched over.Change
fetchUsagealso asks/v1/usage?agent=<scope>andgraftAgentLatencycopieslatMeanMs/latStdDevMs/latSamplesonto the narrowed buckets.foldgroups minutes from the window's oldest, so at the 1h and 6h windows every bucket's time moves each minute, and a minute turning between the reads leaves none in common. A mismatch reads both again once; if the window moves again, it is reported as a latency error.agent=outright: that answer carries no whole-windowungroupedCostMicrosand no windowcurrencies, which the scoped "attributed to no agent" note (costUngroupedRow) needs. So counts, cost and that note stay on the narrowing, unchanged.agentecho means a server that ignored the parameter and answered for every agent. Grafting it would draw the whole window's response times under one agent's name.agent=by narrowing.core/cost/usage/scope.goandcmd/agentop/README.md.[b]under a scope is unchanged: the per-agent ring could honourgroup, but that is a separate feature.Testing
cmd/agentop/tui/usage_agent_latency_test.gocovers:agent=and for one that narrowed;agent=read for Other or within a session;LATENCYsummary cell;TestFetchUsage_ScopedAsksForTheAgentAxisnow checks the narrowing read's query, since the latency read now comes after it.go vet,gofmt -l,cmd/agentop/...andcore/cost/usageall pass.TestRunExec_BeforeFirstStartRunsAndSaysWhatIsLostfails locally only whenSSL_CERT_FILEis set in the shell, and passes with it unset. It is unrelated to this change.fetchUsageagainst a live laptop proxy:claude-code:agentLatency=trueat all three windows (10m, 1h, 6h).Assisted-By: Claude (Anthropic AI) noreply@anthropic.com