feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent - #9391
Conversation
…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.
E2E test report (dry run)Setup: macOS arm64, node v22.23.1, local Steps: one Observed:
Both spans kept identical attributes otherwise ( Unit: core |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
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.parentContextis optional — all existing call sites unaffected ✅ - Sampling behavior: PR correctly notes traceFlags are honored by SDK default sampler, not forced — no sampling manipulation risk ✅
tracestateas 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 ✅contextFromTraceparentValuesvisibility: correctly unexported (function, notexport 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
|
Thanks for the PR — the write-up is thorough, and committing the design note under
Moving on to code review. 🔍 中文说明感谢贡献——PR 描述详实，随 PR 提交到
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo blocking findings. Before reading the diff I sketched the shape I'd expect independently — a shared extraction helper, a new
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 CI evidence (fetched once, still settling)The PR's own CI has not finished on the reviewed commit. Final CI results for
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: 中文说明代码审查无阻塞性发现。读 diff 之前我独立勾勒的预期形态——共享提取 helper、新增
此处未验证：真实运行的 daemon 是否确实导出挂接后的 span——本次为纯静态 + CI 信号审查，PR 中的 dry run 输出仍属作者自述（见下方 CI 证据（一次性拉取，仍在进行中）被审提交上的 PR 自身 CI 尚未跑完。 沙箱验证可以钉住 diff 与单测都无法覆盖的唯一行为声明： — Qwen Code · qwen3.8-max Reviewed at |
|
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:
⏸️ 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 No changes requested. 🙏 中文说明置信度：3/5 —— 审查干净、无发现；封顶是策略要求而非存疑——telemetry 属于需维护者确认的领域，这是触及核心的 fork PR，且 CI 尚未落定。 退一步看：这是一个小而边界清晰的改动，只做一件事——让 daemon 的 HTTP 入口开始承认与 JSON-RPC 路径相同的 W3C 上下文，通过一个共享提取 helper 和一个早已存在的 span 选项实现。diff 中每一行都服务于既定目标，失败即关闭的纪律与 daemon 遥测其余代码一致，测试在机制经过的每一层都钉住了它。半年后维护这段代码不会觉得别扭。 如实说明的保留意见（均不阻塞）：
⏸️ 转交 @yiliang114、@doudouOUC 与 @zjunothing（telemetry 子系统负责人）做人工确认。 方向对齐、代码干净、无未决发现——转交是 telemetry 领域的策略封顶，而非存疑。待 CI 全绿（最好再有一次 不请求修改。🙏 — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
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.extractinvokes the user-installed global propagator, which can throw;req.headerscould theoretically be a Proxy with a throwing getter. Contained. - Node header semantics: Node lowercases
req.headerskeys and joins duplicatedtraceparentvalues with,(onlyset-cookiebecomes 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 bytypeof !== '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:
extractDaemonHttpTraceContextis exported atpackages/core/src/telemetry/index.ts:206.DaemonRequestSpanOptionsis 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
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
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
contextFromTraceparentValuesis line-equivalent to theextractDaemonTraceContextbody onmain— I diffed the two full functions. The four_metacall sites are genuinely unaffected.parentContextis not a dead switch: written by the middleware, read bywithDaemonRequestSpan, consumed bywithDaemonSpan. 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 toundefined, so header-less requests take the exact samestartActiveSpanoverload as before. - Duplicate
traceparentheaders are safe: Node joins them into"00-…-01, 00-…", soparts[3]becomes"01, 00"and fails/^[0-9a-f]{2}$/. - Trust boundary is sound: the middleware is registered after
app.use(authenticate)(server.ts1805 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
HttpInstrumentationalready creates an inboundSpanKind.SERVERspan which this change would orphan. It does not —sdk-impl.tsis loaded via dynamicimport(), sonode:httpis 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.requestis 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:
sdk-impl.tsconstructsnew NodeSDK({ … })with nosampler, andOTEL_TRACES_SAMPLERis not in theOTEL_EXPORTER_ENV_VARSscrub list insdk.ts. The effective sampler is therefore the SDK default,parentbased_always_on.- In
@opentelemetry/sdk-trace-base@2.0.1,ParentBasedSamplerdefaultsremoteParentNotSampledtoAlwaysOffSampler(confirmed inParentBasedSampler.js). - This PR installs
isRemote: truewith the caller'straceFlagsas 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 spansS1 (Suggestion) — leaving SpanKind.INTERNAL costs more than the design note implies
The note defers the INTERNAL → SERVER 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_PROPAGATOR，propagation.extract 是 no-op，所以手工兜底就是生产默认路径；这也意味着 PR 声称的"有无已注册 SDK 行为一致"并不严格成立。
独立确认正确的部分
contextFromTraceparentValues 与 main 上 extractDaemonTraceContext 的函数体逐行等价（我 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）——入站 traceparent 的 sampled=0 会删除该请求的全部 daemon span 及 session 子进程全部 span
机制三段均已代码验证：其一，sdk-impl.ts 构造 new NodeSDK({ … }) 时未传 sampler，且 OTEL_TRACES_SAMPLER 不在 sdk.ts 的 OTEL_EXPORTER_ENV_VARS 清理列表中，故生效的是 SDK 默认 parentbased_always_on；其二，@opentelemetry/sdk-trace-base@2.0.1 中 ParentBasedSampler 的 remoteParentNotSampled 默认为 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-00（isRecording: false）。bridge.ts 在 newSession / 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.ts 中 shouldForceSampled() 存在的唯一理由就是 "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 的代价高于设计文档所述
设计文档把 INTERNAL → SERVER 的切换当作可延后的次要项。但既已确认 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 归属发生变化，建议明确写出。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…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).
Review follow-up — e035931All 10 review threads have been addressed (replies inline, threads resolved). Summary of the follow-up commit: Critical C1 (@doudouOUC) — caller W3C fallback alignment. The hand-rolled parser is replaced by Hot-path gating + diagnosability. Extraction is skipped entirely when the SDK is uninitialized ( Docs and nits. Design doc gains a "Sampling policy" section; SpanKind.INTERNAL vs SERVER is now recorded as a known gap ( Unit tests: core telemetry 944/944, cli telemetry 57/57; typecheck, build, lint, and format all green locally. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.13)
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
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 | 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:
startActiveSpanoverload correctness:withDaemonSpandispatches totracer.startActiveSpan(name, opts, parentContext, run)(4-arg) whenparentContextis set, vsstartActiveSpan(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}))usesextracted(notROOT_CONTEXT) as base, preserving tracestate from the extracted context ✅ isRemote: trueon forced path: spread...spanContextpreservesisRemote: truefrom W3C propagator extraction; verified by testforcedContext?.isRemote === true✅_metapath flag asymmetry:extractDaemonTraceContextdeliberately skipsshouldForceSampled()— correct because that parent was our own span sampled at the HTTP edge; testkeeps caller flags verbatim on the _meta extraction pathlocks it ✅- Caller trust boundary: middleware registered after
authenticateinserver.ts; only authenticated callers can inject trace context ✅ tracestatearray-value handling:typeof tracestate !== 'string'→ silently discarded incontextFromTraceparentValues; 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;W3CTraceContextPropagatoris 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
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Delta re-review @ 2ee0d779
Re-reviewed the two new commits since my last pass at 5d21a171:
e0359316— guard inbound traceparent sampling + align W3C fallback2ee0d779— regenerateNOTICES.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=0deletes all daemon spans): Fixed.extractDaemonHttpTraceContextnow appliesshouldForceSampled()and OR-s inTraceFlags.SAMPLEDfor theparentbased_*/always_oncase, so a remote-unsampled parent no longer delegates toAlwaysOffand silently drops the request span, everything undernext(), 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
W3CTraceContextPropagatorinstance, 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.
extractDaemonHttpTraceContextandtype DaemonRequestSpanOptionsare 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.
Address review feedback on the inbound traceparent linkage: - Keep @opentelemetry/core out of the static graph. The module-level W3CTraceContextPropagator in daemon-tracing.ts pulled the CJS barrel (bot-measured +65,046 bytes) into every closure loading that module, including telemetry-off deployments. daemon-tracing.ts now keeps only a holder + setter (setDaemonFallbackPropagator, typed against @opentelemetry/api — type imports stay free at runtime); the lazy sdk-impl.ts chunk, whose closure already contains @opentelemetry/core via sdk-node/resources, constructs and injects the W3C instance on the successful SDK assembly path. Until injection, extraction returns no parent context: the HTTP edge is already gated on isTelemetrySdkInitialized (nothing changes when telemetry is off), and the _meta edge's consumers (withDaemonSpan / withInteractionSpan) short-circuit on the same flag, so an unresolved pre-init parent never had an observable effect. - Add the mutation-verified fail-closed test for the header-extraction try/catch in daemonTelemetryMiddleware: a throwing extractor leaves the request settling normally (recordDaemonHttpRequest still fires once) with no parentContext on the span options. - Record the rejected traceparent value (truncated to 128 chars) as http.request.header.traceparent on the invalid-header breadcrumb — traceparent only carries trace-id/span-id/flags, so this is privacy-safe and makes broken cross-service joins diagnosable. Also document why the _meta extraction path deliberately skips shouldForceSampled (trusted in-process bridge vs external HTTP input).
Post-review audit: auto-instrumentation SERVER span now lands in a separate traceAudited the final head ( With an inbound Before this PR that was invisible:
So every traced request now exports one redundant orphan trace. Two clean follow-up options (out of scope here, given the Non-goals section):
Verified against the head commit: Everything else checked out in audit: fallback-propagator injection ordering (injected inside |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/telemetry/tracer.ts:239 — [review] shouldForceSampled doc names only the session-root consumer of the now-shared policy seampackages/cli/src/serve/server/telemetry.ts:752 — [review] fail-closed catch swallows extraction exceptions; internal faults surface as the caller-blaming invalid-header breadcrumb
中文说明
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…elemetry off Telemetry off (the default) left daemon logs without any trace id: with no request span, the log trace prefix never fires, so a caller forwarding W3C traceparent could not be joined to its daemon log lines. The middleware now parses the header with a plain regex (extractInboundTraceId — same shape/all-zero/ff rejections as the W3C propagator, no OTel machinery) and stores the trace id on the per-response telemetry context. The access log emits it as the camelCase traceId field of "request completed", keeping the log-based join alive with no telemetry config and no trace backend. With telemetry on nothing changes: the request span already carries the caller's trace id into the log prefix.
|
Follow-up commit Telemetry off (the daemon default) left logs with no trace id at all — with no request span, the
Finish-listener ordering is safe by construction: the access log is the outer middleware (its listener registers first and reads the context), while the telemetry middleware deletes the context in its own later-firing listener. Design doc updated with a "Log correlation" section covering both paths (span prefix with telemetry on, field injection with telemetry off), and unit tests cover the parser, the middleware capture matrix, and the access-log field emission/omission. |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Re-Review at HEAD 52d572c0 (AI Generated)
PR: #9391 feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent
Round: R2 (previous rounds at 5d21a171, 2ee0d779)
New commits: b0d15123 (lazy-load fallback propagator + bot R2 fixes), 52d572c0 (telemetry-off trace id in access log)
Findings Summary
- Critical: 1 (build-breaking, confirmed by CI)
- Major: 0
- Minor: 2
- Nit: 0
Previous Finding Status
| Previous Finding | Status | Evidence |
|---|---|---|
C1 (doudouOUC): sampled=0 drops daemon spans |
Fixed (R1) | shouldForceSampled() on HTTP path, still intact at HEAD |
Bot R2-1: static @opentelemetry/core import defeats lazy OTel loading |
Fixed | b0d15123 replaces the module-level instance with a setDaemonFallbackPropagator seam; sdk-impl.ts injects it. Init ordering verified safe — the setter runs at the end of startTelemetrySdk, before telemetryInitialized = true, so there is no window where the flag is on but the propagator is missing |
Bot R2-2: extraction try/catch untested |
Fixed | Mutation-verified fail-closed test added |
| Bot R2-3: breadcrumb omits rejected value | Fixed | http.request.header.traceparent recorded, truncated to 128 chars — traceparent carries no user content, so this is privacy-safe |
My R1 nit: undocumented _meta vs HTTP sampling asymmetry |
Fixed | Explicit "Keep this asymmetry when refactoring" comment at the _meta path |
All prior findings are resolved. The blocker below is newly introduced by commit 52d572c0.
Critical — build is broken at this HEAD
extractInboundTraceId is exported from daemon-tracing.ts and imported by packages/cli/src/serve/server/telemetry.ts from @qwen-code/qwen-code-core, but it was never added to the packages/core/src/telemetry/index.ts re-export list. That barrel uses an explicit named-export list (not export * from daemon-tracing.js), and packages/core/src/index.ts only does export * from './telemetry/index.js' — so the symbol never reaches the package surface.
This is not a hypothesis. CI at 52d572c0 fails with:
src/serve/server/telemetry.ts(10,3): error TS2305: Module '"@qwen-code/qwen-code-core"'
has no exported member 'extractInboundTraceId'.
Error: Command failed: tsc --build
Note this is the same class of gap the bot caught in R1 for DaemonRequestSpanOptions — worth a habit of updating the barrel in the same commit that adds a cross-package symbol.
Industry Context
The new extractInboundTraceId regex path is a deliberate second parser, justified in its docstring as "a log line only needs a plausible, non-zero trace id." That trade-off is reasonable and the telemetry-off log join is a genuinely useful capability. The caveat is that the project already converged on W3CTraceContextPropagator in e0359316 specifically to stop hand-rolled parsing from drifting from spec; reintroducing a second parser reopens that drift surface in a narrower scope (see Minor R2-2). An alternative worth weighing: derive the log trace id from the already-extracted context when telemetry is on, and reuse one shared shape-check helper for both paths, so there is exactly one definition of "acceptable traceparent."
Additional Audit Coverage
- Circular import risk:
access-log.tsnow imports from./telemetry.js; verifiedtelemetry.tsdoes not importaccess-log.ts— no cycle. - Init-ordering race on the lazy propagator: traced
initializeTelemetry→startTelemetrySdk; the injection precedes thetelemetryInitialized = trueflip, and early-return paths leave both unset consistently. No half-initialized state. _metapath regression from lazy injection:extractDaemonTraceContextnow returnsundefinedbefore injection where it previously used the module-level instance. Covered by the new test and gated by the same SDK flag on consumers — behavior change is not observable.??=change at thehandler_resolvedbranch: correctly preserves aninboundTraceIdalready stored on the response context instead of clobbering it with{}.- Regex correctness:
ffversion rejection, all-zero trace/span id rejection, and^...$anchoring all match the propagator's rejections for the00-shaped case.
Final Verdict
Needs Changes. One line in packages/core/src/telemetry/index.ts unblocks the build. The two Minors are consistency/operability concerns, not correctness. Everything from the previous two rounds is cleanly resolved, and the lazy-propagator refactor in b0d15123 is a genuinely good fix — it addresses the bot's bundle-size concern without weakening spec compliance.
This review was generated by QoderWork AI
- Export extractInboundTraceId from the core barrel: the previous commit exported it from daemon-tracing.ts only, so downstream package builds failed with TS2305. - extractDaemonTraceContext now applies the same shouldForceSampled() matrix as the HTTP edge: the _meta path is also reachable from direct ACP clients (acpAgent newSession/loadSession/unstable_resumeSession and Session.prompt pass caller-controlled _meta), so an external sampled=0 parent no longer silences daemon spans there either. The in-process bridge is unaffected (its injected values are already SAMPLED). - The rejected-header breadcrumb now goes through sanitizeLogText so a crafted traceparent cannot forge log line structure with control characters. - Add the sdk-impl wiring test: after initializeTelemetry the injected W3C fallback propagator resolves inbound HTTP parents.
…n both modes - extractInboundTraceId now mirrors the vendored W3C propagator's acceptance exactly: single optional leading/trailing whitespace and trailing extension fields above version 00 (version 00 must stay four fields). Previously the strict four-field anchor made the two paths disagree on the same forward-compatible header, silently dropping the access-log traceId for exactly the callers the propagator path supports. - The camelCase traceId access-log field is now captured whenever a valid header parses, regardless of telemetry mode, so one saved log query / alert shape works for every deployment; with telemetry on the snake_case span prefix carries the same id redundantly.
…dule 52d572c made the access log statically import the telemetry middleware module to read the captured inbound trace id. The access log sits inside the serve fast-path pre-listen closure (run-qwen-serve imports it directly), so the middleware's core-barrel import graph came along for the ride and check-serve-fast-path-bundle started failing: the 5.6MB core chunk (shell tool, glob, chokidar, @iarna/toml, fzf) became statically reachable from run-qwen-serve. Move the response-context symbol, its type, and the getDaemonTelemetryInboundTraceId getter into a new import-light telemetry-context.ts; the middleware imports the symbol from there and re-exports the getter, so the access log no longer links against the telemetry module at all.
|
CI repair —
Fix: the response-context symbol, its type, and |
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: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/vscode-ide-companion/NOTICES.txt:6414 — [probe] NOTICES.txt diff is 1128 lines of reorder noise; generate-notices.js emission order is insertion-dependent
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Review — inbound W3C traceparent linking
Tier: Deep — wire protocol parsing caller-supplied input, plus silent-failure telemetry code.
A note on sizing first: the -613 is misleading. 564 of those lines are packages/vscode-ide-companion/NOTICES.txt (+564/−564, a regeneration) and one is function → export function. Real source deletions are 48 lines, all in daemon-tracing.ts / telemetry.ts. All 48 were audited.
Findings
3 major (one as a question) · 2 minor, inline. Both majors were settled by execution.
Verification performed
rung 0 CI 19 SUCCESS / 7 SKIPPED / 0 FAILURE (inventory below)
rung 2a non-vacuity: 23-mutant matrix — 21 killed, 2 survived, 1 inconclusive
rung 2c differential oracle: extractInboundTraceId vs the vendored
@opentelemetry/core parseTraceParent vs extractDaemonHttpTraceContext
— 60,823 inputs, and a separate 33-shape suite: 0 divergences
rung 3 real express app, real access-log + telemetry middlewares wired in
server.ts order, real HTTP over loopback
Checked and found clean — with witnesses
W3C Trace Context conformance: clean. The corpus covered absent · empty · wrong length (31/33/15/17) · uppercase hex · non-hex · all-zero trace-id · all-zero span-id · ff · all 256 version bytes with and without trailing fields · leading/trailing whitespace · NBSP, BOM, U+2028, NEL, VT, FF, NUL, ESC · duplicate-header comma joins. Against the spec: all-zero trace-id and parent-id are MUST ignore (§3.2.2.3/§3.2.2.4) — rejected. ff invalid (§3.2.2.1) — rejected. Higher versions parsed for the first three fields with trailing ignored (§3.2.4) — accepted; version 00 with trailing correctly rejected. tracestate limits (32 members / 512 chars) are delegated to the vendored TraceState, which is correct. On duplicate headers: Node joins with ", " and both paths reject it — verified.
The docblock's "mirrors the vendored propagator exactly" claim holds. That is the strongest part of this PR, and the differential is what establishes it rather than the hardcoded-literal tests, which are partly tautological (daemon-tracing.test.ts:48 constructs the same W3CTraceContextPropagator production installs).
Untrusted input reaching a sink: clean. sanitizeLogText neutralizes CR/ESC/NEL/U+2028/RLO/NUL and renders LF as a literal \n, so no log-structure forging — exercised on the real module with CRLF-forge, ANSI/OSC, C1, bidi-RLO and 4 KB payloads. The access-log traceId field only ever receives a regex-validated 32-hex string, and is correctly disjoint from daemon-logger.ts:63's RESERVED_TRACE_CTX_KEYS, so a caller cannot spoof the span prefix.
Trust gate: investigated and refuted. daemonTelemetryMiddleware is mounted at server.ts:1830, after bearerAuth at :1805 — extraction is not on the pre-auth path — and boot refuses to bind beyond loopback without a token (run-qwen-serve.ts:2027-2028). The "unauthenticated remote spoofs our trace ids" scenario does not hold. What remains is a token-less loopback daemon and authenticated-but-untrusted callers; trusting inbound traceparent at a server edge is standard OTel posture and the PR states the tradeoff.
Removed-behaviour audit: no silent loss. The deleted 25-line propagator-free parser is the one removed guarantee — post-PR that path needs daemonFallbackPropagator, installed only at sdk-impl.ts:523. All four consumers (acpAgent.ts:5084/5148/5540, Session.ts:4175) pass the result as a span parent, and withDaemonSpan returns early when the SDK is uninitialized, so the loss is unobservable. No init-order window: telemetryInitialized = true (sdk.ts:121) runs after startTelemetrySdk returns, whose only top-level return is after line 523. No exported symbol lost. INVALID_* → ALL_ZERO_* is a rename with the same values, still enforced.
Cross-file consumer sweep: every changed/added/removed export traced to its callers and to the package entry point. extractDaemonHttpTraceContext, extractInboundTraceId, DaemonRequestSpanOptions are in the barrel; setDaemonFallbackPropagator and shouldForceSampled are intentionally not. DaemonTelemetryResponseContext.inboundTraceId and DaemonRequestSpanOptions.parentContext both have live producers and consumers — no orphan on either side.
Two surviving mutants, one of which matters
| mutant | verdict |
|---|---|
if (context) branch drops the id (telemetry.ts:798-800) |
survived — dead branch. Nothing sets that symbol earlier in a request's life: the only other writer, setDaemonTelemetryWorkspace, mutates an existing object and never creates one, and runs from handlers after this middleware. Unreachable at head; cost is dead code only. |
if (trace.getSpanContext(extracted)) return extracted; |
survived, benign — both propagator branches apply identical W3C acceptance rules, so it is unobservable by construction. Not filed. |
server.ts mount-order swap |
inconclusive — server.test.ts produced 2 then 5 different unrelated failures on two consecutive baseline runs here, so I cannot attribute the mutant's exit code. See the mount-order note below. |
The mount-order fragility, as a question rather than a finding
traceId reaching the access log depends on the access-log finish listener being registered before the telemetry middleware's, which deletes the carrier (telemetry.ts:841-842). That ordering holds only because of the mount lines above. Swapping them in a probe makes the field vanish:
SWAPPED ORDER: [{"route":"GET /daemon/status","status":200,"durationMs":0}] <-- traceId gone
Nothing pins it: access-log.test.ts mocks ./telemetry-context.js, telemetry.test.ts never involves the access log, and no end-to-end test asserts the field. Reading the carrier once and stashing the id as a plain non-deleted property would remove the coupling.
Coverage gap, stated concretely
Every new test mocks either @qwen-code/qwen-code-core wholesale (telemetry.test.ts:29) or the ./telemetry-context.js seam (access-log.test.ts:19), so no automated test drives a real HTTP request through the real middleware stack — and Integration Tests (CLI, No Sandbox) is SKIPPED on this PR. The design doc's own Testing section prescribes exactly that check (a curl with a fixed traceparent, asserting the exported span shares the trace id and parents to its span id) and it is automated nowhere: grep -rn traceparent integration-tests/ hits only cli/gen-ai-telemetry.test.ts:460, which is about outbound headers. That gap is what hid the access-log finding above — it took a hand-built probe to surface.
Not covered
Test (windows-latest), Test (macos-latest), Integration Tests, Post Coverage Comment are SKIPPED — CI is fully non-red rather than fully green. Nothing in the diff is platform-dependent, so I rate the OS matrix low risk. I did not reproduce the PR's own Evidence block against a live daemon, so the claimed parentSpanContext.spanId / isRemote: true output is unverified by me. The ~65 KB per launch bundle argument was read, not measured. NOTICES.txt not read line by line.
Reviewed with AI assistance.
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the core (daemon-tracing.ts, telemetry-context.ts, access-log.ts).
Approve. Clean W3C traceparent linking: span options gain parentContext, and contextFromTraceparentValues extracts via propagation.extract with a guarded fallback propagator, so inbound HTTP spans correctly join the caller's trace. No obvious blockers.
0 unresolved threads.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed at a4f66bdf089423fed71d0ee47ddf2fb115c4431c. C=0 — approving.
Verified against the code as it stands at this commit, not against the earlier rounds: the build break that had all of Test, Serve A/B, Real daemon E2E and web-shell E2E Smoke red (extractInboundTraceId defined in daemon-tracing.ts but missing from the core telemetry barrel, so tsc --build failed in the prepare postinstall) is fixed — the barrel now re-exports it and all four jobs pass. Only review-pr is still in flight; it is not a build gate.
What I checked, and why each concern is closed
Acceptance-set parity between the two extraction paths. The regex path and the propagator path now agree. TRACEPARENT_RE mirrors OTel's parseTraceParent shape (\s? on both ends, optional extension segment) and the explicit rejections line up: version ff, all-zero trace/span ids, and version 00 carrying an extension field. So a header cannot land a traceId in the access log while silently failing to parent the span, or vice versa — the earlier divergence (strict 4-segment regex vs. OTel's future-version leniency) is gone.
The read/delete race on the response context. This was the one that worried me most, and the fix is structural rather than a patch. The caller trace id moved to its own daemonInboundTraceIdContext symbol and is never deleted, while only daemonTelemetryResponseContext is cleared in the finish handler. The access log's finish listener therefore no longer depends on firing before the telemetry middleware's. Keeping the two symbols separate also matters for a second reason the comment calls out correctly: the presence of the telemetry response context is itself the opt-in gate in setDaemonTelemetryWorkspace, so capturing a trace id must not create it — otherwise a caller merely sending a header would move workspace attribution. Both directions are pinned by tests.
Coverage of short-circuited requests. Hoisting capture into daemonInboundTraceIdCaptureMiddleware, mounted ahead of auth, the rate limiter and the body parser, means 401/429/400 and unrouted 404s now carry the caller's trace id too. That is strictly better than gating it behind resolveDaemonTelemetryRoute, which skipped every unmatched route.
Pre-auth capture as an exposure. Acceptable. An unauthenticated caller can influence one structured log field, but the value is constrained to 32 lowercase hex characters by the same validator, the breadcrumb runs sanitizeLogText (truncation plus control-character neutralisation, so a crafted header cannot forge log structure), and the breadcrumb is behind a token bucket, so invalid headers on every request cannot flood the daemon log. The camelCase traceId field also stays distinct from the logger's reserved snake_case trace_id prefix, so the span-derived prefix remains unspoofable.
Ownership and downstream consumers (naming these explicitly rather than asserting the direction looks right):
shouldForceSampledis newly exported but deliberately not added to the public barrel. Exactly two consumers:tracer.ts(createSessionRootContext, pre-existing) anddaemon-tracing.ts(forceSampledUnderSampler, new). It stays core-internal.DaemonRequestSpanOptions.parentContextis not a dead switch — read inwithDaemonSpanand populated by the daemon telemetry middleware.extractDaemonTraceContextis the one with a genuine behaviour change, since the_metapath now runs throughforceSampledUnderSamplertoo. Its consumers are four ACP call sites:acp-integration/session/Session.tsand three inacp-integration/acpAgent.ts. Unifying them is right — the in-process bridge injects already-SAMPLED values so forcing is a no-op there, while a direct ACP client's_metais external input on the same footing as the HTTP header, and it would be odd for the two external edges to disagree.@opentelemetry/corereaches only the dynamically importedsdk-impl.tschunk, so it stays out of the CLI's static startup graph;check-serve-fast-path-bundle.jsclassifies it among the cheap eager-safe packages by design.
Non-blocking, for awareness
Forcing SAMPLED on an inbound parent means a caller running head-based ratio sampling with sampled=0 will export nothing while the daemon subtree is exported under the caller's trace id, so some backends will render a trace whose parent span never arrives. That is the documented and, I think, correct trade — honouring sampled=0 would silently delete the request span, everything under next(), and the forwarded session-subprocess spans — and parentbased_always_off still lets an operator opt out. Worth keeping in mind if trace-completeness alerting ever fires on it.
中文说明
在 a4f66bdf08 上完成审查，C=0，予以 approve。
按当前 commit（而非早先几轮）核对：此前令 Test、Serve A/B、Real daemon E2E、web-shell E2E Smoke 四个任务全红的编译中断（extractInboundTraceId 已在 daemon-tracing.ts 定义但漏在 core telemetry barrel 中导出，导致 prepare postinstall 里 tsc --build 失败）已修复，barrel 已补上导出，四个任务均通过。仅 review-pr 仍在运行，它不是构建门禁。
两条提取路径的接受集已对齐。 TRACEPARENT_RE 现与 OTel parseTraceParent 的形状一致（两端 \s?、允许扩展段），且 ff 版本、全零 trace/span id、00 版本带扩展字段这三类拒绝规则一致。因此不会出现「access log 有 traceId 但 span 没挂上父」或反向的矛盾——先前严格四段正则与 OTel future-version 宽容之间的分歧已消除。
response context 上的读/删竞态。 这是我最担心的一点，且修法是结构性的而非打补丁：调用方 trace id 迁到独立的 daemonInboundTraceIdContext symbol 且从不删除，finish 中只清理 daemonTelemetryResponseContext。access log 的 finish 监听器因此不再依赖注册顺序先于遥测中间件。两个 symbol 分离还有第二重意义（注释判断正确）：telemetry response context 的「存在」本身就是 setDaemonTelemetryWorkspace 的准入开关，所以捕获 trace id 绝不能创建它，否则仅发一个 header 就会改变 workspace 归因。两个方向都有测试锁定。
短路请求的覆盖。 把捕获上提为 daemonInboundTraceIdCaptureMiddleware，挂载在 auth、限流器与 body parser 之前，使 401/429/400 与未匹配路由的 404 也带上调用方 trace id，明显优于此前被 resolveDaemonTelemetryRoute 挡掉所有未匹配路由的做法。
pre-auth 捕获的暴露面。 可接受：未认证调用方只能影响一个结构化日志字段，取值被同一校验器限制为 32 位小写十六进制；breadcrumb 经 sanitizeLogText（截断 + 中和控制字符，crafted header 无法伪造日志结构）并受 token bucket 限流，无法靠每请求非法 header 灌爆日志；camelCase traceId 与 logger 保留的 snake_case trace_id 前缀相互独立，span 派生前缀仍不可伪造。
ownership 与下游消费者（明确点名，而非泛泛地说方向正确）：
shouldForceSampled新增导出但刻意未进公共 barrel，恰好两个消费者：tracer.ts（createSessionRootContext，既有）与daemon-tracing.ts（forceSampledUnderSampler，新增），保持 core 内部可见。DaemonRequestSpanOptions.parentContext不是死开关——在withDaemonSpan中被读取，由 daemon 遥测中间件写入。extractDaemonTraceContext是唯一有真实行为变化的（_meta路径现在也走forceSampledUnderSampler），消费者为四处 ACP 调用点：acp-integration/session/Session.ts及acp-integration/acpAgent.ts中三处。统一是对的：进程内 bridge 注入的值本已 SAMPLED，强制为 no-op；而直连 ACP 客户端的_meta与 HTTP header 同属外部输入，两个外部边界行为不一致才反常。@opentelemetry/core仅进入动态 import 的sdk-impl.tschunk，未落入 CLI 静态启动图；check-serve-fast-path-bundle.js本就将其归为可安全 eager 的轻量包。
非阻断、供留意： 对入站 parent 强制 SAMPLED 意味着调用方若按头部比例采样且 sampled=0，其自身不导出、而 daemon 子树仍以调用方 trace id 导出，部分后端会渲染出「父 span 永不到达」的 trace。这是文档化且我认为正确的取舍——尊重 sampled=0 会静默删掉 request span、next() 下全部内容以及转发出去的 session 子进程 span——且 parentbased_always_off 仍留有 operator 的关闭出口。若将来 trace 完整性告警被触发，记得回看这一点。
|
Released in v0.21.15. |
4 participants
What this PR does
The daemon already forwards trace context outbound: prompt requests carry a
traceparentinside 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 W3Ctraceparentheader (an OTel-instrumented client, a proxy, a gateway) got no linkage back to its own trace.This PR extracts
traceparent/tracestatefrom 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_metaextraction (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
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>curl -H "traceparent: 00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01" http://127.0.0.1:4199/daemon/statusqwen-code.daemon.requestspans: the first must share the header's traceId and carryparentSpanContext.spanId = 00f067aa0ba902b7withisRemote: 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:
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
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
traceparentnow 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).tracingresponse, no response-sidetraceparentinjection, no cross-service sampling decisions; Windows/Linux validated by CI only.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。转发标准 W3Ctraceparentheader 的 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 验证计划
如何验证
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 <某目录>curl -H "traceparent: 00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01" http://127.0.0.1:4199/daemon/statusqwen-code.daemon.requestspan：第一个必须与 header 的 traceId 一致并带parentSpanContext.spanId = 00f067aa0ba902b7且isRemote: true；对照组 span 必须保持自己的新 trace 且无 parent。证据（前后对比）
非 UI 改动，无截图，标注 N/A。本分支同一 dry run：
main（改前）上，第一个请求同样只会产生一个无关的新 traceId、没有 parent。单测覆盖：header 提取（有效/缺失/畸形/全零 id/数组值）、请求 span 挂父上下文、中间件透传（键存在与省略）。
npm run typecheck、npm run build及两个改动测试文件本地通过（core 17/17、cli 54/54）。测试环境
环境（可选）
本地
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 切换可作为后续跟进）。tracingresponse、不做响应侧traceparent注入、不做跨服务采样决策；Windows/Linux 仅由 CI 验证。关联 Issue
暂无；设计说明见
docs/design/2026-08-18-daemon-http-inbound-trace-context.md。