7455: Add support for jolokia JMX service connection #548

skarsaune · 2024-01-25T17:33:09Z

Replaces parts of #332

Progress

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)

Reviewers

Alex Macdonald (@aptmac - Reviewer) ⚠️ Review applies to 0fc1253b

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

bridgekeeper · 2024-01-25T17:34:06Z

👋 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.

openjdk · 2024-01-25T17:35:11Z

@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

mlbridge · 2024-01-25T17:37:50Z

Webrevs

openjdk · 2024-01-25T17:39:56Z

@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.

aptmac

Gave this a quick read through, the builds aren't happy because a couple of the imports need to be updated in ServerConnectionDescriptor and JolokiaTest; both need IConnectionDescriptor and IServerDescriptor to be updated to come from rjmx.common. After these changes it looks like everything builds and tests pass.

I've commented on a handful more nits I found, but haven't actually tested the functionality yet so I can't comment on anything else.

Another PR where the check license script isn't happy though, I'll have to a take a look at why it's trying to check all these different files that weren't touched by your commit.

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

application/org.openjdk.jmc.jolokia/build.properties

...rg.openjdk.jmc.jolokia/src/main/java/org/openjdk/jmc/jolokia/ServerConnectionDescriptor.java

application/tests/org.openjdk.jmc.jolokia.test/META-INF/MANIFEST.MF

...on/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java

application/tests/org.openjdk.jmc.jolokia.test/pom.xml

...on/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java

skarsaune · 2024-01-26T06:58:54Z

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

pom.xml

skarsaune · 2024-01-30T07:41:04Z

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

skarsaune · 2024-02-04T16:32:19Z

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.

…source packabge

skarsaune · 2024-02-04T21:28:25Z

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 ?

skarsaune · 2024-02-06T08:48:44Z

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 (?)

aptmac · 2024-02-06T21:05:21Z

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

…connection

skarsaune · 2024-06-14T19:30:38Z

@aptmac I believe the comments should be addressed now.

aptmac · 2024-06-14T19:32:05Z

@aptmac I believe the comments should be addressed now.

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

aptmac

lgtm. The copyright script is probably just complaining now that upstream/master has new commits, I tried a rebase on-top of master and running the script and it came back clean. For completeness you could just rebase one last time to re-run that script, but should be good to go afterwards.

skarsaune · 2024-06-18T06:12:56Z

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

skarsaune · 2024-07-01T12:24:27Z

@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?

aptmac · 2024-07-01T19:46:34Z

@skarsaune Not sure why the instructions got buried, but refer to this comment from the Skara bot: #548 (comment)

Basically you type /integrate, Skara will wait for a sponsor (I can do that), and then the changes will be pushed as one commit.

Once it's in feel free to rebase and open up new PRs, and if you need any help creating issues in JIRA let me know as well.

skarsaune · 2024-07-02T08:27:40Z

/integrate

openjdk · 2024-07-02T08:29:01Z

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

aptmac · 2024-07-02T12:59:27Z

/sponsor

openjdk · 2024-07-02T12:59:55Z

Going to push as commit 8332466.
Since your change was applied there have been 2 commits pushed to the master branch:

73ad688: 8223: Copyright script is failing
280f173: 8231: Add version to build tarball names

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-02T13:00:04Z

@aptmac @skarsaune Pushed as commit 8332466.

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

openjdk bot added merge-conflict rfr labels Jan 25, 2024

JMC-7455: Add plugin with support for Jolokia connections

ffe7eab

skarsaune force-pushed the jolokia-support-only branch from d477ccf to ffe7eab Compare January 25, 2024 17:39

openjdk bot removed the merge-conflict label Jan 25, 2024

skarsaune mentioned this pull request Jan 25, 2024

7455: Add support for jolokia JMX service connection #332

Closed

2 tasks

skarsaune changed the title ~~JMC-7455: Add plugin with support for Jolokia connections~~ 7455: Add support for jolokia JMX service connection Jan 25, 2024

aptmac reviewed Jan 25, 2024

View reviewed changes

skarsaune added 5 commits January 26, 2024 10:33

JMC-7455: Fix rjmx imports after rebasing with master

07b4f90

JMC-7455: Fix copyright notices (change year) in Jolokia plugin

