fix(agent-sessions): count a stamped tool call by the gateway's verdict on the detail page - #1141
JeremyFunk wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Maple reviewConfidence 3/5 · needs attention Counts a session's tool calls once per
FindingsWarning · F1 ·
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| const lastCopy = new Map<string, AiSessionSpan>() | ||
| for (const span of tools) { | ||
| const callId = span.genAi.toolCallId | ||
| if (callId !== undefined && callId !== "" && recorded(span)) resumedAt.set(callId, spanStartMs(span)) | ||
| if (span.genAi.toolCallId !== undefined) lastCopy.set(span.genAi.toolCallId, span) | ||
| } | ||
| return tools.filter((span) => { | ||
| const resumed = resumedAt.get(span.genAi.toolCallId ?? "") | ||
| return resumed === undefined || resumed <= spanStartMs(span) || recorded(span) || spanFailed(span) | ||
| }) | ||
| return tools.filter( | ||
| (span) => span.genAi.toolCallId === undefined || lastCopy.get(span.genAi.toolCallId) === span, |
There was a problem hiding this comment.
🔴 Repeated IDs hide distinct tool calls
When distinct tool calls reuse an ID, countedToolCalls keeps only the last call. Parallel lanes or per-turn IDs then lose calls, durations, and tool-specific outcomes from the session detail.
Learn more
A tool call ID is not necessarily unique across an entire agent session. Different parallel lanes or turns can reuse it for completed calls. The previous logic preserved completed calls sharing an ID, but the new lastCopy map removes every earlier occurrence irrespective of whether that occurrence was a paused copy. The list's indexSessions and the oversized-session summaryMeasures_ also deduplicate every occurrence of a nonempty ID, so all three paths need a shared distinction between a resumed call and unrelated reuse.
Example: Two parallel run_sql calls with ID toolu_1 both return results, one in trace A and one in trace B. The detail reports one call instead of two and drops the first call's event; the list reports one instead of two.
Recommended fix: Preserve separate completed executions even when their IDs match. Deduplicate only a paused span paired with its resumed execution, using sufficient contextual evidence, and apply the same semantics in countedToolCalls, indexSessions, and summaryMeasures_.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid, fixed in 6b31668. Only a paused copy is merged now: a copy that did not fail and recorded no result, or only ADK's confirmation request. Every copy that recorded an outcome counts, so two completed run_sql calls under toolu_1 are two calls again. An id whose copies are all paused counts once, as its last copy. That is still needed for legacy Strands and for ADK without a result plugin, where no copy has a result. The detail page (countedToolCalls), the list (new IsPausedToolCall index column, same unreleased migration 0035 / local v26) and aiSessionTotalsQuery share this rule. The "keeps two calls that share an id when both returned" test is back, and the e2e fixture now includes a reused id.
| if (span.genAi.toolCallId !== undefined) lastCopy.set(span.genAi.toolCallId, span) | ||
| } | ||
| return tools.filter((span) => { | ||
| const resumed = resumedAt.get(span.genAi.toolCallId ?? "") | ||
| return resumed === undefined || resumed <= spanStartMs(span) || recorded(span) || spanFailed(span) | ||
| }) | ||
| return tools.filter( | ||
| (span) => span.genAi.toolCallId === undefined || lastCopy.get(span.genAi.toolCallId) === span, |
There was a problem hiding this comment.
🟡 Empty IDs collapse unrelated tool calls
When tool spans carry toolCallId: "", countedToolCalls keeps only the last span. The list treats empty IDs as idless and counts every call, so detail and list disagree.
Learn more
A decoded ID is normally absent when the raw attribute is empty, but buildSessionSummary is an exported interface accepting AiSessionSpan objects directly. The old filter treated both undefined and '' as idless. The new map merges all empty IDs, while indexTraces and summaryMeasures_ still count each empty-ID span separately.
Example: Two AiSessionSpan inputs each contain genAi.toolCallId: ''. Detail reports one tool call and the list reports two.
Recommended fix: Keep the empty-string guard in both the map insertion and final filter so empty IDs follow the same idless rule as the SQL queries.
| if (span.genAi.toolCallId !== undefined) lastCopy.set(span.genAi.toolCallId, span) | |
| } | |
| return tools.filter((span) => { | |
| const resumed = resumedAt.get(span.genAi.toolCallId ?? "") | |
| return resumed === undefined || resumed <= spanStartMs(span) || recorded(span) || spanFailed(span) | |
| }) | |
| return tools.filter( | |
| (span) => span.genAi.toolCallId === undefined || lastCopy.get(span.genAi.toolCallId) === span, | |
| if (span.genAi.toolCallId !== undefined && span.genAi.toolCallId !== "") | |
| lastCopy.set(span.genAi.toolCallId, span) | |
| } | |
| return tools.filter( | |
| (span) => span.genAi.toolCallId === undefined || span.genAi.toolCallId === "" || lastCopy.get(span.genAi.toolCallId) === span, | |
| ) |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid, fixed in 8382ec3. The session page reads toolCallId: '' as no id, as the list and the totals SQL do, and there is a test for it. The decoder already drops blank ids, so only spans built by hand could hit this.
| toolCallIds: CH.groupUniqArrayIf(MAX_USAGE_REPORTERS_PER_TRACE)( | ||
| $.ToolCallId, | ||
| $.IsToolCall.eq(1).and($.ToolCallId.neq("")), | ||
| ), |
There was a problem hiding this comment.
🟡 Long traces lose tool calls in lists
When a trace has over 2,000 distinct tool IDs, toolCallIds retains at most 2,000. The list undercounts calls and misapplies tool-count filters, while the oversized-session totals remain exact.
Learn more
The list groups tool IDs per trace before merging those arrays at session level. MAX_USAGE_REPORTERS_PER_TRACE equals the 2,000-span detail-page cap, but the previous list's sum(IsToolCall) had no such bound, and the ungrouped totalsProjection still counts IDs exactly. After the per-trace aggregate hits 2,000 entries, more distinct calls cannot reach the session aggregate.
Example: A single trace has 2,001 tool spans with IDs call_1 through call_2001. The list reports at most 2,000 calls, while the oversized detail totals report 2,001; a toolCallsMin: 2001 filter incorrectly hides the session.
Recommended fix: Compute an exact distinct count across each session without truncating the per-trace ID set. If a bound is necessary, explicitly constrain the supported session size and ensure list filters and totals use the same contract.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid, fixed in 1ee9a36. The per-trace id arrays (pausedToolCallIds / completedToolCallIds) are no longer capped, because nothing quadratic reads them. Completed and id-less copies were already counted exactly with countIf, so the list now matches the exact totals read for any trace size.
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
Maple reviewConfidence 3/5 · needs attention Warning This review ended early; what follows is what it established. Counts a session's tool calls once per
Fixed since the last review
What was checked
Observability coverage: 1 of 1 changes observable
Files not reviewed (1)The review ended before it read these diffs, so nothing above vouches for them.
|
1ee9a36 to
a6d349c
Compare
Maple reviewConfidence 2/5 · risky as written Warning This review ended early; what follows is what it established. The PR makes the sessions list, the session page and the totals read count a tool call once per call id, merging a paused copy into its executed one through new gateway stamps (
FindingsWarning · F2 · Per-turn
|
| }) | ||
| } | ||
|
|
||
| /** A tool call's id; `''` is no id, as the list reads it. */ |
There was a problem hiding this comment.
Session page and list disagree on tool calls for spans ingested before the stamps
F3 · Warning · correctness
For a span with no gateway stamp, toolCallId falls back to gen_ai.tool.call.id and pausedToolCopy merges on the confirmation-request result, so the ADK paused/approved pair counts once on the session page. The list reads ToolCallId off ai_trace_index, which is '' for those rows (the gateway wrote no stamp), so countedToolCopies counts the same pair as 2. Until the legacy rows age out of the 30-day TTL the session page shows one tool call where the list shows two for the same session.
Read the id and the paused verdict from the same source as the list for unstamped spans (treat them as no id), or backfill `ToolCallId`/`IsPausedToolCall` for existing rows, and pin it with a test that runs both reads over unstamped ADK copies.
Prompt for an AI agent
In `packages/agent-sessions/src/session-summary.ts:906-928`: Session page and list disagree on tool calls for spans ingested before the stamps.
For a span with no gateway stamp, `toolCallId` falls back to `gen_ai.tool.call.id` and `pausedToolCopy` merges on the confirmation-request result, so the ADK paused/approved pair counts once on the session page. The list reads `ToolCallId` off `ai_trace_index`, which is `''` for those rows (the gateway wrote no stamp), so `countedToolCopies` counts the same pair as 2. Until the legacy rows age out of the 30-day TTL the session page shows one tool call where the list shows two for the same session.
Suggested fix: Read the id and the paused verdict from the same source as the list for unstamped spans (treat them as no id), or backfill `ToolCallId`/`IsPausedToolCall` for existing rows, and pin it with a test that runs both reads over unstamped ADK copies.
Verify the problem exists at that location before changing it, and keep the fix to those lines.
…ct on the detail page The ingest gateway now stamps maple_ai.tool_call = 0 on the copy a call paused for a human's approval leaves, by its framework's explicit mark, and no maple_ai.error on it even where the framework ends it in error. The list sums the stamps; the detail page and the totals read follow them: - countedToolCalls: a stamped tool span counts on its verdict alone. The result-based pause merge (drop a no-result copy when a later copy with a result shares its call id) serves only pre-stamp spans until the 30-day TTL, so a stamped call that recorded no result is no longer dropped. - spanFailed and the totals read's failure count: on a stamped span the gateway's maple_ai.error alone, so a paused copy the framework ended in error (LlamaIndex, pydantic-ai before instrumentation v5) is no failure.
a6d349c to
7c4c21f
Compare
Maple review🟡 Confidence 3/5 · needs attention Makes the detail page read a gateway-stamped tool span by
Still open from earlier reviews
Fixed since the last review
What was checked
|
JeremyFunk
changed the title
…by the gateway's verdict (folds #1141) The gateway stamps maple_ai.tool_call = 0 on the copy a call paused for a human's approval leaves, and no maple_ai.error on it even where its framework ends it in error. The read path follows: - isCountedToolCall: on a stamped span maple_ai.tool_call = 1. The detail count, the tool histogram, the checks' coverage and the repetition finding use it, so a paused copy is in none of them. The display kind (classifyAiSpan) still reads a paused copy as a tool: the waterfall, span kind and icons show it as one. - countedToolCalls: the result-based pause merge (drop a no-result copy when a later copy under the same call id carries a result) serves only pre-stamp spans until the 30-day TTL, so a stamped call that recorded no result is no longer dropped. - spanFailed and the totals read's failure count: on a stamped span the gateway's maple_ai.error alone, so a paused copy its framework ended in error (LlamaIndex, pydantic-ai before instrumentation v5) is no failure, as on the list.
|
Folded into #1143 |
…1143) * feat(ingest): stamp model-call usage as disjoint maple_ai.usage buckets The gateway now restates a model call's token usage as five disjoint buckets (uncached input, cache read, cache write, visible output, reasoning) plus cost under maple_ai.usage.*, leaving the customer's gen_ai.usage.* as sent. The convention is chosen by emitter, not by gen_ai.provider.name, so inclusive emitters labelled anthropic (OpenRouter Broadcast, OTel genai anthropic, Pydantic AI) no longer double count their cache. Only the span that is the model call carries buckets; agent, step and workflow wrappers get none. Guards: a prompt smaller than its cache is read cache-exclusive, and reasoning is clamped to the completion so a span's total matches the provider's total_tokens. * feat(ingest): mark every stamped span with maple_ai.llm_call The gateway already decides which span is the model call to own the usage buckets; it now says so on every stamped span: 1 on the model call (with or without usage, so a failed call still counts), 0 on the rest. The key's presence tells readers the gateway classified the span, so rows without it keep the op/name heuristics until they age out. An unknown dialect's server span (a proxy's POST /chat/completions) is never the call. Tests cover the heuristic false positives: Spring AI chat_client (op framework), LangSmith ChatPromptTemplate (op chain), DSPy ChatAdapter.__call__ (no op) and the LiteLLM proxy server span. * fix(ingest): treat the GenAI memory operations as known ops in the model-call rule A memory operation naming its embedding model is agent bookkeeping, not a model call; #1142 adds the same ops to the index's known-op list. * feat(agent-sessions): decide every aggregate/filter fact at ingest, the index projects the stamps (#1160) * feat(ingest): stamp every Agent Sessions aggregate and filter fact on the span The gateway now decides, per stamped span, whether it is a tool call, whether it failed, whether it is a tool call's paused copy, and its model, agent, tool, tool call id, response id, tool description and a failed tool call's result, and writes each as a maple_ai.* stamp beside the llm-call marker and usage buckets. One pass over the span's attributes feeds every fact, usage included, in place of a scan per key. OpenAI Agents' OpenInference instrumentor names an agent only in graph.node.id on its AGENT span; that id is the agent name where it equals the span name. * feat(agent-sessions): ai_trace_index_mv projects the gateway's maple_ai.* stamps Migration 0039 (local schema v26; both placeholders, renumbered at merge) recreates ai_trace_index_mv so Model, AgentName, ToolName, ResponseId, IsLlmCall, IsToolCall, IsError, the five token buckets, Tokens, Cost, ToolDescription and FailedToolCallResult each read the fact the ingest gateway stamped on the span. The operation lists, span-name needles, dialect key lists and per-provider usage conventions leave the view; what stays is generic: the environment, error.type, the status message and the failure fingerprint's redaction chain. * feat(agent-sessions): the detail page and /summary read the gateway's stamps On a span the ingest gateway stamped (maple_ai.llm_call present), classifyAiSpan, isLlmCall and spanFailed take its verdicts, spanTokenBuckets and the new spanCost its buckets and cost, and the /summary SQL its verdicts, names and buckets: the facts ai_trace_index sums, so the list and the page cannot disagree. Spans ingested before keep the op/name rules and usage conventions until the 30-day TTL. The materialization e2e seeds carry the stamps the gateway would have written. * chore(ingest): name the stamp key table's entry type, point the stamps at MAPLE_AI_STAMP_ATTRS * docs(agent-sessions): point comments at the gateway's stamps instead of the deleted SQL builders * fix(agent-sessions): number the gateway-stamps view migration 0035 Migration versions must be contiguous, and a BYO-ClickHouse instance at 0039 would skip a lower number landing later. The in-flight view changes rebase onto this one and drop their own migrations of the view. * fix(agent-sessions): the prompt-cache check counts the gateway's cache buckets * fix(agent-sessions): the detail page names the agent the gateway named The span mapper prefers maple_ai.agent.name over the decoded dialect keys, so every reader of genAi.agentName (header, turns, waterfall, filters) shows the agent the list and its facets show, OpenAI Agents' graph node included. * chore(agent-sessions): drop unread stamp keys, unexport in-file constants, fix stale comments * fix(agent-sessions): build the e2e gateway stamps without an open dictionary binding * test(agent-sessions): pin the stamp keys against the ingest gateway's sources * fix(ingest): read a text fact of any OTLP type, as the warehouse Map holds it An integer tool call id or response id, a structured tool call result and a non-string error.type counted when the view read the Map; the stamps now count them too, stringified the way the row encoder writes the Map. Only Google ADK writes the confirmation request, so only its tool results are searched for it. * fix(ingest): stamp a reported $0 cost, so a free call is not read as unpriced * fix(ingest): saturate the cache sum, so an absurd customer figure cannot overflow it * test(agent-sessions): hash the gateway's Rust sources into the stamp-key pin, match only their constants * test(agent-sessions): prove the gateway's failure verdict overrides a span's own error.type * chore(agent-sessions): cut process notes and a duplicate test, rewrap comments * feat(agent-sessions): project the tool call id and the paused-copy stamp onto ai_trace_index Migration 0035 and local schema v26 add ToolCallId (maple_ai.tool.call_id) and IsPausedToolCall (maple_ai.tool.paused) to ai_trace_index, so the list can count a call paused for approval and executed in a later trace once. Only the columns and their projection; the counting stays with the list. * test(agent-sessions): seed the ai-tools e2e spans with the gateway's stamps ai_trace_index_mv projects only the maple_ai.* stamps since migration 0035, so the ai-tools seeds, which carried none, would materialize as neither a call nor a tool. The stamp helper moves to clickhouse-e2e-support so both suites share it. * feat(agent-sessions): a tool call paused for approval is no call, by its framework's explicit mark Drops ToolCallId and IsPausedToolCall from migration 0035 and local schema v26, and the gateway's maple_ai.tool.call_id and maple_ai.tool.paused stamps. Instead the gateway stamps maple_ai.tool_call = 0 on the copy a call paused for a human's approval leaves, so list, summary and detail all count maple_ai.tool_call = 1; a call paused and then rejected counts none. Such a copy is no failure either, though some frameworks end it in error. A pause is read only from a framework's explicit mark, never from a missing result (content capture off, or a tool that returns nothing): - Google ADK: gcp.vertex.agent.tool_response carries the confirmation request (every ADK version writes the key; 2.6 writes no gen_ai.tool.call.result). - OpenAI Agents SDK up to 0.22.0: output.value is a ToolApprovalItem's repr. - pydantic-ai: pydantic_ai.tool.deferral.name = ApprovalRequired. - LlamaIndex's own tracer: the step's status message "Waiting for event". Strands, OpenAI Agents 0.22.1+ and a Mastra tool that suspends itself mark nothing on the paused copy, so it still counts. * fix(agent-sessions): the detail page counts and fails a stamped span by the gateway's verdict (folds #1141) The gateway stamps maple_ai.tool_call = 0 on the copy a call paused for a human's approval leaves, and no maple_ai.error on it even where its framework ends it in error. The read path follows: - isCountedToolCall: on a stamped span maple_ai.tool_call = 1. The detail count, the tool histogram, the checks' coverage and the repetition finding use it, so a paused copy is in none of them. The display kind (classifyAiSpan) still reads a paused copy as a tool: the waterfall, span kind and icons show it as one. - countedToolCalls: the result-based pause merge (drop a no-result copy when a later copy under the same call id carries a result) serves only pre-stamp spans until the 30-day TTL, so a stamped call that recorded no result is no longer dropped. - spanFailed and the totals read's failure count: on a stamped span the gateway's maple_ai.error alone, so a paused copy its framework ended in error (LlamaIndex, pydantic-ai before instrumentation v5) is no failure, as on the list.
1 participant
A tool call paused for a human's approval leaves a paused copy and, once approved, an executed copy in a new trace of the session. Owner decision (in #1143): a paused copy is not a tool call. The ingest gateway stamps
maple_ai.tool_call = "0"on it, by its framework's explicit mark only (Google ADK, OpenAI Agents up to 0.22.0, pydantic-ai, LlamaIndex's own tracer; the table is in #1143). It never infers a pause from a missing result. It stamps nomaple_ai.erroron that copy either, even where the framework ends it in error. So list, summary and detail all counttool_call = 1: a call that is paused and then approved counts once, and one that is paused and then rejected counts 0.The list and
/summaryalready sum those stamps on #1143, so this PR no longer touches list SQL and adds no column. What is left is the detail page and one failure measure.Stacked on #1143, no migration
Stacked on #1143 (
feat/ingest-usage-buckets), the only migration PR.ToolCallId/IsPausedToolCalland themaple_ai.tool.call_id/maple_ai.tool.pausedstamps are gone. CI runs once #1143 merges and GitHub retargets this PR tomain.Changes
countedToolCalls(packages/agent-sessions/src/session-summary.ts): a stamped tool span counts on the gateway's verdict alone. The legacy merge (drop a no-result copy when a later copy under the samegen_ai.tool.call.idcarries a result) now only serves spans ingested before the stamps, until the 30-day TTL. On feat(ingest): stamp model-call usage buckets and the llm-call marker #1143 alone, a stamped call that recorded no result (content capture off, or a tool returning nothing) was still dropped on the page while the list counted it.spanFailed(session-turns.ts) and the totals read's failure count (aiSessionSummaryQuery): on a stamped span,maple_ai.erroralone decides, and the span status is checked only on unstamped spans. A paused copy that its framework ended in error (LlamaIndexWaiting for event, pydantic-ai before instrumentation v5) is therefore not a failure, which matches the list'sIsError.Not covered
suspend()itself.tool_call = 0) is classified byclassifyAiSpanas an agent span, not a tool span. Left as it is; it is an open question for the owner.Tests
agent-sessions:session-summary, with a new stamped case (an ADK paused copy, a LlamaIndex error-status paused copy, and a no-result call that is not merged: 2 calls, 0 failures). Alsosession-turns,failure-text,session-checksandsession-findings.query-engine-integrations:ai-sessions, the full package, and the catalog SQL baseline (regenerated).tsc --noEmitinpackages/agent-sessions.