Re: [PR] feat(desktop): add session-wide token and time breakdowns to the trace panel [maka]

["hqhq1025 (via GitHub)" <gi...@apache.org>]

hqhq1025 commented on code in PR #4386:
URL: https://github.com/apache/maka/pull/4386#discussion_r3910805670


##########
packages/runtime-host/src/protocol/index.ts:
##########
@@ -100,7 +100,10 @@ export const RUNTIME_HOST_REGISTRATION_SCHEMA_VERSION = 1 as const;
 export const RUNTIME_HOST_PROTOCOL_VERSION = 0 as const;
 // 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;

Review 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).



##########
packages/storage/src/sqlite-usage-store.ts:
##########
@@ -198,6 +199,31 @@ class SqliteTelemetryRepo implements TelemetryRepo {
     });
   }
 
+  toolSummary(query: UsageQuery): { requests: number; durationMs: number } {
+    this.assertReady();
+    const { from, to } = resolveRange(query.range);
+    // The invocation table carries no session column, so every query filter
+    // runs over the decoded records — the same fields the tool logs read
+    // filters, plus the session the LLM summary side scopes by. A field the
+    // summary must respect but the row never carried would otherwise let the
+    // tool ring contradict the filtered model totals beside it.
+    let requests = 0;
+    let durationMs = 0;
+    for (const row of this.readToolRows()) {
+      if (row.ts < from || row.ts > to) continue;
+      if (query.sessionId !== undefined && row.sessionId !== query.sessionId) continue;
+      if (query.toolName !== undefined && row.toolName !== query.toolName) continue;
+      if (query.status !== undefined && query.status !== 'all' && row.status !== query.status) {
+        continue;
+      }
+      if (query.providerId !== undefined && row.providerId !== query.providerId) continue;

Review 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org