agentHost: attribute turn preparation and provider send, with MCP startup context - #336847
Vijay Upadya (vijayupadya) wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Disabled-server timing, missing integration coverage, and the unimplemented info-level projection log must be addressed.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Adds telemetry to measure Copilot provider send latency and correlate it with MCP startup readiness.
Changes:
- Introduces MCP readiness tracking and tests.
- Emits
agentHost.providerSendBlockedtelemetry around provider sends. - Adjusts an existing telemetry assertion.
| File | Description |
|---|---|
copilotMcpReadiness.test.ts |
Tests readiness timing and counts. |
copilotAgentSession.test.ts |
Filters the new telemetry event from an existing assertion. |
copilotMcpReadiness.ts |
Tracks MCP startup states and timing. |
copilotAgentSession.ts |
Collects and emits send-blocking telemetry. |
agentHostTelemetryReporter.ts |
Defines and reports the new event. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address PR feedback: - disabled/not_configured servers no longer receive settle timestamps, so a session of only stopped servers reports no startup window instead of 0ms, and a stopped server observed first no longer anchors the window early. - Add mcpStoppedCount so the outcome buckets account for every observed server. - Add CopilotAgentSession tests covering providerSendBlocked wiring: the first/subsequent send flag, the failure flag, and the MCP snapshot fed through the real status-changed subscription. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…P per server Address PR review: - Preparation awaits an MCP inventory refresh that can wait on live server discovery, so a stall there was invisible or misattributed. Time it as its own prepareBlockedMs phase and keep sendBlockedMs to the provider call. - Measure each MCP server's own startup interval and report the longest, so idle time between an early server settling and a later one starting is no longer counted, and a server first seen already settled contributes none. - Derive provider and session from the owning session URI, so peer chats report copilotcli rather than the ahp-chat persistence scheme. - Add turnId so the phases join to turn and first-response timings. - Add tests for phase attribution, per-server timing, and the wiring. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks — this is a substantive review and finding 1 is a real error on my part. All four are addressed in ed56e0d. 1. Timer started after an MCP readiness barrier — confirmed, fixedYou are right, and I verified the chain: The attribution in the PR description was not established by the capture, and I have corrected it. The 29.67s I quoted was measured between two log lines — Fixed by timing preparation as its own phase: Added 2. Stopped servers recorded as completed startup attemptsAlready fixed in 772c62b, before this review landed — 3. Readiness duration includes idle time across later additions — fixedValid, and not covered by the fix above. Reproduced your scenario exactly. Changed from a session-wide window to per-server intervals: each server is timed from the observation showing it starting to the one showing it settled, and the reported figure is the longest of those. Your case now reports 1000ms instead of 601,000ms. This also makes the field mean what its name says, and it gives the same answer for the original capture, where the servers did start together. A related case fell out of the same change: a server first seen already 4. Peer chats report the wrong provider — fixedSwitched to Other corrections
Full |
Close the two gaps flagged in review: - resume() runs the same _prepareSdkTurn but emitted no telemetry, so a resume continuation could pay the MCP inventory cost invisibly. It now reports on the same event, tagged sendKind=resume. - A failure during preparation threw before the instrumented block and emitted nothing. Both phases are now inside one try, and the new outcome field records which phase failed, with cancellation distinguished from other failures. Replaces the sendFailed boolean with outcome, and moves the reporter to a report object rather than nine positional arguments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Local validation with 7 configured MCP servers shows the reconcile is the dominant cost of turn preparation, and that the provider send is not the bottleneck at all: prepare.applyMode=5ms prepare.syncPermissionMode=4ms prepare.applySandboxConfig=44ms prepare.syncShellInitScript=0ms prepare.reconcileMcpServerEnablement=57228ms message prepare=57281ms send=8ms outcome=success Add prepareMcpReconcileMs so that step is attributable on its own instead of being hidden inside the preparation total. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Screenshot ChangesBase: Changed (2)1 insignificant change(s) omitted (≤20 px, Δ≤2). See CI logs for details. |


Splits the two phases that run between a user's message and provider execution — turn preparation and the provider send call — and reports each with the MCP server startup context it overlaps.
Why
Investigating reports that the first turn in a new Agent Host session feels slow, a debug-log capture with MCP servers enabled showed ~38s from Enter to first visible text, of which ~30s was spent between
sendMessagebeing called and the provider send returning, ending 6ms after the last MCP server settled:This was not model latency (turns took 2.4s and 2.6s once dispatched) and not prompt bloat (23,796 input tokens with 7 MCP servers vs 23,262 with none).
That 29.67s cannot be attributed further from the capture, which is the point of this change. Turn preparation awaits an MCP inventory refresh (
_prepareSdkTurn→_reconcileMcpServerEnablement→rpc.mcp.list()) that can itself wait on live server discovery, and the provider send may also block. No log line marks the boundary, so the capture cannot say which phase held the time.Host turn telemetry does not close the gap either:
turnStartedfires before provider execution, soagentHost.turnCompleted.totalTimecontains this time but has no field attributing it to either phase. The problem is attribution, not absence.The blocking behaviour itself is in the Copilot CLI/SDK and is tracked separately in microsoft/vscode-internalbacklog#9483. This PR does not attempt to fix it; it makes the phases separable so the next capture can say where the time goes.
What
agentHost.providerSendBlocked(new):prepareBlockedMs,sendBlockedMs,turnId,isFirstSendOfSession,sendFailed, plus MCP context (mcpServerCount,mcpReadyCount,mcpFailedCount,mcpUnresolvedCount,mcpStoppedCount,slowestMcpServerMs).CopilotMcpReadinessTracker(new): observation-only helper that times each MCP server's own startup interval and reports the longest. Servers start in parallel, so the longest is what gates readiness — and measuring per server rather than across the session means idle time between an early server settling and a later one starting is excluded. Servers that never start (disabled/not_configured) are counted in the inventory but contribute no duration, and a server first seen already settled contributes none either, since its startup was not observed. Only forward progress is recorded, so repeated inventory snapshots cannot inflate the measurement. Clock is injectable for tests.Notes
_prepareSdkTurn→_reconcileMcpServerEnablement→rpc.mcp.list()). Folding them together would attribute a preparation stall to the provider call, or hide it._send. It deliberately does not cover steering messages injected into an already-active turn (copilotAgentSession.ts:3502), orresume()continuations (which userpc.sendMessagesand whose preparation is likewise untimed) — neither is a session-startup path. Sessions with no MCP servers are included: they are the control group that makes the MCP correlation interpretable, so the event is not MCP-gated.finallyaround the send, so a send that throws is still reported, withsendFailed: true. Two limits worth knowing: cancellation is not distinguished from other failures, and a failure during preparation throws before the instrumented block and emits nothing at all. The reporting is itself guarded by atry/catchso it can never mask the provider error being rethrown.providerandagentSessionIdderive from the owning session URI rather than the chat's persistence URI, so peer chats reportcopilotclirather thanahp-chat.Validation
npm run typecheck-clientclean.CopilotAgentSessiontests covering the wiring: phase attribution under a gated preparation await, the first/subsequent send flag, the failure path, and the MCP snapshot fed through the realsession.mcp_server_status_changedsubscription.copilotAgentSession.test.tssuite passes (502 tests).scripts/test.batcould not be run in this environment — it fails on a pre-existing@parcel/watchermodule-resolution error innodejsWatcher.test, reproducible with these changes stashed. Tests above were run directly via mocha.