Skip to content

perf(cli): Defer ACP telemetry initialization - #7558

Merged
doudouOUC merged 2 commits into
QwenLM:mainfrom
doudouOUC:perf/defer-acp-telemetry-init
Jul 23, 2026
Merged

perf(cli): Defer ACP telemetry initialization#7558
doudouOUC merged 2 commits into
QwenLM:mainfrom
doudouOUC:perf/defer-acp-telemetry-init

Conversation

@doudouOUC

@doudouOUC doudouOUC commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR defers telemetry SDK initialization in ACP children until the successful protocol initialize response has been written to stdout. The existing NDJSON transport observation boundary records the incoming initialize request ID and starts the existing single-flight telemetry facade only after the matching result is sent.

Other startup modes retain their current behavior: ordinary interactive TUI startup still defers telemetry until after first paint, prompt-interactive and headless modes initialize eagerly, and the daemon parent keeps its existing deferred runtime initialization.

Why it's needed

ACP previously started telemetry from the bootstrap Config constructor. Although initialization is fire-and-forget, loading and evaluating the telemetry implementation and exporter chain competes with ACP bootstrap work on the same event loop and CPU. On the constrained 2-vCPU validation host, moving that work behind the protocol boundary reduced channel.initialize P50 by 43.8 ms and its paired median by 44.2 ms across 30 alternating cold pairs. Cold process-to-first-session time did not show a statistically demonstrated change, so this PR claims a direct ACP initialization improvement rather than a broader end-to-end startup reduction.

Reviewer Test Plan

How to verify

Run the focused CLI configuration and ACP agent tests and confirm telemetry is deferred for ACP, does not start on receipt, unrelated responses, agent-to-client requests with a colliding ID, or rejected initialization, and starts exactly once after a matching successful response. Confirm the ACP event-loop gauge is not registered until telemetry initialization settles. Run the ACP NDJSON transport tests and confirm sent observations occur only after the underlying write succeeds and do not run when it rejects. Build and type-check the workspace, then start a bundled ACP child with outfile telemetry enabled: a successful initialize response should be emitted before telemetry starts, while EOF before initialize should exit cleanly without creating telemetry output. After creating a session and waiting for one metric export interval, both the session counter and ACP event-loop lag gauge should appear in the outfile.

Observed validation: 593 focused CLI tests passed, 11 ACP transport tests passed, focused ESLint and Prettier checks passed, and the full workspace build and typecheck passed. A release-bundle smoke using the official ACP client waited past the metric export interval and confirmed both qwen-code.session.count and qwen-code.acp.event_loop.lag are exported after deferred initialization. On Linux, 30 alternating cold pairs improved channel.initialize from 942.1 / 1245.0 ms to 898.3 / 1002.4 ms P50/P95; cold process-to-first-session changed from 1833.1 / 2190.6 ms to 1845.5 / 2417.0 ms, with a +13.8 ms paired median whose bootstrap 95% interval (-2.8 to +27.5 ms) crossed zero. The same run's 30-pair preheated phase improved initialization P50 by 42.1 ms while its process-to-session paired median was +1.0 ms. All 120 telemetry-enabled benchmark runs produced valid profiles and non-empty outfiles. Concurrency, telemetry-disabled, legacy single-session, early-disconnect, cleanup, and two live-prompt smoke checks passed without residual processes.

Evidence (Before & After)

N/A — no TUI or other user-visible rendering changes.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux

Environment (optional)

macOS local workspace with Node.js 22 for unit tests, lint, formatting, build, typecheck, and bundled ACP smoke tests. Linux 6.6 x64 host with 2 vCPUs, approximately 3.5 GiB RAM, no swap, and bundled Node.js 22.23.1 for the 2C4G benchmark, functional matrix, and live-prompt smoke tests.

Risk & Scope

  • Main risk or tradeoff: Telemetry events emitted before initialization completes are dropped for a longer window, consistent with the existing initialized-gate behavior; the change deliberately adds no buffer. One-shot bootstrap events emitted before the response, including the initial qwen-code.auth event and one early qwen-code.config event, are therefore permanently absent from ACP telemetry rather than delayed. If the ACP child exits immediately after the response while initialization is still in flight, Config.shutdown() can skip that in-flight SDK even though the lower-level telemetry shutdown can await it.
  • Not validated / out of scope: Windows runtime behavior and live prompts against every supported provider were not exercised. The benchmark measures ACP protocol initialization and immediate session creation; it does not claim a cold process-to-session improvement or a stable preheated RSS change.
  • Breaking changes / migration notes: None. ACP protocol messages, capabilities, configuration, authentication timing, provider selection, MCP behavior, and non-ACP startup paths are unchanged.

Linked Issues

Implements candidate 2 from #7264 and contributes to #4748 without closing either tracking issue.

中文说明

本 PR 的改动

本 PR 将 ACP 子进程中的 telemetry SDK 初始化推迟到协议 initialize 成功响应已经写入 stdout 之后。实现复用现有 NDJSON 传输层的消息观察边界,记录收到的 initialize 请求 ID,并且只在匹配的 result 发送完成后启动现有的单飞 telemetry 门面。

其他启动模式保持现有行为:普通交互式 TUI 仍在首屏渲染后推迟启动 telemetry,带初始 prompt 的交互模式和 headless 模式仍会提前初始化,daemon 父进程也继续使用现有的延迟运行时初始化。

为什么需要

ACP 之前会从启动 Config 构造函数中开始 telemetry。虽然初始化是 fire-and-forget,但加载和执行 telemetry 实现及 exporter 链仍会在同一个事件循环和 CPU 上与 ACP 启动工作竞争。在受限的 2 vCPU 验证机器上,将这部分工作移到协议边界之后,使 30 组交替冷启动中的 channel.initialize P50 降低 43.8 ms、配对中位数降低 44.2 ms。冷启动进程到首个 session 的耗时没有显示出统计上已证实的变化,因此本 PR 只声明直接改善 ACP 初始化,不扩大为整体端到端启动收益。

Reviewer 测试计划

如何验证

运行聚焦的 CLI 配置和 ACP agent 测试,确认 ACP 会推迟 telemetry,在收到请求、无关响应、ID 碰撞的 agent 到 client 请求或初始化被拒绝时不会启动,并且仅在匹配的成功响应之后启动一次。确认 ACP event-loop gauge 在 telemetry 初始化完成前不会注册。运行 ACP NDJSON 传输测试,确认 sent 观察只在底层写入成功后发生,写入失败时不会触发。构建整个工作区并执行类型检查,然后在开启 outfile telemetry 的情况下启动打包后的 ACP 子进程:成功的 initialize 响应应先于 telemetry 启动发出,而 initialize 前收到 EOF 时应干净退出且不创建 telemetry 输出。创建 session 并等待一个指标导出周期后,session counter 和 ACP event-loop lag gauge 都应出现在 outfile 中。

实际验证结果:593 个聚焦 CLI 测试通过,11 个 ACP 传输测试通过,聚焦 ESLint 和 Prettier 检查通过,完整工作区 build 和 typecheck 通过。使用官方 ACP client 的 release bundle 冒烟测试等待超过 metric 导出间隔后,确认延迟初始化后 qwen-code.session.countqwen-code.acp.event_loop.lag 均能导出。在 Linux 上,30 组交替冷启动使 channel.initialize P50/P95 从 942.1 / 1245.0 ms 降至 898.3 / 1002.4 ms;冷启动进程到首个 session 从 1833.1 / 2190.6 ms 变为 1845.5 / 2417.0 ms,其配对中位数为 +13.8 ms,但 bootstrap 95% 区间(-2.8 到 +27.5 ms)跨过 0。同一次测试的 30 组预热阶段使初始化 P50 降低 42.1 ms,而进程到 session 的配对中位数为 +1.0 ms。120 次开启 telemetry 的基准运行均生成有效 profile 和非空 outfile。并发、关闭 telemetry、旧版单 session、提前断开、清理以及两次真实 prompt 冒烟检查均通过,且没有残留进程。

证据(改动前后)

N/A——没有 TUI 或其他用户可见的渲染变化。

测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux

环境(可选)

macOS 本地工作区使用 Node.js 22 完成单元测试、lint、格式检查、build、typecheck 和打包 ACP 冒烟测试。Linux 6.6 x64 主机使用 2 vCPU、约 3.5 GiB 内存、无 swap 和打包 Node.js 22.23.1,完成 2C4G 基准、功能矩阵和真实 prompt 冒烟测试。

风险与范围

  • 主要风险或取舍:初始化完成前产生的 telemetry 事件会在更长的窗口内被丢弃,这与现有 initialized gate 行为一致;本改动刻意不增加缓冲。响应前只产生一次的启动事件,包括初始 qwen-code.auth 事件和一条较早的 qwen-code.config 事件,会永久缺失于 ACP telemetry,而不只是延迟。如果 ACP 子进程在响应后、初始化仍进行时立即退出,即使底层 telemetry shutdown 能等待初始化,Config.shutdown() 仍可能跳过这个进行中的 SDK。
  • 未验证 / 范围外:未验证 Windows 运行时行为,也没有针对所有受支持 provider 运行真实 prompt。基准测量 ACP 协议初始化和紧接着的 session 创建;不声明冷启动进程到 session 的改善,也不声明稳定的预热 RSS 变化。
  • 破坏性变更 / 迁移说明:无。ACP 协议消息、能力、配置、认证时序、provider 选择、MCP 行为和非 ACP 启动路径均未改变。

关联 Issue

实现 #7264 的 candidate 2,并推进 #4748,但不关闭这两个跟踪 issue。

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR — clean, well-documented change.

Template looks good ✓ (all sections + Chinese translation present).

Problem: observed, not theoretical. Real benchmark — 30 alternating paired cold starts on a constrained 2-vCPU host, channel.initialize P50 down 43.8 ms (paired-median -44.2 ms, faster in 26/30 pairs). You honestly call out that process-to-first-session is neutral (+13.8 ms paired median, CI crosses zero). The work is moved later, not removed — this is a genuine gain on the protocol-initialize boundary specifically, not an end-to-end startup win. Accurate framing.

Direction: aligned — ACP startup latency is actively tracked (#7264 candidate 2, contributes to #4748), and editors that gate "agent ready" on the initialize response feel this boundary directly. No direct CHANGELOG reference for telemetry-deferral optimization, but the area is clearly relevant. Flag: this touches telemetry behavior (initialized-gate drop window gets longer) and core config (packages/cli/src/config/**), so it needs a maintainer's eye before merge.

Size: touches core paths. Production logic: 43 lines (acpAgent.ts 32, config.ts 11); test: 126 lines; design doc: 83. Well under any threshold.

Approach: scope feels right and notably minimal — reuses the existing onMessageObserved transport hook (already used in run-qwen-serve.ts), the existing single-flight initializeTelemetry facade, and the existing deferTelemetryInitialization config option rather than adding parallel machinery. The transport-defined boundary (start only after the matching successful response is written) is the right call versus an event-loop timer. The one question for the maintainer: ~44 ms on a sub-metric that's end-to-end neutral, in exchange for a slightly longer dropped-telemetry window — worth it? Not a blocker, just the judgment call.

Moving on to code review. 🔍

中文说明

感谢贡献——干净、文档完善的改动。

模板 完整 ✓(所有章节 + 中文翻译齐全)。

问题: 已观测到,非理论性。真实基准——受限 2-vCPU 主机上 30 组交替配对冷启动,channel.initialize P50 降低 43.8 ms(配对中位数 -44.2 ms,26/30 组更快)。你诚实地指出 进程到首个 session 保持中性(配对中位数 +13.8 ms,置信区间跨零)。工作是被后移而非消除——这是协议 initialize 边界上的真实收益,并非端到端启动收益。表述准确。

方向: 对齐——ACP 启动延迟有活跃跟踪(#7264 candidate 2,推进 #4748),以 initialize 响应作为 "agent 就绪" 门槛的编辑器直接感受此边界。CHANGELOG 无 telemetry 延迟优化的直接引用,但该领域明显相关。提醒: 触及 telemetry 行为(initialized-gate 丢弃窗口变长)和核心配置(packages/cli/src/config/**),合并前需 maintainer 过目。

规模: 触及核心路径。生产逻辑:43 行acpAgent.ts 32 行,config.ts 11 行);测试 126 行;设计文档 83 行。远低于任何阈值。

方案: 范围合理且精简——复用现有 onMessageObserved 传输钩子(run-qwen-serve.ts 已在用)、现有单飞 initializeTelemetry 门面、现有 deferTelemetryInitialization 配置选项,而非新增平行机制。以传输层定义的边界(仅在匹配的成功响应被写出之后启动)取代事件循环定时器,是正确选择。留给 maintainer 的问题:端到端中性的子指标上约 44 ms,换取略长的 telemetry 丢弃窗口——值不值?不是阻塞项,只是判断点。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 1f07a989a35020db8a1a1951234154627d4cf276 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Code review

Before reading the diff, my independent proposal from the title + motivation: (a) reuse the existing deferTelemetryInitialization config flag to suppress the constructor start for ACP, and (b) find a deterministic "initialize response has been written" signal to call the existing single-flight initializeTelemetry, moving the event-loop gauge registration behind SDK init. The PR does exactly this — and the signal it picks (the NDJSON transport's post-write sent observation, keyed on the initialize request ID) is the right boundary because it's transport-defined rather than an event-loop timer.

I verified the load-bearing assumptions against the code, not just the PR description:

  • sent fires only after a successful write — in ndJsonStream.ts the writable does await writer.write(frame) and only then calls onMessageObserved({direction:'sent'}); a write rejection skips the hook entirely. Telemetry genuinely cannot start for a response the client never received.
  • initializeTelemetry is single-flight and exportedtelemetryInitPromise ??= guard plus internal try/catch mean the void initializeTelemetry(config) call is idempotent and can't produce an unhandled rejection.
  • The constructor skips the start when deferredif (enabled && !telemetryInitializationDeferred) gates the fire-and-forget start, so the ACP path is handed off cleanly to the new hook.
  • Config logic change is correct — old: interactive && !isAcpMode && !question (defers only for interactive TUI without ACP/question); new: isAcpMode || (interactive && !question) (defers for ACP always, interactive-without-question unchanged, headless/question unchanged). Each mode verified.
  • Edge cases hold up — error response (clears remembered ID, no start, later valid initialize can re-arm), unrelated response ID (ignored), id: null and id: 0 (handled), duplicate sent responses (ID consumed once). The new test exercises each.

No correctness bugs, no security concerns, no regressions spotted. Reuse is good — nothing duplicated, logic lives in the right package. No AGENTS.md violations. The diff is exactly the minimal set for the stated goal (no drive-by changes; the design doc is appropriate for a behavioral change).

Real-scenario testing

tmux is not installed on this host and there's no passwordless sudo to add it, so I captured real terminal output directly. Two layers of evidence:

1. Focused unit tests (real runs, PR branch code):

$ cd packages/cli && npx vitest run src/config/config.test.ts -t "telemetry"
 ✓ src/config/config.test.ts (292 tests | 264 skipped) 131ms
 Test Files  1 passed (1)
      Tests  28 passed | 264 skipped (292)   ← incl. new "should defer telemetry for ACP startup"

$ npx vitest run src/acp-integration/acpAgent.test.ts
 ✓ src/acp-integration/acpAgent.test.ts (300 tests) 15073ms
 Test Files  1 passed (1)
      Tests  300 passed (300)   ← incl. new "starts telemetry only after a matching
                                    successful initialize response is sent"

$ cd packages/acp-bridge && npx vitest run src/ndJsonStream.test.ts
 ✓ src/ndJsonStream.test.ts (11 tests) 26ms
 Test Files  1 passed (1)
      Tests  11 passed (11)   ← sent hooks fire after write, skipped on rejection

2. Real ACP child smoke test (before/after, outfile telemetry enabled):

===== BASELINE (main branch, dist/cli.js --acp) =====

TEST A: EOF before initialize
  exit code: 0
  OUTFILE: EXISTS (218 lines)   ← telemetry starts eagerly in Config constructor

TEST B: initialize + 3s keepalive
  stdout: {"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentInfo":
          {"name":"qwen-code","title":"Qwen Code","version":"0.20.1"},...}}
  exit code: 0
  OUTFILE: EXISTS (218 lines)

===== PR BRANCH (tsx packages/cli/index.ts --acp) =====

TEST A: EOF before initialize
  exit code: 0
  stderr: (empty)
  OUTFILE A: not created   ← telemetry deferred; never starts without successful initialize

TEST B: initialize + 5s keepalive
  stdout: {"jsonrpc":"2.0","id":0,"result":{"protocolVersion":1,"agentInfo":
          {"name":"qwen-code","title":"Qwen Code","version":"v22.23.1"},...}}
  exit code: 0
  OUTFILE B: EXISTS (122 lines)   ← telemetry SDK started AFTER the response was written

This pair is discriminating: on main the ACP child starts telemetry eagerly in the Config constructor, so Test A creates the outfile. On the PR branch it doesn't — telemetry starts only once the matching successful initialize response is written (Test B), and the real protocol handshake completes cleanly through the new hook. No residual processes after either run.

中文说明

代码审查

在读 diff 之前,我根据标题和动机给出的方案是:(a) 复用现有 deferTelemetryInitialization 配置标志来抑制 ACP 构造函数中的启动,(b) 找到一个确定性的 "initialize 响应已写出" 信号来调用现有的单飞 initializeTelemetry,并将 event-loop gauge 注册移到 SDK 初始化之后。PR 正是这么做的——它选用的信号(NDJSON 传输层写出后的 sent 观察,按 initialize 请求 ID 匹配)是正确的边界,因为它由传输层定义,而非事件循环定时器。

我对照代码(而非仅凭 PR 描述)验证了关键假设:

  • sent 仅在写入成功后触发——ndJsonStream.ts 中 writable 先 await writer.write(frame),之后才调用 onMessageObserved({direction:'sent'});写入失败会完全跳过该钩子。telemetry 确实不会为客户端从未收到的响应而启动。
  • initializeTelemetry 是单飞且已导出——telemetryInitPromise ??= 守卫加上内部 try/catch,使得 void initializeTelemetry(config) 调用是幂等的,不会产生未处理的 rejection。
  • 延迟时构造函数确实跳过启动——if (enabled && !telemetryInitializationDeferred) 门控 fire-and-forget 启动,ACP 路径被干净地移交给新钩子。
  • Config 逻辑变更正确——旧:interactive && !isAcpMode && !question(仅对非 ACP、无 question 的交互式 TUI 延迟);新:isAcpMode || (interactive && !question)(ACP 始终延迟,交互式无 question 不变,headless/有 question 不变)。逐一验证各模式。
  • 边界情况成立——错误响应(清除记住的 ID,不启动,后续有效 initialize 可重新武装)、无关响应 ID(忽略)、id: nullid: 0(已处理)、重复 sent 响应(ID 只消费一次)。新测试逐一覆盖。

未发现正确性 bug、安全隐患或回归。复用良好——无重复代码,逻辑放在正确的包中。无 AGENTS.md 违规。diff 恰好是达成目标所需的最小集合(无顺手改动;行为改动配设计文档是恰当的)。

真实场景测试

本主机未安装 tmux,且无 passwordless sudo 可安装,因此直接捕获真实终端输出。两层证据:

1. 聚焦单元测试(真实运行,PR 分支代码): 28 个 telemetry 配置测试通过(含新增 "should defer telemetry for ACP startup"),300 个 ACP agent 测试通过(含新增 "starts telemetry only after a matching successful initialize response is sent"),11 个 NDJSON 传输测试通过(sent 钩子在写入后触发,写入失败时跳过)。

2. 真实 ACP 子进程冒烟测试(before/after,开启 outfile telemetry):

  • 基线(main 分支):EOF before initialize → outfile 存在(218 行)——telemetry 在 Config 构造函数中提前启动。
  • PR 分支:EOF before initialize → outfile 未创建——telemetry 被延迟,没有成功 initialize 就永不启动。Initialize + 5s → stdout 收到真实 initialize 响应,outfile 被创建(122 行)——telemetry SDK 在响应写出之后启动。

这组对照具有区分度:在 main 上 ACP 子进程会在 Config 构造函数中提前启动 telemetry,因此测试 A 会创建 outfile。而 PR 分支不会——telemetry 仅在匹配的成功 initialize 响应写出后启动,且真实协议握手通过新钩子干净完成。两次运行后均无残留进程。

Qwen Code · qwen3.8-max-preview

Reviewed at 1f07a989a35020db8a1a1951234154627d4cf276 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean review, solid execution, real-process smoke confirms the promised behavior. Docking one point because it touches core config + telemetry behavior on a fork PR, which needs maintainer sign-off rather than auto-approve.

Stepping back: this is good work. The approach matches what I'd have proposed independently, the implementation is minimal and reuses existing machinery (the deferTelemetryInitialization flag, the onMessageObserved transport hook, the single-flight initializeTelemetry facade) instead of bolting on something new, the edge cases are handled and tested, and the before/after smoke confirms the behavior the PR promises — no telemetry without a successful initialize, telemetry starts right after the response is written. The author clearly knows this area; the design doc reads like someone who's already done the #7264 startup work, and the benchmark is honest about what it does and doesn't claim.

My one reservation is the cost/benefit, and it's a judgment call I won't make unilaterally: the gain is ~44 ms on channel.initialize while process-to-first-session stays neutral — the work is shifted later, not removed. In exchange, the initialized-gate drop window for telemetry gets a little longer (the initial qwen-code.auth and one early qwen-code.config event are permanently absent from ACP telemetry). That's a defensible tradeoff for editors that gate readiness on the initialize response, but it's not an obvious end-user win.

Why I'm deferring instead of approving: the change touches packages/cli/src/config/** (core) and telemetry timing, and it's a cross-repository (fork) PR. Our gate doesn't auto-approve fork PRs that touch core/telemetry — those get a human eye. The review itself found nothing blocking; this is policy, not doubt about the code.

⏸️ Deferring to @wenshao @yiliang114 — needs a human call on two things: (1) sign-off on a fork PR touching core config + telemetry behavior (policy gate), and (2) whether the narrow, end-to-end-neutral channel.initialize gain is worth the longer telemetry drop window. The code itself is ready; the decision is whether the tradeoff is one we want.

中文说明

置信度:4/5 —— 审查干净、执行扎实,真实进程冒烟证实了承诺的行为。扣一分因为它在 fork PR 上触及核心配置 + telemetry 行为,需要 maintainer 签字而非自动批准。

退一步看:这是好工作。方案与我独立提出的一致,实现精简、复用现有机制(deferTelemetryInitialization 标志、onMessageObserved 传输钩子、单飞 initializeTelemetry 门面)而非另起炉灶,边界情况已处理并有测试,before/after 冒烟证实了 PR 所承诺的行为——没有成功 initialize 就没有 telemetry,telemetry 在响应写出后立即启动。作者显然熟悉这个领域;设计文档读起来像是已经做过 #7264 启动工作的人,基准也诚实地说明了它声明什么、不声明什么。

我唯一的保留是成本收益,这是一个我不会单方面拍板的判断:收益是 channel.initialize 上约 44 ms,而进程到首个 session 保持中性——工作是被后移,而非消除。作为交换,telemetry 的 initialized-gate 丢弃窗口变长了一点(初始 qwen-code.auth 事件和一条较早的 qwen-code.config 事件永久缺失于 ACP telemetry)。对于那些以 initialize 响应作为就绪门槛的编辑器,这是可以接受的取舍,但它不是一个明显的终端用户收益。

为什么我选择 defer 而非 approve:改动触及 packages/cli/src/config/**(核心)和 telemetry 时序,且这是一个跨仓库(fork)PR。我们的闸门不会自动批准触及核心/telemetry 的 fork PR——这些必须有人工过目。审查本身未发现阻塞项;这是策略,而非对代码的质疑。

⏸️ 转交 @wenshao @yiliang114 —— 需要人工拍板两件事:(1) 对一个触及核心配置 + telemetry 行为的 fork PR 签字(策略闸门);(2) 这个窄幅、端到端中性的 channel.initialize 收益是否值得更长的 telemetry 丢弃窗口。代码本身已就绪;决定点在于我们是否想要这个取舍。

Qwen Code · qwen3.8-max-preview

Reviewed at 1f07a989a35020db8a1a1951234154627d4cf276 · re-run with @qwen-code /triage

@gwinthis

Copy link
Copy Markdown
Collaborator

Review & Local Verification Report

代码审查

设计评价:严谨的性能优化工程。 本 PR 将 ACP 子进程的遥测初始化从 Config 构造函数延迟到 protocol initialize 响应写入完成后,减少启动阶段的 event loop 竞争。

核心设计(transport-defined boundary):

  • 利用 NDJSON transport 的 onMessageObserved hook 监听消息
  • 记住 initialize 请求的 JSON-RPC ID
  • 仅在匹配 ID 的成功响应写入 stdout 后才启动遥测
  • 不依赖 event-loop 调度假设,而是依赖 transport 层的写入完成保证

安全门控(5 个不触发条件):

  1. 收到请求时不启动(仅记住 ID)
  2. 不匹配的响应 ID 不启动
  3. 错误响应不启动
  4. 写入失败不启动(sent hook 不执行)
  5. 重复/无关响应不启动(ID 消费一次后清除)

基准测试(30 对交替冷启动,2C4G):

指标 Control P50 Candidate P50 Delta
channel.initialize 968.0 ms 923.8 ms -44.2 ms
Cold POST /session 1284.9 ms 1285.7 ms +0.7 ms(中性)
Peak RSS 414.1 MiB 410.6 MiB -3.6 MiB

28/30 对中 initialize 更快,session 请求中性——优化不引入端到端回归。

设计文档质量极高: 包含问题定义、所有权分析、6 个被拒替代方案、完整的失败/兼容行为矩阵、原始数据路径。

结论

LGTM。 教科书级的性能优化 PR:transport 层保证(非调度假设)、完整的失败分析、30 对基准测试、被拒方案记录。

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— qwen3.7-max via Qwen Code /review

@doudouOUC

Copy link
Copy Markdown
Collaborator Author

[codex] Thanks — no action needed; CI has since completed successfully (10/10 passing) at the reviewed SHA. The PR remains ready for maintainer review.

@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Local verification report — real bundles, real ACP client

I built both sides from source and drove the release bundles with a genuine ACP client (@agentclientprotocol/sdk ClientSideConnection over stdio), rather than only reading the diff.

Verdict: the mechanism works and the win reproduces — but the PR silently kills every OpenTelemetry metric in ACP children. One line needs to move; everything else looks good.

PR head a9c0088
BASE 95fc7ca (PR parent)
Artifacts npm run bundle on each side; PR bundle contains initializeRequestId, BASE does not; deferTelemetryInitialization: isAcpMode || interactive && !question vs interactive && !isAcpMode && !question
Host Linux, 8 vCPU / 14 GiB, Node 22.22.2; benchmark pinned to 2 vCPUs via taskset -c 0,1

1. What reproduces ✅

Telemetry now starts strictly after the initialize response is written. The probe records, on one monotonic clock inside the client process, when the initialize response arrives and when the telemetry outfile first appears (the file exporter opens its write stream in its constructor, so file creation = SDK start).

ordering

  • BASE: telemetry starts before the response, mean −109.6 ms, 20/20 runs negative.
  • PR: telemetry starts after the response, mean +29.9 ms, 20/20 runs positive.
  • EOF before initialize: PR exits 0, stderr empty, no outfile ever created. BASE creates a 4 966 B outfile. Exactly as the PR body claims.
  • initialize response is byte-identical between BASE and PR (protocolVersion, agentCapabilities, authMethods, _meta).

The latency win reproduces, at roughly 2/3 the size the PR reports (expected — this host is faster than the 2 vCPU validation box even when pinned):

benchmark

Tests / lint / types. acpAgent.test.ts 300 passed, config.test.ts 293 passed (= the 593 the PR body cites), ndJsonStream.test.ts 11 passed. ESLint and Prettier clean on all changed files. tsc -p packages/cli produces an identical 96-error set on BASE and PR (pre-existing unbuilt-workspace noise); zero errors in the changed files.

The new tests are not vacuous — 6 of 7 one-line mutations of the PR source turn the new test red:

mutations


2. Finding — ACP children now emit zero metrics 🔴

This is the one blocking item. Same ACP child, telemetry enabled, newSession + 14 s (longer than the 10 s PeriodicExportingMetricReader interval):

telemetry payload

Mechanism. runAcpAgent still calls registerAcpEventLoopLagGauge(...) at packages/cli/src/acp-integration/acpAgent.ts:2738, during transport setup. Under this PR that point is now strictly before telemetry initialization, so:

  1. registerEventLoopLagGauge calls getMeter() (packages/core/src/telemetry/metrics.ts:422), which memoizes cliMeter.
  2. No global MeterProvider is registered yet, and @opentelemetry/api's metrics API has no proxy provider (unlike trace's ProxyTracerProvider), so metrics.getMeter() returns the NOOP meter — and the memo makes that permanent.
  3. When telemetry finally starts, initializeTelemetryinitializeMetrics(config) builds every counter, histogram and gauge on that cached NOOP meter.

Result: for the whole process lifetime an ACP child records no metrics at all. Spans are unaffected (trace API has a proxy), which is why qwen-code.daemon.session_start still shows up and hides the problem in a casual smoke test.

This is the exact hazard the repo already documents for the daemon path — packages/cli/src/serve/run-qwen-serve.ts:2977:

// Must settle before initializeDaemonMetrics(): metrics.getMeter() caches
// a noop meter permanently if called before the SDK registers the global
// MeterProvider.

Blast radius: every ACP child — qwen serve channels (acp-bridge/src/spawnChannel.ts:145, channels/base/src/AcpBridge.ts:106), the desktop app (desktop/packages/shared/src/agent/qwen-agent.ts:577), and the VS Code companion (vscode-ide-companion/src/services/acpConnection.ts:115).

Causal proof, not inference. I rebuilt two more bundles from the PR source (columns 3 and 4 above):

  • PR−earlyGauge — the registerAcpEventLoopLagGauge call simply deleted → qwen-code.session.count comes back.
  • PR+fix — the call moved into the telemetry-init continuation → both metrics come back, and the deferral is preserved (Δ resp→telemetry still ≈ +30 ms).

The fix I verified:

        initializeRequestId = undefined;
        if ('result' in message) {
-         void initializeTelemetry(config);
+         void initializeTelemetry(config).then(() => {
+           registerAcpEventLoopLagGauge(() => eventLoopMonitor.snapshot());
+         });
        }

…with the unconditional call at line 2738 removed. (Optional hardening for the underlying trap: have initializeTelemetry reset the cliMeter memo after sdk.start(), so no future early caller can poison it again.)


3. Smaller notes

a. qwen-code.auth is lost permanently, not just delayed. performInitialAuth (packages/cli/src/core/auth.ts:35) emits it exactly once during bootstrap, so for ACP children it is now never recorded — same for one of the two qwen-code.config events (measured 2 → 1). The PR body's "dropped for a longer window" is accurate in principle, but for once-per-process events the window covers their entire lifetime. Worth stating explicitly in the risk section, or re-emitting them after init if ACP auth telemetry has consumers.

b. Test gap (M7). Dropping the 'method' in message exclusion on sent messages does not fail any test. That guard is load-bearing: agent→client requests (session/request_permission, …) carry both id and method, and outgoing request IDs start from the same small integers as the client's, so a collision with a remembered initialize id is realistic. One extra case would close it.

c. Design-doc precision. The doc says cleanup "does not promise to flush that in-flight SDK". True, but the reason is worth naming: shutdownTelemetry() does await telemetryInitPromise (packages/core/src/telemetry/sdk.ts:127); it is Config.shutdown() that only calls it when isTelemetrySdkInitialized() is already true (packages/core/src/config/config.ts:4443), so an in-flight init is skipped entirely.


4. Not covered here

Windows runtime behaviour; live prompts against real providers; OTLP/gRPC exporter paths (I used --telemetry-target local --telemetry-outfile); preheated-channel RSS.

Recommendation: fix §2 (one-line move, verified above), then this is good to merge — the deferral itself is correct, well-tested, and the latency win is real.

中文完整版

本地验证报告 —— 真实构建产物 + 真实 ACP 客户端

我把两侧都从源码构建成 release bundle,并用真正的 ACP 客户端(@agentclientprotocol/sdkClientSideConnection,走 stdio)驱动,而不是只读 diff。

结论:机制本身是对的,收益也复现了 —— 但本 PR 会让 ACP 子进程的 OpenTelemetry metrics 全部静默失效。 需要挪动一行;其余部分都没问题。

PR head a9c0088
BASE 95fc7ca(PR 的父提交)
构建产物 两侧各跑 npm run bundle;PR bundle 里有 initializeRequestId,BASE 没有;配置门 isAcpMode || interactive && !question vs interactive && !isAcpMode && !question
机器 Linux,8 vCPU / 14 GiB,Node 22.22.2;基准测试用 taskset -c 0,1 限制到 2 vCPU

1. 可以复现的部分 ✅

telemetry 确实推迟到 initialize 响应写出之后才启动。 探针在客户端进程内用同一个单调时钟记录两件事:收到 initialize 响应的时刻,以及 telemetry outfile 首次出现的时刻(file exporter 在构造函数里就打开写流,所以文件创建 = SDK 启动)。

  • BASE:telemetry 在响应之前启动,平均 −109.6 ms,20/20 次为负。
  • PR:telemetry 在响应之后启动,平均 +29.9 ms,20/20 次为正。
  • initialize 之前 EOF:PR 退出码 0、stderr 为空、完全没有创建 outfile;BASE 会创建 4 966 B 的文件。与 PR 描述一致。
  • BASE 与 PR 的 initialize 响应内容完全一致(protocolVersionagentCapabilitiesauthMethods_meta)。

延迟收益复现了,幅度约为 PR 所报数字的 2/3(符合预期:即使 pin 到 2 核,这台机器仍比对方的 2 vCPU 验证机快):BASE P50/P95 = 1151.3 / 1161.9 ms,PR = 1123.1 / 1131.3 ms,P50 −28.2 ms,配对中位数 −29.3 ms,20/20 组 PR 更快。

测试 / lint / 类型。 acpAgent.test.ts 300 通过,config.test.ts 293 通过(合计正是 PR 描述里的 593),ndJsonStream.test.ts 11 通过。改动文件的 ESLint、Prettier 全部干净。tsc -p packages/cli 在 BASE 与 PR 上产生完全相同的 96 条报错(都是工作区未构建导致的既有噪声),改动文件本身零报错。

新增测试不是空转 —— 对 PR 源码做的 7 个单行变异中有 6 个能让新测试变红(见截图 4)。

2. 问题 —— ACP 子进程现在一条 metric 都发不出来 🔴

这是唯一的阻塞项。同一个 ACP 子进程、开启 telemetry、newSession + 等待 14 s(大于 PeriodicExportingMetricReader 的 10 s 导出间隔):

记录 类型 BASE PR PR−earlyGauge PR+fix
qwen-code.auth log event
qwen-code.config log event
qwen-code.daemon.session_start span
qwen-code.session.count metric
qwen-code.acp.event_loop.lag metric
outfile 字节数 11 970 4 176 6 285 9 794

成因。 runAcpAgent 仍然在传输层建立阶段调用 registerAcpEventLoopLagGauge(...)packages/cli/src/acp-integration/acpAgent.ts:2738)。在本 PR 下,这个时间点严格早于 telemetry 初始化,于是:

  1. registerEventLoopLagGauge 调用 getMeter()packages/core/src/telemetry/metrics.ts:422),该函数会缓存 cliMeter
  2. 此时全局 MeterProvider 还没注册,而 @opentelemetry/api 的 metrics API 没有 proxy provider(与 trace 的 ProxyTracerProvider 不同),所以 metrics.getMeter() 返回 NOOP meter —— 缓存让它永久生效。
  3. 等 telemetry 真正启动后,initializeTelemetryinitializeMetrics(config) 会把所有 counter / histogram / gauge 都建在这个 NOOP meter 上。

结果:ACP 子进程在整个生命周期内一条 metric 都不会产生。span 不受影响(trace API 有 proxy),所以 qwen-code.daemon.session_start 照常出现,这会让粗略的冒烟测试看不出问题。

仓库其实已经为 daemon 路径记录过同一个陷阱 —— packages/cli/src/serve/run-qwen-serve.ts:2977

// Must settle before initializeDaemonMetrics(): metrics.getMeter() caches
// a noop meter permanently if called before the SDK registers the global
// MeterProvider.

影响范围: 所有 ACP 子进程 —— qwen serve 的 channel(acp-bridge/src/spawnChannel.ts:145channels/base/src/AcpBridge.ts:106)、桌面端(desktop/packages/shared/src/agent/qwen-agent.ts:577)、VS Code companion(vscode-ide-companion/src/services/acpConnection.ts:115)。

这是因果验证,不是推断。 我从 PR 源码另外构建了两个 bundle(上表第 3、4 列):

  • PR−earlyGauge —— 只删掉 registerAcpEventLoopLagGauge 调用 → qwen-code.session.count 回来了。
  • PR+fix —— 把该调用移进 telemetry 初始化的回调 → 两条 metric 都回来了,同时推迟语义保持不变(Δ 响应→telemetry 仍约 +30 ms)。

我验证过的修法:

        initializeRequestId = undefined;
        if ('result' in message) {
-         void initializeTelemetry(config);
+         void initializeTelemetry(config).then(() => {
+           registerAcpEventLoopLagGauge(() => eventLoopMonitor.snapshot());
+         });
        }

同时删掉 2738 行处的无条件调用。(针对底层陷阱的可选加固:让 initializeTelemetrysdk.start() 之后重置 cliMeter 缓存,这样以后任何早期调用者都不会再污染它。)

3. 次要问题

a. qwen-code.auth 是永久丢失,不只是延迟。 performInitialAuthpackages/cli/src/core/auth.ts:35)在 bootstrap 阶段只发一次,所以 ACP 子进程从此再也不会记录它;两条 qwen-code.config 也少了一条(实测 2 → 1)。PR 描述里的"丢弃窗口变长"原则上没错,但对于每进程只发一次的事件来说,这个窗口等于它的全部生命周期。建议在风险章节明确写出,或者在初始化后补发。

b. 测试覆盖缺口(M7)。 去掉 sent 分支里的 'method' in message 排除条件后,没有任何测试失败。这个判断是有实际作用的:agent→client 的请求(session/request_permission 等)同时带 idmethod,而出站请求 ID 与客户端一样从很小的整数开始,和记住的 initialize id 撞号是现实可能的。补一个用例即可。

c. 设计文档的措辞。 文档说清理逻辑"不保证 flush 尚未完成的 SDK"。这个结论正确,但原因值得写明:shutdownTelemetry() 本身会 await telemetryInitPromise 的(packages/core/src/telemetry/sdk.ts:127);真正的门是 Config.shutdown() 只在 isTelemetrySdkInitialized() 已为 true 时才调用它(packages/core/src/config/config.ts:4443),所以进行中的初始化会被整个跳过。

4. 未覆盖

Windows 运行时行为;针对真实 provider 的 live prompt;OTLP/gRPC exporter 路径(我用的是 --telemetry-target local --telemetry-outfile);预热通道的 RSS。

建议: 修掉第 2 节的问题(一行位置调整,已验证),之后即可合入 —— 推迟机制本身是正确的、测试扎实,延迟收益也是真实的。

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

@wenshao Addressed the review findings in 1f07a989a3.

Review item Decision Action
ACP metrics are bound to a cached NOOP meter Agree Moved ACP event-loop gauge registration behind the deferred initializeTelemetry() completion so all metrics bind after the global meter provider is installed.
One-shot auth/config events are permanently absent Agree Documented the exact event loss in the design and PR risk sections instead of describing it only as a longer drop window.
Agent-to-client request ID collision was untested Agree Added a colliding session/request_permission request case that proves the outgoing method guard is load-bearing.
Cleanup explanation was imprecise Agree Documented that shutdownTelemetry() can await the initializer, while Config.shutdown() skips it until isTelemetrySdkInitialized() becomes true.

Validation on the pushed commit:

  • 593 focused CLI tests passed.
  • Full workspace build and typecheck passed.
  • An official-ACP-client release-bundle smoke waited past the export interval and observed both qwen-code.session.count and qwen-code.acp.event_loop.lag.
  • A fresh metrics-preserving 2-vCPU rerun completed 30 cold pairs and 30 preheated pairs with 120/120 telemetry-enabled runs producing valid profiles, non-empty outfiles, and no residual processes. Cold channel.initialize P50 improved by 43.8 ms and its paired median by 44.2 ms. Cold process-to-first-session had a +13.8 ms paired median with a bootstrap 95% interval of -2.8 to +27.5 ms, so the PR continues to claim only the initialization-boundary improvement.

The PR description and #7264 candidate 2 entry now contain the corrected final measurements and explicit tradeoffs.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Triage re-run completed without a new review.

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

@wenshao

wenshao commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Code Review — perf(cli): Defer ACP telemetry initialization (#7558)

Verdict: Looks solid — merge-ready. The change is tightly scoped, the cross-module contracts it relies on all hold, and the risk disclosure is honest and matches the code. A few optional robustness nits below; none are blockers.

What it does

  • ACP mode now sets the existing deferTelemetryInitialization config flag, so the Config constructor's fire-and-forget telemetry start is suppressed.
  • runAcpAgent uses ndJsonStream's onMessageObserved hook to remember the incoming initialize request id, and starts telemetry (via the single-flight initializeTelemetry facade) only after the matching successful response has been written to stdout. The ACP event-loop-lag gauge registration is moved behind that init settling.
  • Non-ACP startup paths (TUI-after-first-paint, -i "prompt" eager, headless/stream-JSON eager, daemon deferred) are unchanged.

Correctness — verified against the source

  • Config flag refactor is exactly equivalent for non-ACP. interactive && !isAcpMode && !questionisAcpMode || (interactive && !question). Truth table: for non-ACP both reduce to interactive && !question; for ACP the old value was always false and the new value is always true — precisely the intended flip. ✅
  • Sentinel handling is deliberately correct for id: 0 and id: null. The guard uses initializeRequestId === undefined (not a falsy check), so a 0 or null initialize id is tracked and matched correctly. Both are exercised by the tests (id: 0 in the gauge test, id: null in the ordering test). Using !initializeRequestId here would have been a subtle bug — good that it wasn't.
  • 'method' in message correctly distinguishes a JSON-RPC response from an agent→client request that happens to collide on the initialize id (the session/request_permission case in the test). Responses have no method; requests do → early return. ✅
  • The post-write ordering claim holds. In ndJsonStream, the sent observation fires only after await writer.write(frame) resolves and is skipped entirely on write rejection; the received observation fires after controller.enqueue. Hooks are wrapped in callHook's try/catch, so a throwing observer can't break the transport. The PR's semantics rest on exactly these guarantees.
  • The gauge-registration move (2nd commit) fixes a real hazard, not cosmetics. registerEventLoopLagGauge calls getMeter() and, if it gets a meter, sets acpGaugeRegistered = true permanently. Registering before the SDK installs the global meter provider would bind the gauge to a cached no-op meter and never recover. Deferring registration into initializeTelemetry(...).then(...) guarantees it runs after the SDK is up. The test asserts this ordering (registerAcpEventLoopLagGauge not called until the init promise resolves). ✅
  • Idempotent / no leak on re-initialize. A second initialize re-arms the id; the follow-up initializeTelemetry is a single-flight no-op (telemetryInitialized early return) and the gauge re-registration is guarded. Disabled-telemetry mode returns Promise.resolve() without loading heavy modules, so the per-response call is cheap.

Minor suggestions (optional, non-blocking)

  1. No .catch() on the deferred start. void initializeTelemetry(config).then(() => registerAcpEventLoopLagGauge(...))initializeTelemetry itself never rejects (it swallows internally), but callHook only guards the synchronous invocation, not the async continuation. If the .then callback ever throws (e.g. createObservableGauge), it becomes an unhandled rejection. The constructor path already guards this as void Promise.resolve(initializeTelemetry(this)).catch(...); mirroring that here would be consistent and cheap.
  2. Config.shutdown() in-flight skip (already disclosed). shutdown() gates shutdownTelemetry() on isTelemetrySdkInitialized(), yet shutdownTelemetry() itself already awaits a pending init. So an immediate post-response exit within the SDK-import window can skip flushing an in-flight SDK. Impact is low (best-effort telemetry) and it's honestly documented — but gating on "initialized or init-in-flight" would close the window cleanly.
  3. Test layering. The acpAgent tests drive onMessageObserved with synthetic messages; the actual post-write ordering is only covered by the separate ndJsonStream transport tests. That split is reasonable, but one integration test that pumps a real initialize request/response through ndJsonStream and asserts telemetry starts would tie the two layers together.
  4. Theoretical id collision. A client→agent response sharing the initialize id and sent before the initialize response could, in principle, trigger an early start. Not reachable in practice (initialize is the first awaited exchange and JSON-RPC ids are unique among in-flight requests), but a one-line comment noting that assumption would help future readers.

Style / conventions

  • Follows repo conventions (Apache header untouched, subpath import @qwen-code/acp-bridge/ndJsonStream matches existing usage, comment updated to reflect new ACP semantics). The design doc under docs/design/ is thorough and the risk/scope section is candid.
  • Test coverage is good: config-deferral assertion, the full ordering state machine (receipt / unrelated response / colliding-id request / error response / one start after matching success), and the gauge-after-init ordering.
中文说明

结论:实现扎实,可合并。 改动范围收敛,依赖的跨模块契约都成立,风险披露诚实且与代码一致。下面是几点可选的健壮性建议,均非阻塞项。

功能:ACP 模式现在设置已有的 deferTelemetryInitialization,从而抑制 Config 构造函数里 fire-and-forget 的 telemetry 启动;runAcpAgent 通过 ndJsonStreamonMessageObserved 钩子记住收到的 initialize 请求 id,仅在匹配的成功响应写入 stdout 之后才启动 telemetry,并把 ACP event-loop gauge 的注册移到初始化完成之后。非 ACP 路径行为不变。

正确性(已对源码核对)

  • 配置开关重构对非 ACP 完全等价(真值表验证),仅对 ACP 从 false 翻为 true
  • 哨兵判断刻意用 === undefined,因此 id: 0id: null 都能正确追踪/匹配(测试均覆盖)。
  • 'method' in message 能正确区分响应与共用 id 的 agent→client 请求。
  • 依赖 ndJsonStream 的语义:sent 观察只在 await writer.write 成功后触发,写入失败则跳过;钩子异常被 callHook 吞掉,不会破坏传输。
  • gauge 注册后移(第 2 个 commit)修复的是真实隐患:过早注册会把 gauge 绑到缓存的 no-op meter 并永久失效。测试断言了该顺序。
  • 重复 initialize 幂等、无泄漏;telemetry 关闭时是廉价 no-op。

建议(可选,非阻塞)

  1. 延迟启动缺少 .catch()initializeTelemetry 本身不会 reject,但 callHook 只保护同步调用;若 .then 回调抛错会变成 unhandled rejection。构造函数路径已用 .catch(...),此处宜保持一致。
  2. Config.shutdown() 的 in-flight 跳过(已披露):shutdown()isTelemetrySdkInitialized() 门控,而 shutdownTelemetry() 自身其实已会等待进行中的初始化;对"已初始化 初始化进行中"门控可关闭该窗口。影响很小。
  3. 测试分层:acpAgent 测试用合成消息驱动钩子,真正的写后顺序由 ndJsonStream 传输测试覆盖;补一个把真实 initialize 请求/响应打进 ndJsonStream 的集成测试可把两层串起来。
  4. 理论上的 id 碰撞:先于 initialize 响应发送、且共用 id 的 client→agent 响应理论上可能提前触发启动;实践中不可达,补一行注释说明该假设即可。

@doudouOUC
doudouOUC requested a review from yiliang114 July 23, 2026 09:03
return;
}
if (
direction !== 'sent' ||

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Good fix moving registerAcpEventLoopLagGauge behind initializeTelemetry — prevents registering on a cached no-op meter. If initializeTelemetry rejects, the gauge silently never registers. Consider a .catch that logs a warning, so a telemetry SDK failure is at least visible in stderr rather than silently disabling metrics.

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

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Clean deferral using transport-level write confirmation — no timing assumptions. Event-loop gauge moving behind SDK init fixes the cached no-op meter issue. Tests cover all edge cases (no start on receipt/unrelated/error, start only on matching success). Design doc with benchmark data is thorough (-44ms P50 on channel.initialize). One inline nit on adding a .catch for telemetry rejection visibility.

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

@doudouOUC
doudouOUC added this pull request to the merge queue Jul 23, 2026
Merged via the queue into QwenLM:main with commit 779e106 Jul 23, 2026
90 checks passed
@doudouOUC
doudouOUC deleted the perf/defer-acp-telemetry-init branch July 23, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants