Skip to content

Conversation

@vudayani
Copy link
Contributor

@vudayani vudayani commented Nov 8, 2022

No description provided.

@martinlippert
Copy link
Collaborator

As a quick heads-up: This requires an updated version of the Spring Tools 4 extension, to before you can try this, we need to finish and merge spring-projects/spring-tools#871 and wait for the CI build to produce an updated VSIX snapshot build. I will keep you posted here.

@Eskibear

This comment was marked as off-topic.

@azure-pipelines

This comment was marked as off-topic.

@Eskibear
Copy link
Member

I was trying remote process stuffs last couple of days. I'll try it and review the code ASAP when I have time. One quick comment is, the main.js script for webview is not robust enough. E.g. it uses processKey as id or value of "vscode-option" element. And if processKey contains whitespace (e.g. "remote process - some.host.name", there would be problem constructing the elements, causing wrong payload in consequent requests for metrics. What I did as a workaround was modifying the script to have it base64 encoded/decoded, just for reference. @vudayani-vmw Do you want to fix that together in this PR, or we can discuss it in a new issue?

@vudayani
Copy link
Contributor Author

@Eskibear Thanks for pointing it out. We can fix it as part of this PR.

@vudayani
Copy link
Contributor Author

@Eskibear
The 61c5c4c commit has changes to encode the id and value attributes of the "vscode-option" element using the js Base64 library. Do these changes look good?

There is a small change in the Spring Boot Tools 4 extension #883. The CI build should produce an updated VSIX snapshot build with these changes.

@Eskibear
Copy link
Member

using the js Base64 library

More dependencies bring more modules to load, and potential license issue usually. I would prefer not to introduce additional dependencies, if possible. Does window.atob/btoa() work here?

@Eskibear

This comment was marked as off-topic.

@azure-pipelines

This comment was marked as off-topic.

@Eskibear
Copy link
Member

Thank you @vudayani-vmw . It works well with vscode-spring-boot-1.41.0-202211132321.vsix, the data is still there after switching memory regions or collapsing/expanding the memory view.

Two UX related follow-ups. If you don't want to fix it right in this PR, I'm ok to get it merged. And you might create issues to track them, and address them in separate PRs.

  • Settings about max points and interval are not effective immediately, and no way to apply them to live process already connected unless you stop and re-run it. And there's no hint about when they will be effective. Ideally, it should be applied to existing connections, which can be impl with "workspace.onDidChangeConfiguration" API and additional messages between webview and vscode main process. Before implementing that, at least we can tell users about the behavior by the description.

  • By default the interval is 5s. It feels to me every time I start an app, the pickboxes are shown immediately when connection is established, but I have to wait about 5s to see the graph plotted. At the time the graph is not shown, it makes me feel "something is broken". Can we immediately query the metrics once after the connection in established? Or if we cannot get the data at the very beginning, can we plot a graph with the axes instead of blank canvas?

@Eskibear Eskibear marked this pull request as ready for review November 14, 2022 08:26
@microsoft microsoft deleted a comment from azure-pipelines bot Nov 14, 2022
@vudayani
Copy link
Contributor Author

@Eskibear I will create separate issues for the two improvements you have suggested :-)

@Eskibear
Copy link
Member

Thanks! I'm merging it now. So the coming pre-release version can work with latest vscode-spring-boot snapshot build, where some corresponding payload has been changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants