Repository navigation
Adopt to new vscode log output channel API #1116
Description
Activity
- added this to the milestone
on Dec 9, 2022 - Reacted by Sandeep Somavarapu
I recalled all the discussion we had and I looked into making this backwards compatible with the settings I introduced. However this is quite some work for no real benefit. So I decided to make this a breaking change to align with VS Code's behavior and ship it in 3.18.
- modified the milestones: ,
on Sep 13, 2023 Dirk Bäumer (@dbaeumer) following up on this, are there any major issues pending or anything I can contribute (tests, etc.) to move this along? For the
PowerShellextension we really would like our LSP server messages to have severity filtering available.Alternatively, have you considered implementing something like the DebugAdapterTrackerFactory in vscode?
https://vscode-api.js.org/interfaces/vscode.DebugAdapterTrackerFactory.htmlAs an author, I find this the most useful as I can control how messages from the client are logged.
You can do that in LSP as well. It provides a middleware were you can intercept each message in a specific or generic way.
Reacted by Justin GroteDirk Bäumer (@dbaeumer) there's the info/warn/etc. methods on the LanguageClient though, and I'm concerned if I go the middleware route I'll miss any logs that someone may have sent to the languageclient directly using these methods.
2 remaining items
It will not be a breaking change in terms of API it is more a behavioral breaking change. Currently every extension can control the log via a setting. When I move to the log channel that is not possible anymore. The only way will then be to manage this via the normal VS Code interface.
Dirk Bäumer (@dbaeumer) In the case of the C# extension we're creating our own output window and recently moved it over to the logging form, so we already took the user experience break in that case. In your old PR you had light-up to detect if it was the new kind of output and only use it then -- if you restricted it to that case of externally-provided output windows does that solve the concern?
I think my problem was not only if clients provide their own channel but also I want to solely use the new log channel. That will break. But this is a long time ago and thinks might have changed. I would need to look at the code again and the current VS Code behavior to make a correct statement around this.
What I would see as ideal (and others can chime in):
As "you" are a library we are consuming and not an end user product, you should not be dictating to "us" how we log output from your library. Instead:
- as part of the constructor options, you should provide a optional
logger?option with typevscode.LogOutputChannel | boolean | undefined - If I supply a
LogOutputChannel, you use that as yours. Now I have complete control over how the logs are handled. I can either give you directly aLogOutputChannel, or I can create my own adapter and format/filter/etc. the logs as I see fit (say I want to reduce certain noise, etc.) - If I supply
true, you enable your default implementation and make it easy for me. Both use the same code paths, you're just providing a defaultLogOutputChannelimplementation, either directly viacreateOutputChannelor your own custom formatting. - If logger is not specified (it's set to undefined), then you fall back to the existing behavior so as not to break people who are currently relying/expecting
extension.trace.serverstyle behavior
Ideally all logging is done as structured logging, but this is a nice-to-have.
- as part of the constructor options, you should provide a optional
I'm a little confused here since as best I can tell what Justin Grote (@JustinGrote) is asking for is what Dirk Bäumer (@dbaeumer) more or less already implemented in his branch. LanguageClient's constructor takes a LanguageClientOptions, which already takes a vscode.OutputChannel. You can pass a LogOutputChannel there just fine since LogOutputChannel extends OutputChannel. So extensions (like C#) have already moved to the newer output window type without issue.
The only tricky bit is this line:
8298ef3#diff-1442f78812d67be25f18d3b5d82e8b4191ae2f939a900eac32d4360222c5d845L608-R613
where if an outputChannel isn't already provided one is created; this is opting into making a log window. Just remove that log: true and you don't have a breaking change -- languages can opt-in to the new behavior just by passing a new output channel.
Techatrix (@techatrix) thanks a lot for the PR.
I tried to recall what my problems where when I worked on it last time and the major problem is the UI / setting experience. Currently the LSP client automatically offers a setting
{name}.server.tracewhich allows to control the tracing behavior. When we switch to the new Log Channel we can't keep that setting since we can't programmatically change the log level of a channel. This can only be done by the UI
To avoid any confusion here I actually think that the right way forward would be to break the API and require log channels all together and deprecated the setting. We could add a warning to the channel if a setting value is detected that doesn't match the log channel's severity.
Breaking the API has also the advantage that the
outputChanneland thetraceOutputChannelwould return LogChannel which will avoid that clients need to doischecks on the returned channel to use it as a log channel.Jason Malinowski (@jasonmalinowski) could the C# team live with such a breakage?
Regarding deprecating the setting. What I would do is to use the setting only to control the trace level. Currently we have
off,messages,compactandverbose.offwould disappear since it will be controlled via the UI. The rest of the values we would keep.An alternative might be to remove
compactand mapdebug=>messagesandtrace=>verboseDirk Bäumer (@dbaeumer) We would already be fine -- we've already transitioned to using a log channel and are no longer using a special option. We have always been passing in the log channel we already created, so for us the concerns about 'upgrading' to the new format was fine since we've already taken the user experience break.
Same here for the PowerShell extension.
Created a new PR #1630
Let me know if there are any objections / comments.
New next releases are out:
client@10.0.0-next.15
server@10.0.0-next.13Reacted by Justin Grote, Jason Malinowski and DavidDirk Bäumer (@dbaeumer) thank you!
https://github.com/microsoft/vscode/blob/ed1bc56bd8b6343296ba8310167928e1e955a74e/src/vscode-dts/vscode.proposed.extensionLog.d.ts#L1