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.

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

Need to tell user when start of plugin fails #12951

Closed
tsmaeder opened this issue Mar 21, 2019 · 35 comments
Closed

Need to tell user when start of plugin fails #12951

tsmaeder opened this issue Mar 21, 2019 · 35 comments
Labels
kind/enhancement A feature request - must adhere to the feature request template. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@tsmaeder
Copy link
Contributor

tsmaeder commented Mar 21, 2019

Here's what I do:

  1. Create a Che 7 workspace, set 2G memory, add github.com/eclipse/che as a project, enable "Language support for Java".
  2. Start the workspace
  3. You have no Java support, indicated by:
  • there is no hover, etc. when you open a Java file
  • there is no "Java" section in the dropdown of the Output view
  • there is no Java related container in the "containers" view.

We need to tell the user that the plugin was not started and give her the possibility to investigate the failure (logs, etc.) Currently, you're just stuck.
Also, when I log into the minishift console using the credentials I use for Che, nothing is visible. I have to log in as a different user (cluster admin?)

Correction: the sidecar container for the Java plugin seems to be visible, but I don't seem to have the ability to inspect the logs or anything from Che. Also, I the container has some cryptic display name that does not identify it as belong it to the Java support plugin

@benoitf
Copy link
Contributor

benoitf commented Mar 21, 2019

For your info, I have a warning message on my side telling me that java LSP was started too many times with failure and then has been disabled.

image

@tsmaeder
Copy link
Contributor Author

Not me, it seems I'm running into #12904 (found out after digging around minshift console). In my case, it just silently fails.

@benoitf
Copy link
Contributor

benoitf commented Mar 21, 2019

are you using che-theia:latest or che-theia:next ?

@tsmaeder
Copy link
Contributor Author

Whatever comes up when I use chectl server:start

@benoitf
Copy link
Contributor

benoitf commented Mar 21, 2019

@tsmaeder yes but at some point you choose a stack (or devfile) no ?

@tsmaeder
Copy link
Contributor Author

I choose the "Che 7" stack.

@benoitf
Copy link
Contributor

benoitf commented Mar 21, 2019

@tsmaeder ok then please select in the config list (you've to change from 1.0.0 to next)

image

@tsmaeder
Copy link
Contributor Author

@benoitf This issue is not about the fact that the Java plugins does not start. It is about the fact that we have no indication that the startup has failed, nor can we look at output from the plugin anywhere in Che.
We give the user no chance to find out what has gone wrong and rectify the situation.

@benoitf
Copy link
Contributor

benoitf commented Mar 22, 2019

@benoitf I'm just telling that the issue is reported through vscode-java plug-in. (there is a popup)
If you want more details, then it's the vscode-java extension that should report better status as it's this extension that is starting java process.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Mar 25, 2019

In my case there was absolutely no feedback.

@benoitf
Copy link
Contributor

benoitf commented Mar 25, 2019

yes but I think the no feedback was because you were using latest tag and not next tag (we fixed some issues around plug-ins loading since latest)

@tsmaeder
Copy link
Contributor Author

No, I just had a debugging session with @l0rd, who, for some reason, can't make the java support start (with the same workspace config as I have). It just silently does not start, even with theia:next

@slemeur
Copy link
Contributor

slemeur commented Mar 27, 2019

same on my side, I have not seen the visual feedback.
Should we display that in the "plugins" view ?

@tsmaeder
Copy link
Contributor Author

who, for some reason, can't make the java support start

The reason being that he did not have userland-proxy=false as a docker opt. I guess the silent failure happens when the main theia cannot contact the sidecar.

@benoitf
Copy link
Contributor

benoitf commented Mar 28, 2019

plug-in broker should now send plug-ins to each sidecar instead of sending all of them to the main theia instances. (At some point it was not supported)
Thus, if there is a failure in connecting, the plug-in will not be displayed at all.

To come back to the root problem of userland, I already filed an issue last year: #11018
but it seems no-one work on it

If userland is not set, platform won't work correctly (not limited to plug-ins)

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Mar 28, 2019

This issue is about the failure not being shown to the user.

@benoitf
Copy link
Contributor

benoitf commented Mar 28, 2019

@benoitf stop telling us how to fix the problem. This issue is about the failure not being shown to the user.

I'm saying your issue is a duplicate of #11018

it's not limited to plug-ins so please be respectful

@garagatyi
Copy link

@benoitf even though the reason is the same for both issues I have concerns whether we should qualify this as a duplicate of the issue mentioned by you.
I agree that your issue needs to be tackled and agree with Thomas that it should be tackled before Che 7 GA (he just added a label on that issue) but I believe that even in such a case we should show some adequate message to the user about connectivity issue. It should probably be part of Theia framework, not part of the JDT.LS.

@garagatyi
Copy link

plug-in broker should now send plug-ins to each sidecar instead of sending all of them to the main theia instances. (At some point it was not supported)
Thus, if there is a failure in connecting, the plug-in will not be displayed at all.

Unfortunately it is not accurate to my knowledge. We didn't have a specific prioritized task to do that.
@benoitf @l0rd @slemeur Please let me know if there is a priority to tackle that

@tsmaeder tsmaeder changed the title No indication of failed plugin startup Need to tell user when start of plugin fails Mar 28, 2019
@tsmaeder
Copy link
Contributor Author

Updated title to make it clear this issue is about informing the user, not about the failure to start the plugin.

@l0rd
Copy link
Contributor

l0rd commented Mar 28, 2019

The user doesn't have any feedback about the status of the plugins included in the workspace.

At least we should provide the state (loading/loaded/failed) in the plugin viewl. And a notification if the plugin fails to load. I believe these improvements should be done che-theia/theia side.

@garagatyi avoiding copying plugin in theia container is important but not critical for GA. Is there an issue already? I am sure we mentioned that somewhere

@benoitf
Copy link
Contributor

benoitf commented Mar 28, 2019

maybe I'm alone but here is what is my feeling:

Let say Theia is displaying a very nice message that we can't reach the containers. It's very nice but it's too late and the user will never be able to use the plug-ins as it requires admin changes.
I'm not saying that I won't implement it but it's not a good UX.

This is why I'm in favour of failing fast. When admin is installing Che, we should let him know if what he's installing is gonna work or not. (And maybe user of the platform is not the admin so only admin needs to be aware)

This is why I've issued a PR on chectl.
che-incubator/chectl#70

@garagatyi
Copy link

@l0rd we don't have AFAIK

This is why I'm in favour of failing fast. When admin is installing Che, we should let him know if what he's installing is gonna work or not. (And maybe user of the platform is not the admin so only admin needs to be aware)

@benoitf It is very cool that you added that functionality to chectl when using minishift/minikube but when Che is running on a different cluster there are still might be issues like that and they could occur on a particular cluster or even node, so we might not be able to detect it beforehand.
Which is why it is important to have thoughtful connection error handling to provide good UX

@l0rd
Copy link
Contributor

l0rd commented Mar 29, 2019

@benoitf this issue has been opened originally because jdtls failed silently for OOM. Not because of #11018

@benoitf
Copy link
Contributor

benoitf commented Mar 29, 2019

@l0rd ok in that case it means it's vscode-java that is not reporting the crash of its inner java process ?

Theia is calling start event to the plug-ins but Theia is not aware that some processes start java process or something else.

@tsmaeder
Copy link
Contributor Author

maybe I'm alone but here is what is my feeling:

Let say Theia is displaying a very nice message that we can't reach the containers. It's very nice but it's too late and the user will never be able to use the plug-ins as it requires admin changes.
I'm not saying that I won't implement it but it's not a good UX.

We won't ever be able to guarantee that we are 100% bug free or not flaky. Making the processes that are going on transparent to the user, including failures, will make the user feel more in control. Feeling in control is essential for a for users to be happy with a product. Also, it helps in diagnosing problems. The failure in question is a good example: The conversation on the problem you mentioned could go two ways:

Curently:
User: Java support is not working
Dev: Do you have any indication what could be the error?
User: no, it's just not there.

or, with good logging and failure feedback:

User: Java support is not working
Dev: Do you have any indication what could be the error? Any logs or Dialogs?
User: the log output says it was trying to establish a connection to , and then I got a dialog that said it can't connect.

@benoitf
Copy link
Contributor

benoitf commented Mar 29, 2019

@tsmaeder @l0rd said the issue is about the OOM of Java. Then it's the responsibility of the plug-in to report errors when processes that it starts are killed.

@l0rd
Copy link
Contributor

l0rd commented Mar 29, 2019

@benoitf we may improve jdt ls but we can't control any vs code extension. If a user is installing a vs code extension that fails to start (for whatever reason) we need to inform the user anyway. That's something that will help us protect against buggy extensions: we should tell users that an extension is not behaving as expected.

@benoitf
Copy link
Contributor

benoitf commented Mar 29, 2019

@l0rd AFAIk the start method is not reporting any error.

@benoitf
Copy link
Contributor

benoitf commented Mar 29, 2019

@l0rd if vscode java is not able to start the java process we have the same reporting than VS Code

In VS Code:
image

In Theia:
image

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Apr 1, 2019

@benoitf but both me and @l0rd have observed cases where you do not get any feedback for this. One such case is when -Xmx for the jdt.ls process is bigger than what is available in the sidecar. So there is at least one case we are not detecting. Running such a command line from a terminal in the sidecar just terminates with "killed" written to the terminal. Maybe the process is killed in a way that is not detected by vscode-java.
You said that "the start method is not reporting any error." How do you know?

@benoitf
Copy link
Contributor

benoitf commented Apr 1, 2019

@tsmaeder I played with different options of vscode java (like trying to get 4TB of memory, etc) and start method didn't receive anything.

Basically it's because the start of the LS is handled by vscode-language-client library in an asynchronous way. And first, it tries to restart the process several times by using the ErrorHandler.

it detects when connection is closed (java process exited):
https://github.com/Microsoft/vscode-languageserver-node/blob/04c4f6549d97487375aeb4ad0a43f060fb714c0c/client/src/client.ts#L258-L287

@slemeur
Copy link
Contributor

slemeur commented Apr 2, 2019

On my side, I saw the notification but I think from a UX point of view I feel that it is not obvious and hidden very shortly. So it can be missed by the user.
Having a place where the user can go and see the status of each of the plugin would be helpful, especially if we can provide meaningful information on what's going on and how to fix the issue.
(Using the plugin view? or maybe in the workspace panel as a new section?)

@benoitf
Copy link
Contributor

benoitf commented Apr 2, 2019

Theia can only report what is done by lifeycle methods. (or connection issues with sidecar) : the underlying framework.
Anything beyond that, Theia is not able to display the status because it's kind of a black box. It's the responsibility of the plug-in to behave correctly or provide its own 'status'

The only stuff that could be displayed could be container log or process log but if you expect a red light for VS Code java if java process is not alive, it's not possible by Theia (or VS Code) itself.

@benoitf benoitf added kind/enhancement A feature request - must adhere to the feature request template. and removed kind/usability labels Jul 9, 2019
@che-bot che-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 15, 2020
@che-bot
Copy link
Contributor

che-bot commented Jan 15, 2020

Issues go stale after 180 days of inactivity. lifecycle/stale issues rot after an additional 7 days of inactivity and eventually close.

Mark the issue as fresh with /remove-lifecycle stale in a new comment.

If this issue is safe to close now please do so.

Moderators: Add lifecycle/frozen label to avoid stale mode.

@che-bot che-bot closed this as completed Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

6 participants