92c86ca

JMC-7455: Fixed whitespace of feature declaration

0658a01

JMC-7455: Fix copy paste and outdated errors in Jolokia plugin manifest

0fc546a

JMC-7455: Ensure that jolokia version is set

dc588b6

skarsaune commented Jan 26, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

JMC-7455: Clean up some test comments

d444888

skarsaune added 2 commits February 3, 2024 09:55

JMC-7455: Provide env map for discovered jolokia connections

f64b599

Merge remote-tracking branch 'origin/master' into jolokia-support-only

e2c2275

skarsaune added 2 commits February 4, 2024 17:41

JMC-7455: Remove explicit package imports, as they are exported from …

b41db31

…source packabge

JMC-7455: Include jolokia in latest platform definition

95f48a9

JMC-7455: fix copyrigt notice

a7774a0

skarsaune added 8 commits June 14, 2024 10:47

JMC-7455: Compare certiain retrieved attributes directly with local …

816b353

…connection

JMC-7455: Use explicit imports. Exclude attribute

0af1024

JMC-7455: Use more meaningful message key

333fb18

JMC-7455: Spotless violation fixes

a68aad5

JMC-7455: Need to exclude one more attribute in test

6099e94

JMC-7455: Exclude one more unsafe attribute

0a6c9e8

JMC-7455: Log attribute test data

77b6de7

JMC-7455: Spotless corrections.

da8949b

aptmac approved these changes Jun 17, 2024

View reviewed changes

openjdk bot added the ready label Jun 17, 2024

skarsaune added 2 commits June 18, 2024 07:46

Merge remote-tracking branch 'origin/master' into jolokia-support-only

bea2516

JMC-7455: Added jolokia to platform 2024-03

0fc1253

aptmac approved these changes Jun 18, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into jolokia-support-only

eb5ac76

openjdk bot added the sponsor label Jul 2, 2024

openjdk bot added the integrated label Jul 2, 2024

openjdk bot closed this Jul 2, 2024

openjdk bot removed ready rfr sponsor labels Jul 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7455: Add support for jolokia JMX service connection #548

7455: Add support for jolokia JMX service connection #548

skarsaune commented Jan 25, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 25, 2024

openjdk bot commented Jan 25, 2024

mlbridge bot commented Jan 25, 2024 •

edited

Loading

openjdk bot commented Jan 25, 2024

aptmac left a comment

skarsaune commented Jan 26, 2024

skarsaune commented Jan 30, 2024

skarsaune commented Feb 4, 2024

skarsaune commented Feb 4, 2024

skarsaune commented Feb 6, 2024 •

edited

Loading

aptmac commented Feb 6, 2024 •

edited

Loading

skarsaune commented Jun 14, 2024

aptmac commented Jun 14, 2024

aptmac left a comment

skarsaune commented Jun 18, 2024

skarsaune commented Jul 1, 2024

aptmac commented Jul 1, 2024

skarsaune commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

aptmac commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

7455: Add support for jolokia JMX service connection #548

7455: Add support for jolokia JMX service connection #548

Conversation

skarsaune commented Jan 25, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 25, 2024

openjdk bot commented Jan 25, 2024

mlbridge bot commented Jan 25, 2024 • edited Loading

Webrevs

openjdk bot commented Jan 25, 2024

aptmac left a comment

Choose a reason for hiding this comment

skarsaune commented Jan 26, 2024

skarsaune commented Jan 30, 2024

skarsaune commented Feb 4, 2024

skarsaune commented Feb 4, 2024

skarsaune commented Feb 6, 2024 • edited Loading

aptmac commented Feb 6, 2024 • edited Loading

skarsaune commented Jun 14, 2024

aptmac commented Jun 14, 2024

aptmac left a comment

Choose a reason for hiding this comment

skarsaune commented Jun 18, 2024

skarsaune commented Jul 1, 2024

aptmac commented Jul 1, 2024

skarsaune commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

aptmac commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

openjdk bot commented Jul 2, 2024

skarsaune commented Jan 25, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 25, 2024 •

edited

Loading

skarsaune commented Feb 6, 2024 •

edited

Loading

aptmac commented Feb 6, 2024 •

edited

Loading