Skip to content

feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent - #9391

Open
chiga0 wants to merge 3 commits into
QwenLM:mainfrom
chiga0:feat/telemetry-daemon-http-traceparent
Open

feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent#9391
chiga0 wants to merge 3 commits into
QwenLM:mainfrom
chiga0:feat/telemetry-daemon-http-traceparent

Conversation

@chiga0

@chiga0 chiga0 commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

The daemon already forwards trace context outbound: prompt requests carry a traceparent inside JSON-RPC _meta, and the daemon parents its bridge spans to it. The HTTP surface, however, only recorded request spans — every daemon request span started a fresh trace, so an HTTP caller forwarding the standard W3C traceparent header (an OTel-instrumented client, a proxy, a gateway) got no linkage back to its own trace.

This PR extracts traceparent/tracestate from inbound request headers at the daemon telemetry middleware and parents the request span to that remote context. Extraction goes through the same code path as the existing _meta extraction (global propagator first, then a strict manual fallback so behavior is identical with and without a registered SDK) and fails closed: a request without a valid header produces exactly the same span shape as before.

Why it's needed

Cross-service debugging between a daemon caller and the daemon currently falls back to timestamp correlation. With this change, any W3C-compliant caller gets its daemon-side spans joined into its own trace for free — no vendor-specific headers, no daemon-side configuration. This is also what the OTel HTTP semantic conventions expect at a server edge.

Reviewer Test Plan

How to verify

  1. npm run build, then start the daemon with local telemetry export: QWEN_TELEMETRY_ENABLED=true QWEN_TELEMETRY_OUTFILE=/tmp/spans.json node packages/cli/dist/index.js serve --port 4199 --safe-mode --workspace <some-dir>
  2. Hit a known route with a fixed header: curl -H "traceparent: 00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01" http://127.0.0.1:4199/daemon/status
  3. Hit the same route without the header (control).
  4. Stop the daemon, then inspect the exported qwen-code.daemon.request spans: the first must share the header's traceId and carry parentSpanContext.spanId = 00f067aa0ba902b7 with isRemote: true; the control span must keep its own fresh trace with no parent.

Evidence (Before & After)

Non-UI change; N/A screenshots. Same dry run on this branch:

request with header  -> span traceId: 4bf92f3577b34da6a3ce929d0e0e4736
                       parentSpanContext: {spanId: 00f067aa0ba902b7, isRemote: true}
request without      -> span traceId: c365575cbb8f511c0af934e4b737aa0c (fresh), no parentSpanContext

On main (before), the first request would also produce a fresh unrelated traceId and no parent.

Unit coverage: header extraction valid/absent/malformed/all-zero-ids/array-value, request-span parenting, and middleware pass-through (key present vs omitted). npm run typecheck, npm run build, and the two touched test files pass locally (17/17 core, 54/54 cli).

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Local npm run build + node packages/cli/dist/index.js serve, telemetry to a local outfile; no sandbox, no external OTLP collector.

Risk & Scope

  • Main risk or tradeoff: spans for callers that already send traceparent now join the caller's trace instead of starting their own — that is the point, but backends that group strictly by traceId will see those spans move. Requests without the header are byte-for-byte unchanged. Span kind and attributes are intentionally untouched (an INTERNAL→SERVER kind switch is a possible follow-up).
  • Not validated / out of scope: no W3C tracingresponse, no response-side traceparent injection, no cross-service sampling decisions; Windows/Linux validated by CI only.
  • Breaking changes / migration notes: none.

Linked Issues

None open; design notes in docs/design/2026-08-18-daemon-http-inbound-trace-context.md.

中文说明

这个 PR 做了什么

daemon 此前只在出站方向传播链路上下文(prompt 请求在 JSON-RPC _meta 里携带 traceparent,daemon 会把 bridge span 挂到它下面),而 HTTP 面只"记录"请求 span——每个 daemon 请求 span 都开启一条全新 trace。转发标准 W3C traceparent header 的 HTTP 调用方(OTel 埋点的客户端、代理、网关)无法关联回自己的 trace。

本 PR 在 daemon 遥测中间件处从入站请求 header 提取 traceparent/tracestate,并把请求 span 挂到该远端上下文下。提取走与现有 _meta 提取相同的路径(先走全局 propagator,再走严格的手动回退,保证有无已注册 SDK 行为一致),并且失败即关闭:不带有效 header 的请求产出的 span 与改动前完全一致。

为什么需要

daemon 调用方与 daemon 之间的跨服务排障目前只能靠时间戳对齐。改动后,任何符合 W3C 规范的调用方都能免费把 daemon 侧 span 并入自己的 trace——无需厂商私有 header,daemon 侧也无需任何配置。这也符合 OTel HTTP 语义约定对服务端入口的期望。

Reviewer 验证计划

如何验证

  1. npm run build 后,用本地遥测导出启动 daemon:QWEN_TELEMETRY_ENABLED=true QWEN_TELEMETRY_OUTFILE=/tmp/spans.json node packages/cli/dist/index.js serve --port 4199 --safe-mode --workspace <某目录>
  2. 携带固定 header 请求已知路由:curl -H "traceparent: 00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01" http://127.0.0.1:4199/daemon/status
  3. 不带 header 再请求一次(对照组)。
  4. 停掉 daemon,检查导出的 qwen-code.daemon.request span:第一个必须与 header 的 traceId 一致并带 parentSpanContext.spanId = 00f067aa0ba902b7isRemote: true;对照组 span 必须保持自己的新 trace 且无 parent。

证据(前后对比)

非 UI 改动,无截图,标注 N/A。本分支同一 dry run:

带 header 的请求    -> span traceId: 4bf92f3577b34da6a3ce929d0e0e4736
                      parentSpanContext: {spanId: 00f067aa0ba902b7, isRemote: true}
不带 header 的请求  -> span traceId: c365575cbb8f511c0af934e4b737aa0c(全新),无 parentSpanContext

main(改前)上,第一个请求同样只会产生一个无关的新 traceId、没有 parent。

单测覆盖:header 提取(有效/缺失/畸形/全零 id/数组值)、请求 span 挂父上下文、中间件透传(键存在与省略)。npm run typechecknpm run build 及两个改动测试文件本地通过(core 17/17、cli 54/54)。

测试环境

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

本地 npm run build + node packages/cli/dist/index.js serve,遥测输出到本地 outfile;无沙箱、无外部 OTLP collector。

风险与范围

  • 主要风险或取舍:已经发送 traceparent 的调用方,其 span 会并入调用方的 trace 而不是自立新 trace——这正是目的,但严格按 traceId 分组的后端会看到这些 span 的归属变化。不带 header 的请求逐字节不变。span kind 与属性刻意不动(INTERNAL→SERVER 的 kind 切换可作为后续跟进)。
  • 未验证 / 范围外:不支持 W3C tracingresponse、不做响应侧 traceparent 注入、不做跨服务采样决策;Windows/Linux 仅由 CI 验证。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

暂无;设计说明见 docs/design/2026-08-18-daemon-http-inbound-trace-context.md

…arent

The daemon HTTP surface records a request span per request, but every span
starts a new trace: a caller forwarding the standard W3C traceparent header
(OTel-instrumented clients, proxies, gateways) gets no linkage back to its
own trace.

Extract traceparent/tracestate from inbound request headers in the daemon
telemetry middleware and parent the request span to that remote context.
Extraction reuses the same path as the existing JSON-RPC _meta extraction
(global propagator first, strict manual fallback so behavior is identical
without a registered SDK) and fails closed: requests without a valid header
keep the exact current span shape.
@chiga0

chiga0 commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

E2E test report (dry run)

