feat(telemetry): trace daemon prompt lifecycle by doudouOUC · Pull Request #4556 · QwenLM/qwen-code

doudouOUC · 2026-05-26T14:53:56Z

Summary

What changed: Added daemon OpenTelemetry propagation across qwen serve HTTP routes, ACP bridge prompt dispatch, and ACP child prompt execution. The daemon now injects reserved qwen.telemetry metadata internally and restores the captured OTel context after prompt queueing.
Why it changed: Issue feat(telemetry): cover qwen serve daemon end-to-end with OpenTelemetry #4554 needs prompt traces to connect from daemon request handling through bridge activity into ACP prompt interaction spans, so LLM/tool telemetry can be correlated without frontend protocol changes.
Reviewer focus: Check the daemon trace context carrier behavior, prompt FIFO context restoration, scoped ACP interaction span parenting, and the route-level span/error status mapping.

Validation

Commands run:

cd packages/core && npx vitest run src/telemetry/daemon-tracing.test.ts src/telemetry/session-tracing.test.ts
cd packages/acp-bridge && npx vitest run src/bridge.test.ts
cd packages/cli && npx vitest run src/acp-integration/session/Session.test.ts
cd packages/cli && npx vitest run src/serve/server.test.ts
cd packages/cli && npx vitest run src/serve/runQwenServe.test.ts
npm run build && npm run typecheck

Prompts / inputs used: N/A, this PR is validated through focused unit tests plus package/root build and typecheck.
Expected result: Daemon route spans, bridge spans, ACP prompt interaction spans, and downstream telemetry share the propagated trace context. Clients do not need to send telemetry metadata, and spoofed qwen.telemetry metadata is overwritten or stripped.
Observed result: All listed tests and build/typecheck commands passed locally. Root build emitted existing Browserslist and vscode companion lint warnings, with zero errors.
Quickest reviewer verification path: Run the three focused vitest commands for core telemetry, ACP bridge, and CLI ACP session, then run npm run build && npm run typecheck from the repo root.
Evidence (output, logs, screenshots, video, JSON, before/after, etc.): Local command output showed Test Files ... passed for each focused suite and successful root build/typecheck completion.

Scope / Risk

Main risk or tradeoff: This adds application-level daemon route spans while keeping existing HttpInstrumentation, so backends may show both HTTP instrumentation spans and qwen-code daemon spans.
Not covered / not validated: No live OTLP backend smoke test was run, and Windows/Linux validation was not run locally.
Breaking changes / migration notes: No frontend or external prompt API changes. The qwen.telemetry.* prompt metadata namespace is now reserved for daemon-internal trace propagation.

Testing Matrix

	macOS	Windows	Linux
npm run	tested	not tested	not tested
npx	tested	not tested	not tested
Docker	N/A	N/A	N/A
Podman	N/A	N/A	N/A
Seatbelt	N/A	N/A	N/A

Testing matrix notes:

Validated on the local macOS checkout only.

Linked Issues / Bugs

Closes #4554

📝 描述准确性更新（2026-05-31，作者自查）

更正：宜为 addresses part of #4554；issue 第 6 区（daemon metrics）未在本 PR 交付。

github-actions · 2026-05-26T14:56:33Z

📋 Review Summary

This PR implements end-to-end OpenTelemetry trace propagation for the daemon mode, connecting HTTP route handling through the ACP bridge to prompt execution spans. The implementation is well-structured with comprehensive test coverage and follows established telemetry patterns. The changes enable LLM/tool telemetry correlation without requiring frontend protocol changes.

🔍 General Feedback

Strong architectural separation: The new daemon-tracing.ts module cleanly encapsulates daemon-specific telemetry concerns with clear boundaries from session-tracing
Good test coverage: Focused unit tests validate trace context injection/extraction, spoofing protection, and span lifecycle
Defensive telemetry design: Consistent try-catch blocks throughout ensure telemetry failures never affect request handling
Well-documented security considerations: Trace context validation, reserved metadata namespace, and spoofing protection are clearly documented
Backward compatible: No-op telemetry fallback for non-daemon consumers via NOOP_BRIDGE_TELEMETRY

🎯 Specific Feedback

🟡 High

packages/core/src/telemetry/daemon-tracing.ts:238-247 — The extractDaemonTraceContext function has a fallback manual trace context reconstruction that bypasses OTel's propagation.extract. This dual-path approach could diverge if OTel's extraction logic changes. Consider consolidating to use only the OTel propagation API, or add a comment explaining why the manual fallback is necessary (e.g., handling edge cases where OTel extraction fails).
packages/cli/src/serve/server.ts:2858-2868 — Error logging in sendBridgeError calls recordDaemonError(undefined, err, ...) with an undefined span. While this works (the function falls back to active span), it's unclear why a span isn't being passed from the request span context. Consider capturing the span context at the route handler level and threading it through to error reporting.

🟢 Medium

packages/acp-bridge/src/bridge.ts:95-117 — The NOOP_BRIDGE_TELEMETRY injectPromptContext strips reserved trace metadata but doesn't match the exact implementation in daemon-tracing.ts:68-77. Consider importing and reusing stripReservedTraceMeta from daemon-tracing to avoid duplication and potential drift.
packages/core/src/telemetry/daemon-tracing.ts:108-117 — The withDaemonSpan function has autoOkOnSuccess defaulting to true, but withDaemonRequestSpan overrides to false. This pattern is reasonable but the rationale isn't obvious. Add a comment explaining why request spans require explicit status setting (likely because HTTP status codes determine success/failure semantics).
packages/core/src/telemetry/session-tracing.ts:346-425 — The new withInteractionSpan function is substantial (80 lines) but lacks JSDoc explaining when to use it vs the existing startInteractionSpan/endInteractionSpan pair. Consider adding documentation about the scoped span pattern and why it's beneficial (e.g., "Use when you need a self-contained interaction span that doesn't mutate global context").
packages/cli/src/serve/server.ts:178-203 — The detectFromLoopback function is exported for testing with a comment referencing specific review numbers. While the security rationale is well-documented, consider whether this should be tested via integration tests rather than unit testing an exported helper, to avoid exposing internal implementation details.

🔵 Low

packages/core/src/telemetry/daemon-tracing.ts:1 — The license year says 2026, which matches the conversation context date. However, other files in the repo (e.g., bridge.ts shows "Copyright 2025") may have inconsistent years. Consider a quick pass to verify consistency across all modified files.
packages/acp-bridge/src/bridgeOptions.ts:283-341 — Several new BridgeOptions fields have extensive JSDoc comments referencing specific PR numbers and review threads (e.g., "proposal(serve): Mode B feature-priority roadmap toward v0.16 production-ready #4175 Wave 5 PR 22b/2"). While this context is valuable for current reviewers, consider whether some of this should live in a separate DESIGN.md or ARCHITECTURE.md file for long-term maintainability, as these references may become stale.
packages/cli/src/serve/runQwenServe.ts:89-159 — The validatePolicyConfig function has a detailed comment about deriving valid policies from SERVE_CAPABILITY_REGISTRY, but the function itself could benefit from a concise JSDoc summary at the top explaining its purpose in one sentence before the detailed implementation notes.
packages/core/src/telemetry/daemon-tracing.ts:267-282 — The createDaemonBridgeTelemetry function returns an inline object. Consider extracting the event method's try-catch into a named helper for consistency with other telemetry functions that have explicit error handling comments.

✅ Highlights

Excellent security posture: The stripReservedTraceMeta function and spoofing protection test (injectDaemonTraceContext strips spoofed daemon metadata) demonstrate thoughtful security design for the trace context injection
Clean abstraction boundaries: The BridgeTelemetry interface in bridgeOptions.ts provides a clean seam for telemetry injection without coupling the bridge to OTel internals
Comprehensive error handling: Every telemetry operation is wrapped in try-catch with the consistent pattern "Telemetry must not affect [behavior]" — this is exactly right for observability code
Good test structure: The test for uses bridge telemetry for channel/session/prompt dispatch validates the full integration path with clear assertions on both operations and metadata injection
Thoughtful workspace privacy: The hashDaemonWorkspace function uses SHA256 with 16-char truncation to avoid exposing raw paths in telemetry

🔐 Security Notes

The implementation correctly:

Strips client-provided qwen.telemetry.* metadata to prevent trace context spoofing
Validates traceparent format before extraction (version, traceId, spanId, flags format checks)
Uses hashed workspace paths instead of raw paths in telemetry attributes
Keeps telemetry failures from affecting request handling (fail-open for observability)

One consideration for future review: the detectFromLoopback function in server.ts intentionally reads only req.socket.remoteAddress and ignores X-Forwarded-For. This is correct for the local-only security model, but ensure this is documented in any operator-facing docs about daemon deployment behind reverse proxies.

Copilot

Pull request overview

Adds end-to-end OpenTelemetry trace propagation for qwen serve so prompt execution can be correlated from daemon HTTP handling, through the ACP bridge queue/dispatch, into ACP child prompt interaction spans—without requiring client protocol changes (reserving qwen.telemetry.* internal metadata).

Changes:

Introduces daemon tracing utilities to inject/extract W3C trace context via reserved _meta keys and to create daemon request/bridge spans.
Initializes telemetry in the serve daemon process and adds an Express middleware to wrap key routes with daemon request spans and error recording.
Adds a scoped withInteractionSpan helper and uses it in ACP child prompt handling to parent interaction spans to the daemon-extracted context; extends bridge to capture/restore context across prompt FIFO queueing.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/core/src/telemetry/session-tracing.ts	Adds `withInteractionSpan` for scoped interaction spans with optional explicit parent context.
packages/core/src/telemetry/session-tracing.test.ts	Adds test coverage for scoped interaction spans and parentContext behavior.
packages/core/src/telemetry/sdk.ts	Generalizes telemetry initialization to accept `TelemetryRuntimeConfig` instead of `Config`.
packages/core/src/telemetry/runtime-config.ts	Introduces `TelemetryRuntimeConfig` interface for non-`Config` runtimes (e.g., daemon).
packages/core/src/telemetry/metrics.ts	Updates metric initialization/perf monitoring to accept `TelemetryRuntimeConfig`.
packages/core/src/telemetry/index.ts	Exports new daemon tracing APIs and runtime config type.
packages/core/src/telemetry/daemon-tracing.ts	Adds daemon tracing helpers (request/bridge spans, context capture/restore, traceparent injection/extraction, workspace hashing).
packages/core/src/telemetry/daemon-tracing.test.ts	Adds focused tests for trace context extraction/injection behavior and workspace hashing.
packages/cli/src/serve/server.ts	Adds daemon request-span middleware and emits daemon telemetry on bridge errors.
packages/cli/src/serve/runQwenServe.ts	Initializes telemetry in the daemon process and wires a telemetry seam into the bridge; ensures telemetry shutdown during drain.
packages/cli/src/acp-integration/session/Session.ts	Wraps ACP prompt execution in `withInteractionSpan` and parents to extracted daemon trace context.
packages/acp-bridge/src/bridgeOptions.ts	Introduces `BridgeTelemetry` seam for bridge-level spans/events/context capture and prompt metadata injection.
packages/acp-bridge/src/bridge.ts	Uses bridge telemetry seam to span channel/session/prompt operations and restore context across prompt queueing.
packages/acp-bridge/src/bridge.test.ts	Adds test validating telemetry seam usage and prompt metadata injection behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

doudouOUC · 2026-05-27T11:42:23Z

Review fix summary (commit `43071d2`)

Fixed (10 items)

#	File:Line	Issue	Fix
1	`daemon-tracing.ts:148`	[Critical] `recordDaemonHttpResponse` clobbers ERROR with OK on non-5xx	Removed the `else { setStatus(OK) }` branch — only 5xx sets status now
2	`runQwenServe.ts:1090`	[Critical] Signal listener race during telemetry shutdown	Moved `process.removeListener` calls before `shutdownTelemetry()`
3	`daemon-tracing.ts:275`	Fallback accepts all-zero trace/span IDs	Added `INVALID_TRACE_ID`/`INVALID_SPAN_ID` rejection
4	`daemon-tracing.ts:225`	`propagation.inject` missing try/catch	Wrapped in try/catch with `// Telemetry must not affect prompt forwarding`
5	`bridge.ts:2092`	`injectPromptContext` captures wrong parent span	Moved inside `withSpan('prompt.dispatch')` callback
6	`daemon-tracing.ts:78`	`withDaemonSpan` no telemetry guard	Added `isTelemetrySdkInitialized()` early return
7	`daemon-tracing.ts:43`	`toOtelAttributes` identity function	Removed, pass `attributes` directly
8	`daemon-tracing.ts:282`	Fallback missing `isRemote: true` + tracestate	Added both to `wrapSpanContext` call
9	`daemon-tracing.ts:234`	Adds empty `_meta: {}` when none existed	Early-return `request` unchanged when no active span and no existing `_meta`
11	`daemon-tracing.ts:191`	Redundant `event.timestamp` attribute	Removed — OTel LogRecord has built-in timestamp

Not taking (1 item)

#	File:Line	Issue	Reasoning
10	`daemon-tracing.ts:258`	`propagation.extract` passes active context as base	This is intentional — the daemon request span should be the parent of the extracted context. Passing `ROOT_CONTEXT` would orphan the interaction span from the daemon request trace tree.

Deferred (6 items — follow-up scope)

Related to #5: Fixed bounds checking in RadioButtonSelect component to ensure active… #12 session-tracing.ts:357 — Extract shared attribute helper (refactor)
Qwen-Code still tries to access Google's IP #13 session-tracing.ts:411 — Cancelled prompt status semantics (design discussion)
为什么不能跟cc或者geminicli一样，把密钥放在终端的环境变量里面？ #14 daemon-tracing.test.ts:1 — Test coverage gaps
运行不了 #15 server.ts:338 — Express error middleware
Ubuntu上选择完Open AI以后进入就直接崩溃，Mac没问题 #16 bridgeOptions.ts:100 — JSDoc on BridgeTelemetry interface
OPENAI_BASE_URL 这样设置，运行成功 #17 runQwenServe.ts:704 — resolveTelemetrySettings error handling

Ready for re-review.

wenshao

[Critical] new TraceState(carrier['tracestate']) at line 289 crashes at runtime (TypeError: TraceState is not a constructor). TraceState is a type-only export (interface) from @opentelemetry/api. Build fails (TS1484 + TS2693), and 1 test fails (extracts daemon trace context from reserved prompt metadata keys).

Fix: import createTraceState from @opentelemetry/core and use createTraceState(carrier['tracestate']).

(Re-stating: this was flagged in R4 and remains unfixed at HEAD.)

doudouOUC · 2026-05-27T13:51:08Z

Fix: TraceState constructor crash (commit `5b39d83`)

File:Line	Issue	Fix
`daemon-tracing.ts:289`	[Critical] `new TraceState()` crashes — type-only export from `@opentelemetry/api`	Removed TraceState import and constructor call. Manual fallback now omits tracestate (primary `propagation.extract` path handles it).

Ready for re-review.

doudouOUC · 2026-05-27T13:58:03Z

Fix round 3 (commit `79aff11`)

Fixed (3)

File:Line	Issue	Fix
`daemon-tracing.ts:84`	`as Span` cast passes undefined when SDK off	Pass `undefined as unknown as Span` explicitly — callers use `?.`
`daemon-tracing.ts:227`	`stripReservedTraceMeta` always copies	Added `in` guard before copying, skips copy when no reserved keys
`server.ts:3498`	`err.message` not truncated in log attribute	Added `.slice(0, 1024)`

Deferred (3)

server.ts:329 — close-vs-finish event model (scope expansion)
session-tracing.ts:346 — error/cancelled path test coverage (follow-up)
runQwenServe.ts:714 — telemetry shutdown on boot failure (scope expansion)

Ready for re-review.

doudouOUC · 2026-05-27T18:07:38Z

Fix round 4 (commit `28a4748`)

Fixed (5)

File:Line	Issue	Fix
`daemon-tracing.ts:268`	`propagation.extract` uses active context as base — invalid carrier returns agent's own span	Use `ROOT_CONTEXT` as extraction base
`daemon-tracing.ts:286`	Manual fallback also uses `otelContext.active()` for `trace.setSpan`	Use `ROOT_CONTEXT`
`daemon-tracing.ts:245`	`injectDaemonTraceContext` unconditionally adds `_meta: {}` even with no trace	Skip `_meta` when original had none and no headers injected
`session-tracing.ts:411`	Cancelled prompts get `SpanStatusCode.OK`	Cancelled now gets UNSET; only explicit success sets OK
`daemon-tracing.ts:191`	`emitDaemonLog` uses custom `event.timestamp` attribute	Use OTel built-in `timestamp` field

Deferred (6)

File:Line	Reason
`session-tracing.ts:357`	`withInteractionSpan` deduplication — refactor, scope expansion
`daemon-tracing.test.ts:1`	Additional test coverage — dedicated test PR
`server.ts:338`	Express 4 async error middleware — scope expansion
`bridgeOptions.ts:100`	JSDoc for BridgeTelemetry — follow-up
`runQwenServe.ts:704`	FatalConfigError graceful degradation — scope expansion
`runQwenServe.ts:714`	Boot failure telemetry shutdown — scope expansion

Not taking (3)

File:Line	Reason
`server.ts:329`	Client disconnect handling — scope expansion, status already captured via `finish` event
`session-tracing.ts:346`	Error/cancelled test coverage — deferred to test PR
`server.ts:3498`	Surrogate-safe `.slice()` — project-wide concern, not scoped here

Ready for re-review.

doudouOUC · 2026-05-28T05:24:10Z

Fix round 5 (commit `646226b`)

Fixed (2)

