Skip to content

Send GitHub telemetry forwarding opt-in on the connect handshake#1902

Open
stephentoub wants to merge 2 commits into
mainfrom
stephentoub-fix-sdk-test-failures
Open

Send GitHub telemetry forwarding opt-in on the connect handshake#1902
stephentoub wants to merge 2 commits into
mainfrom
stephentoub-fix-sdk-test-failures

Conversation

@stephentoub

Copy link
Copy Markdown
Collaborator

Why

The runtime moved the enableGitHubTelemetryForwarding opt-in from session.create to the connection-level connect handshake, so it can forward the first session's un-replayable session.start event. The SDKs still only sent the flag on session.create/session.resume, so against a runtime built from that change nothing opted the connection in and GitHub telemetry forwarding never fired. This surfaced as the telemetry-forwarding E2E test timing out in copilot-agent-runtime CI (which builds the runtime from HEAD), across the legs for all six SDKs.

This is not a runtime bug; it is an intentional API refinement. The fix belongs in the SDKs.

What

Dual-send the flag across all six SDKs (Node, C#, Go, Python, Rust, Java): send enableGitHubTelemetryForwarding: true on the connect handshake when a GitHub telemetry handler is registered, in addition to keeping the existing session.create/session.resume send.

This is backward and forward compatible:

  • New runtimes read the flag on connect and ignore the (now redundant) session.create copy.
  • Older CLIs (the currently published 1.0.69-0) still read it on session.create and ignore the unknown connect field.

Each SDK also gets unit tests asserting the connect request carries the flag when a handler is registered and omits it otherwise.

Notes for reviewers

  • The wire field must serialize as exactly enableGitHubTelemetryForwarding (capital "H" in "Hub"). Rust's rename_all = "camelCase" would mangle it to a lowercase h, so that field uses an explicit #[serde(rename = ...)].
  • Rust and Java build the connect params inline rather than editing generated code, per their respective "do not hand-edit generated types" boundaries. The other four hand-apply the field to the generated ConnectRequest type, since the post-change CLI schema is not published yet and regeneration would not add it.

Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com

The runtime moved the `enableGitHubTelemetryForwarding` opt-in from
`session.create` to the connection-level `connect` handshake, so it can
forward the first session's un-replayable `session.start` event. SDKs
only sent the flag on session.create/resume, so against a post-move
runtime nothing opted the connection in and GitHub telemetry forwarding
timed out.

Dual-send the flag across all six SDKs: send it on `connect` (when a
GitHub telemetry handler is registered) in addition to the existing
session.create/resume send. This is backward and forward compatible;
unknown fields are ignored by both old and new runtimes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 3, 2026 15:44
@stephentoub stephentoub requested a review from a team as a code owner July 3, 2026 15:44

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.

Pull request overview

This PR updates all six SDKs (Node, .NET, Go, Python, Rust, Java) to opt into GitHub telemetry forwarding at the connection-level connect handshake (in addition to the existing session.create/session.resume send), matching the runtime’s moved behavior so the first session’s session.start can be forwarded.

Changes:

  • Add enableGitHubTelemetryForwarding: true to the connect request when a GitHub telemetry handler/callback is registered (while keeping the session-level send for backward compatibility).
  • Update generated/hand-built connect request types/params as needed per language (including Rust serde(rename=...) to preserve exact casing).
  • Add/extend unit tests in each SDK to assert the connect handshake includes (or omits) the flag based on handler registration.
Show a summary per file
File Description
rust/tests/session_test.rs Adds tests asserting connect includes/omits enableGitHubTelemetryForwarding.
rust/src/lib.rs Builds connect params inline to include the telemetry opt-in without editing generated types.
python/test_client.py Adds connect-flag tests (but currently appears to merge in the prior telemetry-routing test body).
python/copilot/generated/rpc.py Extends _ConnectRequest to serialize enableGitHubTelemetryForwarding.
python/copilot/client.py Sends the connect-level opt-in when on_github_telemetry is set.
nodejs/test/client.test.ts Adds tests for connect-level opt-in behavior.
nodejs/src/generated/rpc.ts Extends ConnectRequest with enableGitHubTelemetryForwarding?: boolean.
nodejs/src/client.ts Sends the connect-level opt-in when onGitHubTelemetry is set.
java/src/test/java/com/github/copilot/GitHubTelemetryTest.java Captures and asserts connect params include/omit the flag.
java/src/main/java/com/github/copilot/CopilotClient.java Sends the connect-level opt-in by building connect params inline.
go/rpc/zrpc.go Extends ConnectRequest with EnableGitHubTelemetryForwarding.
go/client.go Sends the connect-level opt-in when OnGitHubTelemetry is set.
go/client_test.go Adds tests asserting connect carries/omits the flag.
dotnet/test/Unit/GitHubTelemetryTests.cs Adds connect-level opt-in tests (one assertion can be tightened to require omission).
dotnet/src/Generated/Rpc.cs Extends ConnectRequest with EnableGitHubTelemetryForwarding.
dotnet/src/Client.cs Sends the connect-level opt-in when OnGitHubTelemetry is set.

Review details

Files not reviewed (1)
  • go/rpc/zrpc.go: Generated file
  • Files reviewed: 12/16 changed files
  • Comments generated: 2
  • Review effort level: Low

Comment thread python/test_client.py
Comment thread dotnet/test/Unit/GitHubTelemetryTests.cs
@github-actions

This comment has been minimized.

- python/test_client.py: restore test_event_routes_to_handler as its own
  test method; the connect-omit test had accidentally absorbed the
  telemetry dispatch body, so keep it limited to the connect assertion.
- dotnet/test/Unit/GitHubTelemetryTests.cs: tighten
  Connect_Does_Not_Opt_In_Without_Handler to require the flag be absent
  or null, so it fails if `false` is ever sent (matches the other SDKs).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR updates all six SDK implementations (Node.js, Python, Go, .NET, Java, Rust) consistently. No cross-SDK inconsistencies found.

What was reviewed

Feature parity: All 6 SDKs now send enableGitHubTelemetryForwarding: true on the connect handshake when a GitHub telemetry handler is registered, and omit the field otherwise. The behavior is identical across every language implementation.

Wire format: The serialized field name is enableGitHubTelemetryForwarding (capital "H") in all SDKs. Rust correctly uses #[serde(rename = "enableGitHubTelemetryForwarding")] to override the rename_all = "camelCase" rule that would otherwise produce enableGithubTelemetryForwarding.

Test coverage: Every SDK adds two new tests — one asserting the flag is present when a handler is registered, one asserting the flag is absent when no handler is registered.

Generated type changes: Node.js, Python, Go, and .NET add the new field to their generated ConnectRequest types. Java and Rust build the params inline — consistent with their "do not hand-edit generated types" boundaries, as noted in the PR description.

Minor noted differences (not issues)

  • .NET serializes null rather than omitting the property entirely when no handler is registered (consistent with how Token = null already behaved on the same type). The new test correctly accepts both !present || null to account for this.
  • Rust test uses .is_none_or(Value::is_null) as a symmetric defensive check, same pattern.
  • Java fake server returns protocolVersion: 2 (pre-existing, not introduced here).

These are all expected idiomatic differences, not inconsistencies.

Generated by SDK Consistency Review Agent for issue #1902 · sonnet46 1.3M ·

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