Add StartupTimings per-phase breakdown to Client::start - #2066
Merged
Conversation
Introduce a `StartupTimings` struct that decomposes the CLI spawn + handshake cost into per-phase millisecond fields, so hosts can attribute "time to first token" startup latency to a specific phase instead of reconstructing it from scattered debug lines. Phases captured: - program_resolve_ms: NEW timer around bundled-CLI resolution/extraction (`resolve::copilot_binary_with_extract_dir`), previously untimed and the prime suspect for cold-start cost on Windows. - process_spawn_ms: subprocess `command.spawn()`. - port_wait_ms: TCP port-announcement wait (tcp transport only). - handshake_ms: `verify_protocol_version` connect round-trip. - session_fs_ms / llm_handler_ms: post-handshake provider-registration RPCs. - total_ms: full Client::start wall-clock. Surfacing is non-breaking: timings are stored on ClientInner via a `OnceLock` and exposed through a new `Client::startup_timings()` getter, plus a single structured `debug!` event. `Client::start`'s return type is unchanged. Existing per-phase debug logs are retained. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95834bd3-b7ac-40bc-8bf4-60102600d41a
Contributor
There was a problem hiding this comment.
Pull request overview
Adds per-phase Rust SDK startup timing instrumentation for TTFT analysis.
Changes:
- Introduces the public
StartupTimingstype. - Measures startup phases and exposes them through
Client::startup_timings(). - Emits a consolidated tracing event.
Show a summary per file
| File | Description |
|---|---|
rust/src/startup_timings.rs |
Defines timing fields and duration conversion tests. |
rust/src/lib.rs |
Captures, stores, logs, and exposes startup timings. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Medium
Emit numeric structured fields, cover all transport setup, tighten always-present timing values, and exercise startup timings in Rust E2E tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
rust/src/startup_timings.rs:55
- The PR description documents every timing field as
Option<u64>and omitstransport_setup_ms, while the public API makes this field (along withhandshake_msandtotal_ms) a requiredu64. Please align the PR's advertised API shape with the implementation so the downstream host integration is not planned against the wrong contract.
/// Total transport setup time. This includes spawning and connecting to a
/// subprocess, connecting to an external server, or starting the in-process
/// FFI runtime. `process_spawn_ms` and `port_wait_ms` provide nested detail
/// for spawned transports.
pub transport_setup_ms: u64,
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Medium
Record each optional timing as a numeric span field when present and the string None otherwise, without companion presence flags. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
rust/src/lib.rs:1403
- The timing values are attached to a span, while this
debug!event contains only its message. Event-oriented tracing layers/exporters therefore do not receive the promised structured timing fields without separately collecting and joining span callbacks, contrary to the PR's single-event goal. Emit the fields on the event itself (or change the documented logging contract).
timings_span.in_scope(|| debug!("Client::start timings"));
rust/src/startup_timings.rs:39
- The public shape differs from the stated contract: the PR description says the seven timing fields are all
Option<u64>and does not listtransport_setup_ms, while this struct adds that field and makestransport_setup_ms,handshake_ms, andtotal_msplainu64. Since the follow-up host is expected to consume this API, align the implementation and PR contract before merging.
pub struct StartupTimings {
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Medium
SteveSandersonMS
approved these changes
Jul 28, 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
Adds a
StartupTimingsstruct to the Rust SDK that decomposes the CLI spawn + handshake cost ofClient::startinto per-phase millisecond fields. This is Phase 1 of a cross-repo effort to reduce and instrument time to first token (TTFT): the github-app host currently only measures the totalClient::start(config).awaitduration and can't attribute cold-start latency to a specific phase.What's included
New module
rust/src/startup_timings.rswith a#[non_exhaustive]publicStartupTimingsstruct:program_resolve_msOption<u64>resolve::copilot_binary_with_extract_dir);Nonefor an explicit CLI pathprocess_spawn_msOption<u64>command.spawn();Nonefor external and in-process transportsport_wait_msOption<u64>transport_setup_msu64handshake_msu64verify_protocol_versionconnect round-tripsession_fs_msOption<u64>sessionFs.setProviderRPC when configuredllm_handler_msOption<u64>llmInference.setProviderRPC when configuredtotal_msu64Client::startwall-clock timeRequired fields are always populated on a successful
Client::start. Optional fields are present only when their phase runs.Surfacing (non-breaking)
ClientInnervia aOnceLockand exposed throughClient::startup_timings() -> Option<StartupTimings>."None"otherwise.Client::start's return type is unchanged. Existing per-phase debug logs are retained.Validation
from_streamsbehavior.Scope
SDK crate only. The github-app host will consume these after a vendored SDK bump (separate follow-up); this PR does not touch github-app.