Repository navigation
Create per-server daemon telemetry sessions - #85155
Merged
Merged
Conversation
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
dibarbet
force-pushed
the
dev/dibarbet/daemon-telemetry-sessions
branch
from
September 4, 2026 23:04
7d3a453 to
f7dbc47
Compare
dibarbet
marked this pull request as ready for review
September 4, 2026 23:10
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
There are merge-blocking gaps around cancellation handling in ServiceBrokerConnectHandler and a regression in telemetry flush isolation coverage that should be restored.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/BrokeredServices/ServiceBrokerConnectHandler.cs — The broker connection is started via Task.Run with CancellationToken.None, so a cancellation… |
 |
src/Workspaces/CoreTest/Log/RoslynTelemetryTests.cs — RecordingMetricSink.Flush was changed to a no-op, which removes the ability for these tests to… |
What changed in this PR
Introduces per-language-server telemetry isolation when running under the daemon host model, so each server connection can own its own telemetry session and sinks without cross-contamination between concurrently hosted servers.
Changes:
- Partition daemon discovery/pipe naming by effective telemetry level and plumb
--telemetryLevelthrough thin-client argument parsing. - Create/own per-server
RoslynTelemetry+ optionalTelemetrySessionper daemon connection; re-establish telemetry ambient across request dispatch, service creation, broker activations, and async boundaries. - Move remaining language-server request/project-load/Razor bridge telemetry state to per-server ownership and add/adjust unit tests for daemon isolation and session settings.
| File | Description |
|---|---|
| src/Workspaces/CoreTest/Log/RoslynTelemetryTests.cs | Adjusts core telemetry unit tests alongside new per-instance expectations. |
| src/LanguageServer/roslyn-language-server/ThinClientArguments.cs | Parses and carries --telemetryLevel and forwards it to the server args. |
| src/LanguageServer/roslyn-language-server/Program.cs | Passes parsed thin-client arguments into daemon connect flow. |
| src/LanguageServer/roslyn-language-server/DaemonClient.cs | Partitions daemon pipe selection by resolved telemetry level. |
| src/LanguageServer/Protocol/RoslynLanguageServer.cs | Registers RoslynTelemetry as a base service for LSP services. |
| src/LanguageServer/Protocol/LspServices/LspServices.cs | Captures and reapplies per-server telemetry ambient during lazy service creation. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Telemetry/VSCodeRequestTelemetryLogger.cs | Converts request telemetry logger state from static to per-server instance state. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Telemetry/LanguageServerTelemetry.cs | Creates standalone/devkit sessions per server, registers sinks per RoslynTelemetry, and correlates daemon session IDs. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/Program.cs | Resolves telemetry level once, creates the process/default session, and reports features telemetry at shutdown. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/LanguageServerHost.cs | Creates/owns per-server telemetry instance/session in daemon mode and scopes ambient telemetry for server lifetime. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LanguageServer/LanguageServerConnectionManager.cs | Plumbs daemon session ID into per-connection server host creation. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/WorkspaceProjectFactoryService.cs | Uses per-server request telemetry logger instance for project-load start events. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/Razor/TelemetryReporterWrapper.cs | Bridges Razor telemetry via a RoslynTelemetry→TelemetrySession association. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectTelemetry/ProjectLoadTelemetryReporter.cs | Makes project-load correlation ID per-server instead of static/process-wide. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/ProjectInitializationHandler.cs | Uses per-server request telemetry logger instance for initialization-complete events. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs | Captures per-server telemetry and reapplies it for async project reload batches. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileWatching/LspFileChangeWatcher.cs | Captures/reapplies per-server telemetry across watcher disposal/unregistration async boundary. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/FileWatching/DefaultFileChangeWatcher.FileChangeContext.cs | Captures/reapplies per-context telemetry for filesystem watcher callbacks. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/DevKitProjectLoadingServiceContributor.cs | Passes per-server request telemetry logger into brokered project services. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/BrokeredServices/ServiceBrokerConnectHandler.cs | Re-establishes telemetry ambient when connecting broker services under suppressed execution context. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/BrokeredServices/BrokeredServiceBridgeProvider.cs | Documents execution-context capture point for broker activations. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/WorkspaceProjectFactoryServiceTests.cs | Updates test wiring for per-server request telemetry logger instance. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/VSMetricSinkTests.cs | Refactors test helpers (moved to shared utilities file). |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/RecordingTelemetrySinks.cs | Adds reusable recording sinks/poster for telemetry-related unit tests. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/AbstractLanguageServerHostTests.cs | Updates daemon test harness to model process vs per-server telemetry sessions. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/TelemetryReporterTests.cs | Updates reporter/wrapper construction and adds standalone session settings validation. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/ServiceBrokerFactoryTests.cs | Extends coverage to ensure broker calls/telemetry are isolated per server. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/LanguageServerRequestTelemetryTests.cs | Refactors/moves test poster helper usage. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Daemon/LanguageServerDaemonTests.cs | Adds test asserting isolated per-server telemetry sessions correlated to daemon session. |
| src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Daemon/DaemonPipeNameTests.cs | Updates expectations and adds test for telemetry-level partitioning. |
| src/LanguageServer/DaemonConnection/TelemetryLevelResolver.cs | Centralizes “effective telemetry level” resolution (arg vs env). |
| src/LanguageServer/DaemonConnection/DaemonPipeName.cs | Includes telemetry level in daemon pipe-name hashing input. |
Suppressed comments (1)
src/Workspaces/CoreTest/Log/RoslynTelemetryTests.cs:169
- This PR removes the only regression test that verifies RoslynTelemetry.Flush() only flushes sinks registered on the instance being flushed (not other instances). With per-server telemetry instances, preserving this isolation invariant seems important; consider restoring the test.
Partition daemon clients by telemetry consent and isolate server telemetry attribution and lifetime. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Let each LanguageServerHost create and dispose its daemon child session, while Protocol captures only the ambient RoslynTelemetry instance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: beeb17b9-9104-41c1-a3c0-f95dc9df4d39
Capture telemetry from the ambient context during host and service construction, and remove telemetry plumbing from the connection manager and daemon source APIs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: beeb17b9-9104-41c1-a3c0-f95dc9df4d39
Remove the unused standalone telemetry owner and keep the daemon root owner scoped to the daemon task. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: beeb17b9-9104-41c1-a3c0-f95dc9df4d39
StreamJsonRpc dispatches inbound brokered calls on the execution context captured when the connection was created, so they already carry the owning server's telemetry. Verified by a test covering both a brokered service call and Dev Kit's initialization observer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: beeb17b9-9104-41c1-a3c0-f95dc9df4d39
Share telemetry test sinks and remove unrelated formatting and cleanup from the per-server telemetry diff. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c5d4f77e-69f4-4116-aa63-b7c1ab0822cb
dibarbet
force-pushed
the
dev/dibarbet/daemon-telemetry-sessions
branch
from
September 10, 2026 00:00
f7dbc47 to
158508c
Compare
JoeRobich
approved these changes
Sep 12, 2026
Comment on lines
+266
to
+268
| // Dispose runs on whatever context released the last watch (often a project-system callback with | ||
| // no ambient of its own), and ContinueWith captures that context, so re-establish the owning | ||
| // server's instance for the unregistration request. |
Member
There was a problem hiding this comment.
Do we have confidence this is the server instance versus a particular LSP session? I guess it's fine since we're suppressing state in the batch itself, but that seems subtle...
Comment on lines
+266
to
+268
| // Dispose runs on whatever context released the last watch (often a project-system callback with | ||
| // no ambient of its own), and ContinueWith captures that context, so re-establish the owning | ||
| // server's instance for the unregistration request. |
Member
There was a problem hiding this comment.
Why no similar change for subscribing?
Comment on lines
+152
to
+154
| // A batch runs on the context of whichever AddWork caller started it, which may be a file-change | ||
| // notification or other non-request caller that carries no ambient instance of its own. | ||
| using var _ = RoslynTelemetry.SetCurrent(Telemetry); |
Member
There was a problem hiding this comment.
I almost wonder if this sort of capturing of the async state should apply to our work queues generally....
|
|
||
| if (serverConfiguration.IsDaemon) | ||
| { | ||
| // Every daemon server needs an isolated router even when VS telemetry is disabled, so sinks |
Member
There was a problem hiding this comment.
It's not clear what "router" means here -- just name the type?
Comment on lines
+126
to
+127
| try | ||
| { |
Member
There was a problem hiding this comment.
Move this try before the comment, since if any code gets added we want that in there too?
GoldMikeDev
pushed a commit
to GoldMikeDev/FollyOfTheUnbound
that referenced
this pull request
Sep 18, 2026
…er-server telemetry sessions Syncs 33 upstream commits, most notably .NET 11 RC1 SDK, Copilot code removal, and per-server daemon telemetry sessions (dotnet#85155). The telemetry-session work and this fork's own daemon per-connection isolation feature both changed DaemonPipeName.GetPipeName's signature to add a new "thing that should split clients into separate daemons" -- ours added serverArguments (superseding an older single-string parameter), upstream added telemetryLevel. Reconciled by threading both into the pipe key additively rather than picking one side: GetPipeName now takes serverArguments and telemetryLevel together, and every caller (DaemonClient, LanguageServerHost, LanguageServerConnectionManager, ChildServerHost, ServerExecutable) passes both through so clients that differ in either startup arguments or telemetry level get separate daemons/telemetry sessions, matching the guarantee each side built independently. Also merged, less contentiously: - LanguageServerHost: combined this fork's isDaemonConnection/CleanExitSentinel plumbing with upstream's daemonSessionId-scoped telemetry session. - LanguageServerProjectLoader.ReloadProjectsAsync: kept both context restores (ambient connection token and RoslynTelemetry.SetCurrent) -- unrelated to each other, both needed. - ChildServerHost: took upstream's stdout/stderr forwarding fix (BaseStream + actual cancellation token instead of CancellationToken.None) over this fork's version, which never wired up cancellation. - AbstractLanguageServerHostTests.TestDaemon.CreateAsync: kept this fork's WaitForAcceptLoopStartedAsync wait (avoids a real "Pipe is broken" race in tests) while adopting upstream's daemon-telemetry-session setup. - roslyn-language-server.csproj, ServerExecutable.cs: unrelated additive includes/usings from both sides, kept together. - .github/memory/FILE_MAP.md: kept this fork's more detailed Tools/ description. Verified via a build of Microsoft.CodeAnalysis.LanguageServer.UnitTests.csproj (0 errors) and its full test suite (419 passed, 18 skipped, 0 failed), including the daemon pipe-name and daemon-integration tests exercising the merged GetPipeName logic directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EmdEQ9mhGYnkqR3vjAzkhT
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.
Summary
This PR stacks on #85151. It gives each language server hosted by a daemon its own telemetry session.
vs.roslyn.languageserver.daemonsessionidfor correlation.LanguageServerHostcreate and own each per-server telemetry session from the ambient daemon owner. The connection manager remains transport/lifetime coordination only, and the daemon source captures its daemon instance when constructed for explicit lifecycle logging.RoslynLanguageServer; request dispatch and lazy service construction reapply it, while non-request asynchronous boundaries captureRoslynTelemetry.Currentand restore it with nested scopes.Microsoft Reviewers: Open in CodeFlow