[analytics] include vscode editor settings in our diagnostics report

[pq]

There have been a number of hypotheses around slowness associated w/ enablement of things like format and fix on-save actions and it would be great to find any correlations in user reports.

Settings in particular that are currently not known by server (but could help):

• editor.formatOnSave
• editor.codeActionsOnSave

/fyi @DanTup

[DanTup]

I started looking at this and there are two options:

1. Request the whole editor configuration and just read the values we care about
2. Request only the values we care about individually (editor.formatOnSave, editor.codeActionsOnSave etc.)

The first one results in all of the editor settings be included in the payload (once for global, once for each workspace folder) which isn't terrible, but also means this shows up in instrumentation logs (where it didn't before).

The second seems better, however it currently fails because in the Dart-Code extension we have middleware to set enableSnippets that assumes that the only configuration we ever fetch is an object (eg. the dart section). When requesting editor.formatOnSave (which results in a boolean), we'll try to set a field named enableSnippets on a boolean and fail.

I've opened Dart-Code/Dart-Code#5433 and will fix Dart-Code's middleware to not fail if we do (2), however it does mean we can't do (2) until that fix has been out enough time that we're happy to break compatibility with older versions of the extension (or, we use the version of the extension as an indicator of whether to request these values).

[DanTup]

Another idea is that we send a separate configuration request for the values we care about for the report here, and handle if the request fails (eg. because the middleware throws). That should give us the best of both worlds (although we still need to fix the Dart-Code middleware for the second request to succeed).

[DanTup]

I found another problem with this... It seems we are unable to fetch language-specific configuration with this API. Eg. if the user has:

"editor.formatOnSave": false,
"[dart]": {
	"editor.formatOnSave": true
}

By fetching editor.formatOnSave we get false, and fetching [dart].editor.formatOnSave just returns null. I think the only way to get the real value would be to fetch the configuration in the context of a Dart file, but that means finding a valid Dart file to use (we could pick the first from the known files list, but it's starting to feel quite hacky).

Given these settings are VS Code-specific anyway (there's nothing in LSP that suggests an editor should use editor.formatOnSave as the key for storing such a setting), another option could be for Dart-Code to pass this data to the server instead (it can easily read settings from the Dart-language-specific settings without a specific file and already does this in some places).

There are a few different ways this could be passed to the server:

1. Call a custom method to provide diagnostic info that will be included in the report (and displayed somewhere on the diagnostic pages)
2. Include it in a new key inside initializationOptions (we already show these in the diagnostic pages, and arguably should include these values in the report too)
3. Include it in ClientCapabilities.experimental (where we put some other non-standard LSP data, such as enabling the non-standard snippet syntax for CodeActions), which again already shows up in the diagnostic pages, and could also be added to the collected report

My feeling is that (3) is probably the better of those three, but since I suggest we add initializationOptions to the report anyway, including it there could be convenient (even if it doesn't actually affect initialization).

@bwilkerson interested in your thoughts too.

[DanTup]

Another option:

4. We copy any useful editor settings into the "dart" settings in the Dart-Code middleware that handles the configuration request from the server. That is, when the LSP server asks Dart-Code for configuration, we provide a synthetic "dart.formatOnSave", "dart.codeActionsOnSave", etc. - which are the values from the Dart-scoped editor settings. Then we extract those values (or if they're in a sub-object, that whole sub-object) for the report.

[FMorschel]

Note: I'm not sure if this is relevant because I'm not sure I fully understand where this info will be.

Since running the analyzer from source is slower than from the snapshot, maybe informing that setting is defined, can be good for us to review the data gathered for timings.

[DanTup]

Note: I'm not sure if this is relevant because I'm not sure I fully understand where this info will be.

There's a "Collect Report" button at the top of the diagnostic site (which you can launch from VS Code with the Dart: Open Analyzer Diagnostics command) that creates a json report that can be shared to help debug issues.

Since running the analyzer from source is slower than from the snapshot, maybe informing that setting is defined, can be good for us to review the data gathered for timings.

If we were to include this, I would guess there's a more reliable way to know we're running from the snapshot (for ex. Platform.script?) - although given the steps involved to set up an SDK and run from source, I suspect the number of people who do so could be counted on a small number of fingers.

[pq]

Options 3 and 4 seem reasonable to me but don't feel super strongly.

Anyway, thanks for digging in! I do think this will be really useful in diagnosing issues.

[DanTup]

I'm thinking that maybe it could be useful in future to include additional non-setting information in that diagnostic report that is known to Dart-Code but not the server, so maybe adding something to capabilities.experimental specifically for this purpose that can hold configuration and non-configuration would be best?

For example:

"experimental": {
  "diagnosticInfo": {
    "foo": "bar",
    "settings": {
      "global": { // global settings
        "editor.formatOnSave": true,
      },
      "workspaceFolders": [ // settings for each workspace folder in the workspace (if different to global)
        {
          "editor.formatOnSave": false,
        },
        {
          "editor.formatOnSave": false,
        }
      ]
    }
  }
}

One slight drawback of doing it up-front like this is that if the user modifies it during the session, we'll have stale values. That's probably not terrible though?

[bwilkerson]

Assuming that we care about these settings for debugging purposes, then we should probably do this in a way that allows the server to get updated information when it changes. That probably means a custom notification?

[pq]

I love the idea of capabilities.experimental to capture things like this.

we should probably do this in a way that allows the server to get updated information when it changes

It's more work but I'm inclined to agree. It could be really confusing for debugging if the data we were working with was stale.

[DanTup]

Assuming that we care about these settings for debugging purposes, then we should probably do this in a way that allows the server to get updated information when it changes. That probably means a custom notification?

There is actually a notification that we can get when configuration changes:

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_didChangeConfiguration

(arguably, we should implement this regardless of this issue because right now we only capture a snapshot of settings at server start and rarely refresh it - I think only when contexts change)

Option 4 above would avoid any custom protocol - however, since the settings are client-specific (whether we read editor or copy them into the dart settings in Dart-Code) that doesn't mean we'd get these settings for anything other than VS Code so that difference may not be significant.

I think I'm torn between synthetic config and something completely custom. Synthetic settings feels slightly odd, but avoids custom protocol. Custom protocol makes it easier to capture any information we think is relevant without having to add more and more synthetic settings (which may or may not actually be settings).

Probably the biggest differences comes down whether we want:

1. The client to provide some specific values that the server knows about and wants to capture (eg. editor.formatOnSave)
2. The client to provide an arbitrary map of "values it thinks should be included in the diagnostic report"

I think if we want the first, we should probably use the synthetic config (eg. map editor.formatOnSave into dart.xxx.formatOnSave and implement configurationDidChange). If we want the second, we should probably add a custom request for a client to push diagnostic info to the server (and do so each time that information changes on the client).

I feel like the second is most flexible (and allows capturing additional information with only Dart-Code changes, rather than changes to both Dart-Code and the SDK). WDYT @bwilkerson @pq ?

[bwilkerson]

I can't think of any settings that I'd want to use in the server to customize its behavior, and I like the flexibility of an arbitrary map of values that we could use on any platform. For example, I wouldn't want to use editor.formatOnSave as a signal that we should format as part of making edits because whether the user wants the formatter run on save and whether they want to keep their code formatted are not the same question, and it's the latter that should control the server's behavior.

[pq]

Agreed. #2 sounds great. Thanks, Danny!

[DanTup]

Both parts of this have landed - using a bleeding-edge SDK and the pre-release Dart-Code extension, these now show up in both the diagnostic pages and the export: