feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent - #9391
feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent#9391chiga0 wants to merge 3 commits into
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
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
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
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
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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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)
| type Span, | ||
| } from '@opentelemetry/api'; | ||
| import { logs, type LogAttributes } from '@opentelemetry/api-logs'; | ||
| import { W3CTraceContextPropagator } from '@opentelemetry/core'; |
There was a problem hiding this comment.
[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)
| try { | ||
| parentContext = extractDaemonHttpTraceContext(req.headers); | ||
| } catch { |
There was a problem hiding this comment.
[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 options 为 false。
证据:突变树(删除 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)
| emitDaemonLog( | ||
| 'Rejected invalid inbound traceparent header.', | ||
| { 'http.route': route.route }, |
There was a problem hiding this comment.
[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
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
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.
| if (!extracted) return undefined; | ||
| const spanContext = trace.getSpanContext(extracted); | ||
| if (!spanContext) return undefined; | ||
| if (!shouldForceSampled()) return extracted; |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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.
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。