Repository navigation
Add standalone C# LSP telemetry - #84874
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Moves ownership of telemetry reporting into the standalone language server so Copilot CLI can route server telemetry through Visual Studio telemetry, while keeping existing C# Dev Kit (VS Code collector) behavior intact and adding COPILOT_TELEMETRY_LEVEL-based consent control for standalone usage.
Changes:
- Introduces
LanguageServerTelemetryReportersupport for both Dev Kit telemetry sessions (VS Code collector key) and standalone sessions (VS default session with opt-in gating). - Adds
COPILOT_TELEMETRY_LEVELhost/consent resolution and threads the resultingIsCopilotClisignal throughServerConfigurationto gate telemetry initialization. - Updates unit + process-host tests to cover the new telemetry configuration matrix, and updates repo docs to reflect server-owned telemetry.
Show a summary per file
| File | Description |
|---|---|
| src/VisualStudio/DevKit/Impl/Microsoft.VisualStudio.LanguageServices.DevKit.csproj | Removes VS telemetry packaging/compile inputs from DevKit now that telemetry is owned by the language server. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Telemetry/LanguageServerTelemetryReporter.cs | Implements dual-mode telemetry session creation (Dev Kit vs VS default session) and opt-in gating for standalone. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/ServerConfigurationFactory.cs | Extends ServerConfiguration with IsCopilotCli to drive telemetry decisions. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs | Gates MEF telemetry reporter creation based on Dev Kit vs Copilot CLI host signals. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Microsoft.CodeAnalysis.LanguageServer.csproj | Brings shared VS telemetry + FaultReporter code into the language server and adds telemetry package reference. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerCommandLine.cs | Updates CLI help text and resolves telemetry host/level from CLI args vs COPILOT_TELEMETRY_LEVEL. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/AbstractLanguageServerHostTests.cs | Threads new IsCopilotCli config through default test server configuration. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/TelemetryReporterTests.cs | Adds tests for VS default session settings, Copilot opt-in behavior, and Dev Kit session settings. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/LanguageServerCommandLineTests.cs | Adds tests validating telemetry configuration precedence (Dev Kit vs Copilot env vs CLI). |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.ProcessHost.UnitTests/Utilities/LspServerLaunchOptions.cs | Adds launch options to model telemetry level selection in process-host tests. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.ProcessHost.UnitTests/Utilities/AbstractLanguageServerClientTests.TestLspClient.cs | Plumbs telemetry options/env var into thin-client process start. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.ProcessHost.UnitTests/Lifecycle/SingleServerLifecycleTests.cs | Validates server starts/stops cleanly across telemetry configuration combinations. |
| .github/memory/FILE_MAP.md | Documents server-owned telemetry as part of the LanguageServer area summary. |
| .github/instructions/IDE.instructions.md | Adds guidance on Language Server telemetry ownership and COPILOT_TELEMETRY_LEVEL semantics. |
Review details
Suppressed comments (1)
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/TelemetryReporterTests.cs:62
- This test also uses
Path.GetTempFileName()just to satisfy VS telemetry'sCommonPropertyBagPathrequirement; the created temp file is never deleted. PreferTempRoot.CreateFile().Path(available from the base class) to ensure cleanup.
public void TestStandaloneSessionUsesVSDefaultCollectorSettings()
{
Environment.SetEnvironmentVariable("CommonPropertyBagPath", Path.GetTempFileName());
var settings = JsonNode.Parse(Microsoft.VisualStudio.Telemetry.TelemetryService.DefaultSession.SerializeSettings())!.AsObject();
- Files reviewed: 14/14 changed files
- Comments generated: 3
- Review effort level: Lite
| // VS Telemetry requires this environment variable to be set. | ||
| Environment.SetEnvironmentVariable("CommonPropertyBagPath", Path.GetTempFileName()); | ||
|
|
There was a problem hiding this comment.
If a file never gets created, then we can ignore this suggestion.
There was a problem hiding this comment.
Review details
Suppressed comments (3)
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Telemetry/LanguageServerTelemetryReporter.cs:175
CreateDevKitSessionSettingsbuilds JSON by iterating aDictionary. The method (andTestDevKitSessionPreservesVSCodeSettings) implicitly depend on a stable property order, butDictionaryenumeration order is not guaranteed by contract and can make the serialized settings string nondeterministic/flaky. Use an ordered collection (e.g.,List<KeyValuePair<...>>) to guarantee output ordering.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/TelemetryReporterTests.cs:35TestVSTelemetryLoadedIntoDefaultAlccreates a telemetry reporter that implementsIDisposable, but it is never disposed. This can leak the underlying telemetry session / environment setup across tests and can cause resource warnings in analyzers.
public void TestVSTelemetryLoadedIntoDefaultAlc()
{
var service = CreateReporter(ServerConfigurationWithoutDevKit);
var assembly = Assembly.GetAssembly(service.GetType());
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerCommandLine.cs:45
- The
--telemetryLeveloption description says it "Defaults to 'off'", but the option currently has noDefaultValueFactory, so the default value isnull(which prevents telemetry initialization entirely). Either remove the default claim or provide an actual default value to match the description.
var telemetryLevelOption = new Option<string?>("--telemetryLevel")
{
Description = "Telemetry level for Dev Kit. Supported values are 'all', 'crash', 'error', or 'off'. Defaults to 'off'.",
Required = false,
};
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/TelemetryReporterTests.cs:25
- CreateReporter sets CommonPropertyBagPath using Path.GetTempFileName(), which creates a temp file per call and never restores the previous environment value. Since environment variables are process-wide, this can leak temp files and introduce cross-test global state. Consider setting the env var once per test instance and restoring/cleaning it in Dispose, and avoid pre-creating a temp file.
private ITelemetryReporter CreateReporter(ServerConfiguration serverConfiguration)
{
// VS Telemetry requires this environment variable to be set.
Environment.SetEnvironmentVariable("CommonPropertyBagPath", Path.GetTempFileName());
var reporter = (ITelemetryReporter?)Activator.CreateInstance(typeof(LanguageServerTelemetryReporter), serverConfiguration, LoggerFactory);
Assert.NotNull(reporter);
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs:136
- telemetryLevel is treated as enabled whenever it's non-null, but Environment.GetEnvironmentVariable can return an empty string. With COPILOT_TELEMETRY_LEVEL="" (or whitespace), the server will still instantiate the telemetry reporter and start the VS default telemetry session (though opted out), which undermines the intent to only initialize standalone telemetry when the host explicitly supplies a consent level. Consider treating empty/whitespace as 'not present'.
var telemetryLevel = LanguageServerTelemetryReporter.GetTelemetryLevel(serverConfiguration);
var telemetryReporter = telemetryLevel is not null
? exportProvider.GetExportedValue<ITelemetryReporter>()
: null;
RoslynLogger.Initialize(telemetryReporter, telemetryLevel, serverConfiguration.SessionId);
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/TelemetryReporterTests.cs:38
- TestVSTelemetryLoadedIntoDefaultAlc creates an ITelemetryReporter but doesn't dispose it. Since the reporter owns/initializes VS telemetry objects, this can leak state into later tests. Use a 'using' declaration like the other tests in this file.
public void TestVSTelemetryLoadedIntoDefaultAlc()
{
var service = CreateReporter(ServerConfigurationWithoutDevKit);
var assembly = Assembly.GetAssembly(service.GetType());
Assert.Contains(AssemblyLoadContext.Default.Assemblies, a => a == assembly);
Assert.Contains(AssemblyLoadContext.Default.Assemblies, a => a.GetName().Name == "Microsoft.VisualStudio.Telemetry");
}
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Telemetry/LanguageServerTelemetryReporter.cs:65
- The "Unsupported Copilot CLI telemetry level" log doesn’t include the invalid value, which makes diagnosis harder when
COPILOT_TELEMETRY_LEVELis misconfigured. Include the provided value in the message.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerCommandLine.cs:43 - The option help text says the telemetry level defaults to 'off', but the option has no default and
TelemetryLevelcan benull(e.g., the default test configuration), which results in telemetry being disabled entirely (no reporter/session). Update the description to avoid misleading users about the default behavior.
Description = "Telemetry level for Dev Kit. Supported values are 'all', 'crash', 'error', or 'off'. Defaults to 'off'.",
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/TelemetryReporterTests.cs:34
LanguageServerTelemetryReporterimplementsIDisposable(viaITelemetryReporter). This test creates an instance without disposing it; use ausingdeclaration for consistency with the other tests and to avoid leaking resources if the constructor/implementation changes in the future.
var service = CreateReporter(ServerConfigurationWithoutDevKit);
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Telemetry/LanguageServerTelemetryReporter.cs:65
- The standalone-telemetry log message doesn’t include the actual value provided or what values are supported, which makes diagnosing misconfiguration harder. Logging the value and the environment variable name improves actionability.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServerCommandLine.cs:45 - The option text says it “Defaults to 'off'”, but the option doesn’t actually set a default value (so it will typically be null when omitted). Since Program.cs treats a null telemetry level as “no telemetry reporter”, this description is misleading. Either set an explicit default (and accept that telemetry assemblies will be loaded even when the user omits the option) or update the description to reflect the current behavior (e.g. “If omitted, telemetry is not initialized”).
var telemetryLevelOption = new Option<string?>("--telemetryLevel")
{
Description = "Telemetry level for Dev Kit. Supported values are 'all', 'crash', 'error', or 'off'. Defaults to 'off'.",
Required = false,
};
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/TelemetryReporterTests.cs:83
- The test asserts exact string equality for the serialized JSON settings. This is brittle because JSON object property order and formatting aren’t part of the contract, and a harmless change (or runtime ordering change) could fail the test. Prefer parsing the JSON and asserting individual fields (including CollectorApiKey and ProcessStartTime) instead of comparing the full string.
var expectedSettings = $$"""
{"Id":"test-session","HostName":"Default","TelemetryLevel":"error","IsInitialSession":true,"CollectorApiKey":"0c6ae279ed8443289764825290e4f9e2-1a736e7c-1324-4338-be46-fc2a58ae4d14-7255","AppId":1010,"ProcessStartTime":{{processStartTime}}}
""";
Assert.Equal(expectedSettings, serializedSettings);
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
* upstream/main: (730 commits) Improve recovery for repeated partial type modifiers (#84934) Add standalone C# LSP telemetry (#84874) Suppress AI artifact audit failure issues (#84925) Remove empty ExternalAccessAspNetCoreResources.resx (#84939) Update helix job monitor version (#84940) Add HasPendingUpdates to HotReloadService.Updates to be used in dotnet-watch (#84891) Add LocalizableBranches parameter for OneLocBuild (#84933) Pin version of Microsoft.CodeAnalysis.Analyzers (#84924) Centralize record and union keyword checks (#84928) Caching compiler: support binary additional texts (#84916) Remove unused AdditionalTextComparer (#84917) Remove Try-Both matching mode for unions. (#84897) Update CodeStyleAnalyzerVersion to 5.9.0 (#84919) Fix/84847 source generated rename conflict (#84849) Return MethodNotFound for unsupported LSP method dispatch (#84892) [main] Update dependencies from dotnet/arcade (#84896) [main] Update dependencies from dotnet/arcade (#84882) Track status for feature "Type Parameter Inference from Constraints" (#84869) [main] Source code updates from dotnet/dotnet (#84879) Stop labeling Loc PRs as community (#84878) ...
Moves telemetry reporting into the language server so Copilot CLI can send server telemetry through the Visual Studio telemetry. Existing C# Dev Kit telemetry behavior remains unchanged, and
COPILOT_TELEMETRY_LEVELcontrols standalone consent.Microsoft Reviewers: Open in CodeFlow