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

Improve Extensions logging #159892

Closed
sandy081 opened this issue Sep 2, 2022 · 9 comments · Fixed by #165925
Closed

Improve Extensions logging #159892

sandy081 opened this issue Sep 2, 2022 · 9 comments · Fixed by #165925
Assignees
Labels
api api-finalization feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders log VS Code log issues on-testplan
Milestone

Comments

@sandy081
Copy link
Member

sandy081 commented Sep 2, 2022

For logging, at present all extensions are doing following:

  • Creating an output channel
  • Log messages into this output channel in the format similar to VS Code log format

This is to simplify logging for extensions by providing an API that all extension can use to log in VS Code's log format

For example, This is how GitHub auth extension looks now

image

If VS Code supports logging for extensions, this will look and behave similar to VS Code logs

image

Proposal

@sandy081 sandy081 added feature-request Request for new features or functionality api log VS Code log issues labels Sep 2, 2022
@sandy081 sandy081 added this to the September 2022 milestone Sep 2, 2022
@sandy081 sandy081 self-assigned this Sep 2, 2022
@sandy081
Copy link
Member Author

Proposal:

export enum LogLevel {
		Trace = 0,
		Debug = 1,
		Info = 2,
		Warning = 3,
		Error = 4,
		Critical = 5,
		Off = 6,
	}

	export interface ExtensionContext {

		log(level: LogLevel, message: string, ...args: any[]): void;
		log(level: LogLevel.Error | LogLevel.Critical, e: Error, ...args: any[]): void;

	}

Expose log api on the ExtensionContext object that is passed to each extension during activation. Extensions can call this API to log data.

VS Code also allows user to control the log level per extension.

@jrieken
Copy link
Member

jrieken commented Sep 14, 2022

There is no denying that this is useful and helps to streamline extensions but similar ideas have already been discussed and discard in #85992 and #40053 (comment).

The reasoning is that the existing OutputChannel API is already a logger and that we should improve that. Otherwise we are enabling the discussion about when to use what and to move features from output channels over to loggers and vice versa.

So, instead of the proposal I would suggest something like this:

export interface OutputChannel {
  // all existing

  info(message: string): void;
  warn(message: string): void;
  error(error: string | Error): void;
}

@sandy081
Copy link
Member Author

I do not like combining logger with OutputChannel because of its existing features. For eg., One can append text in the same line and append a line of text as is in the OutputChannel. In contrary, Log API will log the message per line and adds pre-text like ${timestamp} ${name} ${level} ${message}. Mixing these two objects will cause inconsistency and confusion. It is hard to prevent append APIs when using OutputChannel as a logger. Also, a logger content is immutable and clear and replace APIs shall not be supported.

I just noticed that the current proposal is actually one of the recommendations in earlier discussions here

To be clear, the current proposal does not allow extensions to create loggers, instead it provides a logger per extension to log data.

@jrieken
Copy link
Member

jrieken commented Sep 15, 2022

Mixing these two objects will cause inconsistency and confusion. It is hard to prevent append APIs when using OutputChannel as a logger.

Isn't that up to the extension and something that very easy to "fix" Alternatively or in addition you can have a new argument to createOutputChannel that makes this a logger-like channel.

Also, a logger content is immutable and clear and replace APIs shall not be supported.

Well, clear is view property, not a model change. Just like I can clear a true log output view today. Replace needs some thinking but with the isLogger creation argument things become simple

@sandy081
Copy link
Member Author

I changed the proposal by introducing a Logger interface. Every extension gets an handle to a default logger. VS Code shows it in the Output Channel along with VS Code Logs (with a separator).

export interface Logger {
		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;
	}

	export interface ExtensionContext {
		readonly logger: Logger;
	}

image

I am still not sure if this Logger has to be based on OutputChannel or not. Also, this proposal is not to allow extensions creating loggers. IMO, we use Output Channel as an UI for showing loggers. Going forward, we can always detach them from OutputChannel and show them as a separate rich panel where we can add more actions like changing log levels. Another idea is that we can also show an extension log in the extension editor.

@sandy081
Copy link
Member Author

Here is the new proposal - aligning with Output Channel - after discussing in the API call

export interface AbstractOutputChannel {
		/**
		 * The human-readable name of this output channel.
		 */
		readonly name: string;

		/**
		 * Append the given value and a line feed character
		 * to the channel.
		 *
		 * @param value A string, falsy values will be printed.
		 */
		appendLine(value: string): void;

		/**
		 * Reveal this channel in the UI.
		 *
		 * @param preserveFocus When `true` the channel will not take focus.
		 */
		show(preserveFocus?: boolean): void;

		/**
		 * Hide this channel from the UI.
		 */
		hide(): void;

		/**
		 * Dispose and free associated resources.
		 */
		dispose(): void;
	}

	/**
	 * An output channel is a container for readonly textual information.
	 *
	 * To get an instance of an `OutputChannel` use
	 * {@link window.createOutputChannel createOutputChannel}.
	 */
	export interface OutputChannel extends AbstractOutputChannel {

		/**
		 * Append the given value to the channel.
		 *
		 * @param value A string, falsy values will not be printed.
		 */
		append(value: string): void;

		/**
		 * Replaces all output from the channel with the given value.
		 *
		 * @param value A string, falsy values will not be printed.
		 */
		replace(value: string): void;

		/**
		 * Removes all output from the channel.
		 */
		clear(): void;

		/**
		 * Reveal this channel in the UI.
		 *
		 * @deprecated Use the overload with just one parameter (`show(preserveFocus?: boolean): void`).
		 *
		 * @param column This argument is **deprecated** and will be ignored.
		 * @param preserveFocus When `true` the channel will not take focus.
		 */
		show(column?: ViewColumn, preserveFocus?: boolean): void;
	}

	/**
	 * A channel for containing log output.
	 */
	export interface LogOutputChannel extends AbstractOutputChannel {
		/**
		 * Log the given trace message to the channel.
		 *
		 * Messages are only printed when the user has enabled trace logging for the extension.
		 *
		 * @param message trace message to log
		 */
		trace(message: string): void;
		/**
		 * Log the given debug message to the channel.
		 *
		 * Messages are only printed when the user has enabled debug logging for the extension.
		 *
		 * @param message debug message to log
		 */
		debug(message: string): void;
		/**
		 * Log the given info message to the channel.
		 *
		 * Messages are only printed when the user has enabled info logging for the extension.
		 *
		 * @param message info message to log
		 */
		info(message: string): void;
		/**
		 * Log the given warning message to the channel.
		 *
		 * Messages are only printed when the user has enabled warn logging for the extension.
		 *
		 * @param message warning message to log
		 */
		warn(message: string): void;
		/**
		 * Log the given error or error message to the channel.
		 *
		 * Messages are only printed when the user has enabled error logging for the extension.
		 *
		 * @param error Error or error message to log
		 */
		error(error: string | Error): void;
	}

	export namespace window {
		/**
		 * Creates a new {@link LogOutputChannel log output channel} with the given name.
		 *
		 * @param name Human-readable string which will be used to represent the channel in the UI.
		 * @param options Options for the log output channel.
		 */
		export function createOutputChannel(name: string, options: { readonly log: true }): LogOutputChannel;
	}

@roblourens
Copy link
Member

when the user has enabled trace logging for the extension

How does this happen?

@sandy081
Copy link
Member Author

This is not yet supported. Updated the doc.

@sandy081
Copy link
Member Author

sandy081 commented Sep 26, 2022

Updated Proposal

       export enum LogLevel {
		Trace = 0,
		Debug = 1,
		Info = 2,
		Warning = 3,
		Error = 4,
		Critical = 5,
		Off = 6
	}

	export namespace env {

		/**
		 * The current log level of the application.
		 */
		export const logLevel: LogLevel;

		/**
		 * An {@link Event} which fires when the log level of the application changes.
		 */
		export const onDidChangeLogLevel: Event<LogLevel>;

	}

	/**
	 * A channel for containing log output.
	 */
	export interface LogOutputChannel extends OutputChannel {

		/**
		 * The current log level of the channel.
		 */
		readonly logLevel: LogLevel;

		/**
		 * An {@link Event} which fires when the log level of the channel changes.
		 */
		readonly onDidChangeLogLevel: Event<LogLevel>;

		/**
		 * Log the given trace message to the channel.
		 *
		 * Messages are only printed when the user has enabled trace logging.
		 *
		 * @param message trace message to log
		 */
		trace(message: string, ...args: any[]): void;
		/**
		 * Log the given debug message to the channel.
		 *
		 * Messages are only printed when the user has enabled debug logging.
		 *
		 * @param message debug message to log
		 */
		debug(message: string, ...args: any[]): void;
		/**
		 * Log the given info message to the channel.
		 *
		 * Messages are only printed when the user has enabled info logging.
		 *
		 * @param message info message to log
		 */
		info(message: string, ...args: any[]): void;
		/**
		 * Log the given warning message to the channel.
		 *
		 * Messages are only printed when the user has enabled warn logging.
		 *
		 * @param message warning message to log
		 */
		warn(message: string, ...args: any[]): void;
		/**
		 * Log the given error or error message to the channel.
		 *
		 * Messages are only printed when the user has enabled error logging.
		 *
		 * @param error Error or error message to log
		 */
		error(error: string | Error, ...args: any[]): void;
	}

	export namespace window {
		/**
		 * Creates a new {@link LogOutputChannel log output channel} with the given name.
		 *
		 * @param name Human-readable string which will be used to represent the channel in the UI.
		 * @param options Options for the log output channel.
		 */
		export function createOutputChannel(name: string, options: { readonly log: true }): LogOutputChannel;
	}

sandy081 added a commit that referenced this issue Nov 9, 2022
@VSCodeTriageBot VSCodeTriageBot added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Nov 9, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders log VS Code log issues on-testplan
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
@roblourens @jrieken @sandy081 @VSCodeTriageBot and others