Setup: macOS arm64, node v22.23.1, local npm run build, serve --port 4199 --safe-mode, telemetry to a local outfile (QWEN_TELEMETRY_ENABLED=true QWEN_TELEMETRY_OUTFILE=/tmp/wt-spans.json).

Steps: one GET /daemon/status with traceparent: 00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01, one identical request without the header (control), then stopped the daemon and parsed the exported qwen-code.daemon.request spans.

Observed:

request span traceId parentSpanContext
with header 4bf92f3577b34da6a3ce929d0e0e4736 (matches header) {spanId: 00f067aa0ba902b7, traceFlags: 1, isRemote: true} (matches header)
without header fresh c365575cbb8f511c0af934e4b737aa0c absent

Both spans kept identical attributes otherwise (http.request.method=GET, http.route=GET /daemon/status, http.response.status_code=200).

Unit: core daemon-tracing.test.ts 17/17, cli serve/server/telemetry.test.ts 54/54; npm run typecheck, npm run build, eslint and prettier clean on the changed files.

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Overview (AI Generated)

PR: #9391 feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent
Type: New Feature
Change size: +238/-15 across 6 files

Findings Summary

  • Critical/Major: 0 items
  • Minor: 0 items
  • Nit: 0 items

Key Observations

Clean, focused feature addition. The refactoring of contextFromTraceparentValues as a shared helper between extractDaemonTraceContext (existing) and the new extractDaemonHttpTraceContext is the right architecture — keeps the extraction logic in one place and ensures both paths have identical propagator-first + manual-fallback behavior. The middleware integration is correctly fail-closed: the try/catch wrapper and the conditional spread ...(parentContext ? { parentContext } : {}) guarantee that a request without a valid traceparent header produces exactly the same span shape as before.

Independent Verification Checklist

Area Verified Result
withDaemonSpan parentContext wiring ✅ Read full impl Correct — 4-arg startActiveSpan(name, opts, ctx, fn) when parent present, 2-arg otherwise
Array traceparent rejection ✅ Code + test typeof traceparent !== 'string' correctly rejects string[]
All-zero traceId rejection ✅ Test extractDaemonHttpTraceContext({ traceparent: '00-000…-444…-01' })undefined
tracestate propagation Passed to contextFromTraceparentValues, silently dropped if array/missing (intentional fail-closed)
extractDaemonTraceContext semantics unchanged ✅ Diff Refactored to delegate — same validation path, same behavior
parentContext key absent (not undefined) when no header ✅ Test expect('parentContext' in options).toBe(false) — conditional spread ✅
Security: telemetry cannot affect request handling Wrapped in try/catch; span parenting is observability-only
AGENTS.md compliance Maintainer-authored PR; core telemetry change is within maintainer scope

Additional Audit Coverage

  • Backward compatibility: DaemonRequestSpanOptions.parentContext is optional — all existing call sites unaffected ✅
  • Sampling behavior: PR correctly notes traceFlags are honored by SDK default sampler, not forced — no sampling manipulation risk ✅
  • tracestate as array: Node.js HTTP can surface duplicate headers as arrays; typeof tracestate === 'string' guard silently drops it (same as traceparent array case) — acceptable fail-closed behavior ✅
  • contextFromTraceparentValues visibility: correctly unexported (function, not export function) — callers must use the named extractors ✅

Final Verdict

LGTM — Recommend Merge

The implementation is correct and minimal. The refactoring removes duplication without behavior change. Comprehensive test coverage across all stated edge cases (valid / absent / malformed / all-zero-ids / array-value). Design doc included. No issues found.


