Fix LSP fetching bug and Emit getServer after each step of LSP fetching #260
Conversation
…re all event doesn't fire prematurely
nborges-aws
changed the title
| @@ -52,7 +52,7 @@ public static Builder builder() { | |||
| } | |||
|
|
|||
| @Override | |||
| public LspInstallResult getLspInstallation() { | |||
| public synchronized LspInstallResult getLspInstallation() { | |||
There was a problem hiding this comment.
in what scenario are we attempting to make this call twice? I believe this call should only happen during initialization of the language server.
There was a problem hiding this comment.
I discovered a bug where language server initialization process was restarting prior to completion, and confirmed it with @breedloj. GetLspInstallation is being called in the language server setup process, as well as in the set up process of the chat view. It was possible for installResult to still be null by the time chatView was calling the getLspInstallation method, and thus was restarting the fetch process.
| @@ -101,6 +101,7 @@ public LspFetchResult fetch(final PluginPlatform platform, final PluginArchitect | |||
|
|
|||
| // delete invalid local cache | |||
| ArtifactUtils.deleteDirectory(downloadDirectory); | |||
| emitGetServer(Result.FAILED, serverVersion, LanguageServerLocation.CACHE, null, start); | |||
There was a problem hiding this comment.
This is not a valid state to capture. Because this does not indicate that langauge server fetching failed. There is still logic to get to the fallback lsp which indicates that fetching process still succeeded but could not resolve cache. These intermediate states are just noise. There should only be one metric for each step of the lsp process
There was a problem hiding this comment.
Multiple metrics for each step of the lsp process was specifically requested within the telemetry. However you're correct in this emission being unnecessary regardless; it's been removed.
| @@ -36,14 +37,14 @@ public static void emitSetupGetManifest(final Result result, final RecordLspSetu | |||
|
|
|||
| public static void emitSetupGetServer(final Result result, final RecordLspSetupArgs args) { | |||
| emitSetupMetric(result, args, LanguageServerSetupStage.GET_SERVER); | |||
| if (result == Result.FAILED) { | |||
| if (result == Result.FAILED && args.getLocation() == LanguageServerLocation.UNKNOWN) { | |||
There was a problem hiding this comment.
when does unknown get emitted?
There was a problem hiding this comment.
Unknown only is emitted when the getServer process has a complete failure. This logic is necessary given the requested change to fire a telemetry event for each piece of the getServer process. If getServer from remote fails but fallback succeeds, we should not be emitting a setupAll failure event. This check ensures that any intermediate failure does not fire the setupAll metric, but a full failure of getServer does.
Description of changes:
This change emits a failure event for when retrieving the LSP from a certain location fails. This is an intermediate failure and the entire step can still succeed, so these cases should don't cause a setupAll event to be emitted. Also made lsp install getter synchronized, as concurrent execution was causing the lsp server to be fetched twice.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.