File:Line	Issue	Fix
`bridge.ts:2180`	[Critical] duplicate `const normalized` — dead code from round-1 refactor	Removed first declaration; kept the consumed one at line 2216
`session-tracing.ts:340`	`endInteractionSpan` cancelled → OK inconsistent with `withInteractionSpan`	Changed to `else if (status === 'ok')` — cancelled now gets UNSET in both paths; test updated

Not taking (6)

File:Line	Reason
`bridge.ts:88`	NOOP constants intentionally self-contained
`daemon-tracing.ts:317`	`event()` overhead negligible; try/catch already present
`daemon-tracing.ts:79`	`.slice(0,16)` is standard for non-security hashing
`bridge.ts:100`	Same NOOP independence reasoning
`server.ts:3510`	Different truncation per-purpose is correct
`daemon-tracing.ts:163`	Non-5xx path doesn't override ERROR (already fixed in round 1)

Ready for re-review.

doudouOUC · 2026-05-28T05:27:52Z

Review round 5 — wenshao's latest comments (May 28)

File	Comment	Verdict	Action
`bridge.ts:88`	Re-declared meta key constants	Agreed	Imported from core in `e075c6f`
`daemon-tracing.ts:317`	`event()` missing `isTelemetrySdkInitialized()` guard	Agreed	Added guard in `e075c6f`
`daemon-tracing.ts:163`	`recordDaemonHttpResponse` overwrites descriptive ERROR with generic "HTTP 500"	Agreed	Removed `setStatus` for 5xx in `e075c6f`
`session-tracing.ts:410` (×2)	`withInteractionSpan` vs `endInteractionSpan` cancelled mapping inconsistency	Pushed back	Both use `else if (status === 'ok')` — cancelled → UNSET in both. No inconsistency.
`bridge.ts:2180`	[Critical] `const normalized` declared twice	Pushed back	Already fixed in `94befa0` (round 1). Only one declaration remains.
`daemon-tracing.ts:79`	`.slice(0, 16)` magic number	Not taking	Single-use in a 2-line function; function name provides sufficient context
`bridge.ts:100`	Strip-reserved-keys logic duplication	Deferred	Scope expansion; constant import mitigates drift
`server.ts:3510`	Duplicate error extraction in `sendBridgeErrorImpl`	Not taking	Cold path; `recordDaemonError` does internal extraction regardless

Fixed 3 / Pushed back 3 / Not taking 2 / Deferred 1 in commit e075c6f.

Connect qwen serve HTTP routes, ACP bridge dispatch, and ACP child prompt execution through OpenTelemetry context propagation. The daemon injects reserved qwen.telemetry metadata internally so clients do not need to pass trace context. Closes QwenLM#4554 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

Record bridge telemetry events as short daemon bridge spans when they fire outside an active request or prompt context, so asynchronous channel exits remain observable. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

- recordDaemonHttpResponse: don't clobber ERROR with OK on non-5xx - finish(): remove signal listeners synchronously before async telemetry shutdown - extractDaemonTraceContext: reject all-zero IDs, include tracestate, set isRemote - propagation.inject: wrap in try/catch for consistency - injectPromptContext: move inside prompt.dispatch span for correct parent - withDaemonSpan: guard on isTelemetrySdkInitialized() - toOtelAttributes: remove identity function, pass attributes directly - injectDaemonTraceContext: early-return when no active span (avoid empty _meta) - emitDaemonLog: remove redundant event.timestamp attribute - NOOP_BRIDGE_TELEMETRY: drop async, add short-circuit for missing keys

TraceState is a type-only export from @opentelemetry/api (not a runtime constructor). The manual fallback path now omits tracestate since the primary propagation.extract path already handles it.

- withDaemonSpan: pass undefined (not getSpan result) when SDK off - stripReservedTraceMeta: skip copy when no reserved keys present - sendBridgeErrorImpl: truncate error.message in emitDaemonLog

- extractDaemonTraceContext: use ROOT_CONTEXT as extraction base to prevent incorrect parent-child when agent has its own active span - extractDaemonTraceContext (manual fallback): already has isRemote:true and ROOT_CONTEXT from previous fix — confirmed consistent - injectDaemonTraceContext: skip _meta assignment when original had no _meta and no trace headers were injected (match NOOP behavior) - withInteractionSpan: cancelled prompts get UNSET instead of OK so dashboards can distinguish cancelled from successful - emitDaemonLog: use OTel built-in timestamp field instead of custom attribute

- Import DAEMON_TRACEPARENT/TRACESTATE_META_KEY from core instead of redeclaring locally in bridge.ts (drift risk) - Add isTelemetrySdkInitialized() guard to event() in createDaemonBridgeTelemetry for consistency with siblings - Remove setStatus(ERROR, "HTTP 500") from recordDaemonHttpResponse to avoid overwriting the descriptive error message already set by recordDaemonError

