Refactor server communication to JSON-RPC #990

sgraband · 2023-06-22T10:23:22Z

Update to newest tsp-typescript-client next version

To consume the new types introduced with eclipse-cdt-cloud/tsp-typescript-client#83.
Invoke the yarn clean script before running the prepare script.
Make sure all imports are from lib instead of src.

Update types of base package and react-components

Use the newly introduced types of tsp-typescript-client.
The type of TspClient becomes ITspClient.
The constructor for the base implementation becomes HttpTspClient.

Refactor server communication to JSON-RPC

TspClient Proxy:
Provide an implementation of the ITspClient called TheiaRpcTspProxy.
The implementation forwards all calls to a JSON RPC proxy called TheiaClientProxy.
Responses are then mapped to a TspClientResponse.
On the backend the TheiaClientProxy returns a lazyTspClient.
The lazyTspClient returns a HttpTspClient (default implementation).

Trace Server url provider:
The implementation of the provider was moved to the backend.
This way the env variable can be read out directly.
For the port preference a proxy mechanism was added.
Changes to the preference are sent via JSON RPC and handled by the url provider.

Trace Server connection status:
The implementation was moved to the backend.
Frontends can register themselves on the backend.
Each registered frontend will be pinged when the status has changed.

Misc:
Use ITspClient instead of TspClient for types, as TspClient is deprecated.

Fixes #976

Contributed on behalf of STMicroelectronics

sgraband · 2023-06-22T10:29:59Z

@bhufmann regarding these two points in the description:

note that this interface could be moved to tsp-typescript-client

then the TspClient could also follow the same interface

The plan would be to make the TspClient in the tsp-typescript-client package an interface and also offer a DefaultTspClient that implements the new interface, and is the same as the current TspClient. This way we can remove the TspFrontendClient interface from this PR and just implement the new TspClient interface. Then both the DefaultTspClient and the client used in the Theia frontend would follow the same interface.

Is there anything that would speak against this rename and redesign from your perspective? Otherwise, i would also open the contribution on tsp-typescript-client and adjust this PR. In the meantime you could already check out this PR as it would basically only be interface changes and the implementations will not change.

bhufmann · 2023-06-29T12:37:38Z

Thanks for the contribution. I'm a bit stretch with time at the moment for the review and test, and hence I had to backlog it. Having said that, one team member will look into the impact on the latency so that we have an idea what the additional communication step adds for the visualization of large(r) data.

theia-extensions/viewer-prototype/src/common/trace-server-connection-status.ts

packages/base/src/experiment-manager.ts

marco-miller

Thanks for this promising effort, well appreciated! Some comments for me as well, herein.

packages/base/src/tsp-frontend-client.ts

packages/base/src/experiment-manager.ts

packages/react-components/src/components/abstract-xy-output-component.tsx

package.json

theia-extensions/viewer-prototype/src/browser/trace-viewer/trace-viewer-contribution.ts

This commit adds a study on the impact of the JSON-RCP patch on the Theia Front End - Back End - Trace Server communication speed. Relates to eclipse-cdt-cloud#990. Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>

This commit adds a study on the impact of the JSON-RPC patch on the Theia Front End - Back End - Trace Server communication speed. Relates to eclipse-cdt-cloud#990. Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>

sgraband · 2023-08-29T08:27:24Z

@marco-miller I consumed the changes of eclipse-cdt-cloud/tsp-typescript-client#83 and rearranged the commits to hopefully make it easier to review. Your comments of your earlier review should also be incorporated as well as the naming discussions.
Could you take another look?
If you have any suggestions on how to split up the last commit i am happy to incorporate them, but i feel like the changes all are connected somehow so i wasn't sure on how (or if) to split them.

marco-miller · 2023-08-29T14:07:44Z

@marco-miller I consumed the changes of eclipse-cdt-cloud/tsp-typescript-client#83 and rearranged the commits to hopefully make it easier to review. Your comments of your earlier review should also be incorporated as well as the naming discussions. Could you take another look? If you have any suggestions on how to split up the last commit i am happy to incorporate them, but i feel like the changes all are connected somehow so i wasn't sure on how (or if) to split them.

Thanks for this. I recently changed focus at work, so leaving this follow-up and review to our other reviewers listed here. Cheers.

bhufmann

I'll continue with reviewing from Marco. Thanks for the updates.

I did some testing and the main feature is working well. However, I found some issues during testing. Could you please investigate this? I'll continue with this PR once updated.

theia-extensions/viewer-prototype/src/common/trace-server-connection-status.ts

theia-extensions/viewer-prototype/src/node/trace-server-connection-status-backend-impl.ts

theia-extensions/viewer-prototype/src/browser/trace-server-connection-status-client-impl.ts

yarn.lock

theia-extensions/viewer-prototype/src/node/trace-server-url-provider-impl.ts

This commit adds a study on the impact of the JSON-RPC patch on the Theia Front End - Back End - Trace Server communication speed. Relates to eclipse-cdt-cloud#990. Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>

This commit adds a study on the impact of the JSON-RPC patch on the Theia Front End - Back End - Trace Server communication speed. Relates to #990. Signed-off-by: Hoang Thuan Pham <hoang.pham@calian.ca>

Contributed on behalf of STMicroelectronics

