Standardize language server errors by JakeDawkins · Pull Request #1429 · apollographql/apollo-tooling

JakeDawkins · 2019-07-21T21:57:45Z

This PR is the result of investigating how other implementations of the language server protocol handle errors. This PR proposes a new method of handling errors across the language server and its clients (CLI and VS Code). The intention is to produce a more friendly, consistent means by which to log and handle errors, as well as set up the basis for what could become usage metrics and error reporting, and verbose/debugging modes.

The state of things now

No control over the granularity of errors (full stack traces, all the time)
No proper debug logging mechanism to help trace errors.
No standard way to log normal messages/warnings from the language server to all clients
Most language server implementations don't fail out. If an operation is unsuccessful, they just return an empty response, and the clients are responsible for messaging.

Proposed Solution

At a high level, this method would eliminate the throwing of errors, and rely on errors being passed as notifications from the server to a client. For clients that don't consume the language server from a JSON-RPC transport (like the CLI which calls it directly) the server's error mechanism would be to fallback to console logs and errors.

The key with this proposal is to ONLY have graceful exits from the language server. If something goes wrong, we should explain the error and do nothing. Failing out completely isn't great. Instead, clients should see empty responses from failed operations.

Debug classes on the client (vscode) and server.
- The client Debug class is responsible for printing errors, warnings, and info messages to the console. In CLI projects (or where connection is unavailable), the util will log/warn/error to the console
- The server Debug class is responsible to notifying the client of errors via the serverDebugMessage notification.
- There is a serverDebugMessage notification handler on the client which calls the Debug class methods to display errors to the user.
How do we support stack traces for these errors?
- By default, for Errors only (not warnings/info logs) we print the top 4 frames of the stack trace. We do this by creating a stack inside the Debug module and removing the unhelpful frames.
- The server sends these frames as a string to the client, which can print it.
- On the client, if a stack trace hasn't been provided (like if the client itself was erroring), we create a stack trace in the Debug module of the client.
- I think we can support a -v or --verbose mode where errors and warnings have traces. We could also make sure those stacks aren't trimmed.
- The [INFO] messages could be used for debug logs (maybe)

Notes:

Inspiration from the Crane Server implementation.
- One of the few LSP implementations that explicitly handle errors along with returning empty responses.
This will also give us consistent hooks for printing debug info when in a debug mode (not implemented) and telemetry (not implemented).

Future work:

the language server's Debug class could allow for a separate error logger to be passed to it to support arbitrary clients that can't support console logs or notifications from the LSP, but since we don't have any clients for that, we shouldn't worry about that right now.

TODO:

Update CHANGELOG.md* with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

JakeDawkins · 2019-08-09T19:16:20Z

+export class Debug {
+  public static connection?: IConnection;
+
+  public static SetConnection(conn: IConnection) {


using a static connection in this class, so we don't need to instantiate the class and pass it around anywhere. The user can just import the class itself from this file and call it without worrying like in the workspace file below

JakeDawkins · 2019-08-09T19:17:24Z

-            ...existingProjects,
-            ...projectsForConfig
-          ]);
+          if (config) {


Had to modify this file to be able to accept null configs. Since the new pattern would be to return empty responses and log errors, we'll need to do this a few places.

JakeDawkins · 2019-08-09T19:18:01Z


    const config = await this.createConfig(flags);
+
+    if (!config) {


Configs can now be null :)

I use this.error and this.exit here because there aren't many errors that happen in the CLI itself. Most happen in the language server.

Errors in the CLI already have a handler (this.error) and should return with an exit code of 1. We could wrap this up into another function but I don't see the need yet.

JakeDawkins · 2019-08-09T19:23:39Z

+// for errors (and other logs in debug mode) we want to print a stack trace showing where they were thrown.
+// This uses an Error's stack trace, removes the three frames regarding this file (since they're useless) and
+// returns the rest of the trace.
+const createAndTrimStackTrace = () => {


I think it's okay that this logic is duplicated in multiple places (language server and here) since it's only one thing, we don't have a great place to put it, and we could change the formatting based on where it is later.

JakeDawkins · 2019-08-09T19:24:01Z

+   */
+  public static error(message: string, stack?: string) {
+    const stackTrace = stack || createAndTrimStackTrace();
+    Debug.showConsole();


since errors are urgent, show the output window

JakeDawkins · 2019-08-09T19:24:30Z

+   * TODO: enable error reporting and telemetry
+   * Displays and warning message prefixed with [WARN]
+   */
+  // public static sendErrorTelemetry(message: string) {


We could use this to track errors that are thrown in sentry/google analytics/something else.

trevor-scheer

Minor, non-blocking stuff! Can't wait to see the future of error handling :)

* Add debug logging utils for language server and vs code * Add stack traces to debug errors * moved config loader to use debug util

JakeDawkins marked this pull request as ready for review August 9, 2019 02:17

JakeDawkins added 🔨 cli related to the CLI itself 🗒️ component - vscode related to editor tooling ❌ errors labels Aug 9, 2019

JakeDawkins force-pushed the jake/lsp-error-rewrites branch from 4d58675 to 376ae1b Compare August 9, 2019 18:55

JakeDawkins commented Aug 9, 2019

View reviewed changes

trevor-scheer approved these changes Aug 9, 2019

View reviewed changes

Comment thread packages/apollo-language-server/src/utilities/debug.ts Outdated

Comment thread packages/apollo/src/Command.ts Outdated

Comment thread packages/vscode-apollo/src/debug.ts

Comment thread packages/vscode-apollo/src/debug.ts

JakeDawkins and others added 12 commits August 15, 2019 09:44

Add debug logging utils

4fc6a8c

Update debug to warn properly with CLI

2a6b410

Add stack traces to debug errors

25a9386

moved config loader to use debug util

9e31397

wip on fixing config tests

90d6dd0

fix tests for config loader

dbf3aa9

fix failing tests

e9010a2

cleanup

f40106a

update debug classes to be more explicit

b10f135

update debugger to allow logger override

8431cff

remove unused debug util from cli

10eec3a

update changelog

b466e70

JakeDawkins force-pushed the jake/lsp-error-rewrites branch from 87c6c8f to b466e70 Compare August 15, 2019 13:46

JakeDawkins merged commit 400bf99 into master Aug 15, 2019

JakeDawkins deleted the jake/lsp-error-rewrites branch August 15, 2019 15:08

essaji pushed a commit to essaji/apollo-tooling that referenced this pull request Aug 25, 2019

Add Debug class for errors (apollographql#1429)

35325d1

* Add debug logging utils for language server and vs code * Add stack traces to debug errors * moved config loader to use debug util

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize language server errors#1429

Standardize language server errors#1429
JakeDawkins merged 12 commits intomasterfrom
jake/lsp-error-rewrites

JakeDawkins commented Jul 21, 2019 •

edited

Loading

Uh oh!

JakeDawkins Aug 9, 2019

Uh oh!

JakeDawkins Aug 9, 2019

Uh oh!

JakeDawkins Aug 9, 2019

Uh oh!

JakeDawkins Aug 9, 2019

Uh oh!

JakeDawkins Aug 9, 2019

Uh oh!

JakeDawkins Aug 9, 2019

Uh oh!

JakeDawkins Aug 9, 2019

Uh oh!

trevor-scheer left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		const config = await this.createConfig(flags);

		if (!config) {

Conversation

JakeDawkins commented Jul 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The state of things now

Proposed Solution

Uh oh!

JakeDawkins Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

JakeDawkins Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

JakeDawkins Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

JakeDawkins Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

JakeDawkins Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

JakeDawkins Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

JakeDawkins Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

trevor-scheer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JakeDawkins commented Jul 21, 2019 •

edited

Loading