RFR: 7455: Add support for jolokia JMX service connection [v16]

[Alex Macdonald]

On Sat, 4 May 2024 08:26:29 GMT, Martin Skarsaune <duke at openjdk.org> wrote:

>> Replaces parts of https://github.com/openjdk/jmc/pull/332
>
> Martin Skarsaune has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Revert "JMC-7455: Actually measure coverage of jolokia"
>    
>    This reverts commit 17a7eaf8e3e8c4da828633d2d0e1484a47cb4ad3.
>  - Revert "JMC-7455: Attempt to use late property resolution for surefireArgLine"
>    
>    This reverts commit 13210f66dcf56f7790d26397ca00ae86acfb58ed.

Overall this looks good, I left some comments inline.

I rebased this PR ontop of https://github.com/openjdk/jmc/pull/566 so I could review it, and once pull/566 is in I think the only required change will be to the dependencies in the 2024-03 platform-definition.

Thinking longer term it would be nice to start capturing Jolokia-related enhancements in a ticket or doc of some sort. Things like, at the moment when creating a jvm connection the default is the jmx:rmi, and if I write a Jolokia service url and then double-click that button then it re-writes it using jmx:rmi. Just thinking that this is a neat feature and it'd be nice to have it easier to use from a ui perspective. But that's later work.

application/org.openjdk.jmc.jolokia/META-INF/MANIFEST.MF line 5:

> 3: Bundle-Name: %Bundle-Name
> 4: Bundle-SymbolicName: org.openjdk.jmc.jolokia;singleton:=true
> 5: Bundle-Version: 9.0.0.qualifier

Update to 9.1.0.qualifier

application/org.openjdk.jmc.jolokia/src/main/java/org/openjdk/jmc/jolokia/JmcJolokiaJmxConnection.java line 110:

> 108: 		try {
> 109: 			localInfo = ManagementFactory.getPlatformMBeanServer().getMBeanInfo(name);
> 110: 		} catch (Exception | NoClassDefFoundError ignore) {

Does this throw a `NoClassDefFoundError`?  Took a look at https://docs.oracle.com/en/java/javase/17/docs/api/java.management/javax/management/MBeanServer.html#getMBeanInfo(javax.management.ObjectName) and it wasn't in their list of throws. If we're catching Exception anyways and not doing anything specific for the `NoClassDefFoundError` then this could probably just catch Exception instead.

application/org.openjdk.jmc.jolokia/src/main/java/org/openjdk/jmc/jolokia/JmcJolokiaJmxConnection.java line 130:

> 128: 					}
> 129: 				}
> 130: 

Empty space

application/org.openjdk.jmc.jolokia/src/main/java/org/openjdk/jmc/jolokia/messages.properties line 1:

> 1: JmcJolokiaJmxConnectionProvider.0=I only serve Jolokia connections

I think it'd be nicer if the string name here reflected the message that it's providing. Something along the lines of: `JMC_JOLOKIA_JMX_CONNECTION_PROVIDER_EXCEPTION_MSG`

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

> 3: Bundle-Name: Jolokia plugin test
> 4: Bundle-SymbolicName: org.openjdk.jmc.jolokia.test;singleton:=true
> 5: Bundle-Version: 9.0.0.qualifier

Update to 9.1.0.qualifier

application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java line 94:

> 92: 					.getAttributes()) {
> 93: 				if (!unsafeAttributes.contains(attributeInfo.getName())) {
> 94: 					System.out.println("Getting attribute " + objectName + "/" + attributeInfo.getName());

Would prefer to use a logger here, although not completely sure we need to log? Could probably use an assertion to verify the attributes are as expected.

application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java line 99:

> 97: 			}
> 98: 		}
> 99: 

Extra empty space

application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java line 105:

> 103: 	public void testExecuteOperation() throws InstanceNotFoundException, MalformedObjectNameException, MBeanException,
> 104: 			ReflectionException, MalformedURLException, IOException {
> 105: 		jolokiaConnection.invoke(new ObjectName("java.lang:type=Memory"), "gc", new Object[0], new String[0]);

Is there an assertion that could be made on the object returned from `invoke`? Or is the idea that in the case where an invoke isn't possible it will throw one of those exceptions and get caught to fail the test?

application/tests/org.openjdk.jmc.jolokia.test/src/test/java/org/openjdk/jmc/jolokia/JolokiaTest.java line 116:

> 114: 		jolokiaConnection.setAttribute(objectName, new Attribute(attribute, true));
> 115: 		Assert.assertEquals(true, jolokiaConnection.getAttribute(objectName, attribute));
> 116: 

Extra empty space

core/org.openjdk.jmc.rjmx.common/src/main/java/org/openjdk/jmc/rjmx/common/internal/RJMXConnection.java line 592:

> 590: 	private void connectJmxConnector(JMXServiceURL serviceURL, Map<String, Object> env) throws IOException {
> 591: 		if (m_jmxc == null) {
> 592: 			//This will use Java's standard connector, which will not take JMC extensions into account

Space after the `//`

-------------

PR Review: https://git.openjdk.org/jmc/pull/548#pullrequestreview-2104690303
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631281936
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631345593
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631345953
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631295768
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631282336
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631312937
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631297095
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631315378
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631296908
PR Review Comment: https://git.openjdk.org/jmc/pull/548#discussion_r1631297865