perf(telemetry): lazy-load the SDK and split OTLP exporter chains by protocol

[doudouOUC]

What this PR does

Makes telemetry SDK loading lazy, in two stages. First, the telemetry entry point is split into a light facade and a heavy implementation half: processes that never enable telemetry (the default) no longer parse and compile the OpenTelemetry NodeSDK, instrumentations, or any exporter — the heavy half is loaded on demand, behind a single-flight dynamic import, only when telemetry is actually enabled. Second, the OTLP exporter chains are split by protocol: the gRPC chain (including the gRPC transport stack and protobuf runtime) and the HTTP chain (including the shared OTLP serialization layer) each live in their own dynamically imported module, so a process loads at most the one chain its configuration needs — file-based telemetry output loads neither, and a misconfigured gRPC setup without an endpoint loads nothing before it skips.

Two supporting changes keep the split honest in the bundled CLI. The bundler now stubs the exporter packages that the OpenTelemetry NodeSDK eagerly requires for env-var-based auto-configuration (an unsupported configuration surface here), because those eager requires would otherwise drag both protocol chains back into the static closure; the stubs fail loudly if ever constructed. And the existing bundle-closure guard gains a third check that fails CI if either protocol chain ever becomes statically reachable from the implementation half again.

Why it's needed

Daemon cold start (#4748) paid roughly 2.1 MiB of telemetry module parse/compile cost in every ACP child process, including the default case where telemetry is disabled. On a 2C4G reference machine this was worth about 144 ms P50 (-7.5%) of process-to-first-session latency for default configurations. The protocol split then targets the remaining cost for users who do enable telemetry (#7264): both protocol chains loaded even though a configuration uses at most one, and on small machines that extra module loading contended with config construction and bootstrap on the CPU, adding back ~50 ms. With the split, telemetry-enabled cold start improved by a further ~51 ms P50 on the same reference machine, with config-construction time cut in half.

Reviewer Test Plan

How to verify

• Default configuration (telemetry disabled): startup behaves as before; no OpenTelemetry SDK modules load. The bundle guard proves this statically: npm run build && cross-env DEV=true npm run bundle && node scripts/check-serve-fast-path-bundle.js passes three closure checks.
• Telemetry enabled with an outfile: spans/logs/metrics land in the outfile as before; neither OTLP protocol chain loads.
• Telemetry enabled with OTLP HTTP: exporters connect and per-signal endpoint overrides, URL validation, and the logs-to-spans bridge behave as before; the gRPC stack never loads.
• Telemetry enabled with OTLP gRPC: exporters connect with gzip compression as before; the HTTP exporters never load. Without a base endpoint, startup is skipped with the same warning as before, and no protocol module loads.
• Unit tests: cd packages/core && npx vitest run src/telemetry (671 tests) and npm run test:scripts (guard tests including six new boundary cases).
• Edge case: in the bundled CLI, setting OTEL_METRICS_EXPORTER=otlp (env-based exporter auto-configuration, never a supported configuration surface here) now fails loudly inside SDK start — caught and logged by the existing error handling — instead of silently exporting to a default localhost endpoint.

Evidence (Before & After)

N/A (no UI change). Paired benchmark on a 2C4G Linux host, 30 pairs per scenario, P50:

• Telemetry disabled (default): process→first-session −144.4 ms (−7.5%).
• Telemetry enabled (outfile), cold start: process→first-session −50.8 ms (1957.6 → 1906.8); channel initialize −37.7 ms; config construction 45.4 → 23.9 ms; bootstrap config init 160.1 → 124.1 ms.
• Telemetry enabled (outfile), preheated: channel initialize −71.6 ms; first-session latency unchanged (~71 ms, session path untouched).
• Functional checks in the same run: concurrency, telemetry-disabled (zero records), and legacy single-session all pass with no residual processes.

Tested on

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

Environment (optional)

macOS: unit tests, typecheck, lint, bundle guards, and runtime smoke tests against the bundled CLI. Linux (2C4G): paired cold/preheated benchmarks against the bundled CLI.

Risk & Scope

• Main risk or tradeoff: in the bundled CLI, env-var-based exporter auto-configuration through the NodeSDK now throws loudly (caught by existing error handling) instead of silently working; this was never a documented or supported configuration surface, and source/dev mode is unaffected. Telemetry initialization is now asynchronous end to end; call sites were already async or fire-and-forget, and events emitted before initialization settle are dropped by the existing initialized gates, same as before.
• Not validated / out of scope: gRPC export against a live collector (exporter assembly is covered by unit tests and runtime smoke checks); the other five lazy-loading candidates listed in Cold-start follow-ups: remaining lazy-loading candidates from the ACP eager-closure audit #7264.
• Breaking changes / migration notes: none for documented configuration surfaces.

Linked Issues

Part of #4748. Implements the first candidate (per-protocol exporter split) from #7264.

中文说明

本 PR 做了什么

将 telemetry SDK 的加载改为两级懒加载。第一级：把 telemetry 入口拆分为轻量 facade 和重量实现两半——默认不开启 telemetry 的进程（绝大多数）不再解析和编译 OpenTelemetry NodeSDK、instrumentation 及任何 exporter；重的那一半只在 telemetry 真正开启时通过单飞动态 import 按需加载。第二级：把 OTLP exporter 链按协议拆分——gRPC 链（含 gRPC 传输栈和 protobuf 运行时）与 HTTP 链（含共享的 OTLP 序列化层）各自独立成动态导入模块，进程最多只加载配置所需的那一条链：文件输出模式两条链都不加载；gRPC 缺少 endpoint 的错误配置在加载任何协议模块之前就跳过。

两项配套改动保证拆分在打包后的 CLI 中真实生效。打包器现在会对 NodeSDK 为环境变量自动配置而急切 require 的 exporter 包打桩（这从来不是本项目支持的配置面），否则这些急切 require 会把两条协议链重新拖回静态闭包；桩一旦被构造会响亮报错。同时现有的 bundle 闭包守卫新增第三项检查：若任一协议链重新静态可达实现半，CI 直接失败。

为什么需要

Daemon 冷启动（#4748）中每个 ACP 子进程都要支付约 2.1 MiB 的 telemetry 模块解析/编译成本，包括默认关闭 telemetry 的场景。在 2C4G 参考机器上，默认配置的进程到首 session 延迟因此高出约 144 ms P50（-7.5%）。协议拆分进一步针对开启 telemetry 的用户（#7264）：配置最多用到一条协议链却加载了两条，且在小机器上这些额外的模块加载与 config 构建、bootstrap 抢占 CPU，额外增加约 50 ms。拆分后，同一参考机器上开启 telemetry 的冷启动再降约 51 ms P50，config 构建耗时减半。

评审验证计划

如何验证

• 默认配置（telemetry 关闭）：启动行为不变；不加载任何 OpenTelemetry SDK 模块。bundle 守卫静态证明这一点：npm run build && cross-env DEV=true npm run bundle && node scripts/check-serve-fast-path-bundle.js 三项闭包检查全过。
• 开启 telemetry 且配置 outfile：spans/logs/metrics 照常写入文件；两条 OTLP 协议链都不加载。
• 开启 telemetry 且配置 OTLP HTTP：exporter 正常连接，per-signal endpoint 覆盖、URL 校验、logs 转 spans 桥行为不变；gRPC 栈永不加载。
• 开启 telemetry 且配置 OTLP gRPC：exporter 照常以 gzip 压缩连接；HTTP exporter 永不加载。缺少 base endpoint 时以与之前相同的警告跳过启动，且不加载任何协议模块。
• 单元测试：cd packages/core && npx vitest run src/telemetry（671 个）以及 npm run test:scripts（守卫测试含六个新边界用例）。
• 边界情况：打包后的 CLI 中设置 OTEL_METRICS_EXPORTER=otlp（基于环境变量的 exporter 自动配置，从来不是支持的配置面）现在会在 SDK 启动时响亮失败——被现有错误处理捕获并记录——而不是静默导出到默认 localhost 端点。

证据（前后对比）

N/A（无 UI 变化）。2C4G Linux 机器成对基准，每场景 30 对，P50：

• Telemetry 关闭（默认）：进程→首 session −144.4 ms（−7.5%）。
• Telemetry 开启（outfile）冷启动：进程→首 session −50.8 ms（1957.6 → 1906.8）；channel initialize −37.7 ms；config 构建 45.4 → 23.9 ms；bootstrap config init 160.1 → 124.1 ms。
• Telemetry 开启（outfile）预热：channel initialize −71.6 ms；首 session 延迟不变（约 71 ms，session 路径未改动）。
• 同一轮的功能检查：并发、telemetry 关闭（零记录）、legacy 单 session 全部通过，无残留进程。

测试平台

macOS：单元测试、typecheck、lint、bundle 守卫、针对打包 CLI 的运行时冒烟。Linux（2C4G）：针对打包 CLI 的成对冷启动/预热基准。

风险与范围

• 主要风险/权衡：打包后的 CLI 中，通过 NodeSDK 的环境变量 exporter 自动配置现在会响亮抛错（被现有错误处理捕获），不再静默生效；这从来不是文档化或受支持的配置面，源码/开发模式不受影响。telemetry 初始化现在端到端异步；调用方原本就是 async 或 fire-and-forget，初始化完成前产生的事件由现有的已初始化门控丢弃，与之前一致。
• 未验证/超出范围：对真实 collector 的 gRPC 导出（exporter 组装已由单测和运行时冒烟覆盖）；Cold-start follow-ups: remaining lazy-loading candidates from the ACP eager-closure audit #7264 中列出的其余五个懒加载候选。
• 破坏性变更/迁移说明：对文档化配置面无。

关联 Issue

属于 #4748 的一部分。实现了 #7264 中的第一个候选项（按协议拆分 exporter）。

[doudouOUC]

E2E Test & Benchmark Report

Environment: 2C4G Linux (Alibaba Cloud ECS), bundled CLI (dist/cli.js), paired control/candidate runs (alternating, 30 pairs per scenario) to cancel machine drift.

Stage 1 — lazy SDK facade (control = pre-change main)

Default configuration (telemetry disabled), cold start, P50 over 30 pairs:

Metric	Control	Candidate	Δ
process → first session complete	1919.8 ms	1775.4 ms	−144.4 ms (−7.5%)

Stage 2 — protocol split (control = stage 1 build, telemetry enabled with outfile)

Scenario	Metric	Control	Candidate	Δ
cold	channel initialize	1015.8 ms	978.1 ms	−37.7 ms
cold	first session complete	1957.6 ms	1906.8 ms	−50.8 ms (−2.6%)
cold	config construction	45.4 ms	23.9 ms	−21.5 ms
cold	bootstrap config init	160.1 ms	124.1 ms	−36.0 ms
preheated	channel initialize	1174.5 ms	1102.9 ms	−71.6 ms
preheated	first session latency	71.2 ms	73.7 ms	+2.5 ms (noise)

The cold-start gain exceeds the channel-initialize gain because on 2 cores the ~2 MiB protocol-chain module load previously contended with config construction and bootstrap for CPU; removing the load also halves config-construction time.

Functional checks (same run, telemetry enabled builds)

• ✅ Concurrent multi-session over one daemon — all sessions complete, no residual processes
• ✅ Telemetry disabled — zero telemetry records written
• ✅ Legacy single-session path — behaves as before

Static guards & unit tests (macOS, this branch)

• ✅ Bundle closure guard: 3/3 checks (serve fast path, ACP closure, sdk-impl protocol boundary)
• ✅ packages/core telemetry unit tests: 671/671
• ✅ scripts tests: 559 passed, 9 skipped
• ✅ typecheck / lint / prettier clean

[qwen-code-ci-bot]

🩺 serve daemon A/B

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

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

— Qwen Code · serve A/B

[qwen-code-ci-bot]

Thanks for the PR! This is a well-researched performance optimization with solid benchmarks.

Template looks good ✓

Problem: Observed and well-documented. The metafile audit in the design doc identifies the 2.16 MiB telemetry cluster as the single largest coherent block in the ACP child's eager static closure. Paired benchmarks on 2C4G (30 pairs per scenario) show −144 ms P50 for the default-disabled path and −51 ms P50 for the telemetry-enabled path. This is a real, measured cost — not theoretical hardening. Linked to #4748 and #7264.

Direction: Aligned. Cold start latency is a tracked priority (#4748). The two-phase lazy loading strategy is the natural next step after #7182 (TUI module removal). CHANGELOG shows performance improvements are regularly shipped in this area. However, this touches telemetry infrastructure — a sensitive area where a regression silently degrades observability. Flagging for maintainer awareness per the Stage 1c escalation rule.

Size: Touches core paths (packages/core/src/telemetry/*, packages/core/src/config/config.ts). Breakdown (updated for new commit 106c7e6b):

• 1565 production logic lines (additions + deletions) across core telemetry modules, esbuild config, CLI serve/startup, and bundle guard scripts — up from 866 at the previous review; the new commit adds ~234 lines for the init/shutdown race fix and the otlp-urls.ts / sdk-node-exporter-stub.js extractions
• 516 test lines (63 telemetry SDK tests including 4 new lifecycle race tests, 6 stub resolve tests, 25 bundle guard tests)
• 365 docs/design lines (two design documents)
• Not a refactor type → no hard block. 500+ production lines in core → maintainer awareness required. 1000+ advisory: this is a large PR — consider whether any part could be split, though the scope feels coherent as-is.

Approach: Scope feels right. The three-way file split (facade → impl → per-protocol modules) is the minimal decomposition that achieves the stated goal. The new commit (fix(telemetry): close lazy SDK init/shutdown races and make load failure non-fatal) is a natural follow-up: it moves the dynamic import inside the try/catch so a chunk-load failure degrades gracefully, fixes the shutdown/init race by awaiting telemetryInitPromise inside shutdownTelemetry(), and extracts resolveHttpOtlpUrl to a leaf module to avoid a facade/impl cycle. Each change is justified and narrowly scoped. No drive-by refactors or scope creep.

Moving on to code review. 🔍

中文说明

感谢贡献！这是一个经过充分研究的性能优化，有可靠的基准数据。

模板完整 ✓

问题：已观测且有充分文档。 设计文档中的 metafile 审计将 2.16 MiB 的 telemetry 集群识别为 ACP 子进程静态闭包中最大的连贯块。2C4G 上的成对基准（每场景 30 对）显示默认关闭路径 P50 降低 144 ms，开启路径 P50 降低 51 ms。这是真实可测量的成本——不是理论性加固。关联 #4748 和 #7264。

方向：对齐。 冷启动延迟是跟踪中的优先级（#4748）。两阶段懒加载策略是 #7182（TUI 模块移除）后的自然下一步。然而，此改动触及 telemetry 基础设施——一个静默回归会损害可观测性的敏感区域。按 Stage 1c 升级规则标记给维护者关注。

规模：触及核心路径（packages/core/src/telemetry/*、packages/core/src/config/config.ts）。分类（已更新为新提交 106c7e6b）：

• 1565 行生产逻辑（增删合计），跨核心 telemetry 模块、esbuild 配置、CLI serve/startup 和 bundle 守卫脚本——较上次审查的 866 行增加；新提交为 init/shutdown 竞态修复及 otlp-urls.ts / sdk-node-exporter-stub.js 提取新增约 234 行
• 516 行测试（63 个 telemetry SDK 测试含 4 个新生命周期竞态测试、6 个 stub 解析测试、25 个 bundle 守卫测试）
• 365 行设计文档（两份设计文档）
• 不是 refactor 类型 → 无硬性阻止。核心路径 500+ 行生产代码 → 需维护者关注。 1000+ 大 PR 建议：PR 规模较大，可考虑是否有部分可拆分，但整体范围看起来是连贯的。

方案：范围合理。 三层文件拆分（facade → impl → 按协议模块）是实现目标所需的最小分解。新提交（fix(telemetry): close lazy SDK init/shutdown races and make load failure non-fatal）是自然的后续：将动态 import 移入 try/catch 使 chunk 加载失败时优雅降级，通过在 shutdownTelemetry() 中等待 telemetryInitPromise 修复 shutdown/init 竞态，并将 resolveHttpOtlpUrl 提取到叶模块以避免 facade/impl 循环。每项改动都有明确理由且范围精确。没有顺手重构或范围蔓延。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

_{Reviewed at 106c7e6b692db00eef645179b59cdea60f797f0a · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Code Review

I read the diff against an independent proposal formed from the PR title and "Why it's needed" section alone. My independent proposal for the lazy-loading goal arrived at the same architecture: a light facade keeping isTelemetrySdkInitialized() synchronous, a heavy impl behind one dynamic import, and per-protocol exporter modules. The PR matches this and adds the esbuild stub plugin for sdk-node's eager exporter requires — a narrowly-scoped, well-justified addition I wouldn't have thought of without reading the bundler closure.

New commit review (106c7e6b — fix init/shutdown races, make load failure non-fatal):

The commit addresses three real problems introduced by the lazy-loading split:

1. Shutdown/init race. Previously shutdownTelemetry() checked telemetryInitialized synchronously and no-op'd if init was still in flight — leaking a started SDK whose buffered spans never flush. The fix captures telemetryInitPromise at call time and awaits it (with .catch(() => {})) inside the shutdown promise before checking the flag. When the awaited init didn't finish initializing, it clears telemetryShutdownPromise so a later real shutdown isn't short-circuited by a stale no-op. Correct and minimal.

2. Chunk-load failure non-fatal. The dynamic import('./sdk-impl.js') moves inside the try/catch. The comment explains why: "the daemon runtime awaits this without a catch." A failed import now degrades telemetry instead of rejecting. The finally block still clears telemetryInitPromise so a failed import can be retried. Correct.

3. otlp-urls.ts extraction. resolveHttpOtlpUrl moves to a leaf module free of @opentelemetry/* imports, so both sdk.ts (facade) and sdk-impl.ts can import it without forming a cycle. The function is unchanged. Clean.

esbuild stub Proxy fix: the then and __esModule probe handling is correct — without it, await import() of a stubbed module would see a callable then and try to use it as a thenable, causing confusing errors. Returning undefined for these probes makes the stub behave as a plain namespace.

sdk-node-exporter-stub.js extraction: the resolve decision moves to a unit-testable module with Windows path normalization (replace(/\\/g, '/')). The isStubbedSdkNodeExporterImport function is well-documented and the 6 new tests cover the key cases (stubbed vs. non-stubbed packages, sdk-node vs. other importers, Windows backslash paths, empty/missing importer).

Bundle guard extension: adding explicit HTTP exporter packages and @opentelemetry/otlp-exporter-base to the forbidden list makes the guard self-describing and survives upstream dependency restructuring. Belt-and-suspenders, but justified for a security-critical boundary.

Tests: the 4 new lifecycle tests are well-targeted — single-flight init, retry after failure, shutdown/init race regression guard, and stale shutdown promise cleanup. All 63 telemetry SDK tests pass, all 6 stub tests pass, all 25 bundle guard tests pass.

No critical issues. No AGENTS.md violations. The changes are minimal, well-tested, and directly address the stated problems.

Real-Scenario Testing

Default path (telemetry disabled), dev build from PR head 106c7e6b:

$ npm run dev -- -p 'say hello in one word' --output-format text 2>&1 | head -50

> @qwen-code/qwen-code@0.20.0 dev
> node scripts/dev.js -p say hello in one word --output-format text

Warning: QWEN_HOME points to "/home/github-runner/actions-runner-11/_work/_temp/qwen-home" but no settings.json was found there. Existing config remains at "/home/github-runner/.qwen" — OAuth tokens,
settings, memory, extensions, and skills are not auto-migrated. Copy them manually if you want them to apply at the new location.
Warning: QWEN_HOME points to "/home/github-runner/actions-runner-11/_work/_temp/qwen-home" but no settings.json was found there. Existing config remains at "/home/github-runner/.qwen" — OAuth tokens,
settings, memory, extensions, and skills are not auto-migrated. Copy them manually if you want them to apply at the new location.
Hello

CLI starts and responds correctly. The QWEN_HOME warnings are CI environment noise (no settings.json at the CI home), unrelated to this PR. No telemetry errors, no SDK load failures. The default-disabled path works as expected.

Unit test results at PR head:

• packages/core/src/telemetry/sdk.test.ts: 63/63 passed (including 4 new lifecycle race tests)
• scripts/tests/sdk-node-exporter-stub.test.js: 6/6 passed
• scripts/tests/serve-fast-path-bundle-check.test.js: 25/25 passed

中文说明

代码审查

我在阅读 diff 之前，仅根据 PR 标题和"为什么需要"部分形成了独立方案。我的独立方案得出了相同的架构：保持 isTelemetrySdkInitialized() 同步的轻量 facade、在一个动态 import 后面的重量 impl、以及按协议的 exporter 模块。PR 与此匹配，并增加了针对 sdk-node 急切 exporter require 的 esbuild 打桩插件——范围精确、理由充分。

新提交审查（106c7e6b — 修复 init/shutdown 竞态，使加载失败非致命）：

该提交解决了懒加载拆分引入的三个真实问题：

1. Shutdown/init 竞态。 之前 shutdownTelemetry() 同步检查 telemetryInitialized，若 init 仍在进行中则直接 no-op——导致已启动的 SDK 泄漏，缓冲的 spans 永远不会 flush。修复方案在调用时捕获 telemetryInitPromise，在 shutdown promise 内部等待它（带 .catch(() => {})），然后再检查标志。当等待的 init 未完成初始化时，清除 telemetryShutdownPromise，使后续的真实 shutdown 不会被陈旧的 no-op 短路。正确且最小化。

2. Chunk 加载失败非致命。 动态 import('./sdk-impl.js') 移入 try/catch 内部。注释解释了原因："daemon 运行时在没有 catch 的情况下等待它。"失败的 import 现在降级 telemetry 而不是 reject。finally 块仍然清除 telemetryInitPromise，使失败的 import 可以重试。正确。

3. otlp-urls.ts 提取。 resolveHttpOtlpUrl 移到一个不含 @opentelemetry/* import 的叶模块，使 sdk.ts（facade）和 sdk-impl.ts 都能导入它而不形成循环。函数本身未变。干净。

esbuild stub Proxy 修复： then 和 __esModule 探测处理正确——没有它，await import() 一个被打桩的模块会看到可调用的 then 并试图将其用作 thenable，导致令人困惑的错误。对这些探测返回 undefined 使 stub 表现为普通命名空间。

sdk-node-exporter-stub.js 提取： 解析决策移到可单元测试的模块，带 Windows 路径规范化（replace(/\\/g, '/')）。6 个新测试覆盖了关键场景。

Bundle 守卫扩展： 将显式 HTTP exporter 包和 @opentelemetry/otlp-exporter-base 加入禁止列表，使守卫自描述并能承受上游依赖重构。

测试： 4 个新生命周期测试目标明确——单飞 init、失败后重试、shutdown/init 竞态回归守卫、陈旧 shutdown promise 清理。63 个 telemetry SDK 测试全部通过，6 个 stub 测试全部通过，25 个 bundle 守卫测试全部通过。

无关键问题。无 AGENTS.md 违规。改动最小化、测试充分、直接解决所述问题。

真实场景测试

默认路径（telemetry 关闭），PR head 106c7e6b 的 dev 构建：CLI 正常启动并响应，无 telemetry 错误，无 SDK 加载失败。QWEN_HOME 警告是 CI 环境噪音，与 PR 无关。

单元测试结果：63/63、6/6、25/25 全部通过。

— Qwen Code · qwen3.7-max

_{Reviewed at 106c7e6b692db00eef645179b59cdea60f797f0a · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Confidence: 3/5 — clean review across every stage including the new race-fix commit, but 1565 production lines in core telemetry infrastructure and the Stage 1c telemetry escalation rule require a maintainer's sign-off before merge.

Stepping back: this PR has now been through two review passes. The original lazy-loading split (facade/impl/per-protocol) was already well-engineered. The new commit (106c7e6b) closes real gaps that the first pass introduced — the shutdown/init race, the unhandled chunk-load rejection, and the facade/impl import cycle — and does so with minimal, well-tested changes. My independent proposal for each of these three problems matches what the author did.

The evidence remains strong: paired benchmarks show −144 ms P50 for the default-disabled path and −51 ms P50 for telemetry-enabled. All 63 telemetry SDK tests pass (including 4 new lifecycle race tests), all 6 stub resolve tests pass, all 25 bundle guard tests pass. The tmux headless test confirms the default path works with no telemetry errors.

The new commit specifically:

• The shutdown/init race fix is the kind of thing that would silently bite in production — a fire-and-forget init racing a shutdown at process exit, leaking a started SDK. The fix is correct: await the in-flight init inside the shutdown promise, clear the stale shutdown promise when init didn't complete.
• Moving the dynamic import inside the try/catch is the right call — the daemon runtime awaits initializeTelemetry() without a catch, so a chunk-load failure must degrade, not reject.
• The otlp-urls.ts leaf extraction is the cleanest way to break the facade/impl cycle.
• The esbuild Proxy then/__esModule fix prevents a real interop bug with await import() of stubbed modules.

If I had to maintain this in six months, I'd thank the author: the facade/impl boundary is crisp, the race semantics are well-documented in comments, the bundle guards prevent silent regression, and the test coverage is comprehensive.

Why defer instead of approve: the PR touches telemetry infrastructure (Stage 1c escalation area) and totals 1565 production logic lines in core paths — well above the 500-line threshold that flags for maintainer awareness and the 1000-line large-PR advisory. These are policy-driven deferrals, not genuine concerns about the code quality or correctness. A human maintainer should confirm the architectural direction for telemetry loading and sign off on the env-var exporter stubbing behavior change (loud throw instead of silent localhost export — documented, but worth a human eye).

⏸️ Deferring to the maintainer for sign-off. The code is ready — needs a human call on the telemetry infrastructure change scope.

中文说明

置信度：3/5 — 包括新竞态修复提交在内，所有阶段审查均无问题，但核心 telemetry 基础设施中 1565 行生产代码及 Stage 1c telemetry 升级规则要求维护者签字后才能合并。

回顾全局：此 PR 已经历两轮审查。原始的懒加载拆分（facade/impl/按协议）已经工程精良。新提交（106c7e6b）关闭了第一轮引入的真实缺口——shutdown/init 竞态、未处理的 chunk 加载 reject、以及 facade/impl import 循环——并以最小化、充分测试的改动做到这一点。我对这三个问题的独立方案与作者的做法一致。

证据依然有力：成对基准显示默认关闭路径 P50 降低 144 ms，开启路径 P50 降低 51 ms。63 个 telemetry SDK 测试全部通过（含 4 个新生命周期竞态测试），6 个 stub 解析测试全部通过，25 个 bundle 守卫测试全部通过。tmux 无头测试确认默认路径正常工作且无 telemetry 错误。

新提交具体地：

• shutdown/init 竞态修复是那种会在生产中静默咬人的问题——进程退出时 fire-and-forget init 与 shutdown 竞态，泄漏已启动的 SDK。修复正确。
• 将动态 import 移入 try/catch 是正确选择——daemon 运行时在没有 catch 的情况下等待 initializeTelemetry()，所以 chunk 加载失败必须降级而非 reject。
• otlp-urls.ts 叶提取是打破 facade/impl 循环的最干净方式。
• esbuild Proxy then/__esModule 修复防止了 await import() 打桩模块时的真实互操作 bug。

如果六个月后需要维护，我会感谢作者：facade/impl 边界清晰，竞态语义在注释中有充分文档，bundle 守卫防止静默回归，测试覆盖全面。

为什么转交而非批准： PR 触及 telemetry 基础设施（Stage 1c 升级区域），核心路径共 1565 行生产逻辑——远超需维护者关注的 500 行阈值及 1000 行大 PR 建议。这是策略驱动的转交，不是对代码质量或正确性的真实担忧。人类维护者应确认 telemetry 加载的架构方向，并对环境变量 exporter 打桩的行为变更签字。

⏸️ 转交维护者签字。代码已就绪——需要人工对 telemetry 基础设施变更范围做决定。

— Qwen Code · qwen3.7-max

_{Reviewed at 106c7e6b692db00eef645179b59cdea60f797f0a · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Qwen Code review paused — model quota exhausted. Qwen review stopped: the model API quota is exhausted (reset at 07-20 07:32:00 UTC.). Transient errors auto-retry, but a quota reset is too far out to wait on a runner. Re-run once it resets by commenting @qwen-code /review. See workflow logs.

[qwen-code-ci-bot]

Reviewed. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: chunk 1, chunk 7, chunk 4, chunk 3, chunk 5, chunk 6, chunk 8, chunk 9, chunk 2 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Test coverage matrix (whole-diff), Agent 1b: Removed-behavior audit, Agent 1c: Cross-file tracer, Agent 7: Build & test verification, Invariant agent A: state, timers, collections — packages/core/src/telemetry/sdk.ts, Invariant agent B: counters, return values, error taxonomies — packages/core/src/telemetry/sdk.ts, Invariant agent C: config fields, early returns — packages/core/src/telemetry/sdk.ts — its prompt was built, but no agent on record was launched with it. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.

— qwen3.7-max via Qwen Code /review

[wenshao]

Review

Reviewed at ea06eed. Nice piece of work — the two-level split is well motivated, the layering is clean (sdk.ts facade → sdk-impl.ts → per-protocol chain), and gating it behind a metafile closure check rather than a convention comment is the right call. I confirmed the premise independently: @opentelemetry/sdk-node@0.203.0 does eagerly require() all six OTLP exporters plus zipkin/prometheus at the top of build/src/sdk.js and build/src/utils.js, so the bundler stub is genuinely required for the split to hold. I also confirmed the stub is safe: NodeSDK only reaches getSpanProcessorsFromEnv() when spanProcessors is absent, and configureLoggerProviderFromEnv() when logRecordProcessors is absent — this code always passes both (an empty array is truthy, so the env branches stay unreachable), and configureMetricProviderFromEnv() returns early unless OTEL_METRICS_EXPORTER is explicitly set. The documented blast radius matches the code.

Three things I'd like addressed before merge, then some smaller notes.

1. shutdownTelemetry() can now be outrun by initialization (highest-value fix)

initializeTelemetry became async, but shutdownTelemetry still gates on the synchronous flag:

// packages/core/src/telemetry/sdk.ts:154
if (!telemetryInitialized || !sdk) {
  return;
}

Both fire-and-forget callers can now leave an init in flight across that check:

• packages/core/src/config/config.ts:2197 — void Promise.resolve(initializeTelemetry(this)).catch(...)
• packages/cli/src/startup/startup-prefetch.ts — runDeferredTask('telemetry_init', ...)

If shutdown lands inside the window, it early-returns as a no-op; the pending continuation then runs sdk.start() and sets telemetryInitialized = true. Net effect: the SDK is registered after teardown was requested, nothing ever calls sdk.shutdown(), and whatever BatchSpanProcessor / BatchLogRecordProcessor has buffered is never flushed.

This isn't theoretical — the window is exactly the dynamic-import cost the PR is deferring (~50–150 ms cold, per your own benchmark), and it applies to the paths that don't defer: deferTelemetryInitialization is interactive && !isAcpMode && !question (packages/cli/src/config/config.ts:2070), so ACP children and headless -p runs both take the eager Config.initialize() fire-and-forget path. A headless run that fails fast right after config construction, or a short-lived ACP child, is a plausible trigger. nonInteractiveCli.ts:2087 and config.ts:4075 both await shutdownTelemetry() expecting it to mean something.

Either of these closes it:

// A: make shutdown wait for a pending init
export async function shutdownTelemetry(): Promise<void> {
  if (telemetryShutdownPromise) return telemetryShutdownPromise;
  if (telemetryInitPromise) {
    await telemetryInitPromise.catch(() => {});
  }
  if (!telemetryInitialized || !sdk) return;
  ...

// B: make init observe a shutdown request before it starts
// set `shutdownRequested = true` at the top of shutdownTelemetry(), then in the
// init continuation, after `await startTelemetrySdk(config)`:
if (shutdownRequested) return;

A regression test is cheap here: kick off initializeTelemetry(cfg) without awaiting, call await shutdownTelemetry(), await the init, then assert mockNodeSdk.shutdown was called (or that start was not).

2. The try/catch no longer covers the parts that can now fail

In the new initializeTelemetry, only sdk.start() is guarded:

const { startTelemetrySdk } = await import('./sdk-impl.js');   // unguarded
if (telemetryInitialized) return;
const started = await startTelemetrySdk(config);               // unguarded (does its own await import())
if (!started) return;
sdk = started.sdk;
const debugLogger = createDebugLogger('OTEL');
try {
  sdk.start();
  ...
} catch (error) { ... }

Both await import()s sit outside it, so initializeTelemetry can now return a rejected promise where previously it could not fail at all. packages/cli/src/serve/run-qwen-serve.ts:2928 awaits it inside buildRuntime(), and the deps.bridge call site at :4458 has no .catch() — a chunk-load failure would abort daemon runtime startup. Telemetry has never been able to do that before.

Cheap fix: move the try up so it wraps the whole continuation body, leaving the existing debugLogger.error('Error starting OpenTelemetry SDK:', ...) as the single failure sink. Telemetry should stay non-fatal.

3. The "same as before" claim in the PR body holds only for the deferred path

events emitted before initialization settle are dropped by the existing initialized gates, same as before

True for the TUI path, which was already deferred. But on the non-deferred path — ACP mode and headless — Config.initialize() previously had telemetry fully up by the time it returned, and now returns with init still in flight. Anything logged in that window (session-start records, logCliConfiguration-style events) is now silently dropped by isTelemetrySdkInitialized(). That's a real behavior change worth either calling out explicitly in the PR body, or removing by awaiting the init — Config.initialize() is already async (packages/core/src/config/config.ts:2178), so awaiting costs nothing structurally, though it does put the import back on the ACP child's critical path. Your call, but the tradeoff should be a stated decision rather than an implied "no change".

Smaller notes

• Circular import. sdk-impl.ts statically imports resolveHttpOtlpUrl from ./sdk.js, which dynamically imports sdk-impl.js. Runtime-safe (the facade is fully evaluated before the dynamic import fires) and the bundler handles it, but it makes the "light facade / heavy impl" boundary non-obvious. Moving resolveHttpOtlpUrl + OTLP_SIGNAL_PATHS into a leaf module (otlp-urls.ts) would make the layering acyclic and keep both halves honest.
• Proxy stub returns a callable for every string key, including then and __esModule. Harmless for sdk-node's namespace-property access pattern, but if anything ever resolves that namespace through a promise, the thenable check would call then and throw a confusing "Attempted to construct: then". Worth returning undefined for then, __esModule, and Symbol.toStringTag.
• args.importer.includes('@opentelemetry' + path.sep + 'sdk-node') is untested on Windows, which is exactly the platform marked ⚠️ in the test matrix. If esbuild hands back a separator this doesn't expect, the stub silently no-ops and both chains return to the static closure. The good news is that failure mode is loud — the new checkSdkImplProtocolBoundary would flag @grpc/grpc-js — but only if the bundle check runs on Windows in CI. A tiny unit test over the resolve predicate (exported from esbuild.config.js, or a small helper module) would pin this down independently of platform.
• Guard package list. FORBIDDEN_OTLP_PROTOCOL_PACKAGES catches the HTTP chain only indirectly, via @opentelemetry/otlp-transformer. That works today, but listing @opentelemetry/exporter-{trace,logs,metrics}-otlp-http and @opentelemetry/otlp-exporter-base explicitly would make the guard say what it means and survive an upstream dependency reshuffle.
• Retry semantics. Clearing telemetryInitPromise in finally means the gRPC-without-endpoint skip path re-enters and re-warns on every subsequent call. Same as the old behavior, so not a regression — just noting it's intentional-looking and fine.
• Test coverage. The sdk.test.ts delta is a mechanical async/await sweep, and the six new cases all target the guard script with synthetic metafiles. Nothing exercises the new concurrency surface: single-flight (two concurrent calls → one NodeSDK), retry-after-failed-import, or the shutdown race in §1. The first two are a few lines each.
• Design docs are committed with Status: Draft — worth flipping on merge.

Everything else — the OTLP feedback-loop guard, the URL boundary matching, the propagator gate — moved across intact; I diffed the relocated block and found no behavioral drift.

中文版

评审意见

基于 ea06eed 审阅。整体质量很高：两级拆分动机充分，分层清晰（sdk.ts facade → sdk-impl.ts → 按协议的 exporter 链），并且用 metafile 闭包检查而非注释约定来守护边界，方向是对的。我独立验证了前提：@opentelemetry/sdk-node@0.203.0 确实在 build/src/sdk.js 与 build/src/utils.js 顶层急切 require() 了全部六个 OTLP exporter 以及 zipkin/prometheus，因此打桩确有必要。同时也确认了打桩是安全的：NodeSDK 只有在缺少 spanProcessors 时才走 getSpanProcessorsFromEnv()，缺少 logRecordProcessors 时才走 configureLoggerProviderFromEnv()，而本项目始终传入这两者（空数组也是 truthy，所以 env 分支不可达）；configureMetricProviderFromEnv() 在未显式设置 OTEL_METRICS_EXPORTER 时直接返回空。PR 描述中的影响面与代码一致。

有三点希望在合入前处理，之后是一些小建议。

1. shutdownTelemetry() 可能被初始化"反超"（最值得修的一处）

initializeTelemetry 已改为异步，但 shutdownTelemetry 仍然只看同步标志位（packages/core/src/telemetry/sdk.ts:154 的 if (!telemetryInitialized || !sdk) return;）。两处 fire-and-forget 调用方——packages/core/src/config/config.ts:2197 和 startup-prefetch.ts 的 runDeferredTask('telemetry_init', ...)——都可能让一次初始化"悬在"该检查两侧。若 shutdown 落在这个窗口内，它会直接空转返回；随后挂起的续体执行 sdk.start() 并把 telemetryInitialized 置为 true。结果是：SDK 在请求关闭之后才注册，sdk.shutdown() 永远不会被调用，BatchSpanProcessor / BatchLogRecordProcessor 中缓冲的数据全部丢失。

这个窗口并非理论值——它正是本 PR 所延后的动态 import 成本（按你自己的基准，冷启动约 50–150 ms），且作用于不延迟的路径：deferTelemetryInitialization 为 interactive && !isAcpMode && !question（packages/cli/src/config/config.ts:2070），因此 ACP 子进程与 headless -p 都走 Config.initialize() 的 fire-and-forget 分支。一个在 config 构造后很快失败的 headless 运行，或一个短命的 ACP 子进程，都是合理的触发场景。nonInteractiveCli.ts:2087 与 config.ts:4075 都 await shutdownTelemetry()，是期望它真正生效的。

两种修法任选其一：(A) 在 shutdownTelemetry 开头 await telemetryInitPromise.catch(() => {})；(B) 在 shutdownTelemetry 置一个 shutdownRequested 标志，初始化续体在 await startTelemetrySdk(config) 之后检查该标志并提前返回。回归测试也很便宜：不 await 地发起 initializeTelemetry(cfg)，然后 await shutdownTelemetry()，再 await 初始化，最后断言 mockNodeSdk.shutdown 被调用（或 start 未被调用）。

2. try/catch 不再覆盖真正会失败的部分

新的 initializeTelemetry 中只有 sdk.start() 被保护，两处 await import()（包括 startTelemetrySdk 内部按协议的动态 import）都在其外。于是 initializeTelemetry 现在可能返回一个 rejected promise，而此前它根本不会失败。packages/cli/src/serve/run-qwen-serve.ts:2928 在 buildRuntime() 中 await 了它，而 :4458 处的 deps.bridge 调用点没有 .catch()——一次 chunk 加载失败会直接中断 daemon runtime 启动。telemetry 此前从来不具备这种能力。

修法很简单：把 try 上移，包住整个续体，让现有的 debugLogger.error('Error starting OpenTelemetry SDK:', ...) 作为唯一的失败落点。telemetry 应当保持非致命。

3. PR 描述中"与之前一致"的说法只对延迟路径成立

对已经延迟的 TUI 路径成立；但在非延迟路径（ACP 与 headless）上，Config.initialize() 此前返回时 telemetry 已完全就绪，现在返回时初始化仍在进行中。该窗口内产生的事件（session 起始记录、logCliConfiguration 一类）会被 isTelemetrySdkInitialized() 静默丢弃。这是真实的行为变化，建议要么在 PR 描述中显式说明，要么直接 await 掉——Config.initialize() 本身已是 async（packages/core/src/config/config.ts:2178），await 在结构上没有代价，只是会把 import 放回 ACP 子进程的关键路径。取舍由你决定，但应当是一个明示的决策，而不是隐含的"无变化"。

其他小点

• 循环依赖：sdk-impl.ts 静态 import ./sdk.js 的 resolveHttpOtlpUrl，而 sdk.js 又动态 import sdk-impl.js。运行时安全、打包器也能处理，但让"轻 facade / 重实现"的边界变得不直观。把 resolveHttpOtlpUrl 与 OTLP_SIGNAL_PATHS 挪进一个叶子模块（如 otlp-urls.ts）可让分层无环。
• Proxy 桩对任意字符串键都返回可调用函数，包括 then 与 __esModule。对 sdk-node 的命名空间属性访问模式无害，但一旦有人通过 promise 解析该命名空间，thenable 检查会调用 then 并抛出令人困惑的 "Attempted to construct: then"。建议对 then、__esModule、Symbol.toStringTag 返回 undefined。
• args.importer.includes('@opentelemetry' + path.sep + 'sdk-node') 未在 Windows 上验证，而 Windows 正是测试矩阵中标 ⚠️ 的平台。若分隔符不符预期，桩会静默失效、两条链回到静态闭包。好在这种失效是响亮的——新增的 checkSdkImplProtocolBoundary 会报 @grpc/grpc-js——但前提是 bundle 检查在 Windows CI 上运行。为该 resolve 判定加一个小单测，可以与平台无关地钉住这一点。
• 守卫包清单：FORBIDDEN_OTLP_PROTOCOL_PACKAGES 只通过 @opentelemetry/otlp-transformer 间接覆盖 HTTP 链。目前有效，但显式列出 @opentelemetry/exporter-{trace,logs,metrics}-otlp-http 与 @opentelemetry/otlp-exporter-base 会让守卫更自解释，也更能扛住上游依赖结构调整。
• 重试语义：在 finally 中清空 telemetryInitPromise，意味着 gRPC 缺 endpoint 的跳过路径每次调用都会重新进入并重复告警。与旧行为一致，不算回归，仅作记录。
• 测试覆盖：sdk.test.ts 的改动是机械式的 async/await 铺开，六个新用例全部针对守卫脚本的合成 metafile。新增的并发面没有任何覆盖：单飞（两次并发调用只产生一个 NodeSDK）、失败后重试、以及 §1 的 shutdown 竞态。前两者各只需几行。
• 设计文档仍是 Status: Draft，合入时值得翻成正式状态。

其余部分——OTLP 反馈环守卫、URL 边界匹配、propagator 开关——迁移完整；我逐行比对了搬运的代码块，未发现行为漂移。

[doudouOUC]

Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built. Not reviewed: coverage — the plan could not be used (ENOENT: no such file or directory, open '.qwen/tmp/qwen-review-pr-7276-fetch.json'), so this run cannot show that any of the diff was read. Not reviewed: verification — could not check that Step 4 and Step 5 ran (ENOENT: no such file or directory, open '.qwen/tmp/qwen-review-pr-7276-fetch.json').

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

Reviewed — no blockers. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

— qwen3.7-max via Qwen Code /review

[doudouOUC]

[qwen] Thanks for the careful review — all three pre-merge items are addressed in 106c7e6, plus the smaller notes.

1 — shutdown/init race (highest value). shutdownTelemetry now awaits any in-flight init inside the shutdown promise before tearing down, so a start that lands after shutdown was requested can no longer leave a registered-but-never-flushed SDK. I went with option A. One extra wrinkle I fixed while there: on the no-op branch (the awaited init produced no usable SDK, e.g. a failed start() or a gRPC config with no endpoint) I clear the shutdown promise, otherwise a later real shutdown would be short-circuited by the stale resolved promise. Added regression tests: single-flight, retry-after-failure, the shutdown/init race, and the stale-promise case.

2 — try/catch coverage. Both await import()s (including startTelemetrySdk's per-protocol import) now sit inside the try, so a chunk-load failure funnels into the existing debugLogger.error('Error starting OpenTelemetry SDK:', …) sink instead of rejecting. The daemon's await at run-qwen-serve.ts is now safe — telemetry can't abort runtime startup.

3 — behavior change on non-deferred paths. Documented explicitly in the Phase 1 design doc: on the ACP child and headless -p runs telemetry now settles asynchronously, widening the existing (gated) drop window by the import cost. Kept it non-await on purpose — awaiting would put the 2.16 MiB import back on the ACP child's critical path and undo the win; callers that need it ready (the daemon runtime) await explicitly.

Smaller notes: the sdk ↔ sdk-impl cycle is broken via a leaf otlp-urls.ts module; the sdk-node exporter stub now returns undefined for then / __esModule (so thenable and interop probes don't hit "Attempted to construct: then"), and the resolve decision is separator-independent with a dedicated unit test that covers the Windows path; the bundle guard now lists the HTTP exporter packages explicitly (with @opentelemetry/otlp-exporter-base) alongside the shared otlp-transformer; and the Phase 1 design doc is marked implemented.

Independent confirmation of the stub safety analysis matches mine — thank you for verifying the spanProcessors / logRecordProcessors env unreachability at the source. Local verification on this SHA: 675 telemetry unit tests, the new stub resolve unit tests, typecheck / lint / prettier, and all three bundle closure guards pass.

[qwen-code-ci-bot]

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] The metrics path does not follow the same empty-array defense documented for traces and logs on lines 414–421. When metricReader is undefined, the conditional spread ...(metricReader && { metricReader }) omits the key entirely from the NodeSDK config. NodeSDK then falls back to env-driven metrics exporter selection (configureMetricProviderFromEnv()), which reads OTEL_METRICS_EXPORTER and constructs exporters. In the bundled CLI, the esbuild stub makes those constructors throw — aborting sdk.start() and disabling ALL telemetry (traces, logs, metrics) for the process lifetime.

Failure scenario: a user has OTEL_METRICS_EXPORTER=otlp in their environment from an unrelated project, enables qwen-code telemetry but provides no metrics endpoint. NodeSDK reaches the env fallback, hits the stub, throws, and every subsequent isTelemetrySdkInitialized() call returns false.

Suggested change

	...(metricReader && { metricReader }),
	metricReaders: metricReader ? [metricReader] : [],

This makes the env fallback unreachable for metrics (same as traces and logs), leaving the stub purely a bundle-size mechanism.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] No test verifies that initializeDaemonMetrics() runs only after initializeTelemetry resolves. The await here is the sole guard against a silent metrics blackout — if a future edit drops it (a one-word deletion), initializeDaemonMetrics() runs before the SDK's MeterProvider is registered, and metrics.getMeter() caches a noop meter permanently for the process lifetime.

The existing test at run-qwen-serve.test.ts only inspects initializeTelemetry's arguments — it never observes initializeDaemonMetrics or its ordering.

Failure scenario: one-word deletion → daemon Status metrics silently produce no data, undetected by the test suite.

Suggested fix: add a test that spies on both core.initializeTelemetry and core.initializeDaemonMetrics, makes initializeTelemetry return a deferred promise, and asserts that initializeDaemonMetrics is not called until after the telemetry promise resolves.

— qwen3.7-max via Qwen Code /review

[wenshao]

No issues found. LGTM! ✅

— qwen3.8-max-preview via Qwen Code /review

[doudouOUC]

@qwen-code /triage

[qwen-code-ci-bot]

⏸️ Deferring to @wenshao — this PR touches core telemetry infrastructure (Stage 1c escalation area) and totals 1565 production logic lines in core paths, well above the 500-line maintainer-awareness threshold. The code review is clean across both passes (original split + new race-fix commit 106c7e6b), all tests pass, and the benchmarks are solid. Needs a human call on the telemetry loading architecture direction and the env-var exporter stubbing behavior change before merge.

中文说明

⏸️ 转交 @wenshao — 此 PR 触及核心 telemetry 基础设施（Stage 1c 升级区域），核心路径共 1565 行生产逻辑，远超 500 行维护者关注阈值。两轮代码审查（原始拆分 + 新竞态修复提交 106c7e6b）均无问题，所有测试通过，基准数据可靠。合并前需要人工对 telemetry 加载架构方向及环境变量 exporter 打桩行为变更做决定。

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Triage re-run completed without a new review.

The stage comments above were updated with the latest result. View workflow run.

[wenshao]

Review

Overview

Two-stage lazy loading of the telemetry stack: (1) sdk.ts becomes a light facade whose only runtime @opentelemetry/* import is @opentelemetry/api, with the SDK assembly moved into sdk-impl.ts behind a single-flight dynamic import; (2) the OTLP exporter chains split into per-protocol modules (sdk-exporters-grpc.ts / sdk-exporters-http.ts) so a telemetry-enabled process loads at most the one chain its config needs. Supporting changes: an esbuild plugin stubs the exporter packages sdk-node eagerly requires for env-based auto-configuration, and the bundle guard gains a third closure check rooted at the sdk-impl chunk, wired into CI.

What I verified beyond the diff

• Behavior parity of the move: endpoint parsing, per-signal URL validation, the gRPC skip-before-import path, the outfile branch, the log-to-span bridge options (including the interactive-mode diagnostics sink), the feedback-loop guard hooks, and the propagator gate all move verbatim; constructor arguments for all six exporters are unchanged.
• No API breakage: resolveHttpOtlpUrl was never exported from the telemetry/index.ts barrel and has no importers outside the telemetry module + its tests, so moving it to otlp-urls.ts is safe. The barrel's five sdk.js exports are intact.
• Call sites: exactly the three production callers exist, each treated as the design doc prescribes. The daemon's un-caught await is safe because the facade's try wraps both dynamic imports and sdk.start() — the returned promise never rejects.
• Shutdown state machine: the unchanged finally in shutdownTelemetry still resets telemetryInitialized / sdk / activeMetricReader / telemetryShutdownPromise, which the new wait-for-pending-init logic and the stale-no-op-promise clearing depend on.
• No closure leaks: file-exporters.ts and log-to-span-processor.ts have no importers outside sdk-impl.ts / sdk-exporters-http.ts, so nothing drags sdk-logs/sdk-metrics/sdk-trace-base back onto an eager path.
• Guard is real CI: ci.yml runs npm run check:serve-fast-path-bundle ("Check serve fast-path bundle closure") and test:ci includes test:scripts. CI is green on this PR (Windows job skipped, matching the ⚠️ in the matrix; the stub's importer-path check normalizes backslashes and has Windows-path tests).
• The env-exporter edge case, in sdk-node source: configureMetricProviderFromEnv() is invoked unconditionally inside NodeSDK.start() (sdk.js:258 of the bundled version), even when an explicit metricReader is configured — while the traces/logs env paths are short-circuited by the always-passed processor arrays. This confirms finding 1 below.

Findings (all non-blocking)

1. A stray OTEL_METRICS_EXPORTER now silently kills all telemetry in the bundled CLI — consider making the failure visible. Because sdk-node consults OTEL_METRICS_EXPORTER unconditionally in start(), a user who has OTEL_METRICS_EXPORTER=otlp (or prometheus) exported globally for unrelated tooling will hit a stubbed-constructor throw that takes down the whole init — spans and logs of an otherwise correctly configured setup included — and the only trace is a debugLogger.error in the OTEL debug log. Before this PR that env var silently added an extra exporter to a default localhost endpoint (also wrong, but configured telemetry kept working). The PR body flags the tradeoff; my point is the blast radius (entire init, not just metrics) and the visibility. The stub's error message is already descriptive — a console.warn in the facade's catch (same precedent as the resource-attribute warning summary a few lines earlier) or a telemetry-docs note would close the gap.

2. Stub Proxy ergonomics + coverage (nit). The get trap returns a throwing function for every string key, so any non-constructor use inside sdk-node — even accidental stringification of the namespace (String(mod) → toString → invoked → throws Attempted to construct: toString) — fails confusingly. Returning undefined for common Object.prototype keys (toString, valueOf) alongside then/__esModule would keep the error crisp. Relatedly, only the resolve decision is unit-tested; the generated stub body isn't (a syntax error would fail the CI bundle, but an interop regression would only surface at runtime). Moving the contents-generation into scripts/sdk-node-exporter-stub.js next to the decision function would make both testable.

3. Stale line reference (nit). sdk-impl.ts's normalizeOtlpPrefix comment still says "the SAME lenient regex as parseOtlpEndpoint (line 109)" — the line number is stale after the move; better to drop it.

4. License-header drift in the new files (nit). scripts/tests/sdk-node-exporter-stub.test.js carries "Copyright 2026 Qwen Team" while the other new files — including the equally brand-new scripts/sdk-node-exporter-stub.js and otlp-urls.ts — say "Copyright 2025 Google LLC". Keeping the Google header on files that are mostly moved code is defensible; the fully new ones should at least agree with each other.

Code quality

• The init/shutdown lifecycle work is careful and each subtle case has a targeted test: single-flight sharing, retry after a poisoned init, shutdown waiting for an in-flight init, and clearing the stale no-op shutdown promise.
• Error handling actually improves: previously exporter and NodeSDK construction ran outside the try/catch, so a construction throw propagated out of the Config constructor; now everything is inside the closure's try.
• The guard refactor (findRootClosureOffenders) removes the duplicated metafile-reading/closure-walking code, and the six new tests cover offender detection, the dynamic-import allowance, the missing-chunk error, and the CLI exit path.
• The two design docs match the implementation and record the rejected alternatives — this made the review much cheaper.

Verdict

LGTM. All findings are non-blocking; #1 is the one I'd like to see addressed (even just a docs note or a console.warn in the facade catch) before or shortly after merge.

---

🤖 Generated with Claude Code — Claude Fable 5

[yiliang114]

LGTM. Clean two-stage lazy load with proper single-flight guard and shutdown/init race handling. Bundle guards lock the split against regression. Benchmarks are convincing.

[qwen-code-ci-bot]

Released in v0.20.1.

[yiliang114]

[API Error: Connection error. (cause: fetch failed)]

---

❌ failed