Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose log level and log path to extensions #40053

Closed
mjbvz opened this issue Dec 11, 2017 · 11 comments · Fixed by #40142
Closed

Expose log level and log path to extensions #40053

mjbvz opened this issue Dec 11, 2017 · 11 comments · Fixed by #40142
Assignees
Labels
api under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Dec 11, 2017

Problem
The TS extension currently provides its own logging logic to write TSServer events to a file on the disk. We would like to allow extensions like TS to make use of the new logging infrastructure that was added VS Code 1.19, so that logs from both core and extensions can be written to a common logging directory

Proposed API
Add two new properties on ExtensionContext:

export interface ExtensionContext {
    ...

    loggingLevel: LoggingLevel;

    loggingDirectory: string;
}

The loggingDirectory would be unique to each extension and created when the extension first accesses the property. This prevents two extensions from overwriting each other if both use a file called log for example

@mjbvz mjbvz added the under-discussion Issue is under discussion for relevance, priority, approach label Dec 11, 2017
@mjbvz mjbvz added the api label Dec 11, 2017
@mjbvz
Copy link
Collaborator Author

mjbvz commented Dec 11, 2017

A similar API was previously prototyped here: https://github.com/Microsoft/vscode/blob/verbose-logging-mode/src/vs/vscode.proposed.d.ts#L251

That version used different model for enabling or disabling logging: starting code with the code --verbose-logging flag would enable all logging. You could also pass in scopes to specify that only specific items should log. Starting code with code --verbose-logging typescript for example would only enable logging for the "typescript" scope. The scopes were just strings that extensions could use to enable or disable specific types of logging


If we use the loggingLevel approach instead of scopes, TS would only want to enable its logging in for verbose levels such as Trace, since the TS server can generate a significant amount of logs

@mjbvz
Copy link
Collaborator Author

mjbvz commented Dec 11, 2017

// cc @joaomoreno @sandy081 @jrieken

@joaomoreno
Copy link
Member

joaomoreno commented Dec 12, 2017

The loggingDirectory would be unique to each extension and created when the extension first accesses the property. This prevents two extensions from overwriting each other if both use a file called log for example

It should be unique to each (extension host, extension) tuple, otherwise you'll have multiple instances of the same extension using the same folder.

Shouldn't there be a onDidChangeLogLevel event also?

I also fear that providing a loggingDirectory is a special case for the TS extension. Most extensions simply want the logging functions themselves. Why would each extension implement a logger in itself?

@joaomoreno joaomoreno added this to the Backlog milestone Dec 12, 2017
@jrieken
Copy link
Member

jrieken commented Dec 12, 2017

Yeah, why don't we expose a logger? Like vscode.env.logger. Each extension would get it's 'branded'-logger and we tell extensions to simply use that...

@sandy081
Copy link
Member

Yes, I like the idea of exposing logger in the API and extensions can use that. (which was also thought about #39365). Under the hood we can have log file per extension.

@roblourens
Copy link
Member

In cases like debug adapters or language servers, it would take extra effort to pipe log messages back into the extension host, and would create a ton of unnecessary IPC traffic, so it's better to let them write directly to some file. But I would support having a logger and also exposing their log folder.

@roblourens
Copy link
Member

roblourens commented Dec 13, 2017

Working on this -

export namespace env {
    [...]
    export let logger: ILogger;
}

export interface ILogger {
	onDidChangeLogLevel: Event<LogLevel>;
	getLevel(): LogLevel;
	getLogDirectory(): Promise<string>;

	trace(message: string, ...args: any[]): void;
	debug(message: string, ...args: any[]): void;
	info(message: string, ...args: any[]): void;
	warn(message: string, ...args: any[]): void;
	error(message: string | Error, ...args: any[]): void;
	critical(message: string | Error, ...args: any[]): void;
}

@mjbvz
Copy link
Collaborator Author

mjbvz commented Dec 13, 2017

Looks good overall. Not sure if logger should be on env or ExtensionContext though, considering that we want getLogDirectory to return a unique dir per extension.

@roblourens
Copy link
Member

LogLevel is never synced across processes currently, right?

@sandy081
Copy link
Member

@roblourens Log level is synced across processes during start up. But when changed afterwards using the command, then it is applied only to current renderer process. I am planning to sync it across processes.

I suppose each extension gets its own instance of logger thereby logging into a separate file (one log file per extension)?

@roblourens
Copy link
Member

I suppose each extension gets its own instance of logger thereby logging into a separate file (one log file per extension)?

Yep, that's what my PR does.

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants