jmc 7455: Add support for jolokia JMX service connection

Replaces parts of https://github.com/openjdk/jmc/pull/332

Progress

[x] Commit message must refer to an issue
[ ] Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JMC-7455: Add support for jolokia JMX service connection (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jmc.git pull/548/head:pull/548
$ git checkout pull/548

Update a local copy of the PR:
$ git checkout pull/548
$ git pull https://git.openjdk.org/jmc.git pull/548/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 548

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jmc/pull/548.diff

Webrev

Link to Webrev Comment

Jan 25 '24 17:01 skarsaune

:wave: Welcome back skarsaune! 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.

Jan 25 '24 17:01 bridgekeeper[bot]

@skarsaune 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 jolokia-support-only
git fetch https://git.openjdk.org/jmc.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Jan 25 '24 17:01 openjdk[bot]

Webrevs

Jan 25 '24 17:01 mlbridge[bot]

@skarsaune Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

Jan 25 '24 17:01 openjdk[bot]

Great, that is very helpful. Thanks for the quick feedback.

Jan 26 '24 06:01 skarsaune

Update: Something does not seem to work after updating to master. I will debug a bit, but do not have the time the next couple of days

Jan 30 '24 07:01 skarsaune

Ok, so at least in JMC 7.1.0 and 8.3.0 this use to work by the fact that the plugin uses the extension point for JMX protocol: https://github.com/skarsaune/jmc/blob/e2c2275d05ee2e2215ea2a48b720ef2fef456810/application/org.openjdk.jmc.jolokia/plugin.xml#L43 However this no longer seems to work in 9.0.0 :(?). Does anyone know about particular changes in this area?

For instance when trying to connect to a JMC console from the JVM browser I get this:

!ENTRY org.openjdk.jmc.console.ui 4 4 2024-02-04 17:14:27.250 !MESSAGE Could not connect to 127.0.0.1:8778 : Unsupported protocol: jolokia !STACK 0 org.openjdk.jmc.rjmx.common.ConnectionException caused by java.net.MalformedURLException: Unsupported protocol: jolokia at org.openjdk.jmc.rjmx.common.internal.RJMXConnection.connect(RJMXConnection.java:364) at org.openjdk.jmc.rjmx.internal.ServerHandle.doConnect(ServerHandle.java:121) at org.openjdk.jmc.rjmx.internal.ServerHandle.connect(ServerHandle.java:111) at org.openjdk.jmc.console.ui.editor.internal.ConsoleEditor$ConnectJob.run(ConsoleEditor.java:99) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63) Caused by: java.net.MalformedURLException: Unsupported protocol: jolokia at java.management/javax.management.remote.JMXConnectorFactory.newJMXConnector(JMXConnectorFactory.java:366) at org.openjdk.jmc.rjmx.common.internal.RJMXConnection.connectJmxConnector(RJMXConnection.java:591) at org.openjdk.jmc.rjmx.common.internal.RJMXConnection.establishConnection(RJMXConnection.java:572) at org.openjdk.jmc.rjmx.common.internal.RJMXConnection.connect(RJMXConnection.java:357) ... 4 more

This appears to use Javas native JMXConnectorFactory directly . This may fail for a number of reasons:

Jolokia does support service discovery, but only if the Jolokia library jar is on the classpath/same classsloader.
Furthermore this rules out providing a JMC specific connection that could allow us to put in optimizations for JMC.

Anyone know more about the setup of connections or how this may have changed recently.

Feb 04 '24 16:02 skarsaune

Ok, so at least in JMC 7.1.0 and 8.3.0 this use to work by the fact that the plugin uses the extension point for JMX protocol: https://github.com/skarsaune/jmc/blob/e2c2275d05ee2e2215ea2a48b720ef2fef456810/application/org.openjdk.jmc.jolokia/plugin.xml#L43 However this no longer seems to work in 9.0.0 :(?). Does anyone know about particular changes in this area?

For instance when trying to connect to a JMC console from the JVM browser I get this:

!ENTRY org.openjdk.jmc.console.ui 4 4 2024-02-04 17:14:27.250 !MESSAGE Could not connect to 127.0.0.1:8778 : Unsupported protocol: jolokia !STACK 0 org.openjdk.jmc.rjmx.common.ConnectionException caused by java.net.MalformedURLException: Unsupported protocol: jolokia at org.openjdk.jmc.rjmx.common.internal.RJMXConnection.connect(RJMXConnection.java:364) at org.openjdk.jmc.rjmx.internal.ServerHandle.doConnect(ServerHandle.java:121) at org.openjdk.jmc.rjmx.internal.ServerHandle.connect(ServerHandle.java:111) at org.openjdk.jmc.console.ui.editor.internal.ConsoleEditor$ConnectJob.run(ConsoleEditor.java:99) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63) Caused by: java.net.MalformedURLException: Unsupported protocol: jolokia at java.management/javax.management.remote.JMXConnectorFactory.newJMXConnector(JMXConnectorFactory.java:366) at org.openjdk.jmc.rjmx.common.internal.RJMXConnection.connectJmxConnector(RJMXConnection.java:591) at org.openjdk.jmc.rjmx.common.internal.RJMXConnection.establishConnection(RJMXConnection.java:572) at org.openjdk.jmc.rjmx.common.internal.RJMXConnection.connect(RJMXConnection.java:357) ... 4 more

