Skip to content

Fix .NET GitHub telemetry forwarding#1910

Closed
roji wants to merge 2 commits into
mainfrom
roji-fix-sdk-csharp-ci
Closed

Fix .NET GitHub telemetry forwarding#1910
roji wants to merge 2 commits into
mainfrom
roji-fix-sdk-csharp-ci

Conversation

@roji

@roji roji commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Send enableGitHubTelemetryForwarding during the connect handshake, which is where the runtime now negotiates forwarding.
  • Harden the .NET GitHub telemetry E2E to drive a real replayed turn and wait for the notification belonging to the live session instead of peeking the first queued event.
  • Add unit coverage that asserts the forwarding opt-in is present on connect, and add the replay snapshot used by the E2E.

Validation

  • DOTNET_ROLL_FORWARD=Major dotnet test dotnet/test/GitHub.Copilot.SDK.Test.csproj -f net8.0 --filter "FullyQualifiedName~GitHubTelemetryTests|FullyQualifiedName~GitHubTelemetryForwardingE2ETests|FullyQualifiedName~RpcSessionStateExtrasE2ETests"
    • Passed locally (13/13).

Update E2E tests for nullable telemetry session IDs and the generated permissions.setAllowAll signature.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 5, 2026 09:45
@roji roji requested a review from a team as a code owner July 5, 2026 09:45
@roji roji changed the title Fix dotnet E2E test build Stability flaky dotnet E2E test Jul 5, 2026
@github-actions

This comment has been minimized.

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

Updates .NET E2E tests to align with recently generated RPC and telemetry types so the test project compiles and assertions match updated nullability.

Changes:

  • Update SetAllowAllAsync calls to use the named enabled: argument (to avoid binding issues after an optional mode parameter was added).
  • Update GitHub telemetry E2E assertion to be compatible with a nullable SessionId property type.
Show a summary per file
File Description
dotnet/test/E2E/RpcSessionStateExtrasE2ETests.cs Uses the named enabled: argument for SetAllowAllAsync to match the updated generated RPC signature.
dotnet/test/E2E/GitHubTelemetryForwardingE2ETests.cs Adjusts the assertion for notification.SessionId to avoid nullable-type assertion issues.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment on lines +42 to +46
@@ -43,7 +43,7 @@ await TestHelper.WaitForConditionAsync(
timeoutMessage: "Timed out waiting for GitHub telemetry notification.");

Assert.True(notifications.TryPeek(out var notification));
Assert.NotEmpty(notification.SessionId);
Assert.False(string.IsNullOrEmpty(notification.SessionId));
Send telemetry forwarding opt-in during the connect handshake, harden the .NET telemetry E2E to wait for the live session's notification, and add unit coverage for the connect request.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@roji roji changed the title Stability flaky dotnet E2E test Fix .NET GitHub telemetry forwarding Jul 5, 2026
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency: enableGitHubTelemetryForwarding in connect handshake

This PR fixes the .NET SDK to send enableGitHubTelemetryForwarding in the connect handshake (where the runtime now negotiates telemetry forwarding). However, the other five SDK implementations still only send this flag in session.create and session.resume — they don't include it in the connect call.

Interestingly, the generated protocol types already have this field on ConnectRequest in all SDKs, so the fix in each one is a small, targeted addition.

Current state

SDK In connect In session.create In session.resume
.NET (this PR)
Node.js
Python
Go
Java
Rust

Where to apply the equivalent fix

  • Node.jsnodejs/src/client.ts — where the connect request is sent (currently only includes token); add enableGitHubTelemetryForwarding: true when this.onGitHubTelemetry != null. The generated ConnectRequest type in nodejs/src/generated/rpc.ts already has the field.

  • Pythonpython/copilot/client.py, line 3307 — _ConnectRequest(token=self._effective_connection_token) → add enable_git_hub_telemetry_forwarding=True when self._on_github_telemetry is not None. The generated _ConnectRequest dataclass in python/copilot/generated/rpc.py already has the field.

  • Gogo/client.go — in connectToServer, the rpc.ConnectRequest is built with only Token; add EnableGitHubTelemetryForwarding: Bool(true) when c.options.OnGitHubTelemetry != nil. The generated ConnectRequest struct in go/rpc/zrpc.go already has the field.

  • Javajava/src/main/java/com/github/copilot/CopilotClient.java, verifyProtocolVersion method — new ConnectParams(effectiveConnectionToken) needs a field for enableGitHubTelemetryForwarding. The generated ConnectRequest class in java/src/generated/java/ already has the field (check getEnableGitHubTelemetryForwarding() / setEnableGitHubTelemetryForwarding()).

  • Rustrust/src/lib.rs, connect_handshake method, line 1823 — ConnectRequest { token: ... } → add enable_github_telemetry_forwarding: self.inner.on_github_telemetry.is_some().then_some(true). The generated ConnectRequest struct in rust/src/generated/api_types.rs already has the field.

Since the PR description states that the runtime now negotiates forwarding at the connect level, leaving the other SDKs unchanged would silently break GitHub telemetry forwarding for those SDK users after a runtime update.

Generated by SDK Consistency Review Agent for issue #1910 · sonnet46 1.9M ·

@roji roji closed this Jul 5, 2026
@stephentoub stephentoub deleted the roji-fix-sdk-csharp-ci branch July 6, 2026 13:41
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