doudouOUC · 2026-05-28T13:41:03Z

Review round 6 — wenshao's latest CHANGES_REQUESTED (May 28)

File	Comment	Verdict	Action
`daemon-tracing.ts:269`	[Critical] `extractDaemonTraceContext` missing try/catch around OTel calls	Agreed	Wrapped entire body in try/catch with `return undefined` fallback in `2938ad3`
`server.ts:339`	[Suggestion] Redundant `recordDaemonError` — `withDaemonSpan` catch already records on rejection	Agreed	Removed redundant call in `2938ad3`

Fixed 2/2 in commit 2938ad3.

doudouOUC · 2026-05-28T19:19:43Z

Review round 7 — wenshao's CHANGES_REQUESTED (May 28 late)

File	Comment	Verdict	Action
`daemon-tracing.ts:168`	[Suggestion] `recordException` passes raw untruncated Error	Agreed	Pass `new Error(message)` with truncated message in `6f8cb66`
`daemon-tracing.ts:251`	[Critical] `extractDaemonTraceContext` still missing try/catch	Pushed back	Already fixed in `2938ad3`; `/qreview` ran against stale SHA
`server.ts:339`	[Suggestion] Redundant `recordDaemonError` still present	Pushed back	Already fixed in `2938ad3`; same stale-SHA issue
`session-tracing.ts:411`	[Suggestion] finally block missing nested try/catch	Deferred	Pre-existing code, not in PR diff
`bridge.ts:2198`	[Suggestion] prompt.dispatch parents to finished daemon.request span	Deferred	Architectural; tracked in #4602
`daemon-tracing.ts:218`	[Suggestion] Missing happy-path test for injectDaemonTraceContext	Deferred	Test coverage follow-up
`daemon-tracing.ts:148`	[Suggestion] Missing tests for 4 exported functions	Deferred	Test coverage follow-up
`bridge.ts:2237`	[Suggestion] Queued AbortError → ERROR span inconsistency	Deferred	Cancel semantics alignment (#4602)
`bridge.ts:2720`	[Suggestion] session.close cancel path emits ERROR for normal teardown	Deferred	Pre-existing cancel flow
`Session.ts:899`	[Suggestion] Catch includes AbortError → StopFailure hook	Deferred	Pre-existing Session.ts code (#4602)

Fixed 1 / Pushed back 2 / Deferred 7 in commit 6f8cb66.

wenshao

No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review

wenshao · 2026-05-29T06:12:05Z

Maintainer Post-Merge Verification Report

Reviewer: @penwenshao (local build & test)
Branch: feat/daemon-otel-e2e-design → daemon_mode_b_main
Environment: macOS Darwin 25.4.0, Node.js, TypeScript 5.8.3
Note: PR was already merged; this is a post-merge verification for the record.

1. TypeScript Type Check

Package	Result	Notes
`packages/cli` (`tsc --noEmit`)	✅ Pass (0 errors)	Clean — no type errors at all
`packages/core` (`tsc -p`)	✅ Build success	dist produced correctly
`packages/acp-bridge` (`tsc --build`)	✅ Build success	dist produced (TS5055 warnings are pre-existing stale dist, non-blocking)

2. Unit Tests

Test Suite	Tests	Result
`core/telemetry/daemon-tracing.test.ts`	4 passed	✅
`core/telemetry/session-tracing.test.ts`	74 passed	✅
`acp-bridge/bridge.test.ts`	207 passed	✅
`cli/serve/server.test.ts`	337 passed	✅
`cli/serve/runQwenServe.test.ts`	20 passed	✅
`cli/acp-integration/session/Session.test.ts`	81 passed	✅

Total: 723 tests passed, 0 failed

3. Code Review

Architecture (daemon-tracing.ts): ✅

Clean separation: daemon-tracing.ts (338 lines) handles daemon-side OTel propagation, session-tracing.ts (1055 lines) handles ACP-child-side interaction/tool/hook spans
BridgeTelemetry interface in bridgeOptions.ts provides a clean injection seam — bridge is decoupled from OTel SDK, testable with mock telemetry
createDaemonBridgeTelemetry() factory wires the concrete OTel calls at the runQwenServe level

Trace context propagation: ✅

injectDaemonTraceContext() serializes active OTel context into _meta.qwen.telemetry.traceparent/tracestate on outbound prompt requests
extractDaemonTraceContext() deserializes on the ACP child side, with manual W3C traceparent fallback when propagation.extract() returns empty
stripReservedTraceMeta() sanitizes client-supplied _meta to prevent spoofed trace context — good security practice

Context capture/restore for prompt FIFO: ✅

captureDaemonTelemetryContext() snapshots OTel context at HTTP request time
runWithDaemonTelemetryContext() restores it when the prompt reaches the head of the FIFO queue — correct pattern for async queueing

Error handling: ✅

Every OTel call wrapped in try/catch with "Telemetry must not affect X" comments — consistent defensive pattern
recordDaemonError() records exception, sets error attributes and span status in one call
recordDaemonHttpResponse() stamps HTTP status code on the span for route-level correlation

Span naming conventions: ✅

qwen-code.daemon.request for HTTP route spans
qwen-code.daemon.bridge for bridge operation spans
Follows OpenTelemetry semantic conventions for HTTP attributes (http.request.method, http.route, http.response.status_code)

Test coverage: ✅

daemon-tracing.test.ts: 4 focused tests covering extract/inject/hash/event-emit
session-tracing.test.ts: 74 comprehensive tests including withInteractionSpan, TTL safety net, surrogate pair truncation
Bridge test: new telemetry integration test verifying channel/session/prompt dispatch spans and metadata injection

Security: ✅

hashDaemonWorkspace() uses SHA-256 to hash workspace paths — raw paths never appear in telemetry attributes
Reserved qwen.telemetry.* metadata keys are stripped from client-supplied _meta before injection — prevents trace context spoofing

Verdict: ✅ Verified — merge was correct

All 723 tests pass. The OTel propagation design is clean, well-tested, and follows security best practices (workspace hashing, reserved metadata stripping). The BridgeTelemetry injection seam keeps the bridge package decoupled from OTel SDK details.

doudouOUC mentioned this pull request May 26, 2026

feat(telemetry): cover qwen serve daemon end-to-end with OpenTelemetry #4554

Closed

10 tasks

doudouOUC marked this pull request as ready for review May 26, 2026 14:55

Copilot AI review requested due to automatic review settings May 26, 2026 14:55

doudouOUC changed the title ~~[codex] feat(telemetry): trace daemon prompt lifecycle~~ feat(telemetry): trace daemon prompt lifecycle May 26, 2026

doudouOUC self-assigned this May 26, 2026

Copilot started reviewing on behalf of doudouOUC May 26, 2026 14:56 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

Comment thread packages/cli/src/serve/server.ts

Comment thread packages/cli/src/serve/server.ts

Comment thread packages/acp-bridge/src/bridge.ts Outdated

Comment thread packages/core/src/telemetry/daemon-tracing.ts

doudouOUC force-pushed the feat/daemon-otel-e2e-design branch from f145ab1 to 38f75fb Compare May 26, 2026 17:19

doudouOUC force-pushed the daemon_mode_b_main branch 2 times, most recently from 1284383 to 1ca0572 Compare May 27, 2026 02:36

doudouOUC force-pushed the feat/daemon-otel-e2e-design branch from 38f75fb to 7e7f6a5 Compare May 27, 2026 02:48

doudouOUC requested a review from wenshao May 27, 2026 02:54

wenshao reviewed May 27, 2026

View reviewed changes

doudouOUC force-pushed the feat/daemon-otel-e2e-design branch from 7e7f6a5 to 79a14b1 Compare May 27, 2026 07:30

wenshao requested changes May 27, 2026

View reviewed changes

Comment thread packages/core/src/telemetry/daemon-tracing.ts Outdated

Comment thread packages/cli/src/serve/runQwenServe.ts

Comment thread packages/core/src/telemetry/daemon-tracing.ts

doudouOUC requested a review from wenshao May 27, 2026 09:50

wenshao reviewed May 27, 2026

View reviewed changes

doudouOUC requested a review from wenshao May 27, 2026 11:49

wenshao requested changes May 27, 2026

View reviewed changes

Comment thread packages/core/src/telemetry/daemon-tracing.ts Outdated

Comment thread packages/core/src/telemetry/daemon-tracing.ts Outdated

doudouOUC requested a review from wenshao May 27, 2026 13:20

wenshao requested changes May 27, 2026

View reviewed changes

Comment thread packages/cli/src/serve/server.ts

Comment thread packages/core/src/telemetry/daemon-tracing.ts

Comment thread packages/core/src/telemetry/session-tracing.ts

Comment thread packages/cli/src/serve/server.ts Outdated

Comment thread packages/cli/src/serve/runQwenServe.ts

doudouOUC requested a review from chiga0 May 27, 2026 13:46

doudouOUC requested a review from wenshao May 27, 2026 14:01

wenshao reviewed May 27, 2026

View reviewed changes

Comment thread packages/cli/src/serve/server.ts

doudouOUC requested a review from wenshao May 27, 2026 17:55

doudouOUC force-pushed the feat/daemon-otel-e2e-design branch from 28a4748 to 4ff18e0 Compare May 28, 2026 03:22

wenshao reviewed May 28, 2026

View reviewed changes

Comment thread packages/acp-bridge/src/bridge.ts

Comment thread packages/core/src/telemetry/daemon-tracing.ts

Comment thread packages/acp-bridge/src/bridge.ts

Comment thread packages/cli/src/serve/server.ts

Comment thread packages/core/src/telemetry/daemon-tracing.ts

doudouOUC requested a review from wenshao May 28, 2026 06:25

doudouOUC and others added 7 commits May 28, 2026 16:44

fix(telemetry): emit daemon bridge events as spans

91aa391

Record bridge telemetry events as short daemon bridge spans when they fire outside an active request or prompt context, so asynchronous channel exits remain observable. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>

fix(telemetry): remove TraceState constructor usage in manual fallback

9c37a3d

TraceState is a type-only export from @opentelemetry/api (not a runtime constructor). The manual fallback path now omits tracestate since the primary propagation.extract path already handles it.

fix(telemetry): address wenshao review round 3

db06c28

- withDaemonSpan: pass undefined (not getSpan result) when SDK off - stripReservedTraceMeta: skip copy when no reserved keys present - sendBridgeErrorImpl: truncate error.message in emitDaemonLog

doudouOUC force-pushed the feat/daemon-otel-e2e-design branch from 646226b to 9041d56 Compare May 28, 2026 08:47

doudouOUC requested a review from yiliang114 May 28, 2026 09:09

wenshao reviewed May 28, 2026

View reviewed changes

Comment thread packages/core/src/telemetry/session-tracing.ts

doudouOUC requested a review from wenshao May 28, 2026 12:29

wenshao requested changes May 28, 2026

View reviewed changes

Comment thread packages/core/src/telemetry/daemon-tracing.ts

Comment thread packages/cli/src/serve/server.ts

doudouOUC requested a review from wenshao May 28, 2026 14:03

wenshao reviewed May 28, 2026

View reviewed changes

Comment thread packages/core/src/telemetry/session-tracing.ts

Comment thread packages/acp-bridge/src/bridge.ts

Comment thread packages/core/src/telemetry/daemon-tracing.ts

Comment thread packages/core/src/telemetry/daemon-tracing.ts

wenshao requested changes May 28, 2026

View reviewed changes

doudouOUC requested a review from wenshao May 29, 2026 04:09

wenshao approved these changes May 29, 2026

View reviewed changes

chiga0 approved these changes May 29, 2026

View reviewed changes

doudouOUC merged commit 99017f6 into QwenLM:daemon_mode_b_main May 29, 2026
45 checks passed

doudouOUC deleted the feat/daemon-otel-e2e-design branch May 29, 2026 06:03

This was referenced May 31, 2026

feat(telemetry): harden OpenTelemetry configuration, HTTP OTLP behavior, and runtime safety #3731

Open

feat(daemon): merge daemon-mode feature batch into main #4490

Merged

Conversation

doudouOUC commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validation

Scope / Risk

Testing Matrix

Linked Issues / Bugs

Uh oh!

github-actions Bot commented May 26, 2026

📋 Review Summary

🔍 General Feedback

🎯 Specific Feedback

🟡 High

🟢 Medium

🔵 Low

✅ Highlights

🔐 Security Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

doudouOUC commented May 27, 2026

Review fix summary (commit 43071d2)

Fixed (10 items)

Not taking (1 item)

Deferred (6 items — follow-up scope)

Uh oh!

Uh oh!

Uh oh!

wenshao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

doudouOUC commented May 27, 2026

Fix: TraceState constructor crash (commit 5b39d83)

Uh oh!

doudouOUC commented May 27, 2026

Fix round 3 (commit 79aff11)

Fixed (3)

Deferred (3)

Uh oh!

Uh oh!

doudouOUC commented May 27, 2026

Fix round 4 (commit 28a4748)

Fixed (5)

Deferred (6)

Not taking (3)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

doudouOUC commented May 26, 2026 •

edited

Loading

Review fix summary (commit `43071d2`)

Fix: TraceState constructor crash (commit `5b39d83`)

Fix round 3 (commit `79aff11`)

Fix round 4 (commit `28a4748`)

Fix round 5 (commit `646226b`)