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

add extension APIs to get live data #751

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

Eskibear
Copy link
Contributor

@Eskibear Eskibear commented Apr 2, 2022

Currently sts-vscode expose its language client to other extensions. In this PR, besides the language client instance, I also expose some APIs for downstream extension to easily get live process data. (see api.d.ts)

Downstream extension can now listen to events when live connection is connected/disconnected, and decides to fetch live data using the API.

await commands.executeCommand(COMMAND_LIVEDATA_GET, query);
}

// TODO: STS server should send corresponding notification back.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corresponding logic has not been implemented in spring-boot-language-server yet. As this project is well decoupled, I'm not sure where should be the best place to do that. The remaning work should be:

  • define custom notifications in STS4LanguageClient
  • send them at the place whereever live process connection is established/disconnected.

Copy link
Contributor

@BoykoAlex BoykoAlex Apr 4, 2022

Choose a reason for hiding this comment

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

I think org.springframework.ide.vscode.boot.java.livehover.v2.SpringProcessConnectorService is the place to fire off connect/disconnect events.

(For example line 155 would be the place to send connect notification. Note SimpleLanguageServer is in the constructor, store server.getClient() in the field to call notifyConnected(...). Add notifyConnected, notifyDisconnected to the STS4LangugeClient etc. - let us know if you get lost with this, will be happy to help further)
@kdvolder please let us know if you had something else in mind for these notifications

@Eskibear
Copy link
Contributor Author

Eskibear commented Apr 2, 2022

BTW the wiki page doens't cover how to setup dev environment if I'm focusing on sts-vscode's languge server. What I did was build the exec-jar from headless-services/spring-boot-language-server, copy into vscode-extensions/vscode-spring-boot and launch.
I assume there should be more convenient way of debugging the LS, as I do see options like 'DEBUG', CONNECT_TO_LS etc. But I haven't look deeper inside yet. It would be much appreaciated if you share the best practice to debug it.

@martinlippert
Copy link
Member

This is awesome @Eskibear , didn't expect this to include the implementation on the language server side already. :-) @BoykoAlex and @kdvolder will follow with here, provide feedback on the comments, merge the request, etc.

@@ -192,5 +202,42 @@ else if (arg instanceof JsonObject) {

return null;
}

private CompletableFuture<Object> get(ExecuteCommandParams params) {
String processKey = getProcessKey(params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't getProcessKey() be removed completely in favour of getArgumentByKey(String key)?

Copy link
Member

Choose a reason for hiding this comment

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

I think getProcessKey() is just a convenience method, I don't mind having such a method. But I also don't mind if we removed getProcessKey() and instead called getArgumentByKey(params, "processKey").

@BoykoAlex
Copy link
Contributor

BoykoAlex commented Apr 4, 2022

A general comment. Do we really want to fine tune the parameters of the new COMMAND_LIVEDATA_GET to get specific chunks of LiveData rather than just get it all at once and then make sense of it on the client side? If getting all data is fine the then the existing Refresh command already does this i think. @kdvolder your 2 cents would be much appreciated :-)
@Eskibear what's your time frame for this PR? When would you like this to go in?

@kdvolder
Copy link
Member

kdvolder commented Apr 4, 2022

I looked at the PR and what I see sort of makes sense. However what I do not see and do not understand is how would another extension/plugin be able to call on this ExtensionApi. Is there some mechanism in vscode apis that allows calls from one extension to another using this kind of mechanic? Is there a place where I could read about how that works and/or see an example?

I am asking because up until now we have been using commands as a kind of mechanic to allow one extension to call another (or even call 'functions' contributed by another extensions language server). This always seemed like a clunky way to do things, but we didn't / don't know a better way. This PR seems to suggest there is in fact a better way, but I have to admit, only seeing the implementation side of the extension-api does leave me wondering how caller would use the API.

@kdvolder
Copy link
Member

kdvolder commented Apr 4, 2022

to get specific chunks of LiveData rather than just get it all at once and then make sense of it on the client side?

I could go either way on this. Internally all these bits of information are requested individually and then stitched together for the 'refresh' call. So it makes sense to allow a client to only retrieve the data it wants and not pay the price for fetching things it doesn't care about.

Update: looking at it more closely I think the 'get' function only retrieves current data from the cache. So the argument about 'not fetching data you don't want, isn't quite as strong in that case.

I like the api definition on the client side. I.e. the types making it clear what kinds of 'queries' are supported. Whether or not we pull the big json data object apart on the client side or on the server side... it doesn't really matter. Maybe doing it on the server is slightly better because it sends smaller json data between client and server.

@Eskibear
Copy link
Contributor Author

Eskibear commented Apr 5, 2022

how would another extension/plugin be able to call on this ExtensionApi

@kdvolder Via vscode.extensions.getExtension(id).exports. See https://code.visualstudio.com/api/references/vscode-api#extensions . And below is a real example.

  • Here is Java extension's API definition.
  • Here is an example how Java Project Manager extension consume these APIs.

@Eskibear
Copy link
Contributor Author

Eskibear commented Apr 5, 2022

Let's list the use cases that dashboard extension fetch data from sts-vscode and visualize them.

Basic use cases:

  • On startup. When user launchs an app, live connection is connected, sts-vscode sends notificaiton via onDidLiveProcessConnect, dashboad queries beans/mappings data via getLiveProcessData (currently it's from cache, but should be consistent), and visualizes the data in tree view.
  • On stop. When user stops an app (or disconnect the live connection), sts-vsocde sends notification via onDidLiveProcessDisconnect, dashboard then reset the tree view to default.

Advanced use cases (if valid and feasible to implement):

  • On refresh/change/hot reload. AFAIK currently live data is only updated via a manual refresh. There's possiblity that live data in cache is dirty.
    • An ideal way is the "push" model. Whenever sts-vscode knows that cached data is updated, it sends a notification e.g. onDidLiveDataUpdated, then downstream extensions like dashboard can re-fetch the data. If that's not easy to implement, at least whenever a manual 'refresh' finishes, sts-vscode should send the notification, and dashboard can update the corresponding view.
    • A fallback solution is "pull", not preferred. We change the get function a little bit, allowing a forceToRefresh param. Then dashboard can poll sts-vscode with some interval, and always update the UI with latest data. I assume that would bring a lot of overhead.

@Eskibear
Copy link
Contributor Author

Eskibear commented Apr 5, 2022

what's your time frame for this PR? When would you like this to go in?

@BoykoAlex Our plan is to ship a new version of dashboard extension with live data visualization in this month, so I'm expecting extension APIs to be availabe as soon as we have thorough discussion on the API design, and agree on the implementation details. 😉

@kdvolder
Copy link
Member

kdvolder commented Apr 5, 2022

Alex and I started experimenting a bit and implementing the missing api pieces(i.e. the onDidConnect and onDidDisconnect on the server side.

I mostly like this proposed API using onDidConnect and onDidDisconnect (and probably onDidLiveDataUpdated) style notifications. But there is a small problem with this design as it is now. The problem is that if a live process was already connected before a client attaches its listeners then there is no way for the client to discover that process.

We could address this problem by adding an additional function to the api to retrieve the 'current' list of connected processes.

What do you think @Eskibear ? Shall we add something like a getLiveProcesses request to fill this gap? Or did you have something else/better in mind.

@kdvolder kdvolder merged commit f332ae4 into spring-projects:main Apr 5, 2022
@kdvolder
Copy link
Member

kdvolder commented Apr 5, 2022

I have just merged this into main with some extra tweaks / changes that Alex and I did:

  • added a onDidLiveProcessUpdate to the api
  • implemented the server-side bits

Still todo possibly: adding some api function to retrieve existing processKeys (i.e. so client can discover processes that were connected before it registered its listeners).

If we want to do this extra work, let's create a new PR or issue for it. I am closing this PR for now.

@Eskibear
Copy link
Contributor Author

Eskibear commented Apr 6, 2022

Shall we add something like a getLiveProcesses request to fill this gap?

A good catch. Adding an API like this should be fine.

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

Successfully merging this pull request may close these issues.

4 participants