Expose log level and log path to extensions by roblourens · Pull Request #40142 · microsoft/vscode

roblourens · 2017-12-13T03:28:12Z

Exposes a logger and also just the log directory for extensions that need to write directly to a file. LogLevel changing stuff is non functional.

Fixes #40053

…xtension

jrieken · 2017-12-13T08:42:57Z

src/vs/vscode.d.ts

+	}
+
+	export interface ILogger {
+		onDidChangeLogLevel: Event<LogLevel>;


https://github.com/Microsoft/vscode/wiki/Extension-API-guidelines#global-events

Actually depends... When this is in the env it should be surely global when it stays in the extension context maybe not... Needs thinking

@mjbvz are imagining different log levels for different things. I think there's a good argument for being able to enable tracing for just one extension. So I think the logLevel and change event should be on the extension's own logger.

jrieken · 2017-12-13T08:43:17Z

src/vs/vscode.d.ts

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


https://github.com/Microsoft/vscode/wiki/Extension-API-guidelines#sync-vs-async

also, no getter, just a property

It's async because it only creates the directory when requested. It could be a prop of a promise though.

jrieken · 2017-12-13T08:43:50Z

src/vs/vscode.d.ts

+		Off = 7
+	}
+
+	export interface ILogger {


jrieken · 2017-12-13T08:44:22Z

src/vs/vscode.d.ts

 		resolveTask(task: Task, token?: CancellationToken): ProviderResult<Task>;
 	}

+	export enum LogLevel {


everywhere! https://github.com/Microsoft/vscode/wiki/Extension-API-guidelines#jsdoc

I know this matches the enum we defined in VS Code, but I think we should make Off = 0. That way, if a js user writes:

if (logger.level) { ... }

We do the more correct thing

I dunno, the user doesn't need to do that, and that will put the levels out of order. And translating between the enums will be weird.

jrieken · 2017-12-13T08:46:05Z

src/vs/workbench/api/node/extHostLogService.ts

+	}
+
+	trace(message: string, ...args: any[]): void {
+		return this._logService.trace(message, ...args);


extends ILogService?

ExtHostLogger extends ILogService? Maybe? It's not used that way

jrieken · 2017-12-13T08:48:04Z

src/vs/vscode.d.ts

+
+	export interface ILogger {
+		onDidChangeLogLevel: Event<LogLevel>;
+		getLevel(): LogLevel;


no getter, just a property

jrieken · 2017-12-13T08:51:07Z

src/vs/workbench/api/node/extHostLogService.ts

+	constructor(
+		private readonly _extHostLogService: ExtHostLogService,
+		private readonly _logService: ILogService,
+		private readonly _logDirectory: string


Not really sure I understand but it seems like one logger-instance for all extensions but a different log directory for each. Is that correct and iff so is that desired. Should each extension get its own log file? Should each log message be prefixed with the extension ID otherwise?

I think extension authors would be annoyed at having to read logs with messages from other extensions mixed in. Some extensions will be spammy.

Oh and to explain more, one ExtHostLogger per extension. Each has its own ILogService. ExtHostLogService creates the loggers.

jrieken · 2017-12-13T08:51:34Z

src/vs/workbench/api/node/extHostLogService.ts

+	private _loggers: Map<string, ExtHostLogger> = new Map();
+
+	private _onDidChangeLogLevel: Emitter<LogLevel>;
+	get onDidChangeLogLevel(): Event<LogLevel> { return this._onDidChangeLogLevel.event; }


readonly saves a getter

jrieken · 2017-12-13T09:04:21Z

@roblourens What's the reason for skipping the 'proposed'-api stage?

roblourens · 2017-12-13T17:22:10Z

I'll make it proposed, and jsdoc is a todo. I wanted feedback on the API before figuring out how to explain it.

mjbvz · 2017-12-13T19:51:05Z

src/vs/workbench/api/node/extHostLogService.ts

+		this._extHostLogService.onDidChangeLogLevel(logLevel => this._currentLevel = logLevel);
+	}
+
+	getLogDirectory(): TPromise<string> {


memoize this

mjbvz · 2017-12-13T19:54:06Z

src/vs/vscode.d.ts

 		resolveTask(task: Task, token?: CancellationToken): ProviderResult<Task>;
 	}

+	export enum LogLevel {


I know this matches the enum we defined in VS Code, but I think we should make Off = 0. That way, if a js user writes:

if (logger.level) { ... }

We do the more correct thing

mjbvz · 2017-12-13T19:56:26Z

src/vs/workbench/api/node/extHostLogService.ts

+}
+
+export class ExtHostLogger implements vscode.ILogger {
+	private _currentLevel: LogLevel;


…ventions better

… investigation later

DanTup · 2018-04-24T12:50:31Z

What's the status of this - I can't find any open cases relating to it. Are you after feedback on the proposed API? Is it likely to ship anytime soon? I'm trying to improve my handling of logs for CI test runs (currently it's hard for me to get at communication with the debugger, etc., from a failed test run) so I'm wondering whether to hold off for this as it might simplify things, or just do it my own way.

roblourens · 2018-04-24T15:28:21Z

You can follow #43275, we need to move it forward some more next month.

I think your scenario would require a way to change the log level from the extension which probably isn't something we'd add. Also, it won't help you log from a debug adapter. They run in their own process and need to handle their own logging.

DanTup · 2018-04-24T15:56:29Z

Thanks, didn't find that one because I was searching for "logger".

I'm actually running debug adapters in-process now to give us more control over launching them, but still a fair point, I'm trying to keep them such that they can run as separate process to.

I'll see if I can find a simple way to do what I need for now (I have some intermittent debugger tests I really want to fix) and revisit once this is stable to see if it'll help. Thanks!

roblourens · 2018-04-24T17:13:02Z

Check out LoggingDebugSession: https://github.com/Microsoft/vscode-debugadapter-node/blob/master/adapter/src/loggingDebugSession.ts and the logger provided by the debug adapter implementation if you haven't seen that yet.

DanTup · 2018-04-24T17:21:33Z

@roblourens Neat, I hadn't seen that! Though mostly I'm interesting in the communication between my debug adapter and debugger rather than between Code and the DA (however, when setting up my tests, I did struggle with this and ended up hacking a local copy - doh!).

roblourens · 2018-04-24T17:39:36Z

You can still import and setup the logger object from the adapter and use that to write to a file. It will at least do some of the work for you. I use this all over https://github.com/microsoft/vscode-chrome-debug-core to log communication between the debug adapter and Chrome/Node.

DanTup · 2018-04-24T17:49:18Z

Ah, gotcha! Ta! 👍

roblourens added 2 commits December 12, 2017 19:07

Add ExtHostLogService with logger and lazily created log folder per e…

daa3a0e

…xtension

Move extension logger to extension context

b3fde70

roblourens assigned roblourens and mjbvz Dec 13, 2017

roblourens requested review from joaomoreno, jrieken and sandy081 December 13, 2017 03:28

jrieken requested changes Dec 13, 2017

View reviewed changes

mjbvz approved these changes Dec 13, 2017

View reviewed changes

roblourens added 4 commits December 13, 2017 14:02

Make logger API proposed, add jsdoc, and match some extension API con…

d279e2d

…ventions better

More ExtHostLogger tweaks, and remove logLevel syncing code, for real…

35c7351

… investigation later

Merge branch 'master' into roblou/extHostLogger2

f877135

Slightly better "logger" comment

206b62d

roblourens merged commit 91c8e61 into master Dec 15, 2017

joaomoreno deleted the roblou/extHostLogger2 branch December 15, 2017 07:13

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Conversation

roblourens commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrieken commented Dec 13, 2017

Uh oh!

roblourens commented Dec 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanTup commented Apr 24, 2018

Uh oh!

roblourens commented Apr 24, 2018

Uh oh!

DanTup commented Apr 24, 2018

Uh oh!

roblourens commented Apr 24, 2018

Uh oh!

DanTup commented Apr 24, 2018

Uh oh!

roblourens commented Apr 24, 2018

Uh oh!

DanTup commented Apr 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants