Add LSP trace level support ($/setTrace, $/logTrace)#42
Open
clrudolphi wants to merge 3 commits into
Open
Conversation
The server ignored InitializeParams.Trace entirely and had no support for the standard $/setTrace notification or for issuing $/logTrace notifications — no way for a client to opt into protocol tracing (#41). Add a TraceService that tracks the current InitializeTrace level (seeded from InitializeParams.Trace at the initialize handshake, updatable at runtime via a new SetTraceNotificationHandler for $/setTrace) and exposes Trace(message, verboseMessage) to send $/logTrace notifications — a no-op when trace is Off, and only building the verbose detail string when the level is Verbose. Wire it into OperationDurationRecorder, the existing centralized instrumentation seam for the four interactive performance targets, so enabling trace surfaces the same PERF measurements already written to the log file as $/logTrace notifications in the client's trace output. Broader per-request tracing across every handler is a natural follow-up if wanted, but this keeps the change to a single well-tested seam rather than a large cross-cutting refactor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two follow-ups to the $/setTrace / $/logTrace support: - Server: add a --trace <Off/Messages/Verbose> command-line default, mirroring --log-level. Precedence is cmdline -> InitializeParams.Trace -> $/setTrace, but InitializeTrace.Off is indistinguishable from "the client didn't set this field" so it must not clobber an explicit --trace default — only override when the client actually asked for something (Program.ResolveInitialTrace). - VS Code: TeeLogOutputChannel.logLevel was hardcoded to Trace so vscode-languageclient would always capture full wire traffic for the lsp-viewer inspector log file. But vscode-languageclient also reads that same logLevel to decide what to send as InitializeParams.Trace and later $/setTrace — hardcoding it meant every VS Code session told the server to trace at least "messages", regardless of the `reqnroll.trace.server` setting the server now actually listens to. logLevel now mirrors that setting directly (Off when 'off', Trace otherwise), and fires onDidChangeLogLevel on live setting changes so vscode-languageclient's own config-change handling sends an updated $/setTrace without a window reload. VS extension still doesn't set InitializeParams.Trace or send $/setTrace at all, so the new --trace command-line default is the only lever available there for now (a future PR would need to wire the picker/setting through to the server args or send $/setTrace). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…el from protocol logging Two follow-ups from an audit of the logging/tracing story after $/setTrace and $/logTrace landed: - OmniSharp's own internal diagnostics (request dispatch, DryIoc, JSON-RPC plumbing) only ever reached window/logMessage — visible to the connected client, never persisted anywhere we can look at after the fact. Add ProtocolLoggerProvider, an ILoggerProvider that bridges those Microsoft.Extensions.Logging entries into a dedicated reqnroll-*-protocol-*.log file (separate from the main server log, so two independent writers never append to the same file concurrently). - --log-level was doing double duty: gating both our own file logger AND logging.SetMinimumLevel(...) for the OmniSharp-internal pipeline above. Turning up file logging for our own diagnosis would also flood the client's Output panel with library internals, and there was no way to ask for library-internal detail without also cranking our own app log. Add --protocol-log-level as an independent dial (same Off/Error/Warning/ Info/Verbose scale, same Warning default) purely for the OmniSharp pipeline; --log-level now controls only IDeveroomLogger's own file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
|
Follow-up commit based on an audit of the logging/tracing story once this lands:
20 new tests ( |
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
1 participant
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
Fixes #41. The server previously ignored
InitializeParams.Traceentirely and had no support for the standard$/setTracenotification or for issuing$/logTracenotifications.What's here
ITraceService/TraceService— tracks the currentInitializeTracelevel and sends$/logTracenotifications (a no-op when trace isOff; only builds the verbose detail string when the level isVerbose).SetTraceNotificationHandler— handles$/setTrace, letting the client change the level at runtime.--trace <Off/Messages/Verbose>command-line default, mirroring the existing--log-levelflag.--tracecommand-line default →InitializeParams.Trace→$/setTrace.InitializeTrace.Offis indistinguishable from "the client didn't set this field," so the initialize value only overrides the command-line default when it's non-Off(Program.ResolveInitialTrace);$/setTracecan still set any value, including back toOff, at any time.OperationDurationRecorder(the existing centralized instrumentation seam for the four interactive performance targets) — enabling trace surfaces the same PERF measurements already written to the log file as$/logTracenotifications. Broader per-request tracing across every handler is a natural follow-up if wanted, but this keeps the change to one well-tested seam rather than a large cross-cutting refactor.VS Code fix
While wiring this up, found and fixed an accidental interaction:
TeeLogOutputChannel.logLevelwas hardcoded toTracesovscode-languageclientwould always capture full wire traffic into the lsp-viewer inspector log file. But that samelogLevelis also what the client library reads to decide what to send asInitializeParams.Trace(and later$/setTrace) — so every VS Code session was telling the server to trace at "messages" or higher regardless of thereqnroll.trace.serversetting.logLevelnow mirrors that setting directly, and firesonDidChangeLogLevelon live setting changes so toggling it doesn't need a window reload.VS extension
Not wired up yet — VS's LSP client doesn't set
InitializeParams.Traceor send$/setTracetoday, so the new--tracecommand-line default is the only lever available there for now. Wiring a real VS-side setting through is a separate, larger piece of work.Test plan
dotnet test tests/LSP/Reqnroll.IdeSupport.LSP.Server.Tests— 537 passed (10 new:TraceService,SetTraceNotificationHandler,OperationDurationRecorderwiring,ParseTraceLevel,ResolveInitialTrace)npm test, real VS Code test host) — 20 passed (3 new, coveringcreateTraceChannel's dynamiclogLeveland liveonDidChangeLogLevel)🤖 Generated with Claude Code