Skip to content

Add reverse RPC and hook callback timing traces - #2439

Open
aurokin wants to merge 7 commits into
mainfrom
aurokin-reverse-rpc-timing
Open

Add reverse RPC and hook callback timing traces#2439
aurokin wants to merge 7 commits into
mainfrom
aurokin-reverse-rpc-timing

Conversation

@aurokin

@aurokin aurokin commented Aug 28, 2026

Copy link
Copy Markdown

Summary

Add opt-in, measurement-only timing for inbound reverse JSON-RPC requests and SessionHooks callbacks under the github_copilot_sdk::reverse_rpc_timing tracing target.

The trace separates request forwarding and scheduling, hook callback execution, response encoding, writer queue delay, write_all, flush, and final completion. Each phase includes a request-relative start offset so consumers can reconstruct the timeline.

Safety and behavior

  • Keeps request processing, callback, serialization, writer queue, flushing, timeout, and error behavior unchanged.
  • Creates no timing queues, registry, counter, keyed hasher, or timing task unless DEBUG is enabled for the target before client construction.
  • Uses bounded, nonblocking timing queues. Saturation reports records_dropped rather than delaying RPC work.
  • Carries the exact inbound request generation through routing, including reused numeric request IDs.
  • Uses per-client keyed, generation-specific correlation values.
  • Emits only fixed RPC method and hook type labels. It does not trace prompts, hook payloads or results, tool arguments, commands, paths, environment values, raw JSON, session IDs, or secrets.

Validation

  • cargo +nightly-2026-04-14 fmt --check
  • cargo clippy --all-features --all-targets -- -D warnings
  • cargo test --all-features --lib with 256 tests passing
  • RUSTFLAGS='-D warnings' cargo check --all-features
  • github-app combined E2E backend build against the earlier integration commit completed successfully
  • Final independent branch review found no significant issues

Integration

Consumers must install a tracing subscriber with github_copilot_sdk::reverse_rpc_timing=debug before constructing the SDK client. Enabling the directive later does not add timing infrastructure to an existing client.

Hunter Sadler and others added 5 commits August 28, 2026 12:08
Measure inbound scheduling, hook callback execution, response encoding, writer queue delay, write_all, and flush without exposing callback content or changing RPC behavior.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Capture the forwarding boundary explicitly and key request-derived correlation hashes per client to prevent cross-run linkability or offline session ID guessing.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Make reverse-RPC diagnostics opt-in at the tracing target, preserve reconstructible phase placement, and bound timing delivery without affecting response behavior.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Avoid constructing timing infrastructure when collection is disabled, preserve consumer warning-clean builds, and count saturated terminal records once.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bind timing to exact forwarded request generations, preserve forwarding order, and restrict trace labels to privacy-safe values.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings August 28, 2026 22:39
@aurokin
aurokin requested a review from a team as a code owner August 28, 2026 22:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

Review tier: Balanced
Findings: 1 High severity · 1 Medium severity · 1 Low severity

New issues introduced by this change (3)
Severity Finding
Low severity rust/​src/​jsonrpc.rs — Timing remains active for every outbound JSON-RPC frame even when reverse-RPC tracing was disabled…
High severity rust/​src/​session.rs — This drain runs before the receiver is closed, while its router sender remains registered until the…
Medium severity rust/​src/​jsonrpc.rs — The timing consumer still runs on a Tokio worker, and each debug! call invokes the subscriber…
What changed in this PR

Adds opt-in reverse JSON-RPC and hook callback timing traces to the Rust SDK.

Changes:

  • Correlates inbound requests through routing, dispatch, and response writing.
  • Emits bounded timing records for request, hook, encoding, queue, write, flush, and completion phases.
  • Adds extensive timing, saturation, cancellation, and redaction tests.
File Description
rust/​src/​jsonrpc.rs Implements timing infrastructure, correlation, emission, and tests.
rust/​src/​session.rs Propagates traces through session request dispatch.
rust/​src/​router.rs Routes trace-carrying reverse requests.
rust/​src/​lib.rs Enables timing-aware client construction and responses.
rust/​src/​hooks.rs Measures hook callback execution.
rust/​src/​github_token.rs Preserves traces through token dispatch.
rust/​src/​copilot_request_handler.rs Preserves traces through inference dispatch.
rust/​Cargo.toml Adds timing-test dependencies and features.
rust/​Cargo.lock Locks added tracing dependencies.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rust/src/jsonrpc.rs Outdated
Comment thread rust/src/session.rs
Comment thread rust/src/jsonrpc.rs Outdated
Hunter Sadler and others added 2 commits August 28, 2026 17:25
Remove disabled-path clock reads, close request receivers before shutdown draining, and isolate timing subscribers on the opt-in timing thread.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Ensure timing startup fails closed, synchronize terminal ownership through force-close, and let the dedicated timing thread drain and exit even when handlers retain trace senders.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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.

2 participants