Use the newly introduced types of tsp-typescript-client. The type of TspClient becomes ITspClient. The constructor for the base implementation becomes HttpTspClient. Contributed on behalf of STMicroelectronics

TspClient Proxy: Provide an implementation of the ITspClient called `TheiaRpcTspProxy`. The implementation forwards all calls to a JSON RPC proxy called `TheiaClientProxy`. Responses are then mapped to a `TspClientResponse`. On the backend the `TheiaClientProxy` returns a lazyTspClient. The lazyTspClient returns a `HttpTspClient` (default implementation). Trace Server url provider: The implementation of the provider was moved to the backend. This way the env variable can be read out directly. For the port preference a proxy mechanism was added. Changes to the preference are sent via JSON RPC and handled by the url provider. Trace Server connection status: The implementation was moved to the backend. Frontends can register themselves on the backend. Each registered frontend will be pinged when the status has changed. Misc: Use `ITspClient` instead of `TspClient` for types, as `TspClient` is deprecated. Fixes eclipse-cdt-cloud#976 Contributed on behalf of STMicroelectronics

Fix yarn.lock changes. Fix ConnectionStatus RPC communication: - Properly register client. - Save the last status to apply on newly added clients. - Remove clients that disconnect. - Only render on client when the view is visible. Rename setClient to addClient. Contributed on behalf of STMicroelectronics

Also fix a small issue with the preferences. Contributed on behalf of STMicroelectronics

Marco is available for a second review. I will delegate to another committer.

bhufmann

It looks good to me. Thank you very much for the contribution!

MatthewKhouzam

This chain improves the code by making communications simpler. I very much approve it.

Thanks Simon!

sgraband mentioned this pull request Jun 22, 2023

Refactor server communication to JSON-RPC eclipsesource/theia-trace-extension#1

Closed

sdirix requested a review from bhufmann June 22, 2023 10:32

marco-miller requested a review from hoangphamEclipse July 11, 2023 19:05

colin-grant-work mentioned this pull request Jul 17, 2023

React Component Dependencies and Behavior Un- or Underdeclared #997

Open

colin-grant-work reviewed Jul 17, 2023

View reviewed changes

theia-extensions/viewer-prototype/src/common/trace-server-connection-status.ts Outdated Show resolved Hide resolved

packages/base/src/experiment-manager.ts Outdated Show resolved Hide resolved

hoangphamEclipse mentioned this pull request Jul 19, 2023

Measure performance of TSPClient before and after JSON-RPC #999

Merged

marco-miller requested a review from MatthewKhouzam July 20, 2023 15:51

sgraband mentioned this pull request Jul 21, 2023

Make TspClient reuseable interface eclipse-cdt-cloud/tsp-typescript-client#83

Merged

marco-miller self-requested a review July 26, 2023 18:49

marco-miller previously requested changes Jul 31, 2023

View reviewed changes

sgraband force-pushed the 976-JSON-RPC branch from 4915061 to 9a00be1 Compare August 29, 2023 08:22

bhufmann requested changes Aug 30, 2023

View reviewed changes

sgraband added 5 commits September 13, 2023 09:20

Invoke the yarn clean script before running the prepare script

418b9cb

Contributed on behalf of STMicroelectronics

Update types of base package and react-components

10971b4

Use the newly introduced types of tsp-typescript-client. The type of TspClient becomes ITspClient. The constructor for the base implementation becomes HttpTspClient. Contributed on behalf of STMicroelectronics

Fix url not being updated properly

b5b68e1

Also fix a small issue with the preferences. Contributed on behalf of STMicroelectronics

sgraband force-pushed the 976-JSON-RPC branch from 6655b6e to b5b68e1 Compare September 13, 2023 13:22

sgraband requested a review from bhufmann September 13, 2023 13:23

bhufmann approved these changes Sep 14, 2023

View reviewed changes

MatthewKhouzam approved these changes Sep 14, 2023

View reviewed changes

bhufmann merged commit a1d5a0c into eclipse-cdt-cloud:master Sep 15, 2023
2 checks passed

marcdumais-work mentioned this pull request Mar 19, 2024

Test publish: latest (v0.2.0) npm packages #1058

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor server communication to JSON-RPC #990

Refactor server communication to JSON-RPC #990

sgraband commented Jun 22, 2023 •

edited

Loading

sgraband commented Jun 22, 2023 •

edited

Loading

bhufmann commented Jun 29, 2023 •

edited

Loading

marco-miller left a comment

sgraband commented Aug 29, 2023

marco-miller commented Aug 29, 2023

bhufmann left a comment

bhufmann left a comment

MatthewKhouzam left a comment

Refactor server communication to JSON-RPC #990

Refactor server communication to JSON-RPC #990

Conversation

sgraband commented Jun 22, 2023 • edited Loading

sgraband commented Jun 22, 2023 • edited Loading

bhufmann commented Jun 29, 2023 • edited Loading

marco-miller left a comment

Choose a reason for hiding this comment

sgraband commented Aug 29, 2023

marco-miller commented Aug 29, 2023

bhufmann left a comment

Choose a reason for hiding this comment

bhufmann left a comment

Choose a reason for hiding this comment

MatthewKhouzam left a comment

Choose a reason for hiding this comment

sgraband commented Jun 22, 2023 •

edited

Loading

sgraband commented Jun 22, 2023 •

edited

Loading

bhufmann commented Jun 29, 2023 •

edited

Loading