feat(desktop): add session-wide token and time breakdowns to the trace panel - #4386
feat(desktop): add session-wide token and time breakdowns to the trace panel#4386bowenliang123 wants to merge 2 commits into
Conversation
0933512 to
76a820b
Compare
e641770 to
7c10029
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 7c1002982d73bfc8d0363da5c4ceb60649ace516. The change adds session-wide token and recorded-time rings to the Desktop trace panel, extends usage summaries with model-call duration and tool totals, and carries those fields through Storage and the Runtime Host protocol. I found one compatibility blocker and two summary-correctness issues in the inline comments.
Validation: clean npm ci; npm run build:test; full typecheck, lint, format, ASF-header and diff checks; Core 770/770, Storage 1077 passed / 10 skipped, and focused usage/inspector tests 47/47. The single Runtime Host sandbox failure and eight Desktop OAuth cancellations reproduced unchanged on the exact base. A semantic merge onto current main 8b0e43f that preserved main's epoch 83 and assigned this change epoch 84 passed clean install, build, full typecheck, and the focused tests. I also inspected the attached rendered panel screenshot. I did not independently exercise the live Electron hover interaction or non-Linux rendering.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
d83c87e to
61f9853
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 61f985305a322d53ec01f1f1fa62b2a0b321f858. The change adds session-wide token and recorded-time rings to the Desktop trace panel, extends usage summaries with model-call duration and tool totals, and carries those fields through Storage and the Runtime Host protocol. The previous tool-refresh and provider/model/status filtering findings are fixed; I found one current-main protocol blocker and one remaining summary-filter correctness issue in the inline comments.
Validation: clean npm ci; npm run build:test; full typecheck; Core 764/764; focused usage/inspector tests 64/64; changed-file lint/format, ASF headers, renderer architecture, Astryx inventory, and git diff --check. The full Storage suite had one unchanged child-process test fail because Node 22.22.1 wrote its experimental SQLite warning to stderr; that file passed 5/5 with warnings disabled. The sole Runtime Host failure was the unchanged live-sandbox environment test; Desktop completed 1912 passes with 8 existing timing cancellations. Hosted test is green. I did not independently exercise the live Electron hover interaction or non-Linux rendering.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 92 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 93 as const; |
There was a problem hiding this comment.
[P1] Allocate a fresh compatibility epoch after rebasing
Current main (6c632b1) already assigns epoch 93 to the credential-transfer boundary, while this head assigns the same value to the new usage-summary shape. If conflict resolution keeps 93, a Client and Host implementing different closed wire contracts advertise the same epoch and pass the handshake even though their protocol meanings differ. Preserve main’s epoch-93 history and assign this change the next unused epoch when rebasing (currently 94).
There was a problem hiding this comment.
Fixed in 151b145: rebased onto current main and renumbered this change to epoch 94, keeping main's epoch-93 (credential-transfer boundary) history intact. The protocol-epoch guard passes locally.
| if (query.status !== undefined && query.status !== 'all' && row.status !== query.status) { | ||
| continue; | ||
| } | ||
| if (query.providerId !== undefined && row.providerId !== query.providerId) continue; |
There was a problem hiding this comment.
[P2] Keep tool totals scoped by connectionSlug
usage.query summaries accept connectionSlug, and the model side filters by it, but this loop has no corresponding check because ToolInvocationRecord does not persist the connection identity. I inserted one tool record and queried two different slugs; both returned { requests: 1, durationMs: 125 }. A connection-filtered summary therefore combines one connection’s LLM totals with tool totals that were not filtered by connection. Persist and propagate the exact connection slug on tool invocations, or omit/mark toolUsage unavailable when that filter cannot be answered, and add a regression with two slugs sharing a provider/model.
There was a problem hiding this comment.
Fixed in 5720559: ToolInvocationRecord now persists the exact connection slug — the persisted-row decoder admits it as an optional string, so rows written before it existed keep loading — the tool runtime stamps it from the executing connection, and both toolSummary and the connection-filtered tool buckets apply it. Added the regression: tool summary scopes tool totals by connectionSlug seeds two slugs sharing a provider and model (only the connection field can tell the rows apart) and asserts each filtered summary answers with its own row while the unfiltered summary keeps both.
…e panel The Inspector overview answered "what did this session cost" with two figures — estimated cost and cache-hit rate — while token totals and elapsed time lived only in per-turn rows. The panel now opens with two donut charts: metered tokens split the way a bill reads (cache read / uncached input / output incl. reasoning) and recorded time split between LLM calls and tool executions. Each ring is linked to its legend in both directions — hovering either side holds that segment and dims the rest — with a hairline minimum sweep so tiny nonzero shares stay visible and a thin-line floor under the hovered segment. The token split reads the existing Session usage summary: uncached input is the `input − cacheRead` residual (providers that report only the cached share leave the ledger miss at zero), floored by the ledger miss when no prompt total was reported. The time split needs new aggregates: `UsageSummaryV2` gains optional `totalDurationMs` (summed attempt latency from both the legacy store and the canonical ledger) and `toolUsage` (session-scoped totals from a new `TelemetryRepo.toolSummary` over the tool-invocation ledger). Both ride the Runtime Host usage protocol as optional keys, so hosts that predate them keep decoding and the time ring is omitted rather than drawn as zeros. Generated-by: ZCode
61f9853 to
5720559
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I checked the rendered rings myself and they look right, so this pass is about the data underneath them. The shape is good: the two totals ride on the summary that already carries every other per-Session total, recordToolInvocation now goes through the same admission path as model usage, and the merge of totalDurationMs across legacy and canonical rows cannot double count (the two sets are disjoint, same premise as totalRequests). Core, storage and runtime-host tests pass locally on the head.
On the epoch: bumping is right, not a compatible-change declaration. Old clients decode summaries with requireExactRecord, so a new host sending totalDurationMs breaks their usage panel outright. That is a wire-shape change. It just has to be 94, with main's 93 entry kept: I built the merge result locally and the guard exits 1 on 93.
Two things I would treat as blocking, both small:
- The token ring drops cache reads exactly in the case the body says it handles. Core keeps the provider's
cacheReadunclamped when no prompt total was reported (model-call-usage-projection.ts:135), socacheRead > inputis a normal aggregate there. The model then doesMath.min(cacheRead, input), which turns 200k cached tokens into 0 wheninputis 0, and under-counts in mixed sessions. The third test usesinput=0but alsocacheRead=0, so it never sees this. TakecacheReadfrom the ledger as is;totalis already defined as the sum of drawn segments, so nothing needs the clamp. toolSummaryignoresquery.connectionSlugwhile the model side filters on it, so a slug-scoped summary shows two numbers that disagree. I would not persist the slug: it only helps new rows and makes history silently vanish under that filter. The optional field already means unknown, so omittoolUsagewhenconnectionSlugis set. One line in the coordinator.
Worth fixing in the same round:
readToolRowsisSELECT record_json FROM usage_tool_invocationswith noWHERE, decoded in full on every summary query, and usage tables have no retention. Now that every tool call publishes a usage change, that is a full-table decode per refresh on a table that only grows. Atsrange on the query uses the index that is already there.- Tool rows now have two filter policies:
filteredToolRows(range, tool, status) and the inline loop intoolSummary(plus session, provider, model). The test comment says they follow the same contract; they do not. MakefilteredToolRowstake the fullUsageQueryand reducetoolSummaryover it. That also fixes tool buckets not honouringsessionId, which is older than this PR. - The time ring adds up intervals that can overlap (parallel tool settlements, nested Code Mode calls whose outer duration already covers the inner ones) and labels the result "Active Time". Either call it recorded time or say in the model that it is a sum of durations, not wall clock.
durationMs: usage.totalDurationMs ?? 0thenfilter(s => s.durationMs > 0 || (tool && count > 0))makes a host that reports zero model time hide the model row and its call count, while the tool row keeps itself alive on count. Decide presence ontotalDurationMs !== undefinedbefore the filter.
Once the epoch is bumped, the "a host from before these fields" argument in the decoder and its test is unreachable: a 94 client cannot handshake with a 93 host. totalDurationMs can be a required key (the repo already writes it unconditionally), which removes the spread and the ?? 0 in the merge. toolUsage stays optional, but for the slug reason above, not the old-host one.
Smaller, inline or take-or-leave: the ring section is not rendered by any test although session-inspector-composition.test.ts already has the renderToStaticMarkup seam and the first test asserts total === sum(segments), which is the implementation restated; the new --_usage-* palette repeats three composition-band hues byte for byte under different meanings in the same panel; the token- swatch prefix is unnecessary since kinds are unique across both rings; two CSS comments describe the conic-gradient and mask version that was replaced; formatDuration has no hours, so a long session reads 187m0s; the second redistribution round in usageRingArcs cannot run.
One sentence for the body: why the two totals are new summary fields rather than derived from the buckets query on the client (the buckets already merge legacy and canonical with request-weighted latency). I agree with the choice, since summary is the closest seam and the value is exact, but the epoch bump rejects every older client at handshake, and that cost deserves the sentence.
Evidence boundary: static read of 61f98530 against main cdb29399, affected test:dist files green in core, storage, runtime-host and desktop main; the cache-read drop reproduced by calling deriveInspectorOverviewModel directly; overlap in the time ring read from tool-runtime.ts, not reproduced with a real trace.
AI-assisted review: drafted with Maka; I verified the epoch guard result, the projection clamp, the connectionSlug path and the SQL myself.
简体中文
视觉我已经确认过,这轮只看数据。结构认可,epoch 该 bump(老 client 用 requireExactRecord,新字段会让它直接解码失败),但要改成 94 并保留 main 的 93。两条合并前要修:token 环的 Math.min(cacheRead, input) 在「只报 cache 份额、没报 prompt total」时把缓存读取整段抹掉;toolSummary 不认 connectionSlug,建议带 slug 时省略 toolUsage 而不是持久化 slug。同轮建议:readToolRows 加 ts 范围 WHERE;tool 行统一到一个过滤入口;时间环是重叠时长之和,别叫 Active Time;?? 0 抹掉了 undefined 与 0 的区别。epoch bump 后 totalDurationMs 可改必填。正文补一句为什么不从 buckets 派生。
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 93 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 94 as const; |
There was a problem hiding this comment.
main is at 93 for the credential-transfer boundary. This needs to be 94 with the 93 line kept below; on a locally built merge result keeping 93 the guard exits 1.
There was a problem hiding this comment.
Already true at head 151b14538 — the rebase resolution assigns 94 and keeps main's 93 entry below it; the guard passes. This comment landed against the pre-rebase head.
| if (input + output <= 0) return undefined; | ||
| const uncachedInput = Math.max(input - cacheRead, cacheMiss, 0); | ||
| const segments = [ | ||
| { kind: 'cacheRead' as const, tokens: Math.min(cacheRead, input) }, |
There was a problem hiding this comment.
Core keeps cacheRead unclamped when the provider reported no prompt total, so cacheRead > input is a normal aggregate. With input=0, cacheRead=200000, cacheMiss=60000 this segment becomes 0 and the ring shows 65k total. Use cacheRead as is; uncachedInput already floors the other way.
There was a problem hiding this comment.
Fixed in ed9c5a6: the split takes cacheRead from the ledger as reported and the clamp is gone; total stays the sum of the drawn segments. Regression: keeps a cache-read share the provider reported without a prompt total seeds input=0 / cacheRead=200k / cacheMiss=60k, the shape the clamp used to erase.
| { | ||
| kind: 'model' as const, | ||
| count: usage.totalRequests, | ||
| durationMs: usage.totalDurationMs ?? 0, |
There was a problem hiding this comment.
After ?? 0 the filter below cannot tell "host reported zero" from "host predates the field", and the model row disappears with its call count while the tool row survives on count > 0. Decide presence on totalDurationMs !== undefined first.
There was a problem hiding this comment.
Fixed in ed9c5a6: presence is decided on totalDurationMs (now a required summary key, since the epoch bump already refuses hosts that predate it) and on toolUsage before any filtering, so a host reporting zero model time keeps its row and its call count. Regressions: a host that measured zero model time keeps its row and its call count and a row with neither a clock nor a count is dropped.
| // their totals ride beside the merged summary rather than inside it — | ||
| // the same owner split the tool buckets path already follows. | ||
| const { provenance, ...summary } = merged; | ||
| const toolUsage = await this.#stores.telemetry.toolSummary(input.query); |
There was a problem hiding this comment.
toolSummary does not filter on connectionSlug but the model totals in the same response do. Since the field is optional and absent means unknown, skip it when input.query.connectionSlug !== undefined. Persisting the slug would only cover new rows.
There was a problem hiding this comment.
Adopted in ed9c5a6: the coordinator omits toolUsage when query.connectionSlug is set, and the persisted-slug attempt is gone from the branch — you are right that it only covered new rows while making history silently vanish under the filter. The row decoder never grew the field. Regression: a connection-scoped summary omits the tool split instead of sending an unscoped one asserts the omitted key and the unchanged model totals.
| // tool ring contradict the filtered model totals beside it. | ||
| let requests = 0; | ||
| let durationMs = 0; | ||
| for (const row of this.readToolRows()) { |
There was a problem hiding this comment.
readToolRows() is SELECT record_json FROM usage_tool_invocations with no WHERE, so every summary query decodes the whole tool table, and the table has no retention. Pass the ts range down; (ts DESC, id) is already indexed. Session filtering after decode is fine once the range has narrowed it.
There was a problem hiding this comment.
Fixed in ed9c5a6: readToolRows takes the resolved range into the query (WHERE ts >= ? AND ts <= ?, served by the existing usage_tool_invocations(ts DESC, id) index), and the remaining filters run over the rows it returns. Also in the same commit: filteredToolRows is now the single filter policy for summary, buckets, and logs — so tool buckets honour Session, provider, and model too, which they previously ignored — with tool summary applies the full summary query to the tool rows and tool buckets answer the full summary query, including Session and provider as regressions.
|
Timing note: the review above was written against 简体中文评审写于 |
Second review round, all under the ring math rather than its shape. The token split clamped cacheRead to input, but the ledger keeps the provider's cached share unclamped when no prompt total was reported, so `cacheRead > input` is a normal aggregate — 200k cached tokens with zero prompt became a zero segment. The split now reads cacheRead as reported; `uncachedInput` already floors the other way, and the total remains the sum of the drawn segments. The time split decided presence through `totalDurationMs ?? 0`, which made a host that measured zero model time drop its row and its call count while the tool row survived on count. `totalDurationMs` is now a required summary key — the epoch bump already refuses peers that predate it, so the optional decoder path and its test were unreachable — and the split decides presence on the reported fields before filtering. The ring label says "Recorded Time": the figures are sums of per-call durations, not wall clock, and parallel or nested calls overlap. `toolUsage` stays optional for a data reason instead of a version one: tool rows predate connection attribution, so a `connectionSlug`-scoped query cannot answer the tool ledger honestly. The Host omits the split for that query rather than persist a slug that would only cover new rows and let history silently vanish under the filter. Every tool read now shares one row filter, so tool buckets honour the same Session, provider, and model filters the summary applies — they previously ignored Session identity. And the ts range moves into the invocation query: the ledger has no retention, and the unbounded decode ran on every summary refresh; the `(ts DESC, id)` index answers it. Generated-by: ZCode
5720559 to
ed9c5a6
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Approving ed9c5a6f. Both blockers from last round are closed the way I hoped, the slug persistence is pulled out cleanly, and making totalDurationMs required removed the fallbacks with it. Nice round.
Two mechanical things before you press merge, both the reason CI is red: main took 94 with #4486 this morning, so this needs 95 (keep the 94 and 93 lines; #4308 is also on 95, so re-check right before merge), and stories/session-workbar.stories.tsx lines 704 and 735 need the now-required totalDurationMs. Those are the only two misses I could find.
One suggestion for a follow-up: the panel queries with range: 'all', which resolves to from: 0, so the new WHERE still selects the whole tool table for the one query this feature adds, and every row is parsed on each refresh. The LLM table pushes session_id into SQL; the same column on the tool table is the existing seam. A sentence in the body that tool buckets now honour sessionId, providerId and modelId would help too.
Evidence boundary: static read of ed9c5a6f against main 92fa5281; affected test:dist files green in core, storage, runtime-host and desktop main; desktop typecheck reproduced.
AI-assisted review: drafted with Maka; I verified the slug rollback, the range: 'all' path and the epoch result myself.
简体中文
批准合并。上轮两条阻塞都按建议关了,这轮是净减法。合并前两件机械事,也是 CI 红的原因:epoch 改 95 并保留 94、93;storybook 704/735 补 totalDurationMs。后续建议:面板查询 range: 'all' 让 WHERE 仍选整表,给 tool 表补 session_id 列下推;正文补一句 tool buckets 现在也按 session/provider/model 过滤。
Summary
The trace (Inspector) panel added with two figures — estimated cost and cache-hit rate — while token totals and elapsed time lived only in per-turn rows. The panel now opens with two donut charts:
Ring and legend are linked in both directions: hovering either highlights that segment and dims the rest. A hairline minimum sweep keeps tiny nonzero shares visible, and the hovered segment draws a thin line so the highlight lands — the legend keeps the true figures either way.
Data sources stay session-scoped and independent of which trace pages are loaded:
The token split reads the existing Session usage summary. Uncached input is the
input − cacheReadresidual (providers that report only the cached share leave the ledger miss at zero), floored by the ledger miss when no prompt total was reported.The time split needs new aggregates:
UsageSummaryV2gainstotalDurationMs(summed attempt latency from both the legacy store and the canonical ledger) andtoolUsage(session-scoped totals from a newTelemetryRepo.toolSummaryover the tool-invocation ledger, attached by the usage coordinator). They ride the Runtime Host usage protocol on the summary rather than being derived client-side from thebucketsquery: the summary is the closest seam, and the figures it returns are exact rather than a client-side re-projection of buckets that already merge legacy and canonical rows with request-weighted latency.totalDurationMsis a required key — the compatibility-epoch bump refuses hosts that predate it at the handshake.toolUsagestays optional for a data reason: tool rows predate connection attribution, so aconnectionSlug-scoped query omits the split instead of sending an unscoped one.Before:
After:
Verification
@maka/core,@maka/storage,@maka/runtime-host,@maka/desktop(all four desktop tsconfigs); Biome clean.toolSummary, protocol optional keys, ring arc geometry and minimum-sweep floors).main, after = this branch).AI use
Select exactly one:
Tool(s) and scope: ZCode authored the implementation (desktop renderer + runtime-host protocol/coordinator + storage + core aggregates) and the tests, human-directed and reviewed locally by @bowenliang123 before opening. The commit carries the
Generated-by: ZCodetrailer.Checklist
Does this PR entail a change in behavior?