feat(telemetry): Trace main agent invocations - #9107
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Re-run at the author's request, now covering Template: complete ✓ Problem: real, verified against the baseline code in the previous pass — today the interaction span ends before a ToolResult continuation begins, so tool execution and follow-up model calls become unrelated trace roots, and success sets Direction: aligned — a continuation of this repo's GenAI/ARMS alignment work. Telemetry contracts are a maintainer-sign-off area, so this PR lands with a human sign-off rather than an auto-approval; that's procedural, not doubt. Size: core infrastructure across two packages. Current breakdown: 1,352 production lines (1,038 added / 314 deleted), 2,009 test lines (1,927 added / 82 deleted), 186 doc lines. Still past both the 500-line maintainer-awareness mark and the 1,000-line large-PR advisory — the maintainer-awareness flag stands. The fix commit itself adds only 66 production lines. Approach: the delta since the last review is exactly one focused commit implementing the R1-1 fix — the interaction TTL becomes an inactivity window refreshed by identity-guarded activity — plus five regression tests pinning the failure scenarios. No scope creep, no drive-by edits. The 35 non-blocking Risk: Moving on to the updated code review. 🔍 中文说明应作者请求 re-run，现覆盖 模板：完整 ✓ 问题：真实存在，上一轮已对照基线代码核实——目前 interaction span 在 ToolResult 续接开始前结束，工具执行与后续模型调用成为互不相关的 trace 根节点；成功路径设置 方向：对齐——是本仓库 GenAI/ARMS 对齐工作的延续。telemetry 契约属于需维护者确认的领域，本 PR 将由人工确认而非自动批准——这是流程要求，并非质疑。 规模：跨两个包触及核心基础设施。当前明细：1,352 行生产代码（+1,038/−314）、2,009 行测试（+1,927/−82）、186 行文档。仍超过 500 行维护者关注线与 1,000 行大 PR 提示线——维护者关注标记保留。修复提交本身仅新增 66 行生产代码。 方案：距上次审查的增量恰为一个聚焦提交，实现 R1-1 修复——interaction TTL 变为由身份守卫的活跃度刷新的不活跃窗口——外加 5 个钉住失败场景的回归测试。无越界改动、无顺手修改。35 条非阻塞 风险： 进入更新后的代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review — re-run at
|
| File | What changed |
|---|---|
packages/core/src/telemetry/session-tracing.ts |
Prompt-scoped interaction registry, invoke_agent attributes, UNSET status rework, redaction; the fix adds inactivity-based TTL with owner-guarded refresh |
packages/core/src/core/client.ts |
Interaction stays open across tool turns, ownership guard, agent output capture wiring; the fix records continuation activity |
packages/cli/src/ui/hooks/useGeminiStream.ts |
Tracks interaction owner per prompt and tool call, ends it at every continuation terminal |
packages/cli/src/nonInteractiveCli.ts |
Ends the headless interaction on every exit path with status and error type |
packages/cli/src/acp-integration/session/Session.ts |
ACP capture extended to agent output, final message written to the span |
packages/core/src/telemetry/detailed-span-attributes.ts |
Agent input/output message attributes and the bounded output capture class |
packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts |
Separates abort-driven cancellation from real errors racing an abort |
packages/core/src/core/coreToolScheduler.ts |
Writes low-cardinality error type on failed tool spans |
packages/core/src/telemetry/sdk.ts |
Shutdown closes all registered interactions |
packages/core/src/telemetry/index.ts |
Exports the new agent attribute helpers and the activity recorder |
packages/core/src/telemetry/session-tracing.test.ts |
Prompt registry, replacement, UNSET semantics, redaction; five new TTL-refresh regression tests |
packages/core/src/core/client.test.ts |
Lifecycle across continuations, retry, goal, steer, JSON schema; continuation activity assertion |
packages/cli/src/ui/hooks/useGeminiStream.test.tsx |
Ownership handoff and stale-tool-batch guard |
packages/cli/src/nonInteractiveCli.test.ts |
Headless continuation, budget, structured-output failure paths |
packages/cli/src/acp-integration/session/Session.test.ts |
ACP prompt, teammate continuation, budget error spans |
packages/core/src/telemetry/detailed-span-attributes.test.ts |
Agent message projections and capture bounds |
packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts |
Cancellation vs error race cases |
packages/core/src/core/coreToolScheduler.test.ts |
Successful tool spans keep UNSET status |
packages/core/src/telemetry/sdk.test.ts |
Shutdown ends every interaction before SDK teardown |
integration-tests/cli/gen-ai-telemetry.test.ts |
Pins the single-interaction trace tree, message projections, UNSET status |
docs/design/telemetry-main-agent-spans-design.md |
New design doc for the invoke_agent interaction contract |
docs/design/gen-ai-arms-field-alignment.md |
Field contract gains the main-agent row, identity section updated |
docs/design/telemetry-subagent-spans-design.md |
Subagent doc aligned: no provider name or agent id, UNSET on success |
docs/developers/development/telemetry.md |
Developer docs refreshed for the new attributes and status semantics |
Testing evidence
The PR's own CI on the reviewed commit — fetched via the API, nothing re-run here (fork code is never executed by triage):
| Check | Conclusion |
|---|---|
| Qwen Code CI / Test (ubuntu-latest, Node 22.x) | ✅ success |
| Qwen Code CI / Desktop Shell (ubuntu-22.04) | ✅ success |
| Qwen Code CI / Desktop Shell (windows-2022) | ✅ success |
| Qwen Code CI / web-shell E2E Smoke (ubuntu-latest, Node 22.x) | ✅ success |
| Qwen Code CI / Classify PR | ✅ success |
| Qwen Code CI / Test (macos-latest, Node 22.x) | ⏭️ skipped |
| Qwen Code CI / Test (windows-latest, Node 22.x) | ⏭️ skipped |
| Qwen Code CI / Integration Tests (CLI, No Sandbox) | ⏭️ skipped |
| SDK Java (all legs incl. Real daemon E2E) | ✅ success |
Pending pull_request-event runs on this commit: 0 — CI has settled, both workflows green.
Reading of this signal: the ubuntu unit suite is green on the fix commit, and the job log confirms the new regression tests actually ran (session-tracing.test.ts at 176 tests vs 171 before; suite totals 19,556 core / 20,010 CLI). Fork CI again skipped the macOS/Windows legs and — the important one — Integration Tests (CLI, No Sandbox), whose gen-ai-telemetry.test.ts is the only test pinning the trace topology end-to-end against a real headless run.
Sandboxed verification status: the earlier @qwen-code /verify run passed on the pre-fix head db4d01ae — 2049/2049 scripted assertions, including the A/B proof (head emits one correctly-parented trace where base emits four orphaned roots) and a load-bearing null-check. A fresh verify run against the reviewed head is in flight right now, triggered by the same comment that re-ran this triage; its report will land as a separate comment. Until it posts, the end-to-end contract on the new head rests on unit + delta coverage, not an independent A/B — not verified here.
Not verified: macOS/Windows unit legs (skipped in fork CI). Nothing user-visible changes, so live TUI behavior is N/A.
中文说明
代码审查 — re-run（97d9931c）
距上次审查的增量为一个提交：R1-1 修复。已核实——无阻塞问题。
- R1-1 指出：跨工具续接拓扑保持打开的 interaction 会被从
startTime起算的 30 分钟 TTL 收割，超过 30 分钟的调用会永久丢失终态。修复将 TTL 变为不活跃窗口：SpanContext.lastActivityTime在 LLM/工具/hook 子 span 的开始与结束、续接轮次（新增recordInteractionActivity，在client.ts解析续接属主处接线）以及终结时被刷新。sweep 对 interaction 从最后活跃时刻起算（其他 span 类型仍从startTime），身份保留改从终结时刻起算。 - 每次刷新都有精确属主守卫：
touchInteractionContext检查activeInteractionsByPromptId.get(promptId) === spanCtx，子 span 在开始时捕获interactionOwner——被替换的属主无法刷新替代者，被替换 interaction 的旧子 span 也无法为替代者续命。两个隔离方向与真正的"不活跃过期"均由 5 个新测试钉住。 - 测试钉住了 R1-1 的失败形态：跨越 30 分钟的健康拓扑在多次 sweep 后存活，并以
turn_status=ok、完整interaction.duration_ms与保留的输出正常结束；真正 30 分钟不活跃仍被以ttl_expired收割；旧子 span 与被替换属主的刷新返回false。CI 证实这些测试确实运行——ubuntu 单元中session-tracing.test.ts现为 176 个测试（此前 171），全部通过。 - 一个残留边界，非阻塞且为任何 TTL 安全网所固有：单个工具调用运行 30 分钟以上且期间无任何其他 span 活动时，仍会被中途收割（工具执行期间无刷新点）。PR 之前 interaction 在每个 turn 边界结束，此类场景不存在；这是 keep-open 设计的代价，由安全网兜底。不足以阻塞。
上一轮对全量 diff 的结论不变：未发现阻塞；两个人工关注项保留（headless Teammate 发送类型顺序调整；interaction 时长涵盖审批等待）。
（时序图与文件清单见英文部分。）
测试证据
来自 API 的该提交 CI 结果（triage 不运行 fork 代码）：ubuntu 单元套件在修复提交上为绿，作业日志证实新回归测试确实运行（session-tracing.test.ts 176 个测试，此前 171；套件总量 core 19,556 / CLI 20,010）。fork CI 再次跳过 macOS/Windows 单元与集成测试；后者的 gen-ai-telemetry.test.ts 是唯一在真实 headless 运行上钉住 trace 拓扑的测试。沙箱验证状态：此前针对修复前 head db4d01ae 的 /verify 已通过（2049/2049 断言，含 A/B 证明与承重空值检查）；针对当前 head 的新一轮 verify 正在运行（由触发本次 triage 的同一条评论发起），报告将另行发布。在其发布前，新 head 的端到端契约依赖单元与增量测试覆盖，尚无独立 A/B 复核——此处标记为未验证。未验证：macOS/Windows 单元（fork CI 跳过）；TUI 无用户可见变化，不适用。
— Qwen Code · qwen3.8-max
Reviewed at 97d9931c026950b6a2a29a74f48d9622ba1e5272 · re-run with @qwen-code /triage
|
Confidence: 3/5 — clean review across both passes; the cap is policy, not doubt (1,352 production lines of core is past the 500-line maintainer-awareness mark, and telemetry contracts are a maintainer-sign-off area). The question this re-run had to answer: does How the author handled the rest of the review is worth naming: 35 non-blocking Suggestions were explicitly deferred under the round-balloon rule rather than quietly folded in, with each deferral recorded in its resolved thread — nothing silently dropped, and the PR didn't widen past its original intent. My carried-over reservations, none blocking:
Action taken this pass: the round-1 ⏸️ Deferring to @yiliang114 and @doudouOUC (subsystem owner, write access) for the human sign-off. Same two policy reasons as the previous pass — the size mark and the telemetry area — not any open finding; the gate has none left. Once the in-flight No changes requested. 🙏 中文说明置信度：3/5 —— 两轮审查都很干净；封顶是策略要求，而非存疑（1,352 行核心生产代码超过 500 行维护者关注线，且 telemetry 契约属于需维护者确认的领域）。 本次 re-run 需要回答的问题： 作者对其余审查意见的处理值得点名：35 条非阻塞建议按"避免审查轮次膨胀"规则被显式推迟，而非悄悄并入，每条推迟都记录在已解决的线程中——没有任何内容被静默丢弃，PR 也没有超出原有意图而膨胀。 保留意见（均不阻塞）：
本轮采取的行动：第一轮 ⏸️ 转交 @yiliang114 与 @doudouOUC（子系统负责人，有写权限）做人工确认。 原因与上一轮相同——规模关注线与 telemetry 领域——而非存在未决问题；门禁已无任何未决发现。待运行中的 不请求修改。🙏 — Qwen Code · qwen3.8-max Reviewed at |
|
@qwen-code /verify |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2049 passed · 0 failed · 2049 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2049 通过 · 0 失败 · 2049 总计 Verification reportPR #9107 verification — feat(telemetry): Trace main agent invocationsVerdict: 中文摘要
Central claim and A/BCentral claim: one main-agent invocation = one trace. The Harness (
The base arm additionally shows the first model call itself was unparented in base (its llm span is a root), confirming the PR body's claim that "tool and follow-up model call became unrelated root traces" understated the base breakage. Cross-check: the PR's own integration suite passes 3/3 on head ( Vacuity / load-bearing proof ( Reviewer Test Plan, per step
FindingsNo new blocking findings. The following are pre-existing observations, reproduced identically on the base build (A/A), hence not introduced by this PR and not merge conditions:
Not covered
MethodologyEnvironment: Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI and its suite (integration-tests/cli/gen-ai-telemetry.test.ts) did not run locally.
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed. One disclosure: the finding's ALS-propagation reasoning is a static trace (call-site graph + Node ALS semantics), no…, and 18 more.
Test Plan (not a blocker): 551 passed — this review observed 19564, 19930, 1364, 1539, 494, 3502, 559 passed; 940 passed — this review observed 19564, 19930, 1364, 1539, 494, 3502, 559 passed; 3 passed — this review observed 19564, 19930, 1364, 1539, 494, 3502, 559 passed.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — 'Integration Tests (CLI, No Sandbox)' was skipped in CI and its suite (integration-tests/cli/gen-ai-telemetry.test.ts) did not run locally。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks above completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — all checks I started were completed. One disclosure: the finding's ALS-propagation reasoning is a static trace (call-site graph + Node ALS semantics), no…，另有 18 条。
Test Plan（非阻断）：551 passed — this review observed 19564, 19930, 1364, 1539, 494, 3502, 559 passed; 940 passed — this review observed 19564, 19930, 1364, 1539, 494, 3502, 559 passed; 3 passed — this review observed 19564, 19930, 1364, 1539, 494, 3502, 559 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed the blocking R1-1 review in The 30-minute interaction TTL now behaves as an inactivity safety window instead of an absolute invocation lifetime. Exact-owner continuation activity and LLM/tool/hook child start/end activity refresh the deadline; all refreshes are span-identity guarded so stale continuations or old children cannot keep a replacement interaction alive. Total Regression coverage now includes a healthy tool topology crossing 30 minutes, a long-running child ending near the sweep boundary, true 30-minute inactivity expiry, stale/replaced owner isolation, terminal status/output preservation, and post-end identity retention/expiry. Verification: core session tracing + client suites (508 tests), targeted ESLint, repository typecheck, build, bundle, and two post-fix open-ended audits passed. CI was green before this review fix; new checks are pending on the pushed SHA. Resolved 36 review threads: R1-1 fixed; 35 non-blocking Suggestions were explicitly deferred under the repository rule that after roughly five review rounds only Critical correctness/security/data-loss/regression fixes should widen the PR. No workflow tracing or other out-of-scope behavior was added. |
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2136 passed · 0 failed · 2136 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：2136 通过 · 0 失败 · 2136 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) — follow-up round at new head Ran the PR in an isolated, token-free container: A/B against the base build at the new base tip, mock-free harness assertions, mutation vacuity proof, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 2136 passed · 0 failed · 2136 total 中文 — 判定：✅ 通过 · 可合入（agent 判定，跟进轮）
Previous-round findings status (follow-up round)Verified head moved from
No new blocking findings. The deferred rows were re-measured rather than diffed: none worsened (envelope, hook spans, and status message are byte-identical across arms). Central claim and A/BCentral claim (unchanged from round 1): one main-agent invocation = one trace. The Harness (
Head total 93/93; base control total 25/25 (every control assertion is "the base is broken in exactly the shape the PR fixes", so a red base behavior counts as a passed control assertion). Correction to the previous report's base description: round 1 stated the base's first model call was also unparented ("all roots"). At the current base tip the first llm span is parented under the interaction (status OK); what fractures is the tool span and the follow-up model call, which become roots of fresh traces. This matches the PR body's own wording ("tool and follow-up model call became unrelated root traces") exactly. The correction is about the description of the control, not about the code: the PR's claims all verify identically. Reviewer Test Plan, per step
Vacuity / load-bearing proofMutation M1 = the single line
The mutant reproduces the base fracture shape (interaction ended after turn 1; tool + follow-up llm orphaned), and the two omitted-behavior tests pass on mutant and base by construction — same split as round 1. The vacuity collation ( Targeted gatesChanged suites at head, run per workspace: core 6 files 1053/1053 ( FindingsNo new findings introduced by this PR. The three pre-existing observations stand (status table above); they reproduce identically on the base build and are not merge conditions. One non-finding observation worth recording: the next-speaker judge side query remains a standalone trace root on head (by design — side queries with a mismatched prompt stay standalone and correlate via Not covered
MethodologyEnvironment: Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved after full review of head 97d9931.
Prior Critical (R1-1) fixed and tested. Interaction TTL now refreshes from lastActivityTime: every LLM/tool/hook span start+end touches its interactionOwner (session-tracing.ts), and sweepStaleSpans measures interaction TTL from activity instead of startTime. Pinned by expires interactions after inactivity instead of absolute lifetime + expires an interaction after 30 minutes without activity.
Span lifecycle is exactly-once on all paths. finalizeInteractionContext is guarded by the ended flag; registerInteractionContext cancels (not silently replaces) an unfinished span with the same promptId; client.ts ends on every terminal branch (max turns/token limit/loop/API error/abort/structured-output-missing) with a !normalCompletion finally safety net; the TUI (endToolInteraction covers all continuation-exit branches incl. admission/delivery failure), headless runner (endActiveInteraction incl. budget/loop/catch-all), and shutdownTelemetry → endAllInteractionSpans close the remaining paths. Owner-identity guards (getActiveInteractionSpan(promptId) !== owner) prevent double-end of a replacement span.
Daemon isolation improved. The module-level lastInteractionCtx fallback is gone; explicit promptIds resolve only an exact owner, mismatched prompts stay standalone (tested), and resolveGenAiParentContext blocks reparenting onto a mismatched ALS interaction. No cross-session leak path found.
Semconv + back-compat. invoke_agent/gen_ai.agent.name=qwen-code/gen_ai.conversation.id on the interaction span; tool spans inherit agent name from the actual parent (subagent name vs qwen-code vs omitted — all three pinned); omission of gen_ai.request.model/gen_ai.provider.name on the agent span is documented rationale (dynamic model selection). UNSET-on-success/cancel + ERROR with low-cardinality error.type is consistent across interaction/LLM/tool/execution/hook/subagent spans and documented. All pre-existing compatibility attributes retained; the two behavior changes (interaction.duration_ms now includes tool/approval wait; Retry/Goal create new spans) are disclosed in telemetry.md and the new design doc. No metric recording changes → no cardinality impact. Sensitive input/output capture is flag-gated, size-capped (omit-on-overflow, never truncate), and excludes intermediate candidates (integration test asserts this).
CI: Test (ubuntu, Node 22) success; SDK matrix success; only 'Integration Tests (CLI, No Sandbox)' skipped — the new integration suite ran in the reported local test plan (3 passed) but not in CI. Not a failure state.
Non-blocking nits (P2/P3), fine as follow-ups:
- Subagent spans don't carry an
interactionOwnertouch, so a single foreground-subagent run (or any single child op) >30 min with zero span activity can still be swept mid-flight. Now bounded and stampedqwen-code.span.ttl_expired=true, so backends can distinguish it — matches the documented walk-away safety-net semantics. - client.ts keeps a now-redundant Goal-only
loopDetector.reset/lastPromptIdblock (~line 2670) after the unifiedstartsInteractionblock (prior R1-5). interactionOwnersByToolCallIdRefin useGeminiStream retains finalized Span refs for callIds that never reach a terminal batch until a later batch deletes them; bounded, but an abort-path clear would tidy it.
Re-run of /triage at the author's request verified the R1-1 fix in commit 97d9931: the interaction TTL is now an inactivity window refreshed by identity-guarded activity (continuation turns and LLM/tool/hook child boundaries), with five regression tests pinning the exact failure scenarios — green in CI on this head. The 35 non-blocking Suggestions were explicitly deferred by the author under the round-balloon guidance, each recorded in its resolved thread. Clearing the stale CHANGES_REQUESTED state so the gate reflects the code as it stands; human sign-off on this telemetry contract remains the maintainer's call (see the stage-3 triage comment).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: reverse audit — ended at the 5-round cap without dry convergence (rounds 3, 4 and 5 each reported findings; the round-5 finding was verified before composing).
Not explored to full depth (tool budget reached): "You are review agent reverse-audit — Reverse audit agent…": none — all planned checks completed within budget.; "PR #9107 (telemetry: make the qwen-code.interaction span…": none — all checks I started completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — every check above was run to completion (~18 tool calls).; "You are review agent reverse-audit — Reverse audit agent…": the test bodies in Session.test.ts that consume agentTelemetry (lines 5887+, 11404+, 23094+) lie outside my chunk (chunk 4 onward) — I did not inspect the corre…; "You are review agent reverse-audit — Reverse audit agent…": Session.test.ts bodies that consume the agentTelemetry mock (L5887+, L11404+, L23094+) are outside my chunk (chunk 4+); their assertion correctness was not wa…, and 29 more.
Test Plan (not a blocker): 551 passed — this review observed 19561, 19935, 1364, 1539, 494, 3502, 559 passed; 940 passed — this review observed 19561, 19935, 1364, 1539, 494, 3502, 559 passed; 3 passed — this review observed 19561, 19935, 1364, 1539, 494, 3502, 559 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：reverse audit — ended at the 5-round cap without dry convergence (rounds 3, 4 and 5 each reported findings; the round-5 finding was verified before composing)。
未探索到全部深度（达到工具调用预算）："You are review agent reverse-audit — Reverse audit agent…"：none — all planned checks completed within budget.；"PR #9107 (telemetry: make the qwen-code.interaction span…"：none — all checks I started completed within budget.；"You are review agent reverse-audit — Reverse audit agent…"：none — every check above was run to completion (~18 tool calls).；"You are review agent reverse-audit — Reverse audit agent…"：the test bodies in Session.test.ts that consume agentTelemetry (lines 5887+, 11404+, 23094+) lie outside my chunk (chunk 4 onward) — I did not inspect the corre…；"You are review agent reverse-audit — Reverse audit agent…"：Session.test.ts bodies that consume the agentTelemetry mock (L5887+, L11404+, L23094+) are outside my chunk (chunk 4+); their assertion correctness was not wa…，另有 29 条。
Test Plan（非阻断）：551 passed — this review observed 19561, 19935, 1364, 1539, 494, 3502, 559 passed; 940 passed — this review observed 19561, 19935, 1364, 1539, 494, 3502, 559 passed; 3 passed — this review observed 19561, 19935, 1364, 1539, 494, 3502, 559 passed。
— qwen3.8-max via Qwen Code /review (v0.21.11)
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #9107 feat(telemetry): Trace main agent invocations
Type: New Feature (OTel telemetry)
Change size: +3087/-460 across 24 files
Findings Summary
- Critical/Major: 0
- Minor: 2 (one new, one confirmed from prior review)
- Nit: 2
- Suggestion: 1
Key Observations
Solid, well-designed feature. The registry-based ownership model (activeInteractionsByPromptId) correctly replaces the fragile single-lastInteractionCtx global. The stale-owner verification pattern in endCurrentInteraction / endActiveInteraction is the right approach for concurrent-prompt isolation. Test coverage across TUI / headless / ACP entry points is thorough and the AgentOutputMessageCapture lifecycle (begin → append → observeFinishReason → commit → writeToSpan) is clean and correct.
Cross-Validation
| Finding | Prior review (qwen-code-ci-bot) | My Assessment |
|---|---|---|
| R1-1 Critical: TTL sweeps healthy long-running interactions | Raised as Critical | RESOLVED — PR uses lastActivityTime as TTL reference for interaction spans; touchInteractionContext refreshes it after every child LLM / tool / hook span ends. A 5-hour tool chain stays live. ✅ |
R1-5 Nit: Redundant Goal loop-detector reset |
Raised | CONFIRMED — still present at client.ts line ~2670; see inline comment |
R1-9 Minor: Unreachable getErrorType(event.value) branch |
Raised | CONFIRMED — GeminiEventType.Error value is always {error: StructuredError}, never an Error instance; the branch is dead code |
R1-11 Minor: loggingStreamWrapper idle-timeout path missing cancelled flag |
Raised | CONFIRMED — the spanEndedByTimeout path (original ~line 729) is not modified by this PR; a user-cancelled hung stream ends as ERROR instead of UNSET |
| R1-12 Suggestion: Integration test has no error-path status assertions | Raised | CONFIRMED — all status.code assertions are success-path toHaveLength(0) |
Unique: endHookSpan sets error.type on UNSET-status spans |
Not raised | New finding — see inline comment |
Additional Audit Coverage
- Registry correctness:
registerInteractionContextcorrectly callsfinalizeInteractionContexton any unfinished previous span for the samepromptId, preventing orphan spans on rapid-fire prompts. withInteractionSpan(ACP path): correctly funnels throughbuildInteractionAttributesandregisterInteractionContext, so ACP spans carry the same new OTel attributes as the CLI path.endAllInteractionSpanssafety: iterates over a spread copy ([...activeInteractionsByPromptId.values()]) before callingfinalizeInteractionContext, which itself mutates the map — no concurrent-modification issue.AgentOutputMessageCaptureoverflow: setsoverflow = trueand clears accumulated text when the response exceedsmaxLength, correctly omitting the attribute rather than truncating JSON.normalizeAgentFinishReasonfallthrough: unknown values lowercase through;MALFORMED_FUNCTION_CALL→malformed_function_callis intentional and tested.llm_request.contextsemantics change: the'interaction'value is now gated ongetInteractionContext(promptId)succeeding (prompt-ID match) rather than any ALS interaction being present — a correctness improvement for cross-prompt LLM calls.- Shutdown ordering:
sdk.test.tsnow assertsendAllInteractionSpansfires beforeNodeSDK.prototype.shutdown— correct ordering verified. clearSessionTracingForTesting: clearsactiveInteractionsByPromptId✅;interactionSequence = 0✅;lastInteractionCtxremoval ✅.
Final Verdict
LGTM — ready to merge after addressing the Minor findings. The two Nits are optional; the endHookSpan error.type and idle-timeout cancellation classification are worth fixing before merging to keep the OTel status contract consistent.
This review was generated by QoderWork AI
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
LGTM — the registry-based ownership model, stale-owner verification, and OTel UNSET/ERROR status semantics are all sound. Two Minor suggestions (endHookSpan error.type on UNSET spans, idle-timeout cancellation classification) noted in the previous comment but do not block merge.
This review was generated by QoderWork AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Follow-up review handling is available in #9121 at
Validation: 755 touched-file unit tests passed (1 skipped), |
|
Released in v0.21.12. |
* codex: address PR review feedback (QwenLM#9107) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (QwenLM#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
4 participants
What this PR does
This PR aligns the existing
qwen-code.interactionspan with the OpenTelemetry GenAI agent conventions and makes it represent one complete main-agent invocation. It adds theinvoke_agentoperation, stable main-agent and conversation identity, prompt-scoped ownership, correct parentage for LLM and tool spans, and OpenTelemetry-compliant success, cancellation, and error status semantics.The interaction now remains open across tool approval, execution, and model continuations, including TUI, headless, and ACP entry points. Concurrent prompts are isolated by prompt ID, stale owners cannot end or mutate replacement spans, shutdown and TTL cleanup close registered spans, and tools inherit the actual parent agent name while standalone tools omit it.
When sensitive span attributes are enabled, the main-agent span records one bounded
gen_ai.input.messagesvalue containing only the trusted original user prompt and one boundedgen_ai.output.messagesvalue containing only the final user-visible assistant response. Expanded context, history, reasoning, tool prefaces, tool results, system instructions, images, failed attempts, and intermediate continuations are excluded; structured JSON output is represented as compact JSON text withfinish_reason=tool_call.The telemetry design, developer documentation, ARMS alignment guidance, and integration coverage are updated to describe and verify the resulting trace contract.
Why it's needed
Previously, the interaction span ended after the first model response in a tool turn. The tool and follow-up model call therefore became unrelated root traces, successful spans used
OKinstead of the recommended unset status, and the interaction lacked standard agent identity and operation fields. This made a single user invocation impossible to reconstruct reliably in OpenTelemetry backends.The new lifecycle and attributes make the full main-agent invocation observable as one trace while preserving privacy defaults. Operators can correlate the user prompt and final answer when they explicitly enable sensitive attributes without duplicating the provider-level request, leaking expanded context, or recording partial and superseded outputs.
Reviewer Test Plan
How to verify
qwen-code.interaction, twoqwen-code.llm_request, and oneqwen-code.toolspan. All four spans should share a trace ID, both LLM spans and the tool span should be children of the interaction, and their operations should beinvoke_agent,chat, andexecute_toolrespectively.gen_ai.agent.name=qwen-code, the current session asgen_ai.conversation.id, one input message containing only the original user prompt, and one output message containing only the final assistant answer. The output must not contain the tool preface, reasoning, tool result, historical messages, or alternate candidates.ERRORwith a low-cardinalityerror.type; no partial output should be written for failed or cancelled invocations.gen_ai.output.type=jsonand an output message containing compact JSON withfinish_reason=tool_call. Using only--output-format jsonmust not set the model output type.Local verification on the rebased branch completed successfully: Core telemetry/client tests (551 passed), CLI headless/TUI/ACP tests (940 passed, 1 skipped), GenAI telemetry integration tests (3 passed), build, bundle, typecheck, and lint.
Evidence (Before & After)
N/A — this changes exported telemetry and documentation, not the user-visible TUI.
Tested on
Environment (optional)
macOS 26.4.1, Node.js v22.22.3, npm 10.9.8; the telemetry integration test used the locally built bundle with sandboxing disabled and the file exporter.
Risk & Scope
OK, longer interaction durations across tool turns, additional Retry/Goal interactions, and the two optional sensitive message attributes when sensitive capture is enabled.Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 将现有的
qwen-code.interactionspan 与 OpenTelemetry GenAI Agent 规范对齐，并使其表示一次完整的主 Agent invocation。它新增invoke_agentoperation、稳定的主 Agent 与会话身份、基于 prompt 的归属关系、LLM 与工具 span 的正确父子关系，以及符合 OpenTelemetry 规范的成功、取消和错误状态语义。Interaction 现在会跨越工具审批、执行和模型 continuation 保持打开，并覆盖 TUI、headless 与 ACP 入口。并发 prompt 按 prompt ID 隔离，过期 owner 不能结束或修改替代 span，shutdown 与 TTL 清理会关闭 registry 中的 span，工具继承真实父 Agent 名称，而 standalone 工具不写 Agent 名称。
启用敏感 span 属性后，主 Agent span 会记录一个有界的
gen_ai.input.messages，其中只包含可信的原始用户 prompt；并记录一个有界的gen_ai.output.messages，其中只包含最终用户可见的 assistant 回答。展开上下文、历史、reasoning、工具前言、工具结果、system instructions、图片、失败 attempt 和中间 continuation 都不会进入这些属性；结构化 JSON 输出以紧凑 JSON 文本表示，并使用finish_reason=tool_call。本 PR 同步更新 telemetry 设计、开发者文档、ARMS 字段对齐说明与集成测试，以描述并验证最终 trace 契约。
为什么需要
此前，interaction span 会在工具轮次的第一次模型响应后结束，因此工具和后续模型调用会成为互不关联的 root trace；成功 span 使用
OK，而不是规范建议的 unset 状态；interaction 也缺少标准 Agent 身份与 operation 字段。这使得 OpenTelemetry 后端无法可靠重建一次完整的用户 invocation。新的生命周期和属性让完整的主 Agent invocation 能够作为一条 trace 被观测，同时保留默认隐私保护。运维人员仅在显式启用敏感属性时才能关联用户 prompt 与最终回答，同时不会重复 provider 级请求、泄漏展开上下文，或记录部分输出与已被替代的输出。
Reviewer 测试计划
验证方法
qwen-code.interaction、两个qwen-code.llm_request和一个qwen-code.toolspan。四个 span 应共享同一个 trace ID，两个 LLM span 和工具 span 都应以 interaction 为父节点，operation 应分别为invoke_agent、chat和execute_tool。gen_ai.agent.name=qwen-code、以当前 session 作为gen_ai.conversation.id、一个只包含原始用户 prompt 的输入消息，以及一个只包含最终 assistant 回答的输出消息。输出中不得包含工具前言、reasoning、工具结果、历史消息或备选 candidate。ERROR并写入低基数error.type；失败或取消 invocation 不得写入部分输出。gen_ai.output.type=json，并且输出消息包含紧凑 JSON 和finish_reason=tool_call。仅使用--output-format json时不得设置模型输出类型。在 rebase 后的分支上，本地验证全部成功：Core telemetry/client 测试（551 passed）、CLI headless/TUI/ACP 测试（940 passed，1 skipped）、GenAI telemetry 集成测试（3 passed）、build、bundle、typecheck 和 lint。
证据（Before & After）
N/A — 本 PR 修改导出的 telemetry 与文档，不涉及用户可见的 TUI。
测试平台
环境（可选）
macOS 26.4.1、Node.js v22.22.3、npm 10.9.8；telemetry 集成测试使用本地构建 bundle、关闭 sandbox，并使用 file exporter。
风险与范围
OK、工具轮次带来的更长 interaction duration、额外的 Retry/Goal interaction，以及启用敏感采集时新增的两个可选敏感消息属性。关联 Issue
N/A