plugin: Support LogOutputChannel #12429

ndoschek · 2023-04-18T07:24:39Z

What it does

Support LogOutputChannel
Support LogLevel
Support in namespace/env: logLevel & onDidChangeLogLevel

Resolves #12017

Contributed on behalf of STMicroelectronics.

Remark:

This PR contributes the basic VS Code API support for LogOutputChannels, to fully support all aspects of a LogOutputViewChannel as in VS Code some additional features will be needed:

In VSCode there is support to set the "Developer log level" in the application which also configures the log level for LogOutputviewChannels. Currently the log level for LogOutputChannels is set to the default log level, i.e. info.
VSCode also writes the logs to a separate file per extension and offers a command and tool in the output view to directly open the extensions's log file.

I suggest to extract those additional features into separate follow-up issues if you agree.

[Edit 2023-04-21]: Extracted to follow-up issue #12440

How to test

To test, I created a simple extension that creates a LogOutputChannel and contributes commands to send log messages in different levels:
[Edit 2023-04-21: updated VSIX test extension] vscode-extension-log-output-channel-12017-0.0.1.zip

Download and install the test extension vscode-extension-log-output-channel-12017-0.0.1.vsix in your Theia test workspace (via Extensions view - top menu bar More actions... - Install from VSIX).
On activation of the extension, the log output channel LogOutputChannel-Test-12017 is shown in the output view and the initial message (LogLevel set to: Info) should be shown.
Execute the commands (Log a <loglevel> message via LogOutputChannel (with args) / Throw an ERROR and log msg/stack via LogOutputChannel / Test replace LogOutputChannel / Append text content (as new line) to LogOutputChannel) via the command palette and check for the messages in the output channel.
Remark: Currently the loglevel is set to default INFO log level.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

kittaakos

It works great 👍 I even verified with an Error after modifying your test VSIX:

2023-04-20T07:52:54.937Z [Error] Error: Logging an ERROR message via LogOutputChannel
    at /Users/a.kitta/dev/git/theia/plugins/vscode-extension-log-output-channel-12017-0.0.1/extension/out/extension.js:40:34
    at /Users/a.kitta/dev/git/theia/packages/plugin-ext/lib/plugin/command-registry.js:61:59
    at CommandRegistryImpl.executeLocalCommand (/Users/a.kitta/dev/git/theia/packages/plugin-ext/lib/plugin/command-registry.js:108:20)
    at CommandRegistryImpl.$executeCommand (/Users/a.kitta/dev/git/theia/packages/plugin-ext/lib/plugin/command-registry.js:70:25)
    at /Users/a.kitta/dev/git/theia/packages/plugin-ext/lib/common/proxy-handler.js:91:71
    at process.processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async RpcProtocol.handleRequest (/Users/a.kitta/dev/git/theia/packages/core/lib/common/message-rpc/rpc-protocol.js:167:28)

Regarding the API, when creating the LogOutputChannel, options must be defined, and log must be true to be in sync with Code API. What do you think?

packages/plugin/src/theia.d.ts

packages/plugin-ext/src/plugin/output-channel/logoutput-channel.ts

packages/plugin-ext/src/plugin/output-channel-registry.ts

kittaakos · 2023-04-20T07:59:42Z

packages/plugin-ext/src/plugin/output-channel/logoutput-channel.ts

+    replace(value: string): void {
+        this.info(value);
+        this.proxy.$append(this.name, value, this.pluginInfo);
+    }


Can you please explain what it does? I could not figure out how to verify it via the test VSIX. Thanks!

Thanks for catching that! It basically should replace the channel's content with the given text value.
With the updated implementation, we reuse the OutputChannelImpl and can reuse the logic, which clears the content first and then appends the given text value.
Also, I updated the test extension, which also offers a command now to call the replace function.

packages/plugin/src/theia.d.ts

packages/plugin-ext/src/common/plugin-api-rpc.ts

tsmaeder

I don't think using the logger implementation from VS Code is a good idea in this case. Feel free to convince me, though :-)

packages/plugin/src/theia-proposed.d.ts

packages/plugin-ext/src/plugin/output-channel-registry.ts

packages/plugin-ext/src/plugin/output-channel/logoutput-channel.ts

packages/plugin-ext/src/plugin/type-converters.ts

ndoschek · 2023-04-20T14:58:50Z

Thank you both for your reviews!
I updated my implementation and also updated the test extension in the PR description (it now offers a few more commands like replace and error throwing)

Regarding the additional log file functionality in VS Code (see my remark in the PR description), is it okay if I add a follow-up for this?

packages/plugin-ext/src/plugin/output-channel-registry.ts

tsmaeder · 2023-04-21T06:45:44Z

packages/plugin-ext/src/plugin/output-channel/log-output-channel.ts

+    readonly onDidChangeLogLevel: theia.Event<theia.LogLevel> = this.onDidChangeLogLevelEmitter.event;
+    public logLevel: theia.LogLevel;
+
+    constructor(override readonly name: string, protected override readonly proxy: OutputChannelRegistryMain, protected override readonly pluginInfo: PluginInfo) {


Since name is public on the superclass, do we need the override readonly?

It is also readonly in the superclass, so I guess this is okay this way

But why would you redeclare a variable that is already present in the superclass and visible just fine in the subclass? It's unnecessary for all three constructor params, IMO.

Yes basically I agree, but as I had to write a custom constructor (to initalize the log level) I have no other choice, as Typescript requires them to be overridden due to the same variable names, see This parameter property must have an 'override' modifier because it overrides a member in base class 'OutputChannelImpl'.ts(4115)

This works for me:

constructor(name: string, proxy: OutputChannelRegistryMain, pluginInfo: PluginInfo) { super(name, proxy, pluginInfo); this.setLogLevel(LogLevel.Info); }

The "readonly" is a property of the implicitly created field, not the constructor parameter.

Thanks I was not aware of that, that works great! I pushed another update for this.

packages/plugin-ext/src/plugin/output-channel/log-output-channel.ts

tsmaeder · 2023-04-21T06:52:20Z

packages/plugin-ext/src/plugin/output-channel/log-output-channel.ts

+    }
+
+    trace(message: string, ...args: any[]): void {
+        if (this.checkLogLevel(LogLevel.Trace)) {


This seems like odd code. Why do we duplicate the level check in each method an not just move it into log(level, ...)?

That's true, thanks for catching that! I changed it to your suggestion.

tsmaeder · 2023-04-21T06:53:46Z

packages/plugin-ext/src/plugin/output-channel/log-output-channel.ts

+    }
+
+    protected log(level: theia.LogLevel, message: string): void {
+        const now = new Date();


I would suggest moving the level checking here instead the methods below.

tsmaeder · 2023-04-21T06:56:14Z

packages/plugin-ext/src/plugin/output-channel/log-output-channel.ts

+    error(message: string | Error, ...args: any[]): void {
+        if (this.checkLogLevel(LogLevel.Error)) {
+            if (message instanceof Error) {
+                const array = Array.prototype.slice.call(arguments) as unknown[];


What's the reason for this processing? And wouldn't arguments.slice() work as well?

I don't really understand what this outputs in the case of an error object. Could you give an example?

Yes I agree, I had another look and it is sufficient to pass the error's stack member instead of this processing. I updated the test extension, it now covers both cases with error stack and only with an error message.

tsmaeder · 2023-04-21T07:05:06Z

packages/plugin-ext/src/plugin/output-channel/log-output-channel.ts

+        }
+    }
+
+    private format(args: any): string {


Isn't this just doing this?

args.map(arg => stringify the arg here).join(' ');

packages/plugin-ext/src/plugin/output-channel/output-channel-item.ts

- Support LogOutputChannel - Support LogLevel - Support in namespace/env: logLevel & onDidChangeLogLevel - Remark: Needs further extension of application to fully support all aspects of a LogOutputViewChannel (i.e. developer logger service, logging to file, extension of output view UI) Resolves eclipse-theia#12017 Contributed on behalf of STMicroelectronics. Signed-off-by: Nina Doschek <[email protected]>

- Ensure options to identify LogOutputChannel is never optional - LogOutputChannelImpl extends OutputChannelImpl instead of vscode's AbstractMessageLogger - Remove obsolete type converter again - Update Changelog Contributed on behalf of STMicroelectronics. Signed-off-by: Nina Doschek <[email protected]>

- Ensure LogOutputview is validated before logging or appending content - Simplify processing of error objects - Simplify formatting of arguments in log messages - Update Changelog Contributed on behalf of STMicroelectronics. Signed-off-by: Nina Doschek <[email protected]>

packages/plugin-ext/src/plugin/output-channel/output-channel-item.ts

tsmaeder

Some nitpicks left.

tsmaeder · 2023-04-21T11:52:42Z

@ndoschek I just have two nitpicks left, then we're good to go. Works as advertised.

ndoschek · 2023-04-21T11:55:23Z

Thank you @tsmaeder for your review! I left my replies in the open threads.

Contributed on behalf of STMicroelectronics. Signed-off-by: Nina Doschek <[email protected]>

tsmaeder

Looks good to me, now.

ndoschek force-pushed the issues/12017 branch from 61298da to 17b615c Compare April 18, 2023 07:34

ndoschek mentioned this pull request Apr 20, 2023

plugin: Support LogOutputChannel eclipsesource/theia#76

Closed

1 task

ndoschek force-pushed the issues/12017 branch from 17b615c to a45e5cd Compare April 20, 2023 07:25

ndoschek marked this pull request as ready for review April 20, 2023 07:27

kittaakos reviewed Apr 20, 2023

View reviewed changes

tsmaeder self-requested a review April 20, 2023 08:58

tsmaeder requested changes Apr 20, 2023

View reviewed changes

ndoschek force-pushed the issues/12017 branch from a45e5cd to 78b5498 Compare April 20, 2023 14:56

ndoschek requested review from tsmaeder and kittaakos April 20, 2023 14:58

tsmaeder reviewed Apr 21, 2023

View reviewed changes

ndoschek added 3 commits April 21, 2023 11:10

ndoschek force-pushed the issues/12017 branch from 78b5498 to c930230 Compare April 21, 2023 09:10

ndoschek requested a review from tsmaeder April 21, 2023 09:11

tsmaeder reviewed Apr 21, 2023

View reviewed changes

packages/plugin-ext/src/plugin/output-channel/output-channel-item.ts Show resolved Hide resolved

tsmaeder self-requested a review April 21, 2023 11:39

tsmaeder reviewed Apr 21, 2023

View reviewed changes

ndoschek mentioned this pull request Apr 21, 2023

[plugin] Enhance features of LogOutputChannelImpl #12440

Open

Address review comments #3

a9067c8

Contributed on behalf of STMicroelectronics. Signed-off-by: Nina Doschek <[email protected]>

tsmaeder approved these changes Apr 21, 2023

View reviewed changes

tsmaeder merged commit 17e5269 into eclipse-theia:master Apr 21, 2023

ndoschek deleted the issues/12017 branch April 21, 2023 12:38

vince-fugnitto added this to the 1.37.0 milestone Apr 27, 2023

This was referenced Nov 27, 2023

[Snyk] Fix for 6 vulnerabilities magnologan/theia#18

Open

[Snyk] Security upgrade @theia/application-package from 1.31.0 to 1.37.0 magnologan/theia#19

Open

This was referenced Dec 21, 2023

[Snyk] Fix for 6 vulnerabilities magnologan/theia#63

Open

[Snyk] Fix for 6 vulnerabilities magnologan/theia#64

Open

[Snyk] Fix for 6 vulnerabilities magnologan/theia#65

Open

[Snyk] Fix for 6 vulnerabilities magnologan/theia#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugin: Support LogOutputChannel #12429

plugin: Support LogOutputChannel #12429

ndoschek commented Apr 18, 2023 •

edited

Loading

kittaakos left a comment

kittaakos Apr 20, 2023

ndoschek Apr 20, 2023

tsmaeder left a comment

ndoschek commented Apr 20, 2023

tsmaeder Apr 21, 2023

ndoschek Apr 21, 2023

tsmaeder Apr 21, 2023

ndoschek Apr 21, 2023

tsmaeder Apr 21, 2023

ndoschek Apr 21, 2023

tsmaeder Apr 21, 2023

ndoschek Apr 21, 2023

tsmaeder Apr 21, 2023

tsmaeder Apr 21, 2023

tsmaeder Apr 21, 2023

ndoschek Apr 21, 2023

tsmaeder Apr 21, 2023

tsmaeder left a comment

tsmaeder commented Apr 21, 2023

ndoschek commented Apr 21, 2023

tsmaeder left a comment

plugin: Support LogOutputChannel #12429

plugin: Support LogOutputChannel #12429

Conversation

ndoschek commented Apr 18, 2023 • edited Loading

What it does

Remark:

How to test

Review checklist

Reminder for reviewers

kittaakos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder left a comment

Choose a reason for hiding this comment

ndoschek commented Apr 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder commented Apr 21, 2023

ndoschek commented Apr 21, 2023

tsmaeder left a comment

Choose a reason for hiding this comment

ndoschek commented Apr 18, 2023 •

edited

Loading