Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improved handling of lsp failure state #309

Merged
merged 6 commits into from
Feb 4, 2025
Merged

improved handling of lsp failure state #309

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
13c55ac
improved handling of lsp failure state
Dec 12, 2024
548a9da
add static mock to lsp connection test case
Dec 13, 2024
374e549
Integrate Event Broker in LspStatusManager
taldekar Jan 27, 2025
34db4dc
fix to display logic when failure condition overlaps with lsp failure
nborges-aws Jan 28, 2025
9f846b1
track expired state in defaultView method and fix if condition on lsp…
Feb 3, 2025
5198a20
revert unintended change to MANIFEST file
nborges-aws Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugin/META-INF/MANIFEST.MF
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ Require-Bundle: org.eclipse.core.runtime;bundle-version="3.31.0",
org.apache.commons.logging;bundle-version="1.2.0",
slf4j.api;bundle-version="2.0.13",
org.apache.commons.lang3;bundle-version="3.14.0"
Bundle-Classpath: target/classes/,
Bundle-Classpath: .,
target/dependency/annotations-2.28.26.jar,
target/dependency/apache-client-2.28.26.jar,
target/dependency/auth-2.28.26.jar,
Expand Down
12 changes: 12 additions & 0 deletions plugin/plugin.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@
icon="icons/AmazonQ.png"
class="software.aws.toolkits.eclipse.amazonq.views.ChatAssetMissingView">
</view>
<view
id="software.aws.toolkits.eclipse.amazonq.views.LspStartUpFailedView"
name="Amazon Q"
icon="icons/AmazonQ.png"
class="software.aws.toolkits.eclipse.amazonq.views.LspStartUpFailedView">
</view>
<view
id="software.aws.toolkits.eclipse.amazonq.views.ToolkitLoginWebview"
name="Amazon Q"
Expand Down Expand Up @@ -142,6 +148,12 @@
relationship="stack"
visible="false">
</view>
<view
id="software.aws.toolkits.eclipse.amazonq.views.LspStartUpFailedView"
relative="software.aws.toolkits.eclipse.amazonq.views.ToolkitLoginWebview"
relationship="stack"
visible="false">
</view>
</perspectiveExtension>
</extension>
<extension
Expand Down
3 changes: 2 additions & 1 deletion ...src/software/aws/toolkits/eclipse/amazonq/handlers/AbstractQChatEditorActionsHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import software.aws.toolkits.eclipse.amazonq.chat.models.GenericCommandParams;
import software.aws.toolkits.eclipse.amazonq.chat.models.SendToPromptParams;
import software.aws.toolkits.eclipse.amazonq.chat.models.TriggerType;
import software.aws.toolkits.eclipse.amazonq.lsp.manager.LspStatusManager;
import software.aws.toolkits.eclipse.amazonq.plugin.Activator;
import software.aws.toolkits.eclipse.amazonq.telemetry.ToolkitTelemetryProvider;
import software.aws.toolkits.eclipse.amazonq.telemetry.metadata.ExceptionMetadata;
Expand All @@ -27,7 +28,7 @@ public abstract class AbstractQChatEditorActionsHandler extends AbstractHandler
@Override
public final boolean isEnabled() {
try {
return Activator.getLoginService().getAuthState().isLoggedIn();
return Activator.getLoginService().getAuthState().isLoggedIn() && !LspStatusManager.getInstance().lspFailed();
} catch (Exception e) {
return false;
}
Expand Down
8 changes: 4 additions & 4 deletions plugin/src/software/aws/toolkits/eclipse/amazonq/handlers/QOpenLoginViewHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@

import org.eclipse.core.commands.AbstractHandler;
import org.eclipse.core.commands.ExecutionEvent;
import software.aws.toolkits.eclipse.amazonq.plugin.Activator;

import software.aws.toolkits.eclipse.amazonq.lsp.manager.LspStatusManager;
import software.aws.toolkits.eclipse.amazonq.views.ViewVisibilityManager;

public class QOpenLoginViewHandler extends AbstractHandler {
@Override
public final Object execute(final ExecutionEvent event) {
if (Activator.getLoginService().getAuthState().isLoggedIn()) {
ViewVisibilityManager.showChatView("statusBar");
if (LspStatusManager.getInstance().lspFailed()) {
ViewVisibilityManager.showLspStartUpFailedView("statusBar");
} else {
ViewVisibilityManager.showLoginView("statusBar");
ViewVisibilityManager.showDefaultView("statusBar");
}
return null;
}
Expand Down
35 changes: 22 additions & 13 deletions plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/connection/QLspConnectionProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,32 +14,39 @@
import software.amazon.awssdk.utils.StringUtils;
import software.aws.toolkits.eclipse.amazonq.lsp.encryption.DefaultLspEncryptionManager;
import software.aws.toolkits.eclipse.amazonq.lsp.manager.LspManager;
import software.aws.toolkits.eclipse.amazonq.lsp.manager.LspStatusManager;
import software.aws.toolkits.eclipse.amazonq.lsp.manager.fetcher.RecordLspSetupArgs;
import software.aws.toolkits.eclipse.amazonq.plugin.Activator;
import software.aws.toolkits.eclipse.amazonq.preferences.AmazonQPreferencePage;
import software.aws.toolkits.eclipse.amazonq.providers.LspManagerProvider;
import software.aws.toolkits.eclipse.amazonq.telemetry.LanguageServerTelemetryProvider;
import software.aws.toolkits.eclipse.amazonq.telemetry.metadata.ExceptionMetadata;
import software.aws.toolkits.eclipse.amazonq.util.ProxyUtil;
import software.aws.toolkits.telemetry.TelemetryDefinitions.Result;
import software.aws.toolkits.eclipse.amazonq.plugin.Activator;
import software.aws.toolkits.eclipse.amazonq.preferences.AmazonQPreferencePage;

public class QLspConnectionProvider extends AbstractLspConnectionProvider {

public QLspConnectionProvider() throws IOException {
super();
LanguageServerTelemetryProvider.setAllStartPoint(Instant.now());
LspManager lspManager = LspManagerProvider.getInstance();
var lspInstallResult = lspManager.getLspInstallation();
try {
LanguageServerTelemetryProvider.setAllStartPoint(Instant.now());
LspManager lspManager = LspManagerProvider.getInstance();
var lspInstallResult = lspManager.getLspInstallation();

setWorkingDirectory(lspInstallResult.getServerDirectory());

setWorkingDirectory(lspInstallResult.getServerDirectory());
var serverCommand = Paths.get(lspInstallResult.getServerDirectory(), lspInstallResult.getServerCommand());
List<String> commands = new ArrayList<>();
commands.add(serverCommand.toString());
commands.add(lspInstallResult.getServerCommandArgs());
commands.add("--stdio");
commands.add("--set-credentials-encryption-key");
setCommands(commands);
} catch (Exception e) {
LspStatusManager.getInstance().setToFailed();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this caught by startProcess on L70?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, both these exceptions are caught at separate changes so no overlap. The exception on L47 is caught by eclipse during instantiation of the class, and the exception on L70 is caught once eclipse begins the LSP init process in the earlyStartup process.

throw(e);
}

var serverCommand = Paths.get(lspInstallResult.getServerDirectory(), lspInstallResult.getServerCommand());
List<String> commands = new ArrayList<>();
commands.add(serverCommand.toString());
commands.add(lspInstallResult.getServerCommandArgs());
commands.add("--stdio");
commands.add("--set-credentials-encryption-key");
setCommands(commands);
}

@Override
Expand Down Expand Up @@ -70,10 +77,12 @@ public final void start() throws IOException {

lspEncryption.initializeEncryptedCommunication(serverStdIn);
} catch (Exception e) {
LspStatusManager.getInstance().setToFailed();
emitInitFailure(ExceptionMetadata.scrubException(e));
Activator.getLogger().error("Error occured while initializing communication with Amazon Q Lsp Server", e);
}
} catch (Exception e) {
LspStatusManager.getInstance().setToFailed();
emitInitFailure(ExceptionMetadata.scrubException(e));
throw e;
}
Expand Down
10 changes: 10 additions & 0 deletions plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/manager/LspState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

package software.aws.toolkits.eclipse.amazonq.lsp.manager;

public enum LspState {
ACTIVE,
FAILED,
PENDING
}
49 changes: 49 additions & 0 deletions plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/manager/LspStatusManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

package software.aws.toolkits.eclipse.amazonq.lsp.manager;

import software.aws.toolkits.eclipse.amazonq.plugin.Activator;
import software.aws.toolkits.eclipse.amazonq.views.ViewVisibilityManager;

public final class LspStatusManager {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can avoid having the LspStatusManager. The EventBroker keeps track of the latest state, so if we emit to the event bus, then every subscriber will get the latest LSP state when the subscribe.


private static final LspStatusManager INSTANCE;
private LspState lspState;

static {
INSTANCE = new LspStatusManager();
}

private LspStatusManager() {
lspState = LspState.PENDING;
Activator.getEventBroker().post(lspState);
}

public static LspStatusManager getInstance() {
return INSTANCE;
}


public boolean lspFailed() {
return (lspState == LspState.FAILED);
}

public void setToActive() {
lspState = LspState.ACTIVE;
Activator.getEventBroker().post(lspState);
ViewVisibilityManager.showDefaultView("restart");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does the string value represent? It is unclear what it does to me. If its somehow related to metrics it should be indicated so with a enum class that holds these string eg. if this represents a click event then Source.Restart and similar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string value is indeed metric related, and you're right that it should be an enum. I'll start work to refactor all these instances. These metric strings are widely used and I don't want to dilute this PR with the changes, thus I'll open a separate PR with the edits.

}

public void setToFailed() {
if (lspState != LspState.FAILED) {
ViewVisibilityManager.showLspStartUpFailedView("update");
lspState = LspState.FAILED;
}

Activator.getEventBroker().post(lspState);
}
public LspState getLspState() {
return lspState;
}
}
2 changes: 2 additions & 0 deletions plugin/src/software/aws/toolkits/eclipse/amazonq/providers/LspProviderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.eclipse.lsp4j.services.LanguageServer;

import software.aws.toolkits.eclipse.amazonq.lsp.AmazonQLspServer;
import software.aws.toolkits.eclipse.amazonq.lsp.manager.LspStatusManager;
import software.aws.toolkits.eclipse.amazonq.lsp.manager.fetcher.RecordLspSetupArgs;
import software.aws.toolkits.eclipse.amazonq.telemetry.LanguageServerTelemetryProvider;
import software.aws.toolkits.telemetry.TelemetryDefinitions.Result;
Expand Down Expand Up @@ -52,6 +53,7 @@ public void setAmazonQServer(final LanguageServer server) {
future.complete(server);
}
emitInitializeMetric();
LspStatusManager.getInstance().setToActive();
}
}

Expand Down
5 changes: 4 additions & 1 deletion plugin/src/software/aws/toolkits/eclipse/amazonq/views/AmazonQChatWebview.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import software.aws.toolkits.eclipse.amazonq.configuration.PluginStoreKeys;
import software.aws.toolkits.eclipse.amazonq.lsp.AwsServerCapabiltiesProvider;
import software.aws.toolkits.eclipse.amazonq.lsp.auth.model.AuthState;
import software.aws.toolkits.eclipse.amazonq.lsp.manager.LspStatusManager;
import software.aws.toolkits.eclipse.amazonq.lsp.model.ChatOptions;
import software.aws.toolkits.eclipse.amazonq.lsp.model.QuickActions;
import software.aws.toolkits.eclipse.amazonq.lsp.model.QuickActionsCommandGroup;
Expand Down Expand Up @@ -143,7 +144,9 @@ public final void onEvent(final AuthState authState) {
Optional<String> content = getContent();
if (!content.isPresent()) {
canDisposeState = true;
ViewVisibilityManager.showChatAssetMissingView("update");
if (!LspStatusManager.getInstance().lspFailed()) {
ViewVisibilityManager.showChatAssetMissingView("update");
}
} else {
browser.setText(content.get()); // Display the chat client
}
Expand Down
44 changes: 44 additions & 0 deletions plugin/src/software/aws/toolkits/eclipse/amazonq/views/LspStartUpFailedView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

package software.aws.toolkits.eclipse.amazonq.views;
import java.util.concurrent.CompletableFuture;

import software.aws.toolkits.eclipse.amazonq.lsp.manager.LspStatusManager;

public final class LspStartUpFailedView extends BaseView {
public static final String ID = "software.aws.toolkits.eclipse.amazonq.views.LspStartUpFailedView";

private static final String ICON_PATH = "icons/AmazonQ64.png";
private static final String HEADER_LABEL = "Language Server failed to start.";
private static final String DETAIL_MESSAGE = "Restart Eclipse or review error logs for troubleshooting";

/* TODO: After refactor of LSP error handling is completed,
* add logic to base detail_message on error code returned from exception
* */
@Override
protected String getIconPath() {
return ICON_PATH;
}

@Override
protected String getHeaderLabel() {
return HEADER_LABEL;
}

@Override
protected String getDetailMessage() {
return DETAIL_MESSAGE;
}

@Override
protected void showAlternateView() {
ViewVisibilityManager.showDefaultView("restart");
}

@Override
protected CompletableFuture<Boolean> isViewDisplayable() {
return CompletableFuture.completedFuture(LspStatusManager.getInstance().lspFailed());
}

}
16 changes: 15 additions & 1 deletion plugin/src/software/aws/toolkits/eclipse/amazonq/views/ViewVisibilityManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,26 @@ private ViewVisibilityManager() {
private static final String RE_AUTHENTICATE_VIEW = ReauthenticateView.ID;
private static final String CHAT_ASSET_MISSING_VIEW = ChatAssetMissingView.ID;
private static final String CODE_REFERENCE_VIEW = AmazonQCodeReferenceView.ID;
private static final String LSP_STARTUP_FAILED_VIEW = LspStartUpFailedView.ID;
private static final String ERROR_LOG_VIEW = "org.eclipse.pde.runtime.LogView";

private static final Set<String> MUTUALLY_EXCLUSIVE_VIEWS = Set.of(
TOOLKIT_LOGIN_VIEW,
CHAT_VIEW,
DEPENDENCY_MISSING_VIEW,
RE_AUTHENTICATE_VIEW,
CHAT_ASSET_MISSING_VIEW
CHAT_ASSET_MISSING_VIEW,
LSP_STARTUP_FAILED_VIEW
);

public static void showDefaultView(final String source) {
if (Activator.getLoginService().getAuthState().isLoggedIn()) {
showChatView(source);
} else {
showLoginView(source);
}
}

public static void showLoginView(final String source) {
showMutuallyExclusiveView(TOOLKIT_LOGIN_VIEW, source);
}
Expand All @@ -56,6 +66,10 @@ public static void showChatAssetMissingView(final String source) {
showMutuallyExclusiveView(CHAT_ASSET_MISSING_VIEW, source);
}

public static void showLspStartUpFailedView(final String source) {
showMutuallyExclusiveView(LSP_STARTUP_FAILED_VIEW, source);
}

public static void showCodeReferenceView(final String source) {
showView(CODE_REFERENCE_VIEW, source);
}
Expand Down
Loading
Loading