7198: Add websocket server that pushes data on selection updates #306

cimi · 2021-09-09T16:32:58Z

This PR adds a websocket server that pushes event data as JSON to all connected clients whenever the user updates their current selection in JMC. The server is disabled by default and can be enabled and disabled from JMC preferences. This is a rework of the proof-of-concept code in #225 and uses the JSON serialiser introduced in #279.

The three endpoints available on the server are:

/events/ - raw serialised events
/tree/ - StacktraceTreeModel, data used in the flame graph view
/graph/ - StacktraceGraphModel, data used in the graph view

Please see this Observable collection for examples on how to connect.

This PR adds several new dependencies:

org.eclipse.jetty.websocket:websocket-jetty-server:10.0.5
org.eclipse.jetty.websocket:websocket-javax-server:10.0.5
org.eclipse.jetty.websocket:websocket-jetty-api:10.0.5
org.apache.aries.spifly:org.apache.aries.spifly.dynamic.bundle:1.34

After adding the new dependencies, I had issues with resolving transitive dependency conflicts (when I was trying to use Jetty 9.x) then with fixing the eclipse launcher - please see the comments for details.

Progress

Commit message must refer to an issue
Change must be properly reviewed

Issue

JMC-7198: Add websocket server that pushes data on selection updates

Reviewers

@bric3 (no known github.com user name / role) ⚠️ Review applies to 32d530b
Marcus Hirt (@thegreystone - Reviewer) ⚠️ Review applies to b5b6ade
Alex Macdonald (@aptmac - Committer) ⚠️ Review applies to 3d0d3e5

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/306/head:pull/306
$ git checkout pull/306

Update a local copy of the PR:
$ git checkout pull/306
$ git pull https://git.openjdk.java.net/jmc pull/306/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 306

View PR using the GUI difftool:
$ git pr show -t 306

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/306.diff

bridgekeeper · 2021-09-09T16:34:07Z

👋 Welcome back cimi! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-09-10T13:30:38Z

@cimi this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout cimi/websocket-server
git fetch https://git.openjdk.java.net/jmc master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

mlbridge · 2021-09-13T10:33:04Z

Webrevs

bric3

Nice stepping stone toward flexible UIs.
There's still some println here and there ;)
Stopping the websocket server is not yet implemented.

bric3 · 2021-09-14T11:40:28Z

...openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/JfrEditor.java

+		}
+		websocketServerEnabledListener = e -> {
+			if (e.getProperty().equals(PreferenceKeys.PROPERTY_ENABLE_WEBSOCKET_SERVER)) {
+				if ((Boolean) e.getNewValue()) {


For readability purpose maybe this code could benefit from an explicit equality check

This is refactored, we don't use a boolean setting to see if we should enable the server.

bric3 · 2021-09-14T11:56:11Z

...htrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/websocket/WebsocketServer.java

+import org.openjdk.jmc.flightrecorder.serializers.json.IItemCollectionJsonSerializer;
+
+public class WebsocketServer {
+	private static int PORT = 8029;


I think the port should be configurable.

This is done now - instead of a checkbox to enable the server, users need to pick a port number with 0 being the default value. When 0 is selected the webserver is disabled.

We had a discussion about this and @jpbempel pointed out that 0 is a valid port number that means the operating system should pick an open port. Since we don't display the port number in the UI anywhere, binding to a random port will be confusing for the user because they won't know what to connect to 😃 So I think it's ok to keep 0 as the disabled value, what do you think?

In general I think it's ok to allow automatic port binding as long as this information is being sent back to the user. But for this use case it's not critical, so using zero to disable the server is OK for me. And if this prove to be too much misleading this can be changed without too much effort.

bric3 · 2021-09-14T12:08:45Z

...htrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/websocket/WebsocketServer.java

+	private void startServer() {
+		Server server = new Server();
+		ServerConnector connector = new ServerConnector(server);
+		connector.setPort(PORT);


I'm not quite sure how this feature will be used, but can it make sense to only bind to locahost ?

👍 Yup, definitely! This was in the plan 😉 it's done now.

bric3 · 2021-09-14T12:14:09Z

releng/third-party/pom.xml

@@ -45,8 +46,10 @@
 		<hdrhistogram.version>2.1.12</hdrhistogram.version>
 		<jakarta.activation.version>1.2.1</jakarta.activation.version>
 		<jakarta.mail.version>1.6.5</jakarta.mail.version>
+		<javax.websocket.version>1.1</javax.websocket.version>
+		<jetty.version>9.4.43.v20210629</jetty.version>


I have seen in the PR description this codes needs to stick with 9.4.43…. But I noticed the coordinate of the artefacts may have changed after version 9 (group:org.eclipse.jetty.websocket, artefact: wesocket-server -> websocket-jetty-server). I haven't yet checked the dependencies in the POM, maybe that helps.

By the way there's a more recent version of jetty 11.0.6.

Interesting, thanks for pointing this out!

I picked 9.4.43 because it was the latest version I saw here https://mvnrepository.com/artifact/org.eclipse.jetty.websocket/websocket-server I didn't notice the rename for later versions. 😭

Maybe I can get rid of the dependency issues if I just use 10.0.5 which is already required by something (I don't know what) in the uitests.

😭 😭 I've just tried using 10.0.5 and I get this error:

Caused by: org.osgi.framework.BundleException: Bundle org.openjdk.jmc.console.agent cannot be resolved:org.openjdk.jmc.console.agent [130] Unresolved requirement: Require-Bundle: org.openjdk.jmc.flightrecorder.ui -> Bundle-SymbolicName: org.openjdk.jmc.flightrecorder.ui; bundle-version="8.2.0.qualifier"; singleton:="true" org.openjdk.jmc.flightrecorder.ui [132] Unresolved requirement: Require-Bundle: org.eclipse.jetty.websocket-api -> Bundle-SymbolicName: org.eclipse.jetty.websocket-api; bundle-version="1.1.2" org.eclipse.jetty.websocket-api [93] Unresolved requirement: Require-Capability: osgi.extender; filter:="(osgi.extender=osgi.serviceloader.processor)"

It seems this needs SPI Fly now...

So it seems you finally go this working :)

bric3 · 2021-09-15T07:22:00Z

releng/platform-definitions/platform-definition-2021-03/platform-definition-2021-03.target

@@ -46,6 +46,9 @@
            <unit id="org.adoptopenjdk.jemmy-browser" version="2.0.0"/>
            <unit id="org.adoptopenjdk.jemmy-core" version="2.0.0"/>
            <unit id="org.adoptopenjdk.jemmy-swt" version="2.0.0"/>
+            <unit id="javax.servlet-api" version="3.1.0"/>
+            <unit id="javax.websocket-api" version="1.1.0"/>
+            <unit id="org.eclipse.jetty.websocket.server" version="9.4.43.v20210629"/>


It seems there's still references to jetty 9 in older platform definition.

I'm not familiar with eclipse based applications, but should those target be removed once the platform is upgraded to a more recent version ?

🙈 Cleaned up now, I was just fiddling around with this config trying to get the dependencies to resolve properly and I was just updating the active one.

bric3 · 2021-09-15T07:22:46Z

releng/platform-definitions/platform-definition-2021-06/platform-definition-2021-06.target

+            <unit id="org.eclipse.jetty.websocket.servlet" version="10.0.5"/>
+            <unit id="org.eclipse.jetty.websocket.javax.server" version="10.0.5"/>
+            <unit id="org.apache.aries.spifly.dynamic.bundle" version="1.3.4"/>
+            .servlet-api_4.0.6.jar


I think this shouldn't be there ;)

Oops 😂 glad it didn't complain...

application/org.openjdk.jmc.feature.flightrecorder/feature.xml

....jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/FlightRecorderUI.java

jpbempel · 2021-09-20T15:37:12Z

...openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/JfrEditor.java

@@ -123,7 +126,29 @@ public JfrEditor() {
 				}
 			}
 		};
+		if (FlightRecorderUI.getDefault().isWebsocketServerEnabled()) {
+			long websocketServerPort = FlightRecorderUI.getDefault().getWebsocketPort();


why long for the variable type? it forces you to cast to int when calling WebsocketServer constructor below while getWebsocketPort() returns int anyway

This is a leftover after refactoring, I was initially using IQuantity for the port because other numeric configs were using that, then I changed to long and finally to int. Good spot, fixed now!

...trecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/messages/internal/Messages.java

...ightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/preferences/GeneralPage.java

...i/src/main/resources/org/openjdk/jmc/flightrecorder/ui/messages/internal/messages.properties

releng/platform-definitions/platform-definition-2019-12/platform-definition-2019-12.target

releng/third-party/pom.xml

jpbempel · 2021-09-20T15:49:30Z

...htrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/websocket/WebsocketServer.java

+	private List<WebsocketConnectionHandler> handlers = new ArrayList<>();
+	private List<WebsocketConnectionHandler> treeHandlers = new ArrayList<>();
+	private List<WebsocketConnectionHandler> graphHandlers = new ArrayList<>();


Since there is more than one thread involved into accessing those handlers I would use CopyOnWriteArrayList to be safe on the access.

aptmac · 2021-11-16T19:18:40Z

I've updated the Eclipse launchers to include the additional asm plugins and now I'm no longer seeing startup errors, everything seems to work fine. I've added a commit with these changes.

I think these look good, after fighting with my Eclipse for a bit I was able to verify that the additional plugins here get it working.

Having said that, I wonder if the JMC-Eclipse-JDK-8.launch should be removed.. the jetty dependencies are having a fit because they require jdk 11, and using certain target platforms it's not even possible to launch JMC with jdk 8 anyways.

openjdk · 2021-11-25T21:25:20Z

⚠️ @cimi the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout cimi/websocket-server
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2021-11-25T21:25:24Z

@cimi This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

7198: Add websocket server that pushes data on selection updates

Reviewed-by: hirt, aptmac

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@thegreystone, @aptmac) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

cimi · 2021-11-26T22:21:51Z

/integrate

openjdk · 2021-11-26T22:22:58Z

@cimi
Your change (at version b120657) is now ready to be sponsored by a Committer.

cimi · 2021-11-29T12:31:18Z

/integrate

openjdk · 2021-11-29T12:32:55Z

@cimi
Your change (at version 63817ca) is now ready to be sponsored by a Committer.

thegreystone · 2021-11-29T13:08:40Z

/sponsor

openjdk · 2021-11-29T13:09:26Z

Going to push as commit ca20442.

openjdk · 2021-11-29T13:09:49Z

@thegreystone @cimi Pushed as commit ca20442.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Add setting to enable websocket server in preferences

fdaecb3

openjdk bot added the merge-conflict label Sep 10, 2021

cimi force-pushed the cimi/websocket-server branch from d023281 to a098a18 Compare September 10, 2021 13:31

openjdk bot removed the merge-conflict label Sep 10, 2021

cimi force-pushed the cimi/websocket-server branch from a098a18 to 4e2a004 Compare September 10, 2021 17:34

cimi added 4 commits September 10, 2021 18:45

Add dependency on javax.websocket and eclipse jetty in flightrecorder.ui

349681e

Add listener in JfrEditor for websocket configuration changes

6b02505

Add proof of concept websocket server implementation

7c66929

Refactor WebsocketServer and embed inside JfrEditor

6ac7278

cimi force-pushed the cimi/websocket-server branch from 4e2a004 to 6ac7278 Compare September 10, 2021 17:45

cimi added 2 commits September 11, 2021 11:42

Remove explicit dependency on javax.servlet-api

7cb3433

Require specific jetty bundle version to avoid conflict in ui tests

8f3d3ce

cimi marked this pull request as ready for review September 13, 2021 10:28

openjdk bot added the rfr label Sep 13, 2021

bric3 reviewed Sep 14, 2021

View reviewed changes

cimi force-pushed the cimi/websocket-server branch from 2077969 to 16c3ca7 Compare September 14, 2021 18:37

bric3 reviewed Sep 15, 2021

View reviewed changes

cimi added 3 commits September 15, 2021 12:24

Upgrade jetty to 10.0.5 and add spifly

75a7c58

Replace prints in websocket server with logger

3f5dc4b

Make port a constructor argument for WebsocketServer

b1c86c0

cimi force-pushed the cimi/websocket-server branch from 16c3ca7 to 15f9f81 Compare September 15, 2021 11:24

Make websocket server port configurable

eb04ded

cimi force-pushed the cimi/websocket-server branch from 15f9f81 to eb04ded Compare September 15, 2021 11:49

cimi added 2 commits September 16, 2021 22:01

Revert whitespace changes in core/features.xml

857339e

Add tree and graph handlers to websocket server

32d530b

bric3 approved these changes Sep 17, 2021

View reviewed changes

jpbempel reviewed Sep 20, 2021

View reviewed changes

thegreystone changed the title ~~7198: add websocket server that pushes data on selection updates~~ 7198: Add websocket server that pushes data on selection updates Nov 25, 2021

openjdk bot added the merge-conflict label Nov 25, 2021

thegreystone approved these changes Nov 25, 2021

View reviewed changes

Merge branch 'master' into cimi/websocket-server

3d0d3e5

openjdk bot added ready and removed merge-conflict labels Nov 25, 2021

aptmac approved these changes Nov 25, 2021

View reviewed changes

Update full name

b120657

openjdk bot added the sponsor label Nov 26, 2021

Merge branch 'master' into cimi/websocket-server

5f544a3

openjdk bot removed the sponsor label Nov 29, 2021

Update full name

63817ca

openjdk bot added the sponsor label Nov 29, 2021

openjdk bot closed this Nov 29, 2021

openjdk bot added integrated and removed ready rfr sponsor labels Nov 29, 2021

cimi mentioned this pull request Jan 9, 2022

7494: Unable to view JMC Help Contents #347

Closed

2 tasks

cimi deleted the cimi/websocket-server branch January 10, 2022 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7198: Add websocket server that pushes data on selection updates #306

7198: Add websocket server that pushes data on selection updates #306

cimi commented Sep 9, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 9, 2021

openjdk bot commented Sep 10, 2021

mlbridge bot commented Sep 13, 2021 •

edited

Loading

bric3 left a comment

bric3 Sep 14, 2021

cimi Sep 16, 2021

bric3 Sep 14, 2021

cimi Sep 16, 2021

bric3 Sep 17, 2021

bric3 Sep 14, 2021

cimi Sep 16, 2021

bric3 Sep 14, 2021

cimi Sep 14, 2021

cimi Sep 14, 2021

bric3 Sep 15, 2021

bric3 Sep 15, 2021

cimi Sep 16, 2021

bric3 Sep 15, 2021

cimi Sep 16, 2021

jpbempel Sep 20, 2021

cimi Sep 20, 2021

jpbempel Sep 20, 2021

aptmac commented Nov 16, 2021

openjdk bot commented Nov 25, 2021

openjdk bot commented Nov 25, 2021 •

edited

Loading

cimi commented Nov 26, 2021

openjdk bot commented Nov 26, 2021

cimi commented Nov 29, 2021

openjdk bot commented Nov 29, 2021

thegreystone commented Nov 29, 2021

openjdk bot commented Nov 29, 2021

openjdk bot commented Nov 29, 2021

7198: Add websocket server that pushes data on selection updates #306

7198: Add websocket server that pushes data on selection updates #306

Conversation

cimi commented Sep 9, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 9, 2021

openjdk bot commented Sep 10, 2021

mlbridge bot commented Sep 13, 2021 • edited Loading

Webrevs

bric3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aptmac commented Nov 16, 2021

openjdk bot commented Nov 25, 2021

openjdk bot commented Nov 25, 2021 • edited Loading

cimi commented Nov 26, 2021

openjdk bot commented Nov 26, 2021

cimi commented Nov 29, 2021

openjdk bot commented Nov 29, 2021

thegreystone commented Nov 29, 2021

openjdk bot commented Nov 29, 2021

openjdk bot commented Nov 29, 2021

cimi commented Sep 9, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 13, 2021 •

edited

Loading

openjdk bot commented Nov 25, 2021 •

edited

Loading