RFR: 6996: Properly support converters

[Gunnar Morling]

On Wed, 25 Nov 2020 21:18:37 GMT, Marcus Hirt <hirt at openjdk.org> wrote:

> Including simple test to verify that the resulting instrumented methods are ok.

A few comments/suggestions inline. Some surely based on my lack of familiarity with the code base, so bear with me ;)

agent/src/main/java/org/openjdk/jmc/agent/impl/AbstractConvertable.java line 1:

> 1: package org.openjdk.jmc.agent.impl;

License header missing?

agent/src/main/java/org/openjdk/jmc/agent/converters/FileConverter.java line 51:

> 49: 			return file.getCanonicalPath();
> 50: 		} catch (Throwable e) {
> 51: 			Agent.getLogger().warning("Agent failed to convert file to String: " + e.getMessage());

Should propagate the original exception instead of just its message?

agent/src/main/java/org/openjdk/jmc/agent/impl/ResolvedConvertable.java line 1:

> 1: package org.openjdk.jmc.agent.impl;

License header missing?

agent/src/main/java/org/openjdk/jmc/agent/impl/ResolvedConvertable.java line 10:

> 8: public class ResolvedConvertable extends AbstractConvertable implements Convertable {
> 9: 	private final static String CONVERTER_METHOD = "convert";
> 10: 	private final transient Class<?> converterClass;

When is this ResolvedConvertable serialized, i.e. why is this transient?

agent/src/main/java/org/openjdk/jmc/agent/impl/ResolvedConvertable.java line 41:

> 39: 		}
> 40: 		for (Method m : converterClass.getDeclaredMethods()) {
> 41: 			if (CONVERTER_METHOD.equals(m.getName())) {

Should this not also take the parameter type into account? So to make sure multiple `convert()` methods for different types declared on the same class are invoked correctly based on assignability.

agent/src/test/resources/org/openjdk/jmc/agent/converters/test/jfrprobes_template.xml line 86:

> 84: 						</description>
> 85: 						<contenttype>None</contenttype>
> 86: 						<converter>org.openjdk.jmc.agent.converters.test.GurkConverterInt

Thinking more about this, would it actually make more sense to specify a _method_ here? In the end, that's what we want to identify. That'd allow for usage of existing methods named other than `convert()`. WDYT?

core/tests/org.openjdk.jmc.flightrecorder.rules.jdk.test/baseline/Generated_JfrRuleBaseline.xml line 1:

> 1: <?xml version="1.0" encoding="UTF-8" standalone="no"?>

What's the purpose of this and the next file?

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

PR: https://git.openjdk.java.net/jmc/pull/168