Add an LSP boot flag that enables flaky diagnostics tracking#4710
Add an LSP boot flag that enables flaky diagnostics tracking#4710weswigham wants to merge 6 commits into
Conversation
| s.logger.Error(diff) | ||
|
|
||
| if s.telemetryEnabled { | ||
| _ = sendNotification(s, lsproto.TelemetryEventInfo, lsproto.TelemetryEvent{ |
There was a problem hiding this comment.
I'm also sending a request failure telemetry event with a new flakeLog method if telemetry is enabled, too - with just the diagnostic codes in the payload. @DanielRosenwasser do you have any thoughts on that?
There was a problem hiding this comment.
It's not exactly the ideal way to do this, but it will automatically appear in the dashboards so it's probably fine for now.
There was a problem hiding this comment.
Pull request overview
Adds optional LSP diagnostic-instability tracking to help investigate issue #4527.
Changes:
- Runs diagnostics before and after emit, logging, reporting, or panicking on differences.
- Adds CLI modes and VS Code configuration.
- Adds diagnostic comparison and formatting utilities.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/lsp/server.go |
Performs and reports flaky-diagnostic checks. |
internal/lsp/lsproto/util.go |
Compares and formats diagnostics. |
cmd/tsgo/lsp.go |
Adds the LSP startup flag. |
_extension/src/client.ts |
Maps extension settings to CLI arguments. |
_extension/package.json |
Declares the new setting. |
_extension/package.nls.json |
Adds localized setting descriptions. |
9a00d4c to
6ec948e
Compare
| return false | ||
| } | ||
|
|
||
| func CompareDiagnostics(preEmit []*Diagnostic, postEmit []*Diagnostic, sanitizedDiffEnabled bool) (string, string) { |
There was a problem hiding this comment.
Since this is specific to pre/post emit, should this be named differently? Perhaps it doesn't much matter but it is sort of special and in the lsproto package, whereas I sort of expected it to live somewhere else
There was a problem hiding this comment.
Fair point - I've pushed the actual string generation out of lsproto, making CompareDiagnostics only do the set comparison.
There was a problem hiding this comment.
Er, well I meant moreso that all of this code is more specific to the flaky diag code and not inherent to lsproto, so could all be moved, no? But that's a bikeshed I know
Specifically, the
--lspflag now allows atrackFlakyDiagnosticsserver init flag with values ofnone,log, orpanic, which configure the LSP server to do extra work to track if and when we return diagnostics that users might find flaky or weirdly conditional. Correspondingly, there is ajs/ts.server.trackFlakyDiagnosticsflag with valueslog,panic,never, orauto.automeaninglogon vscode insiders orneverelsewhere, operating similarly to the existingjs/ts.server.showFailedResponsessetting.Under
panicmode it panics and crashes the LS, like so:otherwise under
logmode it only quietly logs the error like so:This can allow our fuzzer, end users, or telemetry to capture when the checker has an issue like those exposed in our tests over in #4527, and assist in tracking down the root cause so we don't have spurious errors that only-sometimes-appear.