test(telemetry): Cover daemon metrics init ordering and document metricReader asymmetry

[doudouOUC]

test(telemetry): cover daemon metrics init ordering and document metricReader asymmetry

What this PR does

Two small follow-ups to the lazy telemetry SDK work that merged in #7276, both raised as non-blocking review suggestions there. First, it adds a test asserting that daemon metrics initialization only runs after telemetry initialization has fully settled — the dependency became asynchronous in #7276 and previously had no ordering assertion. Second, it documents why the SDK options use the singular metricReader field while traces and logs pass processor arrays: @opentelemetry/sdk-node@0.203.0 accepts only one metric reader and offers no empty-array opt-out, so env-based reader auto-configuration can only be suppressed by providing an explicit reader — an intentional asymmetry that is easy to "fix" incorrectly.

Why it's needed

The init-ordering guarantee is load-bearing (metrics recorded before the provider exists are dropped) but was only enforced by code shape, not by a test. The comment prevents a future cleanup from converting metricReader to a metricReaders: [] pattern that the SDK does not support.

Reviewer Test Plan

How to verify

• cd packages/cli && npx vitest run src/serve/run-qwen-serve.test.ts — includes the new ordering test (telemetry-start → telemetry-resolved → daemon-metrics).
• The sdk-impl change is comment-only; no behavior change.

Tested on

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

Risk & Scope

• Main risk or tradeoff: none — one new test plus a comment.
• Breaking changes / migration notes: none.

Linked Issues

Follow-up to #7276.

[qwen-code-ci-bot]

Thanks for the PR!

Template looks good ✓ (minor: the "Evidence (Before & After)" heading is missing — would just be N/A here — and the Chinese <details> translation is absent, but neither carries substance for a test+comment PR).

Problem: observed gap, not theoretical. Both items were raised as non-blocking review suggestions in #7276 — the init-ordering guarantee had no test, and the metricReader singularity was undocumented. Real gaps in coverage and documentation.

Direction: aligned. Test coverage for a load-bearing async ordering guarantee, plus a comment preventing a future "cleanup" from breaking the SDK config. CHANGELOG: no direct reference, but this is follow-up hygiene on a merged feature — squarely in scope.

Size: packages/core/src/telemetry/sdk-impl.ts is a core path, but the change is comment-only (0 production logic lines). Test file adds 40 lines. Not applicable for size thresholds.

Approach: minimal and focused — one test, one comment. Exactly what the #7276 review asked for. Nothing to cut.

Moving on to code review. 🔍

中文说明

感谢贡献！

模板完整 ✓（小瑕疵：缺少 "Evidence (Before & After)" 标题——此处只需填 N/A——以及缺少中文翻译，但对测试+注释 PR 无实质影响）。

问题：已观测到的缺口，非理论性加固。两项均来自 #7276 的非阻塞 review 建议——初始化顺序保证缺少测试，metricReader 单数形式缺少文档。

方向：对齐。为承载性的异步顺序保证添加测试覆盖，加注释防止未来"清理"破坏 SDK 配置。

规模：packages/core/src/telemetry/sdk-impl.ts 属核心路径，但改动仅为注释（0 行生产逻辑）。测试文件增加 40 行。不触发规模阈值。

方案：最小且聚焦——一个测试、一条注释。正是 #7276 review 所建议的。无可删减。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

_{Reviewed at 1567df3f9351b7580b33dcbff4d8b59e35d638ff · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Code Review

Independent proposal: given the problem (killed/SIGTERM is dead code because the outer fetchInfoWithTimeout timer wins the race; ETIMEDOUT is a TCP-level timeout, not the application-level one), I would: add ETIMEDOUT to NETWORK_ERROR_CODES, hoist the UpdateCheckTimeoutError check to a direct instanceof at the top, remove the dead killed/SIGTERM and matchesCode('ETIMEDOUT') branches, and update tests. The PR does exactly this — no simpler path missed.

Correctness:

• The UpdateCheckTimeoutError hoist is safe. It's always the top-level error from fetchInfoWithTimeout's Promise.race rejection — never nested as a cause. The old errors.some(… instanceof UpdateCheckTimeoutError) was unnecessarily broad.
• The killed/SIGTERM removal is correct. The outer setTimeout(5000) rejection is a single macrotask callback; the killed-child path (kill → child exit → error event → promise rejection) takes multiple ticks, so the Promise.race always settles with UpdateCheckTimeoutError first.
• ETIMEDOUT → offline is the right bucket. It's a TCP connection timeout ("unreachable"), not the application-level "did not respond within 5s" message that refers to UpdateCheckTimeoutError.
• The error.cause traversal from fix(cli): classify nested update-check network errors #7428 is preserved — the errors array still includes error.cause, so the Node 22 TypeError: fetch failed + ENOTFOUND on cause.code path still classifies as offline.

No critical blockers. No convention violations.

Testing

Unit tests: 40/40 pass (packages/cli/src/ui/utils/updateCheck.test.ts).

Direct module verification (built PR code, exercising classifyUpdateCheckError + describeUpdateCheckFailure):

UpdateCheckTimeoutError: timeout → registry did not respond within 5s
ENOTFOUND in cause:     offline → registry unreachable
ETIMEDOUT:              offline → registry unreachable
killed/SIGTERM:         registry → registry error
ECONNREFUSED:           offline → registry unreachable
generic:                registry → registry error

All classifications match the PR's stated intent. The killed/SIGTERM → registry fallback is the dead-code path that never fires in practice (outer timeout wins the race).

tmux: not available on this CI runner (tmux: command not found). The direct module verification above exercises the exact classification logic end-to-end; the TUI rendering of the warning string is unchanged by this PR (same describeUpdateCheckFailure switch).

中文说明

代码审查

独立方案： 鉴于问题（killed/SIGTERM 是死代码，因为外层 fetchInfoWithTimeout 计时器赢得竞争；ETIMEDOUT 是 TCP 级超时，不是应用层超时），我会：将 ETIMEDOUT 加入 NETWORK_ERROR_CODES，将 UpdateCheckTimeoutError 检查提升为顶部的直接 instanceof，删除死代码 killed/SIGTERM 和 matchesCode('ETIMEDOUT') 分支，更新测试。PR 完全这样做了——没有遗漏更简路径。

正确性：

• UpdateCheckTimeoutError 提升是安全的。它始终是 fetchInfoWithTimeout 的 Promise.race 拒绝的顶层错误——不会嵌套为 cause。
• killed/SIGTERM 删除正确。外层 setTimeout(5000) 拒绝是单个宏任务回调；被杀子进程路径需要多个 tick，所以 Promise.race 总是先以 UpdateCheckTimeoutError 结算。
• ETIMEDOUT → offline 是正确的分类。它是 TCP 连接超时（"不可达"），不是指 UpdateCheckTimeoutError 的应用层 "5 秒内未响应" 消息。
• fix(cli): classify nested update-check network errors #7428 的 error.cause 遍历被保留——errors 数组仍包含 error.cause。

无关键阻塞。无规范违反。

测试

单元测试： 40/40 通过。

直接模块验证（构建 PR 代码）：所有分类符合 PR 声明的意图。

tmux： 此 CI 运行器不可用。直接模块验证已端到端测试了分类逻辑。

— Qwen Code · qwen3.7-max

_{Reviewed at 1134c3764111cde7e743888feeba71af85202d0d · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Confidence: 5/5

Clean follow-up to #7276 review feedback. One test that correctly pins the async init ordering, one comment that documents a real SDK asymmetry. Both are minimal, accurate, and follow existing conventions. All 190 tests in the file pass, typecheck is clean. Nothing to change.

LGTM, approving. ✅

中文说明

置信度：5/5

干净的 #7276 review 反馈跟进。一个测试正确固定了异步初始化顺序，一条注释记录了真实的 SDK 不对称性。两者都最小化、准确、遵循现有规范。文件中全部 190 个测试通过，类型检查通过。无需修改。

LGTM，批准。✅

— Qwen Code · qwen3.7-max

_{Reviewed at 1567df3f9351b7580b33dcbff4d8b59e35d638ff · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[qwen-code-ci-bot]

🩺 serve daemon A/B

Built the PR base vs this PR head 1567df3, 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

[wenshao]

Review — PR #7456

Tidy follow-up; both items raised in #7276 landed cleanly. I checked the two claims against the actual @opentelemetry/sdk-node@0.203.0 source in node_modules rather than taking them on faith, and both hold up.

metricReader comment (sdk-impl.ts) — accurate

Traced NodeSDK (build/src/sdk.js) to confirm each clause:

• The constructor only reads the singular configuration.metricReader — there is genuinely no metricReaders/metricReader: [] array field to opt out with.
• start() calls configureMetricProviderFromEnv() unconditionally, then only falls back to those env readers when no explicit reader was set (if (readers.length === 0) { metricReadersFromEnv.forEach(...) }). So providing an explicit reader really is the only suppression mechanism.
• The asymmetry is real: for spans, configuration.spanProcessors (even [], which is truthy) sets _tracerProviderConfig, and start() then uses it verbatim instead of getSpanProcessorsFromEnv(); logs behave the same via the if (configuration.logRecordProcessors) branch. An empty array disables env fallback for those two signals but not for metrics.

This is exactly the "looks fixable but isn't" trap that earns a comment — good call, and the wording is precise.

Ordering test (run-qwen-serve.test.ts) — meaningful, not racy

• It genuinely guards the await, not just code shape: the mock pushes telemetry-start, yields a microtask, then telemetry-resolved, while the synchronous initializeDaemonMetrics mock pushes daemon-metrics. Drop the await on initializeTelemetry and the order collapses to [telemetry-start, daemon-metrics, telemetry-resolved] → red. That's the load-bearing invariant (metrics recorded before the provider registers get a cached noop meter).
• No race: the assertion runs after await runQwenServe(...), and the sibling uses a daemon-scoped telemetry service instance id test already reads initializeTelemetry.mock.calls[0] right after the same await — proving the deferred runtime load settles before the handle resolves. So this is deterministic, not timing-dependent.
• Conventions match the surrounding block: qws-tm- tmp prefix, makeRuntimeBridge(), daemonLogBaseDir, try/finally handle.close(), and the enclosing describe's afterEach already does vi.restoreAllMocks() + rmSync(tmpDir), so nothing leaks.

Minor / non-blocking

• The new test mocks resolveTelemetrySettings with enabled: true while the sibling uses enabled: false; both reach the init path so it's immaterial — flagging only in case you want them uniform.
• expect(callOrder).toEqual([...]) already pins each call to exactly one occurrence in order, so there's nothing more worth asserting.

Nothing blocking — the test protects a real invariant and the comment is verifiably correct. LGTM.

---

🤖 Generated with Claude Code — Claude Opus 4.8 (1M context)