This appears to use Javas native JMXConnectorFactory directly . This may fail for a number of reasons:

Jolokia does support service discovery, but only if the Jolokia library jar is on the classpath/same classsloader.

Furthermore this rules out providing a JMC specific connection that could allow us to put in optimizations for JMC.

Anyone know more about the setup of connections or how this may have changed recently.

Hmm, or is OsgiServicesJmxProviderProxy supposed to be picked up by JMXConnectorFactory, and that in turn will use the protocol providers registered in JMC?. Do you know @aptmac ?

Feb 04 '24 21:02 skarsaune

Update 2: So https://github.com/openjdk/jmc/blob/master/application/org.openjdk.jmc.rjmx.ext/META-INF/services/javax.management.remote.JMXConnectorProvider is not detected by javax.management.remote.JMXConnectorFactory' s classloader. I also tried to use org.openjdk.jmc.rjmx.common.internal.RJMXConnection 's classloader, but that no longer works as the .ext module is not visible from rjmx.common (?).

		Map<String, Object> envCopy = new HashMap<>(env);
		envCopy.put(JMXConnectorFactory.PROTOCOL_PROVIDER_CLASS_LOADER, this.getClass().getClassLoader());

Bottom line seems to me that the JMX protocol extension mechanism no longer works in JMC 9 (?)

Feb 06 '24 08:02 skarsaune

Update 2: So https://github.com/openjdk/jmc/blob/master/application/org.openjdk.jmc.rjmx.ext/META-INF/services/javax.management.remote.JMXConnectorProvider is not detected by javax.management.remote.JMXConnectorFactory' s classloader. I also tried to use org.openjdk.jmc.rjmx.common.internal.RJMXConnection 's classloader, but that no longer works as the .ext module is not visible from rjmx.common (?).
		Map<String, Object> envCopy = new HashMap<>(env);
		envCopy.put(JMXConnectorFactory.PROTOCOL_PROVIDER_CLASS_LOADER, this.getClass().getClassLoader());
Bottom line seems to me that the JMX protocol extension mechanism no longer works in JMC 9 (?)

Hi! What you mentioned about how the .ext module is not visible from common is what I think the issue here is. When bringing over the bulk of the rjmx code from jmc/application to jmc/core one of the problems I faced was how to get the Services loaded by the core-side code if they are only visible in jmc/application. For the rjmx services, I came up with an initializer class [0] that looks at the rjmx.service extension and adds them to a List to be consumed by ServerHandle when creating a ConnectionHandle. This way the services can be passed from jmc/application into jmc/core through the ConnectionHandle.

Unfortunately it looks like I only considered the case of the rjmx.service extension, and not rjmx.ext. I think the ServiceFactoryInitializer is probably the best place to start looking to fix this, and making sure it can also handle the other valid extension points.

Edit: I've opened a bug for this on the JMC jira: https://bugs.openjdk.org/browse/JMC-8178

[0] https://github.com/openjdk/jmc/blob/master/application/org.openjdk.jmc.rjmx/src/main/java/org/openjdk/jmc/rjmx/internal/ServiceFactoryInitializer.java#L49

Feb 06 '24 21:02 aptmac

@aptmac : I believe I have addressed all comments so far, apart from enabling coverage test (next on my list). Status: Test connection :white_check_mark: Do flight recording :white_check_mark: Open JMX console :red_circle: - here I only get a spinner. I have tried to debug, but I'm struggling a bit with my debug setup

Mar 09 '24 11:03 skarsaune

@aptmac : I believe I have addressed all comments so far, apart from enabling coverage test (next on my list). Status: Test connection ✅ Do flight recording ✅ Open JMX console 🔴 - here I only get a spinner. I have tried to debug, but I'm struggling a bit with my debug setup

Nice! I gave this a bit of a look and something interesting is going on but I can't tell just what yet. If I'm running the built application and connecting to jolokia I can confirm that JFR looks to be working correctly, and that the JMX Console pages are working.

But, if I'm running it through Eclipse using our run configurations then the JMX Console pages are all blank (and this is true for any JVM and not just the one with jolokia attached to it). At the moment it's not obvious to me what's happening, the ConsoleEditor is running through the code okay to create the pages but for some reason nothing is showing up. I'll poke around a bit more when I can find some time.

Mar 11 '24 15:03 aptmac

@aptmac : I believe I have addressed all comments so far, apart from enabling coverage test (next on my list). Status: Test connection ✅ Do flight recording ✅ Open JMX console 🔴 - here I only get a spinner. I have tried to debug, but I'm struggling a bit with my debug setup

Nice! I gave this a bit of a look and something interesting is going on but I can't tell just what yet. If I'm running the built application and connecting to jolokia I can confirm that JFR looks to be working correctly, and that the JMX Console pages are working.

But, if I'm running it through Eclipse using our run configurations then the JMX Console pages are all blank (and this is true for any JVM and not just the one with jolokia attached to it). At the moment it's not obvious to me what's happening, the ConsoleEditor is running through the code okay to create the pages but for some reason nothing is showing up. I'll poke around a bit more when I can find some time.

I think this may end up being due to the Eclipse 2023-12 update..?

I've been using Eclipse 2023-03 IDE this whole time (quite a bit behind ..) and just noticed that even on the master branch it wasn't showing the JMX Console when running JMC using a run configuration.

I updated to 2023-12 IDE and now the JMX Console pages are showing as normal, both on the master branch and with this PR applied.

The only thing I can think of right now is that Eclipse 2023-12 had changes to use jakarta.inject (https://eclipse.dev/eclipse/news/4.30/platform.php#support-jakarta-annotations), and the Console pages make use of the @Inject annotation to create the page content. When using a debugger, I can see that the console pages are created but the createPageContent is never hit. However, the other methods in the page class (OverviewTab.java for example) do get hit. This makes me think that the injection isn't working properly in versions prior to 2023-12 when running from Eclipse.

Try using the latest Eclipse and see if that fixes the problem for you. This might need an issue to be created for it on the JMC Jira.

Mar 11 '24 20:03 aptmac

@skarsaune 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:

7455: Add support for jolokia JMX service connection

Reviewed-by: 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 2 new commits pushed to the master branch:

73ad68826ef4b437a7556c18c21d284ab328d3f8: 8223: Copyright script is failing
280f173d55afbfd284163914e115e503042a4a8f: 8231: Add version to build tarball names

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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 (@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).

Mar 13 '24 20:03 openjdk[bot]

Any chance to see this feature in the next release ?

Mar 25 '24 12:03 RoiSoleil

Any chance to see this feature in the next release ?

This would be included in the next release when it's available, version 9.X.0

Mar 25 '24 13:03 aptmac

@aptmac : I have now upgraded to the latest version of Jolokia (whole new stream) , to make it easier to take advantage of upstream dependencies. In the process I was not able to get the discovery mechanism in as an OSGi dependency, however I felt it was better to cut scope and focus on the basics.

Would you be able to help me look at coverage support? If that is in place, then I think I can mark it as ready for review again. I have been clicking around the different

Mar 29 '24 18:03 skarsaune

@aptmac : I have now upgraded to the latest version of Jolokia (whole new stream) , to make it easier to take advantage of upstream dependencies. In the process I was not able to get the discovery mechanism in as an OSGi dependency, however I felt it was better to cut scope and focus on the basics.

Would you be able to help me look at coverage support? If that is in place, then I think I can mark it as ready for review again. I have been clicking around the different

Sounds good, I'll re-run this PR again next week and post back about the coverage and/or anything else that comes up.

Mar 29 '24 18:03 aptmac

I guess another note: The current way to hook the connector into RJMXConnection does work, and it was the cleanest way to hook it in that I came up with. However currently RJMXConnection does quite a lot based on the fact that it usually is an RMI connection. I suppose at some point it would be nice to better separate the protocol specific concerns.

Mar 30 '24 08:03 skarsaune

@aptmac : Did you get any time to look at it? I could have a crack at the coverage tests myself, if I could get some pointers or be directed to some doc. Primarily: What is the best way to run the coverage tests?

Apr 19 '24 08:04 skarsaune

@aptmac : Did you get any time to look at it? I could have a crack at the coverage tests myself, if I could get some pointers or be directed to some doc. Primarily: What is the best way to run the coverage tests?

Ah, no I hadn't found the time to look at it. I tried giving it a run but I seem to be running into some issues running the jolokia-jvm jar, I'll try to check it out again next week.

For the test coverage, you'll have to use the coverage maven profile to run it, so it'll be: mvn clean verify -P coverage

and then the coverage report will be in jmc/application/coverage.

In order for the jolokia test coverage to be included both org.openjdk.jmc.jolokia and org.openjdk.jmc.jolokia.test will need to be added to the coverage pom.xml.

org.openjdk.jmc.jolokia under here: https://github.com/openjdk/jmc/blob/master/application/coverage/pom.xml#L54, and org.openjdk.jmc.jolokia.test under here: https://github.com/openjdk/jmc/blob/master/application/coverage/pom.xml#L353, and then hopefully the coverage report should work using the maven profile.

Apr 19 '24 19:04 aptmac

Overall this is looking nice. The tests pass and I was able to connect over Jolokia to the test jvm and verify that the mbean browser & recordings work as expected. To test I added this to the test class:
diff --git a/application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java b/application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java
index 7b65e835..a245f4b1 100644
--- a/application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java
+++ b/application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java
@@ -81,7 +81,11 @@ public class JolokiaTest {
 		Awaitility.await().atMost(Duration.ofSeconds(15))//Note: hard code property to avoid module dependency on agent
 				.until(() -> (jolokiaUrl = System.getProperty("jolokia.agent")) != null);
 		jolokiaConnection = getJolokiaMBeanConnector();
-
+		try {
+			Thread.sleep(500000000);
+		} catch (Exception e) {
+			// do nothing, let's take a look at the jolokia ..
+		}
 	}
 
 	@Test
and then added a new connection and connected using the custom jmx service url.

What is your current plans for this PR? Did you want to try and get the JVM Discovery working before turning it from draft to ready for review? Or is the current functionality what you're currently hoping to have integrated?

Just to be sure I understood the code change: That was in order to manually test, right?

It seems some people would be very happy to have Jolokia support, so I suggest we push ahead with the connectivity. Discovery is more of a niche feature that I can continue working on (ran into some issues with the new 2.x modules there). I will mark as ready for review once I sorted the jolokia version issue you noticed and added coverage.

Apr 30 '24 07:04 skarsaune

Overall this is looking nice. The tests pass and I was able to connect over Jolokia to the test jvm and verify that the mbean browser & recordings work as expected. To test I added this to the test class:
diff --git a/application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java b/application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java
index 7b65e835..a245f4b1 100644
--- a/application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java
+++ b/application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java
@@ -81,7 +81,11 @@ public class JolokiaTest {
 		Awaitility.await().atMost(Duration.ofSeconds(15))//Note: hard code property to avoid module dependency on agent
 				.until(() -> (jolokiaUrl = System.getProperty("jolokia.agent")) != null);
 		jolokiaConnection = getJolokiaMBeanConnector();
-
+		try {
+			Thread.sleep(500000000);
+		} catch (Exception e) {
+			// do nothing, let's take a look at the jolokia ..
+		}
 	}
 
 	@Test
and then added a new connection and connected using the custom jmx service url. What is your current plans for this PR? Did you want to try and get the JVM Discovery working before turning it from draft to ready for review? Or is the current functionality what you're currently hoping to have integrated?
Just to be sure I understood the code change: That was in order to manually test, right?

Yeah, just incase anyone coming in wants to try it out/review without doing configuration locally. It was just a hack on my side to test this out a bit faster.

It seems some people would be very happy to have Jolokia support, so I suggest we push ahead with the connectivity. Discovery is more of a niche feature that I can continue working on (ran into some issues with the new 2.x modules there). I will mark as ready for review once I sorted the jolokia version issue you noticed and added coverage.

Sounds good to me, and just the connectivity would be enough to satisfy the description of JMC-7455, we can make another jira ticket to track the discover-ability afterwards.

Apr 30 '24 12:04 aptmac

@aptmac : Marked it as ready for review. You may see from the last commits that when I tried to get test coverage it broke the regular test run. Hence I reverted back. I see you are working on that in #562 .

May 06 '24 13:05 skarsaune

@skarsaune This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Jun 03 '24 17:06 bridgekeeper[bot]

@skarsaune This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Bah, sorry for the delay in the review. Trying to run JMC through the Eclipse IDE has been a dreadful experience the last little while, but looks to be fixed with: https://github.com/openjdk/jmc/pull/566

Once that is in I should have a much better time testing out this PR.

Jun 03 '24 17:06 aptmac

@aptmac I believe the comments should be addressed now.

Jun 14 '24 19:06 skarsaune

@aptmac I believe the comments should be addressed now.

Excellent. I'm off today but will take a look on Monday!

Jun 14 '24 19:06 aptmac

@aptmac : Merged in master and added jolokia to 2024-03.

Jun 18 '24 06:06 skarsaune

@aptmac : What is the process going forward here? I actually have a couple of follow up PR's where it would be good to get review started. How and when do PRs get merged?

Jul 01 '24 12:07 skarsaune

jmc jmc copied to clipboard

7455: Add support for jolokia JMX service connection

Progress

Issue

Reviewing

Webrev

Webrevs

jmc
jmc copied to clipboard