Introduce Traceformat to allow JSON format log #366

octref · 2018-06-22T09:49:15Z

Things I plan to do but haven't done yet:

When verbosity is set to message, ignore params on Request / Notification messages
Expose traceFormat to client's initialization params

Questions:

Can we rename this to [langId].lsp.trace? server is misleading as the logs are actually logged from the client perspective. I'll write documentation for this too so I hope we can get this right.

@dbaeumer Would like to get some high-level insight - let me know if this is the right direction, what more things you would want to me to add, etc.

More details are at microsoft/language-server-protocol-inspector#8

dbaeumer · 2018-06-22T14:24:32Z

@octref goes absolutely into the right direction. Only one little comment so far.

octref · 2018-08-22T06:17:22Z

Hey Dirk, there are a bunch of updates that happened, so I'm wondering if you can take another look at this PR. Also /cc @aeschli in case you are interested.

High Level Changes

This PR remain as it is, so:

trace.server continues to work

trace.server can be an object now, for example:

"mls.trace.server": {
  "verbosity": "verbose", // values same as old `trace.server`
  "format": "json" // or `text` for old behavior
}

For streaming support, it can be implemented trivially by Language Server authors using the existing API LanguageClientOptions.outputChannel. For example: octref/vscode-language-server-template@4181af8.

This goes against what we initially agreed on, e.g:

{
  "format": "json" | "text",
  "verbosity": "off" | "messages" | "verbose",
  "output": "channel" | "log" | "websocket"
}

the benefit of leaving out logging / websocket-streaming is that we don't add dependencies (ws for websocket) and we give ext authors more freedom on handling logs.

How Things Work Now

There are 3 repos:

octref/vscode-lsp-inspector: The Inspector in webview with streaming support. It:
- launches the web app in webview
- opens a Websocket server to listen to incoming connections that would send LSP log data
- sends LSP log data into the web app and update UI.
Microsoft/language-server-protocol-inspector: I'll merge octref/vscode-lsp-inspector into this. So this repo will have 2 build targets. One is on LSP Website. The other one is the webview extension.
Microsoft/vscode-languageserver-node: People using the new version of the lib can specify output format of the LSP log. They can then save the log, stream it into the Inspector or do whatever they want.

Play With It

Download the VSIX: https://github.com/octref/vscode-lsp-inspector/releases/tag/v0.0.1
Build and Run the stream branch of: https://github.com/octref/vscode-language-server-template/tree/stream
Open a MD file. The LS should connect to the LSP Inspector Webview extension's WebSocket and start streaming data into it.

dbaeumer · 2018-08-22T10:34:04Z

jsonrpc/src/messages.ts

+    type: LSPMessageType;
+    message: RequestMessage | ResponseMessage | NotificationMessage;
+    timestamp: number;
+}


LSP uses tabs not spaces

dbaeumer · 2018-08-22T10:37:35Z

jsonrpc/src/main.ts

@@ -205,6 +206,36 @@ export namespace Trace {
 	}
 }

+export enum TraceFormat {


Instead of having a number enum and a TraceFormatValues would it make sense to use a string Enum which is now supported by TS?

Was aligning to TraceValues, string enum makes sense.

dbaeumer · 2018-08-22T10:45:34Z

jsonrpc/src/main.ts

@@ -224,6 +255,7 @@ export namespace LogTraceNotification {

 export interface Tracer {
 	log(message: string, data?: string): void;
+	logLSP(message: string): void;


This is a breaking API change. We should either make it optional or reuse the log signature. I looked at the client code and I was under the impression that the only difference is that the log prints [Trace - ...] and logLSP prints [LSP - ...]. Since the client knows the log format could this be decided on the client side.

Actually we should have a log method that takes an JSON object literal. So the JSON RPC code shouldn't even convert it to a strings. Then someone could plug in a tracer that writes the JSON objects to a file.

Actually we should have a log method that takes an JSON object literal.

NVM, found a way to do that.

dbaeumer · 2018-08-22T10:47:28Z

protocol/src/main.ts

@@ -157,7 +157,7 @@ export interface ProtocolConnection {
 	/**
 	 * Enables tracing mode for the connection.
 	 */
-	trace(value: Trace, tracer: Tracer, sendNotification?: boolean): void;
+	trace(value: Trace, traceFormat: TraceFormat, tracer: Tracer, sendNotification?: boolean): void;


This is breaking as well. We should support this in a non breaking way by having two signatures. If the old one is used we do the old text tracing.

dbaeumer · 2018-08-22T10:51:56Z

client/src/client.ts

+		});
+	}
+
+	public set traceFormat(value: TraceFormat) {


Would it make sense to fold that into trace as an optional param. If not provided it is text. I doubt that users set this independently.

Sure, since trace function already covers the format as an optional param. However I'm wondering if you mind adding traceFormat as one of the LanguageClientOptions? I feel this is something people want to configure on LS Client init but not setting it each time.

dbaeumer · 2018-08-22T10:52:29Z

protocol/src/main.ts

@@ -157,7 +157,7 @@ export interface ProtocolConnection {
 	/**
 	 * Enables tracing mode for the connection.
 	 */
-	trace(value: Trace, tracer: Tracer, sendNotification?: boolean): void;
+	trace(value: Trace, traceFormat: TraceFormat, tracer: Tracer, sendNotification?: boolean): void;


Same as on the implementation side. This is breaking...

dbaeumer · 2018-08-22T10:53:19Z

@octref provided by comments.

octref · 2018-08-24T06:50:37Z

client/src/client.ts

@@ -2498,6 +2505,17 @@ export abstract class BaseLanguageClient {
 		}
 	}

+	private logObjectTrace(data: any): void {
+		if (data.isLSPMessage && data.type) {


@dbaeumer Not sure if this is what you meant by "if the data is an object then log it by JSON stringify here". Also not sure if that isLSPMessage is the best way to go. If you have better suggestions, let me know!

No, in this case I would simply do

this.outputChannel.append(`[LSP - ${(new Date().toLocaleTimeString())}] `); this.outputChannel.appendLine(`${JSON.stringify(data)}`);

octref · 2018-08-24T06:58:13Z

Addressed comments, but left some questions too.

dbaeumer · 2018-08-24T08:15:19Z

client/src/client.ts

@@ -161,7 +161,7 @@ function createConnection(input: any, output: any, errorHandler: ConnectionError
 		sendNotification: (type: string | RPCMessageType, params?: any): void => connection.sendNotification(Is.string(type) ? type : type.method, params),
 		onNotification: (type: string | RPCMessageType, handler: GenericNotificationHandler): void => connection.onNotification(Is.string(type) ? type : type.method, handler),

-		trace: (value: Trace, tracer: Tracer, sendNotification: boolean = false): void => connection.trace(value, tracer, sendNotification),
+		trace: (value: Trace, tracer: Tracer, sendNotification: boolean = false, traceFormat: TraceFormat = TraceFormat.Text): void => connection.trace(value, tracer, sendNotification, traceFormat),


I know it is easier to implement if we simply add an optional arg at the endd but I would prefer to have to signatures like this:

trace: (value: Trace, tracer: Tracer, sendNotification: boolean = false) trace: (value: Trace, traceFormat: TraceFormat, tracer: Tracer, sendNotification: boolean = false)

to keep the args together and may be deprecate the old signature.

@dbaeumer Decided to have this:

trace(value: Trace, tracer: Tracer, sendNotification?: boolean): void; trace(value: Trace, tracer: Tracer, traceOptions?: TraceOptions): void; // In json-rpc export interface TraceOptions { sendNotification?: boolean; traceFormat?: TraceFormat; }

I initially had what you suggested, but later abandoned it in favor of traceFormat? at the end, as

I'd prefer to keep traceFormat optional

A call with three args can be of either signature.

With this new one it's easier to detect the params of the overloaded functions, and in the future it's easier to add more options without breaking backward compatibility.

No, this is not what I meant. I was suggesting to have this:

trace: (value: Trace, tracer: Tracer, sendNotification: boolean = false) trace: (value: Trace, traceFormat: TraceFormat, tracer: Tracer, sendNotification: boolean = false)

with one implementation overload like this:

function trace(arg0: Trace, arg1: Tracer | TraceFormat, arg2?: Tracer | boolean, arg3?: boolean)

There is enough information in the signatures to decide which on is meant.

dbaeumer · 2018-08-24T08:17:00Z

protocol/src/main.ts

@@ -158,6 +158,7 @@ export interface ProtocolConnection {
 	 * Enables tracing mode for the connection.
 	 */
 	trace(value: Trace, tracer: Tracer, sendNotification?: boolean): void;
+	trace(value: Trace, tracer: Tracer, sendNotification?: boolean, traceFormat?: TraceFormat): void;


Same as above

dbaeumer · 2018-08-24T08:24:17Z

client/src/client.ts

-				this.logTrace(message, data);
-			}
+			log: (...params: any[]) => {
+				if (typeof params[0] === 'string') {


There is a Is.string() helper I consistently use.

dbaeumer · 2018-08-24T08:25:42Z

client/src/client.ts

-			log: (message: string, data?: string) => {
-				this.logTrace(message, data);
-			}
+			log: (...params: any[]) => {


Can this not still be message & data and message can be string | any. ...params makes it harder to understand what data can actually come.

Introduce Traceformat to allow JSON format log

631cbd1

octref assigned dbaeumer Jun 22, 2018

octref requested a review from dbaeumer June 22, 2018 09:49

This was referenced Jun 26, 2018

Spec JSON format for loading octref/lsp-inspector#7

Closed

Spec JSON format for loading microsoft/language-server-protocol-inspector#7

Closed

octref mentioned this pull request Aug 3, 2018

support for other language servers microsoft/language-server-protocol-inspector#30

Open

kieferrm mentioned this pull request Aug 11, 2018

Iteration Plan for August 2018 microsoft/vscode#55927

Closed

39 tasks

octref changed the title ~~[WIP] Introduce Traceformat to allow JSON format log~~ Introduce Traceformat to allow JSON format log Aug 22, 2018

dbaeumer reviewed Aug 22, 2018

View reviewed changes

octref added 3 commits August 23, 2018 15:18

Address feedback

fa968d7

Don't break existing API

299f725

Fix whitespace

8e3a1c8

octref commented Aug 24, 2018

View reviewed changes

Minor type fix

78cfde6

dbaeumer reviewed Aug 24, 2018

View reviewed changes

dbaeumer approved these changes Aug 24, 2018

View reviewed changes

dbaeumer reviewed Aug 24, 2018

View reviewed changes

Add TraceOptions and address feedback

fa28e47

octref mentioned this pull request Aug 28, 2018

Test: LSP Inspector JSON log output and streaming microsoft/vscode#57310

Closed

3 tasks

dbaeumer merged commit b065c9c into master Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Traceformat to allow JSON format log #366

Introduce Traceformat to allow JSON format log #366

octref commented Jun 22, 2018 •

edited

Loading

dbaeumer commented Jun 22, 2018

octref commented Aug 22, 2018

dbaeumer Aug 22, 2018

dbaeumer Aug 22, 2018

octref Aug 23, 2018

dbaeumer Aug 22, 2018

dbaeumer Aug 22, 2018

octref Aug 23, 2018 •

edited

Loading

dbaeumer Aug 22, 2018

dbaeumer Aug 22, 2018

octref Aug 24, 2018

dbaeumer Aug 22, 2018

dbaeumer commented Aug 22, 2018

octref Aug 24, 2018

dbaeumer Aug 24, 2018

octref commented Aug 24, 2018

dbaeumer Aug 24, 2018

octref Aug 24, 2018

dbaeumer Aug 27, 2018 •

edited

Loading

dbaeumer Aug 24, 2018

dbaeumer Aug 24, 2018

dbaeumer Aug 24, 2018

Introduce Traceformat to allow JSON format log #366

Introduce Traceformat to allow JSON format log #366

Conversation

octref commented Jun 22, 2018 • edited Loading

dbaeumer commented Jun 22, 2018

octref commented Aug 22, 2018

High Level Changes

How Things Work Now

Play With It

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

octref Aug 23, 2018 • edited Loading

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

dbaeumer commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octref commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbaeumer Aug 27, 2018 • edited Loading

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

octref commented Jun 22, 2018 •

edited

Loading

octref Aug 23, 2018 •

edited

Loading

dbaeumer Aug 27, 2018 •

edited

Loading