Add LSP tracing #5127

abgruszecki · 2018-09-19T16:55:18Z

LSP communication can now be traced remotely.

See https://github.com/lampepfl/dotty-remote-tracer for server implementation.

Duhemm · 2018-10-02T07:18:54Z

vscode-dotty/src/tracer.ts

+            );
+        });
+        zip.pipe(output);
+        zip.glob('./**/*.{scala,sbt}', { cwd: rootPath });


Should this include .dotty-ide.json and .dotty-ide-artifact? We should be able to recreate them from the build definition, but they may be useful?

This should include the java sources too, because they may be needed to build the project.

Duhemm · 2018-10-02T07:24:05Z

vscode-dotty/src/tracer.ts

+        if (!fs.existsSync(storagePath)) fs.mkdirSync(storagePath);
+        const outputPath = path.join(storagePath, 'workspace-dump.zip');
+        if (fs.existsSync(outputPath)) fs.unlinkSync(outputPath);
+        let output = fs.createWriteStream(outputPath);


Could be a const

Duhemm · 2018-10-02T07:58:56Z

vscode-dotty/src/tracer.ts

+
+        const localLspOutputChannel = vscode.window.createOutputChannel('Dotty LSP Communication')
+        try {
+            return this.createRemoteLspOutputChannel(remoteTracingUrl, localLspOutputChannel);


I think that it would be good to also add something in the status bar to show that everything is being recorded and sent

Duhemm · 2018-10-02T08:15:14Z

vscode-dotty/src/tracer.ts

+                'Do you want to help EPFL develop this plugin by uploading your usage data? ' +
+                'PLEASE BE AWARE that this will upload all of your keystrokes and all of your code, ' +
+                'among other things.',
+                'yes', 'no'


I think that just saying "among other things" is scary. We should either exhaustively list what's being uploaded here or include a link to a page that exhaustively lists what's being uploaded.

Also, I would replace "all of your keystrokes" by "every action performed in Dotty IDE". Keystrokes outside of the IDE are not recorded.

abgruszecki · 2018-10-12T16:48:00Z

@Duhemm fixed most of the things you mentioned. Tracing popup now directly mentions that we simply upload all LSP communication, which contains all the files and all the keystrokes. Added a statusbar item which tells the user tracing is on and allows them to adjust their consent.

There are still some outlying issues (Dotty codebase cannot be really traced, b/c the files it contains are too large; websocket connections get dropped if internet connection disappers for a second and cannot be recreated because the server does not support it; we should probably clean the LSP output channel from time to time to avoid too much trash, but do so in a way consistent with other plugins), but this was meant to work for students and it should be sufficient for them. @smarter?

smarter · 2018-10-12T17:06:46Z

Dotty codebase cannot be really traced, b/c the files it contains are too large

What's the exact issue here? Can we work around it by sending compressed files ?

abgruszecki · 2018-10-15T11:01:59Z

@smarter I think I found a good solution. The problem starts with the fact that LSP sometimes sends entire files in one message, at least when the file is opened. This causes issues, since there's a limit on the size of a message sent through WebSocket (probably both in Play Framework and Nginx), and it may not be possible (or desireable) to completely remove it. Allowing arbitrary size messages may open the server to easy DoS, and setting a limit on the size of message will ultimately just delay the problem.

The solution would be to send messages in chunks if they are too large, and save each chunk to disk immediately after it is received. This avoid the problem of too large messages, but creates a possibility where the last line of the log on the disk is malformed (because not all chunks were sent/received). I think we can work around that limitation more easily, since it's enough to just check if the file ends in a newline.

Duhemm · 2018-10-17T16:57:46Z

vscode-dotty/package-lock.json

@@ -4,10 +4,58 @@
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
+<<<<<<< HEAD


It looks like conflicts haven't been properly fixed in this file?

smarter

Let's merge this once it's rebased

An item in status bar now shows tracing status and can be clicked to adjust consent. Tracing and workspace dump now take into account that consent can be given/revoked - workspace dump is only initialized after consent is given, tracing socket is only created lazily as necessary.

Remote useless info, say when starting to zip.

Note that server doesn't care if the messages are proper JSON or not.

abgruszecki · 2018-10-29T12:41:17Z

Rebased, updated my local npm to a version which actually notices and uses the package-lock.json, added a setting for maximum message size. Could not get the connection to fail while opening files in Dotty repo.

smarter

Suggestion: instead of "trace"/"tracing" let's call this "telemetry" since this is the term vscode and others use.

smarter · 2018-10-30T13:23:59Z

vscode-dotty/src/tracer.ts

+        this.sessionId = new Date().toISOString()
+    }
+
+    run(): { lspOutputChannel?: vscode.OutputChannel } {


Why not run(): vscode.OutputChannel | undefined ?

smarter · 2018-10-30T13:27:39Z

vscode-dotty/src/tracer.ts

+
+    private initializeAsyncWorkspaceDump() {
+        if (this.remoteWorkspaceDumpUrl === undefined) return
+        // convince TS that this is a string


You can convince TS of this in a more direct way:

const url = this.remoteWorkspaceDumpUrl if (url) { // ... }

smarter · 2018-10-30T13:32:59Z

vscode-dotty/src/tracer.ts

+                        type: event.type
+                    }))
+                )
+                vscode.window.showWarningMessage('An error occured in Dotty LSP remote tracing connection.')


I would rather not show any messages to users if something goes wrong, it's distracting and not very useful for them. What we could do is change the status bar message to have On/Off/Failed

smarter · 2018-10-30T13:35:23Z

vscode-dotty/src/tracer.ts

+    value: T
+    valueToObject: (t: T) => {}
+
+    constructor(value: T, valueToObject: (t: T) => {}) {


You can directly create an immutable class field and assign by writing constructor(readonly value: T, ...)

smarter · 2018-10-30T13:35:50Z

vscode-dotty/src/tracer.ts

+    return new SafeJsonifier(e, (e) => e.toString())
+}
+
+class SafeJsonifier<T> {


Some documentation would be nice :)

smarter · 2018-10-30T13:49:10Z

vscode-dotty/src/tracer.ts

+
+        this.tracingConsent = new TracingConsentCache(ctx.extensionContext.workspaceState)
+
+        this.remoteWorkspaceDumpUrl = this.ctx.extensionConfig.get<string>('remoteWorkspaceDumpUrl')


I think these should be in "tracing." like maximumMessageSize

smarter · 2018-10-30T13:58:34Z

vscode-dotty/src/tracer.ts

+    }
+
+    private createLspOutputChannel(): vscode.OutputChannel | undefined {
+        if (!this.remoteTracingUrl) return undefined


Unless I'm missing something, it looks like this means that we don't get any outputchannel, even a local one, when remoteTracingUrl is undefined. Suggestion: make an LspOutputChannel that extends OutputChannel and take as argument a (local) OutputChannel, then every method of LspOutputChannel just forwards to the underlying OutputChannel and also does network stuff if tracing is enabled.

abgruszecki added the area:ide label Sep 19, 2018

abgruszecki requested a review from smarter September 19, 2018 16:55

abgruszecki force-pushed the lsp-tracing branch 2 times, most recently from a08f22a to 97af302 Compare September 20, 2018 16:03

allanrenucci assigned smarter Oct 1, 2018

Duhemm reviewed Oct 2, 2018

View reviewed changes

abgruszecki force-pushed the lsp-tracing branch 2 times, most recently from b7d6d17 to 277dd8b Compare October 12, 2018 16:41

Duhemm reviewed Oct 17, 2018

View reviewed changes

smarter approved these changes Oct 19, 2018

View reviewed changes

Aleksander Boruch-Gruszecki added 4 commits October 29, 2018 12:54

Add new dependencies to vscode-dotty

1a37fac

Implement LSP tracing for vscode-dotty

f48a19c

VsCode plugin: make workspace dump more informative

623a0a0

Remote useless info, say when starting to zip.

abgruszecki force-pushed the lsp-tracing branch from 277dd8b to 4f0292a Compare October 29, 2018 12:31

VsCode: add max message size setting

9f8e730

Note that server doesn't care if the messages are proper JSON or not.

abgruszecki force-pushed the lsp-tracing branch from 4f0292a to 9f8e730 Compare October 29, 2018 12:40

smarter added 2 commits October 30, 2018 14:08

vscode-dotty: don't use semi-colons

e4a1e64

vscode-dotty: update package-lock.json

7320339

smarter force-pushed the lsp-tracing branch from fae0272 to 7320339 Compare October 30, 2018 13:09

smarter approved these changes Oct 30, 2018

View reviewed changes

smarter requested changes Oct 30, 2018

View reviewed changes

Adjust after review

803843c

smarter approved these changes Oct 31, 2018

View reviewed changes

smarter added 2 commits October 31, 2018 17:04

Update the message asking for tracing consent

1912f86

Fix indentation for object literal

b68c35a

Add a maximum size to the local lsp log buffer

d3f45e9

smarter merged commit 2f685d2 into scala:master Oct 31, 2018

Blaisorblade deleted the lsp-tracing branch November 14, 2018 19:42


		this.tracingConsent = new TracingConsentCache(ctx.extensionContext.workspaceState)

		this.remoteWorkspaceDumpUrl = this.ctx.extensionConfig.get<string>('remoteWorkspaceDumpUrl')

Add LSP tracing #5127

Add LSP tracing #5127

Uh oh!

Conversation

abgruszecki commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

abgruszecki commented Oct 12, 2018

Uh oh!

smarter commented Oct 12, 2018

Uh oh!

abgruszecki commented Oct 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

abgruszecki commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter left a comment

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!

Uh oh!

abgruszecki commented Sep 19, 2018 •

edited

Loading

abgruszecki commented Oct 29, 2018 •

edited

Loading