fix: add missing TelemetryRuntimeConfig methods and remove obsolete test#4730
Merged
doudouOUC merged 1 commit intoJun 3, 2026
Conversation
- Add isInteractive() and getOutboundCorrelationPropagateTraceContext() to TelemetryRuntimeConfig interface (required by sdk.ts) - Add implementations in createDaemonTelemetryRuntimeConfig - Remove httpAcpBridge.test.ts (tests moved to acp-bridge/bridge.test.ts) These fixes were applied in the initial merge resolution but lost when the merge commit was recreated with proper two-parent structure.
Contributor
📋 Review SummaryThis PR restores critical fixes that were lost during merge conflict resolution in #4490. The changes add two missing methods to the 🔍 General Feedback
🎯 Specific Feedback🔴 CriticalNo critical issues identified. The changes are necessary to fix compilation errors and restore lost functionality. 🟡 HighNo high priority issues identified. 🟢 MediumNo medium priority issues identified. 🔵 Low
✅ Highlights
📝 Verification NotesPer the PR description:
Recommendation: Approve and merge. The changes are minimal, necessary, and low-risk. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores telemetry runtime API compatibility by extending TelemetryRuntimeConfig with two methods used by the telemetry SDK, wiring minimal daemon-side implementations in qwen serve, and removing an obsolete CLI-side ACP bridge test suite that has been relocated.
Changes:
- Extend
TelemetryRuntimeConfigwithisInteractive()andgetOutboundCorrelationPropagateTraceContext(). - Implement the two methods in the daemon telemetry runtime config used by
packages/cli’srunQwenServe. - Delete
packages/cli/src/serve/httpAcpBridge.test.tsas obsolete after the test migration to@qwen-code/acp-bridge.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/telemetry/runtime-config.ts | Adds two required methods to the telemetry runtime config interface. |
| packages/cli/src/serve/runQwenServe.ts | Implements the new runtime-config methods for daemon telemetry initialization. |
| packages/cli/src/serve/httpAcpBridge.test.ts | Removes the legacy CLI-side ACP bridge test suite (now covered elsewhere). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+122
to
+123
| isInteractive: () => false, | ||
| getOutboundCorrelationPropagateTraceContext: () => false, |
ytahdn
approved these changes
Jun 3, 2026
yiliang114
approved these changes
Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isInteractive()andgetOutboundCorrelationPropagateTraceContext()toTelemetryRuntimeConfiginterface (required bysdk.ts)createDaemonTelemetryRuntimeConfiginrunQwenServe.tshttpAcpBridge.test.ts(tests moved toacp-bridge/bridge.test.tsper F1)These fixes were applied during the merge conflict resolution in #4490 but lost when the merge commit was recreated with proper two-parent structure.
Test plan
npm run build)🤖 Generated with Qwen Code