This review was generated by QoderWork AI

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — the write-up is thorough, and committing the design note under docs/design/ makes the intent easy to check.

  • Template: complete ✓ — all required sections present, including the full bilingual body.
  • Problem: real and demonstrated, not theoretical. Today every qwen-code.daemon.request span starts a fresh trace even when the HTTP caller forwards a standard W3C traceparent, so callers get no linkage back into their own trace. The PR shows the before/after span shapes, and since the outbound direction (_meta traceparent on prompt requests) already exists, this closes the obvious missing half of the loop.
  • Direction: aligned — standard OTel server-edge behavior, no daemon-side configuration, no vendor-specific headers. One note up front: telemetry is a maintainer sign-off area, so this run will end by deferring the merge decision to a human even if the code review comes back clean — that is policy, not a reflection on the PR.
  • Size: touches core paths (packages/core/src/telemetry/**) — 62 production lines (52 in daemon-tracing.ts, 9 in the serve middleware, 1 barrel export), 132 test lines, 59 docs lines. Well under any awareness threshold.
  • Approach: the scope feels right. Reuse the existing _meta extraction through a shared helper instead of adding a parallel parser, thread an optional parentContext through the already-existing withDaemonSpan option, fail closed in the middleware. The design note's rejected alternatives (switching to SpanKind.SERVER, extracting inside core from a raw header bag) are the right ones to reject. Nothing in the diff beyond the stated goal.
  • Risk: no elevated risk signals — none of the changed files match the revert-correlated paths.

Moving on to code review. 🔍

中文说明

感谢贡献——PR 描述详实,随 PR 提交到 docs/design/ 的设计说明让意图很容易核对。

  • 模板:完整 ✓——所有必需章节齐全,包含完整双语正文。
  • 问题:真实且有演示,不是理论问题。当前即使 HTTP 调用方转发标准 W3C traceparent,每个 qwen-code.daemon.request span 仍各自开启全新 trace,调用方无法关联回自己的 trace。PR 给出了前后 span 形态对比,且出站方向(prompt 请求 _meta 里的 traceparent)已经存在,本 PR 补上了闭环中显而易见的一半。
  • 方向:对齐——标准 OTel 服务端入口行为,daemon 侧零配置,无厂商私有 header。先说明一点:telemetry 属于需维护者确认的领域,即使代码审查全部通过,本次运行最终也会把合并决定转交人工——这是策略要求,而非对 PR 本身的质疑。
  • 规模:触及核心路径(packages/core/src/telemetry/**)——62 行生产代码(daemon-tracing.ts 52 行、serve 中间件 9 行、barrel 导出 1 行)、132 行测试、59 行文档,远低于任何关注阈值。
  • 方案:范围合理。通过共享 helper 复用现有 _meta 提取逻辑而不是另写一套解析器,把可选 parentContext 透传给 withDaemonSpan 已有的选项,在中间件中失败即关闭。设计说明中否决的两个替代方案(切换 SpanKind.SERVER、在 core 内部从原始 header 包提取)确实应该否决。diff 中没有超出既定目标的改动。
  • 风险:无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。

进入代码审查。🔍

Qwen Code · qwen3.8-max

Reviewed at 5d21a171fdda224dd7171191f7592ae4a4e26723 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Code review

No blocking findings. Before reading the diff I sketched the shape I'd expect independently — a shared extraction helper, a new extractDaemonHttpTraceContext(headers), an optional parentContext on the request-span options, fail-closed middleware wiring — and the PR matches it; I don't see a simpler path. The load-bearing invariants check out against the base code:

  • withDaemonSpan already accepted an explicit parentContext and takes the exact old code path when it's undefined, so requests without a valid header produce the identical span shape as before.
  • contextFromTraceparentValues is a line-faithful move of the extraction body out of extractDaemonTraceContext (global propagator first, strict 00-version manual fallback, all-zero-id rejection) — the four existing _meta call sites in acpAgent.ts and session/Session.ts see unchanged behavior, and the signature is untouched.
  • Extraction is fail-closed end to end: array header values are rejected, non-string tracestate is ignored, the middleware wraps extraction in try/catch, and the options key is omitted (not set to undefined) when there's no valid header.
  • Tests pin the mechanism at all three layers: header parsing (valid / absent / malformed / all-zero ids / array value), parenting asserted on the startActiveSpan arguments, and middleware pass-through with the key present vs omitted.
  • Conventions hold up: additive optional field, barrel export in alphabetical order, hoisted mocks in the CLI test per the project's vi.hoisted() rule, and a design note committed under docs/design/.

Not verified here: that a live daemon actually exports the joined span — this is a static, CI-signal-only run, so the PR's dry-run output remains the author's claim (see the /verify line below).

CI evidence (fetched once, still settling)

The PR's own CI has not finished on the reviewed commit. Security Checks (TruffleHog secret scan, dependency CVE audit) and SDK Java (including the Real daemon E2E) are green; in Qwen Code CI both Desktop Shell jobs passed while Test (ubuntu-latest, Node 22.x) is still running, and the macOS/Windows unit legs plus the integration suite are skipped — fork PRs don't run the full matrix here, so the platform gap is covered only by the author's local macOS testing for now. Serve A/B is also still running. The table below updates in place once CI settles.

Final CI results for 5d21a17 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
macos-latest / Java 21 ✅ success
Real daemon E2E / Java 11 ✅ success
Secret scan (TruffleHog) ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
windows-latest / Java 21 ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification would settle the one claim neither the diff nor the unit suite can: @qwen-code /verify — that on a live daemon, a request carrying an inbound traceparent exports a qwen-code.daemon.request span under the header's traceId with a remote parent, while a request without the header stays on a fresh trace. The unit tests pin the plumbing, but the end-to-end evidence so far is the author's local macOS dry run, not independently re-run here; the author has write access, so a maintainer can trigger this lane directly.

中文说明

代码审查

无阻塞性发现。读 diff 之前我独立勾勒的预期形态——共享提取 helper、新增 extractDaemonHttpTraceContext(headers)、请求 span 选项上加可选 parentContext、中间件失败即关闭——与 PR 一致;也想不出更简的路径。关键不变量已对照 base 代码核实:

  • withDaemonSpan 本就接受显式 parentContext,未提供时走与现状完全相同的代码路径,因此不带有效 header 的请求产生的 span 形态与改前一致。
  • contextFromTraceparentValues 是把提取主体从 extractDaemonTraceContext 中逐行忠实搬出(先全局 propagator,再严格 00 版本手动回退,拒绝全零 id)——acpAgent.tssession/Session.ts 中四处既有 _meta 调用点行为不变,函数签名未动。
  • 提取全程失败即关闭:数组 header 值被拒绝,非字符串 tracestate 被忽略,中间件对提取包了 try/catch,无有效 header 时选项键直接省略(而非置为 undefined)。
  • 测试在三个层面钉住机制:header 解析(有效/缺失/畸形/全零 id/数组值)、以 startActiveSpan 实参断言挂父上下文、中间件透传(键存在与省略)。
  • 规范达标:可选字段增量添加、barrel 导出按字母序、CLI 测试按项目 vi.hoisted() 约定使用 hoisted mock,设计说明已提交至 docs/design/

此处未验证:真实运行的 daemon 是否确实导出挂接后的 span——本次为纯静态 + CI 信号审查,PR 中的 dry run 输出仍属作者自述(见下方 /verify 一行)。

CI 证据(一次性拉取,仍在进行中)

被审提交上的 PR 自身 CI 尚未跑完。Security Checks(TruffleHog 密钥扫描、依赖 CVE 审计)与 SDK Java(含 Real daemon E2E)已绿;Qwen Code CI 中两个 Desktop Shell 任务通过,Test (ubuntu-latest, Node 22.x) 仍在运行,macOS/Windows 单测与集成套件被跳过——fork PR 在本仓库不跑完整矩阵,平台缺口目前只有作者的 macOS 本地测试覆盖。Serve A/B 也在运行中。下表会在 CI 落定后原地更新。

沙箱验证可以钉住 diff 与单测都无法覆盖的唯一行为声明:@qwen-code /verify——在运行中的 daemon 上,携带入站 traceparent 的请求导出的 qwen-code.daemon.request span 应挂在 header 的 traceId 下且带远端 parent,不带 header 的请求保持全新 trace。单测钉住了管线,但端到端证据目前只有作者的 macOS 本地 dry run,未在此独立复跑;作者有写权限,维护者可直接触发该通道。

Qwen Code · qwen3.8-max

Reviewed at 5d21a171fdda224dd7171191f7592ae4a4e26723 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — clean review with no findings; the cap is policy, not doubt — telemetry is a maintainer sign-off area, this is a fork PR touching core, and CI is still settling.

Stepping back: this is a small, well-bounded change doing exactly one thing — the daemon's HTTP edge starts honoring the same W3C context the JSON-RPC path already honors, via one shared extraction helper and an already-existing span option. Every line in the diff serves the stated goal, the fail-closed discipline matches the rest of the daemon telemetry code, and the tests pin the mechanism at every layer it crosses. If I were maintaining this in six months it would read as obvious.

My honest reservations, none blocking:

  • The end-to-end claim rests on the author's local macOS dry run — fork CI skips the integration suite and the full matrix, and this review is static. The @qwen-code /verify lane named in the review comment would settle it against a live daemon.
  • Span kind stays INTERNAL rather than switching to SERVER — the design note defers that deliberately, which is reasonable, but a merging maintainer should consciously accept the deferred semconv alignment.
  • CI on the reviewed commit is still in flight (ubuntu unit leg and Serve A/B); the table in the review comment updates in place when it lands.

⏸️ Deferring to @yiliang114, @doudouOUC and @zjunothing (telemetry subsystem owners) for the human sign-off. Direction aligned, code clean, no open findings — the defer is the telemetry-area policy cap, not doubt. Once CI lands green (and ideally a /verify run confirms the live-daemon span shape), the merge is a human call.

No changes requested. 🙏

中文说明

置信度:3/5 —— 审查干净、无发现;封顶是策略要求而非存疑——telemetry 属于需维护者确认的领域,这是触及核心的 fork PR,且 CI 尚未落定。

退一步看:这是一个小而边界清晰的改动,只做一件事——让 daemon 的 HTTP 入口开始承认与 JSON-RPC 路径相同的 W3C 上下文,通过一个共享提取 helper 和一个早已存在的 span 选项实现。diff 中每一行都服务于既定目标,失败即关闭的纪律与 daemon 遥测其余代码一致,测试在机制经过的每一层都钉住了它。半年后维护这段代码不会觉得别扭。

如实说明的保留意见(均不阻塞):

  • 端到端声明目前依赖作者的 macOS 本地 dry run——fork CI 跳过集成套件与完整矩阵,本次审查为静态审查。审查评论中点名的 @qwen-code /verify 通道可以在运行中的 daemon 上补上这一环。
  • span kind 保持 INTERNAL 而不切换为 SERVER——设计说明有意推迟,合理,但合并时的维护者应有意识地接受这一推迟的语义约定对齐。
  • 被审提交上的 CI 仍在进行(ubuntu 单测与 Serve A/B);落定后审查评论中的表格会原地更新。

⏸️ 转交 @yiliang114@doudouOUC@zjunothing(telemetry 子系统负责人)做人工确认。 方向对齐、代码干净、无未决发现——转交是 telemetry 领域的策略封顶,而非存疑。待 CI 全绿(最好再有一次 /verify 确认运行中 daemon 的 span 形态)后,合并由人工决定。

不请求修改。🙏

Qwen Code · qwen3.8-max

Reviewed at 5d21a171fdda224dd7171191f7592ae4a4e26723 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 2ee0d77, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

No response changes against the PR base across 4 scenario(s).

Qwen Code · serve A/B

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Overview (AI Generated)

PR: #9391 feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent
Type: Small feature / telemetry hardening (Core Infrastructure — packages/core/src/telemetry, packages/cli/src/serve/server)
Change size: 238 additions across 6 files; reviewed at HEAD 5d21a171

中文说明

这是一个小而聚焦的可观测性增强:在 daemon 的 HTTP telemetry 中间件里提取入站 traceparent/tracestate,让每个 qwen-code.daemon.request span 挂到调用方的 remote 父上下文。核心做法是把原来内联在 extractDaemonTraceContext 里的"propagator 优先 → 手工严格 v00 兜底"逻辑抽成共享 helper contextFromTraceparentValues,再新增一个 extractDaemonHttpTraceContext(headers)重构等价性我逐行核对过,无回归_meta 路径行为完全保留。整体 fail-closed 设计(extractor 返回 undefined + 中间件 try/catch + 只在 parentContext 命中时展开)稳。

盲评发现没有 Critical / Major,仅有几处 Minor(都不是本 PR 引入、而是随共享 helper 从 _meta 通路一并暴露到了 HTTP 通路),以及一些 Suggestion。因此给 COMMENT,是否合并交由维护者判断。

Findings Summary (verified at HEAD 5d21a171)

  • Critical / Major: 0
  • Minor: 4 (W3C 前向兼容、tracestate 兜底路径丢失、SDK 未初始化仍抽取、caller-controlled sampled bit 的 doc 缺失)
  • Nit: 2 (v00 允许多余字段;barrel 未导出 DaemonRequestSpanOptions
  • Suggestion: 3 (长度上限预检、补充测试用例、请求 span 加一个 remote_parent 布尔属性)

Cross-Validation

No prior maintainer reviews at HEAD (author self-comment only). All findings below are from an independent blind review with two focused audit passes (spec/behavior + security/integration). During Phase 2 verification I falsified one candidate Major finding (a hypothesized parent-chain divergence between HTTP-header parent and _meta-based ACP handlers): extractDaemonTraceContext currently has no production caller outside the telemetry module (only a vi.fn() mock in acpAgent.worktree.test.ts), so the "different parents at request-span vs. session-span" scenario is not reachable at HEAD. Dropped.

Refactor equivalence (audited)

extractDaemonTraceContext before → after: identical guard order (_meta shape → traceparent-string+length → propagation.extract → strict v00 manual fallback with the same predicates: hex regex, INVALID_TRACE_ID / INVALID_SPAN_ID rejection, flags parsed as hex byte). Both the propagator-first strategy and the manual fallback survive intact. No regression risk to the _meta path from this refactor.

Additional Audit Coverage

  • Fail-closed: try/catch at telemetry.ts:747-752 is not dead code — propagation.extract invokes the user-installed global propagator, which can throw; req.headers could theoretically be a Proxy with a throwing getter. Contained.
  • Node header semantics: Node lowercases req.headers keys and joins duplicated traceparent values with , (only set-cookie becomes an array). A comma-joined duplicate fails the regex in the manual fallback and typically fails the propagator too → fail-closed. Array values (from non-Node shims) are rejected by typeof !== 'string'.
  • Sampling / cost: the sampled bit is caller-controlled (per Non-goals). Daemon defaults to loopback, so the practical exposure is limited; still worth a threat-model note (see Minor #4 below).
  • Barrel: extractDaemonHttpTraceContext is exported at packages/core/src/telemetry/index.ts:206. DaemonRequestSpanOptions is not (see Nit).

Final Verdict

COMMENT (not APPROVE). Small, well-scoped change with good tests and a clean fail-closed design. I found no blocking issues, but this touches maintainer-only core telemetry infrastructure and I have 4 low-severity Minor findings + several Suggestions that the maintainer may want to weigh in on before merging. All flagged issues are pre-existing behavior surfaced through the refactor (or Non-goal-adjacent), not regressions.


This review was generated by QoderWork AI

Comment thread packages/core/src/telemetry/daemon-tracing.ts Outdated
Comment thread packages/core/src/telemetry/daemon-tracing.ts Outdated
Comment thread packages/cli/src/serve/server/telemetry.ts Outdated
Comment thread packages/core/src/telemetry/index.ts

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent

Verdict: Request changes — C=1, S=2. The direction is right and the extraction refactor is a faithful, behavior-preserving move. But the PR adopts the caller's sampled flag verbatim as a remote parent, and under this repo's effective sampler that silently deletes every daemon-side span for the request — and every span in the session subprocess the request forwards to. Everything below was measured by running the real OTel SDK against this repo's actual telemetry configuration, not inferred from the diff.

Reviewed at 5d21a171fdda224dd7171191f7592ae4a4e26723.


Cross-validation against existing findings

Finding Source My assessment
"No blocking findings" qwen-triage stage=2 Not confirmed. The triage validated the diff in isolation — plumbing, fail-closed behavior, test coverage — but never evaluated the new remote parent against the configured sampler. C1 below is invisible from the diff alone.
Manual fallback rejects version > '00' author self-review Confirmed and agreed — not duplicating. Worth noting it is the production default path, since NOOP_PROPAGATOR makes propagation.extract a no-op unless outboundCorrelation.propagateTraceContext is on. That also means the PR's "identical with and without a registered SDK" claim does not strictly hold.
Manual fallback drops tracestate author self-review Confirmed.
Extraction runs even when the SDK is not initialized author self-review Confirmed.
DaemonRequestSpanOptions not re-exported from the barrel author self-review Confirmed.

Independently verified as correct

  • contextFromTraceparentValues is line-equivalent to the extractDaemonTraceContext body on main — I diffed the two full functions. The four _meta call sites are genuinely unaffected.
  • parentContext is not a dead switch: written by the middleware, read by withDaemonRequestSpan, consumed by withDaemonSpan. Full chain present.
  • Fail-closed holds: array header values, non-string tracestate, malformed input and all-zero ids are all rejected, and the options key is omitted rather than set to undefined, so header-less requests take the exact same startActiveSpan overload as before.
  • Duplicate traceparent headers are safe: Node joins them into "00-…-01, 00-…", so parts[3] becomes "01, 00" and fails /^[0-9a-f]{2}$/.
  • Trust boundary is sound: the middleware is registered after app.use(authenticate) (server.ts 1805 vs 1830), so only authenticated callers can inject trace context. Trace-poisoning exposure is limited.
  • I also checked and ruled out a concern that looked likely at first: that HttpInstrumentation already creates an inbound SpanKind.SERVER span which this change would orphan. It does not — sdk-impl.ts is loaded via dynamic import(), so node:http is already resolved by the time instrumentation registers and the server side is never patched. Your own control request (parentSpanContext: absent) corroborates this. qwen-code.daemon.request is a true root span today. That matters for S1.

C1 (Critical) — an inbound traceparent with sampled=0 deletes every daemon span for that request, and every span in the session subprocess

Mechanism, all three legs verified in code:

  1. sdk-impl.ts constructs new NodeSDK({ … }) with no sampler, and OTEL_TRACES_SAMPLER is not in the OTEL_EXPORTER_ENV_VARS scrub list in sdk.ts. The effective sampler is therefore the SDK default, parentbased_always_on.
  2. In @opentelemetry/sdk-trace-base@2.0.1, ParentBasedSampler defaults remoteParentNotSampled to AlwaysOffSampler (confirmed in ParentBasedSampler.js).
  3. This PR installs isRemote: true with the caller's traceFlags as the request span's parent.

Measured with the real SDK, no sampler configured, next() invoked inside the span exactly as the middleware does:

Scenario Spans exported
main, no inbound header 2 (daemon.request + daemon.bridge)
This PR, flags=01 2, correctly joined to the caller's trace ✅
This PR, flags=00 0 — everything dropped

Because next() runs inside withDaemonRequestSpan, every span produced downstream in that request is a child of the request span and dies with it.

The blast radius reaches the session subprocess. Once the request span is non-recording, injectDaemonTraceContext still emits a valid traceparent — just an unsampled one:

daemon.request isRecording : false
_meta traceparent injected : 00-4bf92f3577b34da6a3ce929d0e0e4736-d773cf0db74acfc3-00

bridge.ts forwards that _meta on newSession / loadSession / prompt. The subprocess's extractDaemonTraceContext accepts flags=00 and parents to it, so the prompt / model / tool spans on the session side are dropped by the same rule. One header disables tracing across two processes.

This fires in practice, not in theory. The callers this PR is written for — OTel-instrumented clients, proxies, gateways — are exactly the population that does head-based ratio sampling, and sampled=0 is the normal W3C encoding for it. A gateway at 10% sampling costs the daemon ~90% of its spans. Meanwhile recordDaemonHttpRequest is a metrics call and is unaffected, so dashboards stay green while traces vanish — the failure is silent.

This repo has already documented this exact hazard. shouldForceSampled() in tracer.ts exists solely because "parentbased_* samplers delegate to localParentNotSampled (default AlwaysOff) … otherwise zero traces are exported". This PR reintroduces that failure mode at the HTTP edge, with the flag now supplied by an external caller and no guard.

Suggested fix — do not adopt the caller's sampling decision. Either force TraceFlags.SAMPLED on the extracted parent (reusing the shouldForceSampled() decision matrix so parentbased_always_off operators are still honored), or attach the caller via a span link instead of a parent, which preserves correlation without handing the caller control over whether daemon spans exist. A regression test asserting a span is still recorded for flags=00 would lock this in.

Repro (node repro.cjs, needs only the repo's own deps):

const { trace, ROOT_CONTEXT, SpanKind } = require('@opentelemetry/api');
const { NodeTracerProvider } = require('@opentelemetry/sdk-trace-node');
const { InMemorySpanExporter, SimpleSpanProcessor } = require('@opentelemetry/sdk-trace-base');

const exporter = new InMemorySpanExporter();
new NodeTracerProvider({ spanProcessors: [new SimpleSpanProcessor(exporter)] }).register();
const tracer = trace.getTracer('qwen-code');

const remoteParent = (flags) => trace.setSpan(ROOT_CONTEXT, trace.wrapSpanContext({
  traceId: '4bf92f3577b34da6a3ce929d0e0e4736', spanId: '00f067aa0ba902b7',
  traceFlags: Number.parseInt(flags, 16), isRemote: true,
}));

for (const flags of ['01', '00']) {
  exporter.reset();
  tracer.startActiveSpan('qwen-code.daemon.request', { kind: SpanKind.INTERNAL }, remoteParent(flags), (s) => {
    tracer.startActiveSpan('qwen-code.daemon.bridge', (c) => c.end());
    s.end();
  });
  console.log(`flags=${flags} -> exported ${exporter.getFinishedSpans().length} spans`);
}
// flags=01 -> exported 2 spans
// flags=00 -> exported 0 spans

S1 (Suggestion) — leaving SpanKind.INTERNAL costs more than the design note implies

The note defers the INTERNALSERVER switch as cosmetic-ish. Given that there is verifiably no other SERVER span on the daemon's inbound path, qwen-code.daemon.request is the service entry point. Backends that derive service topology and RED metrics from SERVER spans (Tempo's service-graph processor, ARMS) will not recognize the daemon as a service inside the caller's trace — which is a meaningful slice of the cross-service-debugging benefit this PR is after. Fine to defer, but worth stating in the design note as a known gap rather than a neutral non-goal.

S2 (Suggestion) — Risk & Scope understates what moves

"spans for callers that already send traceparent now join the caller's trace" reads as if only daemon.request relocates. In fact the whole subtree moves, including the session-subprocess spans reached through _meta (prompt, model, tool). Anyone aggregating or alerting by traceId will see session-side spans change ownership too. Worth spelling out.

中文说明

结论

Request changes — C=1, S=2。 方案方向正确,提取逻辑的重构是忠实的等价搬迁。但本 PR 把调用方的 sampled 位原样当作远端 parent,在本仓库实际生效的 sampler 下,这会静默删除该请求的全部 daemon 侧 span,以及该请求转发到的 session 子进程的全部 span。以下结论均为对照本仓库真实遥测配置、用真实 OTel SDK 实测得出,非从 diff 推断。

审查基于 5d21a171fdda224dd7171191f7592ae4a4e26723

与既有意见的交叉核对

qwen-triage stage=2 的"无阻塞发现"未能确认:它只在 diff 范围内验证了管线、fail-closed 与测试覆盖,没有把新的远端 parent 与已配置的 sampler 放在一起评估——C1 从 diff 本身是看不出来的。作者自审的 4 条(版本 > 00 被拒、tracestate 丢失、SDK 未初始化仍执行提取、DaemonRequestSpanOptions 未从 barrel 导出)我均确认成立,不重复。补充一点:由于默认装的是 NOOP_PROPAGATORpropagation.extract 是 no-op,所以手工兜底就是生产默认路径;这也意味着 PR 声称的"有无已注册 SDK 行为一致"并不严格成立。

独立确认正确的部分

contextFromTraceparentValuesmainextractDaemonTraceContext 的函数体逐行等价(我 diff 了两个完整函数),四处 _meta 调用点确实不受影响;parentContext 不是 dead switch,写入、读取、消费链路完整;fail-closed 成立,无有效 header 时是省略 key 而非置 undefined,走与改前完全相同的 startActiveSpan 重载;重复 header 会被 Node 合并成 "00-…-01, 00-…"parts[3] 变为 "01, 00" 而被拒;信任边界安全,中间件注册在 app.use(authenticate) 之后(server.ts 1805 vs 1830),仅已认证调用方可注入。另外我排查并排除了一个起初看起来很可能成立的疑点:HttpInstrumentation 是否已在创建入站 SERVER span、而本改动会将其孤立。答案是没有——sdk-impl.ts 经 dynamic import() 延迟加载,注册时 node:http 早已解析完成,server 侧从未被 patch;你自己的对照组(parentSpanContext: absent)也印证了这点。qwen-code.daemon.request 今天是真正的 root span,这一点对 S1 很关键。

C1(Critical)——入站 traceparentsampled=0 会删除该请求的全部 daemon span 及 session 子进程全部 span

机制三段均已代码验证:其一,sdk-impl.ts 构造 new NodeSDK({ … })未传 sampler,且 OTEL_TRACES_SAMPLER 不在 sdk.tsOTEL_EXPORTER_ENV_VARS 清理列表中,故生效的是 SDK 默认 parentbased_always_on;其二,@opentelemetry/sdk-trace-base@2.0.1ParentBasedSamplerremoteParentNotSampled 默认为 AlwaysOffSampler(已在 ParentBasedSampler.js 确认);其三,本 PR 将 isRemote: true调用方的 traceFlags 作为请求 span 的 parent。

实测(真实 SDK、未配置 sampler、按中间件方式在 span 内调用 next()):main 无入站 header 导出 2 个 span(daemon.request + daemon.bridge);本 PR flags=01 导出 2 个并正确并入调用方 trace;本 PR flags=00 导出 0 个,全部丢弃。由于 next()withDaemonRequestSpan 内部执行,该请求下游产生的每个 span 都是它的子 span,随之一同消失。

影响会传导进 session 子进程。 请求 span 变为非记录状态后,injectDaemonTraceContext 仍会注入一个合法但未采样的 traceparent:00-4bf92f3577b34da6a3ce929d0e0e4736-d773cf0db74acfc3-00isRecording: false)。bridge.tsnewSession / loadSession / prompt 上转发该 _meta,子进程的 extractDaemonTraceContext 接受 flags=00 并挂到其下,于是 session 侧的 prompt / model / tool span 被同一规则丢弃。一个 header 让两个进程的 tracing 同时失效。

这是会真实触发的场景。 本 PR 面向的调用方——OTel 埋点客户端、代理、网关——正是做 head-based ratio sampling 最多的那批,而 sampled=0 就是其正常的 W3C 编码。一个 10% 采样率的网关会让 daemon 丢掉约 90% 的 span。同时 recordDaemonHttpRequest 属 metrics 调用不受影响,因此看板依旧正常、trace 却消失——故障完全静默。

本仓库已经记录过这个坑。 tracer.tsshouldForceSampled() 存在的唯一理由就是 "parentbased_* samplers delegate to localParentNotSampled (default AlwaysOff) … otherwise zero traces are exported"。本 PR 在 HTTP 入口重新引入了同一失败模式,而这次 flag 由外部调用方提供且没有任何 guard。

修复建议——不要采纳调用方的采样决定。要么对提取出的 parent 强制 TraceFlags.SAMPLED(复用 shouldForceSampled() 的决策矩阵,从而仍尊重 parentbased_always_off 的运维意图),要么改用 span link 而非 parent 来关联调用方,这样既保留关联又不把"daemon span 是否存在"的控制权交给调用方。建议补一条断言 flags=00 时 span 仍被记录的回归测试来锁住行为。上文英文部分附有可直接运行的复现脚本。

S1(Suggestion)——保留 SpanKind.INTERNAL 的代价高于设计文档所述

设计文档把 INTERNALSERVER 的切换当作可延后的次要项。但既已确认 daemon 入站路径上没有其他 SERVER span,qwen-code.daemon.request 本身就是服务入口。依赖 SERVER span 生成服务拓扑与 RED 指标的后端(Tempo service-graph processor、ARMS)在调用方 trace 里无法识别出 daemon 这个服务——而这正是本 PR 想要的跨服务排障收益中相当一块。延后可以接受,但建议在设计文档里写成"已知缺口"而非中性的 non-goal。

S2(Suggestion)——Risk & Scope 把影响面写窄了

"已发送 traceparent 的调用方其 span 会并入调用方 trace" 读起来像只有 daemon.request 会迁移。实际迁移的是整棵子树,包含经 _meta 到达的 session 子进程 span(prompt、model、tool)。任何按 traceId 聚合或告警的系统都会看到 session 侧 span 归属发生变化,建议明确写出。

Comment thread packages/cli/src/serve/server/telemetry.ts

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment thread packages/cli/src/serve/server/telemetry.test.ts
Comment thread packages/core/src/telemetry/daemon-tracing.ts Outdated
Comment thread packages/core/src/telemetry/daemon-tracing.ts
Comment thread packages/cli/src/serve/server/telemetry.ts
Comment thread packages/core/src/telemetry/daemon-tracing.ts
…back

- Force TraceFlags.SAMPLED on inbound HTTP parents via the existing
  shouldForceSampled() matrix: an unsampled remote parent under the
  default parentbased_always_on sampler silently dropped the request
  span, the whole next() subtree, and the session-subprocess spans
  forwarded via _meta (review C1).
- Replace the hand-rolled manual fallback parser with a direct
  W3CTraceContextPropagator instance so acceptance rules (future
  versions, tracestate, all-zero ids, version-00 extension field)
  match the registered path with or without an initialized SDK.
- Gate middleware extraction behind isTelemetrySdkInitialized() to
  skip the hot-path parse when telemetry is off, and emit a debug
  daemon log when a present-but-invalid traceparent header is
  rejected.
- Re-export DaemonRequestSpanOptions from the core barrel and add a
  type-level guard so the parentContext field cannot silently
  disappear (vitest alone cannot catch its removal).
@chiga0

chiga0 commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Review follow-up — e035931

All 10 review threads have been addressed (replies inline, threads resolved). Summary of the follow-up commit:

Critical C1 (@doudouOUC) — caller sampled=0 deleted the whole subtree. Inbound HTTP parents now force TraceFlags.SAMPLED through the session-root shouldForceSampled() matrix: parentbased_* defaults and always_on force sampling; parentbased_always_off honors the operator opt-out; non-parentbased samplers (e.g. traceidratio) keep the caller flags and decide per span. The _meta path keeps flags verbatim — that parent is our own span, already sampled under this policy at the HTTP edge. Rationale: sampled=0 is the caller's head-based ratio sampling, not a request to drop daemon telemetry. Regression tests cover flags 00 → recorded under the default sampler, sampler opt-outs, and the verbatim _meta path.

W3C fallback alignment. The hand-rolled parser is replaced by propagation.extract + a direct W3CTraceContextPropagator instance, so future traceparent versions (01-...), tracestate, all-zero ids, and 00-with-extension-field rejection are identical with and without a registered global propagator, and independent of the propagateTraceContext setting.

Hot-path gating + diagnosability. Extraction is skipped entirely when the SDK is uninitialized (isTelemetrySdkInitialized()); a present-but-invalid string traceparent emits qwen-code.daemon.traceparent.invalid at debug severity with http.route, so a rejected header is diagnosable from daemon logs alone.

Docs and nits. Design doc gains a "Sampling policy" section; SpanKind.INTERNAL vs SERVER is now recorded as a known gap (daemon.request is the only SERVER-adjacent span, so service-graph views like Tempo/ARMS will not model the daemon as a service); the Risk & Scope section now states that the whole subtree — including session-subprocess spans reached via _meta — relocates to the caller's trace. DaemonRequestSpanOptions is exported from the core barrel, the stale helper-count comment is rewritten to not hard-code a number, and a compile-time assertion in the test file guards the parentContext field (vitest does not type-check; CI typecheck does).

Unit tests: core telemetry 944/944, cli telemetry 57/57; typecheck, build, lint, and format all green locally.

@danialzivehdadr

This comment has been minimized.

@danialzivehdadr

This comment has been minimized.

@danialzivehdadr

This comment has been minimized.

@danialzivehdadr

This comment has been minimized.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline.

中文说明

已审查。 建议见行内评论。

— qwen3.8-max via Qwen Code /review (v0.21.13)

type Span,
} from '@opentelemetry/api';
import { logs, type LogAttributes } from '@opentelemetry/api-logs';
import { W3CTraceContextPropagator } from '@opentelemetry/core';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] First static import of @opentelemetry/core pulls the whole core barrel into every full-session CLI startup, defeating the repo's deliberate lazy-loading of OTel internals — Failure scenario: every full-session startup (interactive / non-interactive / MCP / daemon / ACP, including telemetry-off deployments) now parses ~65 KB of @opentelemetry/core that previously loaded only when telemetry initialized; before this PR the package was reachable only through the lazy sdk-impl.ts dynamic import. Verified by full A/B bundle build (base vs PR tree, identical esbuild config): base startup closures contain 0 core bytes (the 33-file CJS barrel sits in a chunk imported only by the lazy sdk-impl/sdk-exporters chunks); each PR startup closure grows by +65,046 bytes, because esbuild cannot tree-shake the CJS barrel down to the propagator. This contradicts the documented boundary in sdk.ts (the hot-path static set's "only runtime @opentelemetry/* dependency is @opentelemetry/api") and the intent of sdkNodeExporterStubPlugin (issue 7264).

Suggested fix: keep @opentelemetry/core out of the static graph — construct the propagator in the dynamic sdk-impl.ts path at SDK init and store it in a module-level holder that contextFromTraceparentValues consults, or await import('@opentelemetry/core') behind the isTelemetrySdkInitialized() gate. Both call sites only consume the result when the SDK is initialized.

Witness: BASE: startup closure coreBytes=0 (core barrel in lazy chunk, importers: sdk-impl/sdk-exporters only) vs PR: startup closure coreBytes=61,495, closureBytes +65,046 (new chunk statically imported by startup entries).

中文说明

首次以静态方式导入 @opentelemetry/core,把整个 core barrel 带入了每次完整会话的 CLI 启动路径,破坏了仓库刻意维护的 OTel 懒加载边界——失败场景:每次完整会话启动(交互式/非交互式/MCP/daemon/ACP,包括关闭遥测的部署)现在都会解析约 65 KB 此前只有在遥测初始化时才会加载的 @opentelemetry/core 代码;此 PR 之前该包只能通过 sdk-impl.ts 的动态 import 懒加载到达。经完整 A/B 打包验证(base 与本 PR 使用同一 esbuild 配置):base 的启动闭包包含 0 字节 core 代码(33 个文件的 CJS barrel 位于仅被懒加载 sdk-impl/sdk-exporters 块引用的 chunk 中);本 PR 每个启动闭包增加 +65,046 字节,因为 esbuild 无法把 CJS barrel 摇树到只剩 propagator。这与 sdk.ts 中记录的边界(热路径静态集合"运行时唯一的 @opentelemetry/* 依赖是 @opentelemetry/api")以及 sdkNodeExporterStubPlugin(issue 7264)的意图相矛盾。

修复建议:把 @opentelemetry/core 移出静态依赖图——在 SDK 初始化时于动态加载的 sdk-impl.ts 路径中构造 propagator 并存入模块级 holder,供 contextFromTraceparentValues 取用;或在 isTelemetrySdkInitialized() 门后 await import('@opentelemetry/core')。两处调用点都只在 SDK 已初始化时消费结果。

证据:BASE: 启动闭包 coreBytes=0(core barrel 位于懒加载 chunk,仅被 sdk-impl/sdk-exporters 引用) 对比 PR: 启动闭包 coreBytes=61,495,closureBytes +65,046(新 chunk 被各启动入口静态引用)

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment on lines +752 to +754
try {
parentContext = extractDaemonHttpTraceContext(req.headers);
} catch {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The fail-closed try/catch around header extraction has no test, while every sibling defensive catch in this middleware is pinned by throwing-mock tests — Failure scenario: mutation-verified — deleting the try/catch leaves all 57 tests green. If extraction ever throws at runtime (e.g. the SDK-registered global propagator that propagation.extract delegates to fails on a carrier), the middleware throws synchronously and Express converts a telemetry-only feature into a failed request (500), violating the invariant the catch's own comment states ("Telemetry must not affect request handling"). The suite's dedicated fail-closed section ('keeps pre-resolved resolver failures from affecting request settlement', 'keeps late hash and span attribute failures from affecting metrics') pins every other such catch with throwing mocks — this one is the exception.

Suggested fix: add a test mirroring the existing pattern: coreMocks.extractDaemonHttpTraceContext.mockImplementationOnce(() => { throw new Error('extract failed'); }), send a request with a present traceparent string header, assert the middleware call not.toThrow(), the response still settles (recordDaemonHttpRequest called once), and 'parentContext' in options is false.

Witness: mutated tree (try/catch removed) — a discriminator probe fails with "expected [Function] to not throw an error but 'Error: extract failed' was thrown" while the suite stays 57/57 green; original tree — the probe passes.

中文说明

围绕 header 抽取的 fail-closed try/catch 没有测试,而本中间件中每一个同类的防御性 catch 都有抛异常 mock 测试锁定——失败场景:已经突变验证——删除该 try/catch 后全部 57 个测试依旧通过。若抽取在运行时抛出异常(例如 propagation.extract 委托的 SDK 注册全局 propagator 在 carrier 上失败),中间件会同步抛出,Express 会把一个纯遥测功能变成失败请求(500),违反该 catch 注释自己声明的不变量("Telemetry must not affect request handling")。测试套件专门的 fail-closed 区块('keeps pre-resolved resolver failures from affecting request settlement'、'keeps late hash and span attribute failures from affecting metrics')用抛异常 mock 锁定了每一个其他此类 catch——唯独此处例外。

修复建议:仿照现有模式补一个测试:coreMocks.extractDaemonHttpTraceContext.mockImplementationOnce(() => { throw new Error('extract failed'); }),发送带 traceparent 字符串 header 的请求,断言中间件调用 not.toThrow()、响应仍正常完成(recordDaemonHttpRequest 被调用一次)、且 'parentContext' in optionsfalse

证据:突变树(删除 try/catch)——判别探针失败,报 "expected [Function] to not throw an error but 'Error: extract failed' was thrown",而套件仍 57/57 通过;原始树——探针通过。

— qwen3.8-max via Qwen Code /review (v0.21.13)

Comment on lines +767 to +769
emitDaemonLog(
'Rejected invalid inbound traceparent header.',
{ 'http.route': route.route },

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The rejected-header breadcrumb records neither the offending header value nor the rejection reason, leaving distinct failure causes indistinguishable in daemon logs — Failure scenario: a caller or intermediate proxy sends a structurally W3C-shaped but semantically rejected traceparent (all-zero trace id, reserved ff version, version 00 with an extension field, or a duplicate header joined into one string); the cross-service join silently fails, and the oncall reading this breadcrumb learns only that some request on some route was rejected — the fix differs per cause (caller instrumentation bug vs proxy mangling vs unsupported future version), so they still have to capture or replay the exact request headers: the very step the breadcrumb was added to eliminate. Verified: emitDaemonLog adds only event.name; all rejection causes produce byte-identical log records, undercutting the comment's stated goal ("diagnosable from daemon logs alone instead of requiring a request replay").

Suggested fix: attach the offending value (bounded, since caller-controlled) or a reason to the log attributes, e.g. { 'http.route': route.route, 'http.request.header.traceparent': inboundTraceparent.slice(0, 128) }.

中文说明

拒绝 header 的 breadcrumb 既未记录问题 header 值,也未记录拒绝原因,导致不同的失败原因在 daemon 日志中无法区分——失败场景:调用方或中间代理发来结构上符合 W3C 但语义上被拒绝的 traceparent(全零 trace id、保留版本 ff、带扩展字段的 00 版本、或被合并为一个字符串的重复 header);跨服务关联静默失败,值班人员读到这条 breadcrumb 只知道某个路由上的某个请求被拒绝——不同原因的修复方式不同(调用方埋点 bug vs 代理篡改 vs 暂不支持的未来版本),因此仍须抓取或重放原始请求 header:恰是这条 breadcrumb 本要消除的步骤。已验证:emitDaemonLog 只附加 event.name;所有拒绝原因产生逐字节相同的日志记录,与注释声明的目标("仅凭 daemon 日志即可诊断,而无需重放请求")相悖。

修复建议:在日志属性中附上问题值(调用方可控,需截断限长)或拒绝原因,例如 { 'http.route': route.route, 'http.request.header.traceparent': inboundTraceparent.slice(0, 128) }

— qwen3.8-max via Qwen Code /review (v0.21.13)

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Overview (AI Generated)

PR: #9391 feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent
Type: New Feature — Core Telemetry
Change size: +1081/-598 across 10 files (core logic: +139/-33, tests: +318/0, NOTICES.txt: ±564 generated)
Reviewed at HEAD: 2ee0d779


Findings Summary

  • Critical/Major: 0
  • Minor: 0
  • Nit: 0 (1 informational note)

Key Observations

The implementation is correct and the second commit (e0359316) directly addressed all actionable concerns. The contextFromTraceparentValues refactor is a faithful, line-equivalent extraction of the existing extractDaemonTraceContext body. The shouldForceSampled() guard is properly applied on the HTTP path, correctly asymmetric with the _meta path (which keeps caller flags verbatim — intentional and documented). The isTelemetrySdkInitialized() hot-path gate, the fail-closed try/catch, the invalid-header breadcrumb log, and the DaemonRequestSpanOptions.parentContext type-level guard are all present and tested.


Cross-Validation

Finding Reviewer Status at HEAD 2ee0d779 Evidence
C1: sampled=0 inbound header drops all daemon and session-subprocess spans under parentbased_always_on doudouOUC (at 5d21a171) Fixed in commit e0359316 extractDaemonHttpTraceContext calls shouldForceSampled() + forces TraceFlags.SAMPLED; test forces the sampled flag on inbound HTTP parents locks it in
S1: SpanKind.INTERNAL vs SERVER — service-topology gap doudouOUC ⚠️ Intentionally deferred Design doc updated with explicit "known gap" framing
S2: Risk & Scope understates subtree relocation doudouOUC ✅ Addressed Design doc and PR description now explicitly call out session-subprocess span migration
Manual fallback rejects version > '00' CI bot R1 Fixed contextFromTraceparentValues now delegates to W3CTraceContextPropagator instance directly; test accepts future traceparent versions
No tracestate test on HTTP path CI bot R1 Fixed Test preserves inbound tracestate on the extracted HTTP context
Silent rejection with no log CI bot R1 Fixed emitDaemonLog('Rejected invalid inbound traceparent header.') + test logs at debug severity when a present traceparent header is rejected
DaemonRequestSpanOptions not barrel-exported CI bot R1 Fixed packages/core/src/telemetry/index.ts exports DaemonRequestSpanOptions; type-level guard in test file
R2-1: Static import of @opentelemetry/core could defeat lazy OTel loading CI bot R2 (at 2ee0d779) Open (Suggestion) daemon-tracing.ts imports W3CTraceContextPropagator at module top level; impact depends on whether this module is on an eager import path for non-telemetry code paths
R2-2: try/catch around extraction has no test CI bot R2 Open (Suggestion) No throwing-mock test; extraction itself is pure and unlikely to throw
R2-3: Rejected-header breadcrumb omits value/reason CI bot R2 Open (Suggestion) Only http.route logged; intentionally omitting header value is defensible from a security/noise standpoint

Additional Audit Coverage

Areas I independently verified beyond existing findings:

  • startActiveSpan overload correctness: withDaemonSpan dispatches to tracer.startActiveSpan(name, opts, parentContext, run) (4-arg) when parentContext is set, vs startActiveSpan(name, opts, run) (2-arg) otherwise — matches the OTel API spec ✅
  • SAMPLED flag reapplication on forced path: trace.setSpan(extracted, trace.wrapSpanContext({...spanContext, traceFlags: spanContext.traceFlags | TraceFlags.SAMPLED})) uses extracted (not ROOT_CONTEXT) as base, preserving tracestate from the extracted context ✅
  • isRemote: true on forced path: spread ...spanContext preserves isRemote: true from W3C propagator extraction; verified by test forcedContext?.isRemote === true
  • _meta path flag asymmetry: extractDaemonTraceContext deliberately skips shouldForceSampled() — correct because that parent was our own span sampled at the HTTP edge; test keeps caller flags verbatim on the _meta extraction path locks it ✅
  • Caller trust boundary: middleware registered after authenticate in server.ts; only authenticated callers can inject trace context ✅
  • tracestate array-value handling: typeof tracestate !== 'string' → silently discarded in contextFromTraceparentValues; in practice Node.js HTTP combines same-name headers with , , so array values are rare on this path ✅
  • New dependency @opentelemetry/core@^2.0.1: well-maintained, trusted, already in the OTel ecosystem; W3CTraceContextPropagator is the canonical implementation ✅

Final Verdict

LGTM at HEAD 2ee0d779 — no blockers. The C1 Critical (doudouOUC) is cleanly fixed. The three open CI bot R2 suggestions are improvement opportunities, not correctness issues; they can be addressed in a follow-up or in-band at author discretion. The PR is mergeable as-is.


This review was generated by QoderWork AI

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delta re-review @ 2ee0d779

Re-reviewed the two new commits since my last pass at 5d21a171:

  • e0359316 — guard inbound traceparent sampling + align W3C fallback
  • 2ee0d779 — regenerate NOTICES.txt (mechanical, no review needed)

Verdict: all my prior findings are resolved, and @doudouOUC's C1 Critical is correctly fixed. No new blocking issues. One optional consistency note below.

Resolved since last review

  • C1 (doudouOUC, Critical — sampled=0 deletes all daemon spans): Fixed. extractDaemonHttpTraceContext now applies shouldForceSampled() and OR-s in TraceFlags.SAMPLED for the parentbased_*/always_on case, so a remote-unsampled parent no longer delegates to AlwaysOff and silently drops the request span, everything under next(), and the _meta-forwarded subprocess spans. The reuse of the session-root decision matrix is the right call — behavior stays consistent with existing sampler semantics. This was the same risk I had under-classified as a Minor; doudouOUC's escalation + repro was correct.
  • My prior Minor (spec-non-compliant manual v00 fallback): Resolved. The hand-rolled regex fallback is replaced by a W3CTraceContextPropagator instance, so future traceparent versions, tracestate, and all-zero-id rejection now match the registered-propagator path exactly, SDK initialized or not.
  • My prior Minor (SDK-init gating): Resolved. Middleware extraction is now behind isTelemetrySdkInitialized().
  • My prior Nit (barrel export): Resolved. extractDaemonHttpTraceContext and type DaemonRequestSpanOptions are exported from the telemetry index.

One optional consistency note

extractDaemonTraceContext (the _meta subprocess path) does not apply the new shouldForceSampled() guard, while the HTTP path does. This is likely intentional — the _meta parent comes from a trusted in-process bridge that already made its own sampling decision, whereas the HTTP header is attacker/proxy-influenced. But the asymmetry is undocumented and a future reader may "fix" one to match the other. A one-line comment on why only the HTTP edge force-samples would prevent that. Non-blocking.

Posting as COMMENT (not APPROVE): the code is in good shape, but the bot still has open Suggestions (eager import, untested try/catch, breadcrumb detail) and I defer the final merge decision to the maintainers.

if (!extracted) return undefined;
const spanContext = trace.getSpanContext(extracted);
if (!spanContext) return undefined;
if (!shouldForceSampled()) return extracted;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix for doudouOUC's C1. Correct: sampled=0 on an inbound header is head-based ratio sampling, not a request to drop daemon telemetry, so forcing SAMPLED under parentbased_*/always_on is the right semantic. Reusing the session-root decision matrix keeps this consistent with the rest of the tracer. ✅

// instance. Acceptance rules — future traceparent versions, tracestate,
// all-zero ids — then match the registered path exactly, with or without an
// initialized SDK.
const w3cTraceContextPropagator = new W3CTraceContextPropagator();

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change — replacing the hand-rolled v00 regex fallback with a W3CTraceContextPropagator instance means future traceparent versions, tracestate, and all-zero-id rejection now match the registered-propagator path exactly, with or without an initialized SDK. Resolves my earlier spec-compliance concern.

}
const extracted = propagation.extract(ROOT_CONTEXT, carrier);
if (trace.getSpanContext(extracted)) return extracted;
return contextFromTraceparentValues(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (non-blocking): the _meta path here does not apply shouldForceSampled(), while the HTTP edge (extractDaemonHttpTraceContext) does. Probably intentional — _meta comes from a trusted in-process bridge, the HTTP header does not — but the asymmetry is undocumented. A one-line comment on why only the HTTP edge force-samples would stop a future reader from "aligning" the two by mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants