Send GitHub telemetry forwarding opt-in on the connect handshake#1902
Open
stephentoub wants to merge 1 commit into
Open
Send GitHub telemetry forwarding opt-in on the connect handshake#1902stephentoub wants to merge 1 commit into
stephentoub wants to merge 1 commit into
Conversation
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>
Contributor
There was a problem hiding this comment.
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: trueto theconnectrequest 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 on lines
+2402
to
2415
| async def test_connect_omits_forwarding_without_handler(self): | ||
| client = CopilotClient(connection=RuntimeConnection.for_stdio(path=CLI_PATH)) | ||
| captured = {} | ||
|
|
||
| class _FakeClient: | ||
| async def request(self, method, params, **kwargs): | ||
| captured[method] = params | ||
| return {"ok": True, "protocolVersion": 3, "version": "test"} | ||
|
|
||
| client._client = _FakeClient() | ||
| await client._verify_protocol_version() | ||
| assert "enableGitHubTelemetryForwarding" not in captured["connect"] | ||
|
|
||
| from copilot.generated.rpc import GitHubTelemetryNotification |
Comment on lines
+82
to
+86
| var connectParams = server.LastConnectParams ?? throw new InvalidOperationException("connect was not captured."); | ||
| var optedIn = connectParams.TryGetProperty("enableGitHubTelemetryForwarding", out var flag) | ||
| && flag.ValueKind == JsonValueKind.True; | ||
| Assert.False(optedIn); | ||
| } |
Contributor
Cross-SDK Consistency Review ✅All six SDK implementations (Node.js, Python, Go, .NET, Java, Rust) are consistently updated in this PR. Here's what was verified: Feature parity — all SDKs now:
Wire format consistency:The field name serializes as exactly Implementation approach differences (intentional):
These differences are consistent with each language's conventions and are explicitly documented in the PR description. No issues found.
|
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.
Why
The runtime moved the
enableGitHubTelemetryForwardingopt-in fromsession.createto the connection-levelconnecthandshake, so it can forward the first session's un-replayablesession.startevent. The SDKs still only sent the flag onsession.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 incopilot-agent-runtimeCI (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: trueon theconnecthandshake when a GitHub telemetry handler is registered, in addition to keeping the existingsession.create/session.resumesend.This is backward and forward compatible:
connectand ignore the (now redundant)session.createcopy.1.0.69-0) still read it onsession.createand ignore the unknownconnectfield.Each SDK also gets unit tests asserting the
connectrequest carries the flag when a handler is registered and omits it otherwise.Notes for reviewers
enableGitHubTelemetryForwarding(capital "H" in "Hub"). Rust'srename_all = "camelCase"would mangle it to a lowercaseh, so that field uses an explicit#[serde(rename = ...)].connectparams 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 generatedConnectRequesttype, 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