From a7fdf1a33111c79201d490f4d6dc3bdb9de2c8c7 Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 7 Nov 2024 14:29:44 +0100 Subject: [PATCH 1/6] make rmi instrumentation indy-compatible --- .../RmiContextPropagationInstrumentationModule.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/RmiContextPropagationInstrumentationModule.java b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/RmiContextPropagationInstrumentationModule.java index 20317582d5c8..bd09178336e8 100644 --- a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/RmiContextPropagationInstrumentationModule.java +++ b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/RmiContextPropagationInstrumentationModule.java @@ -20,13 +20,6 @@ public RmiContextPropagationInstrumentationModule() { super("rmi", "rmi-context-propagation"); } - @Override - public boolean isIndyModule() { - // java.lang.IllegalAccessError: class - // io.opentelemetry.javaagent.instrumentation.rmi.context.client.RmiClientContextInstrumentation$StreamRemoteCallConstructorAdvice (in unnamed module @0x740ee00f) cannot access class sun.rmi.transport.Connection (in module java.rmi) because module java.rmi does not export sun.rmi.transport to unnamed module @0x740ee00f - return false; - } - @Override public List typeInstrumentations() { return asList(new RmiClientContextInstrumentation(), new RmiServerContextInstrumentation()); From 1a9a687ff64722a61702489f59ae734c6c3eb810 Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Fri, 13 Dec 2024 18:19:00 +0100 Subject: [PATCH 2/6] add module opener --- ...ntextPropagationInstrumentationModule.java | 14 +++- .../ExperimentalInstrumentationModule.java | 15 ++++ .../javaagent/tooling/ModuleOpener.java | 76 +++++++++++++++++++ .../indy/IndyModuleRegistry.java | 37 +++++++++ 4 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java diff --git a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/RmiContextPropagationInstrumentationModule.java b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/RmiContextPropagationInstrumentationModule.java index bd09178336e8..deadedc02875 100644 --- a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/RmiContextPropagationInstrumentationModule.java +++ b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/RmiContextPropagationInstrumentationModule.java @@ -10,12 +10,17 @@ import com.google.auto.service.AutoService; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation; +import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule; import io.opentelemetry.javaagent.instrumentation.rmi.context.client.RmiClientContextInstrumentation; import io.opentelemetry.javaagent.instrumentation.rmi.context.server.RmiServerContextInstrumentation; +import java.util.Arrays; +import java.util.Collections; import java.util.List; +import java.util.Map; @AutoService(InstrumentationModule.class) -public class RmiContextPropagationInstrumentationModule extends InstrumentationModule { +public class RmiContextPropagationInstrumentationModule extends InstrumentationModule + implements ExperimentalInstrumentationModule { public RmiContextPropagationInstrumentationModule() { super("rmi", "rmi-context-propagation"); } @@ -24,4 +29,11 @@ public RmiContextPropagationInstrumentationModule() { public List typeInstrumentations() { return asList(new RmiClientContextInstrumentation(), new RmiServerContextInstrumentation()); } + + @Override + public Map> jpmsModulesToOpen() { + String witnessClass = "sun.rmi.transport.StreamRemoteCall"; + return Collections.singletonMap( + witnessClass, Arrays.asList("sun.rmi.server", "sun.rmi.transport")); + } } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java index af8d33d78659..3c404aa5e2c7 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java @@ -11,6 +11,7 @@ import io.opentelemetry.javaagent.extension.instrumentation.internal.injection.ClassInjector; import java.util.Collections; import java.util.List; +import java.util.Map; /** * This class is internal and is hence not for public use. Its APIs are unstable and can change at @@ -61,4 +62,18 @@ default String getModuleGroup() { default List agentPackagesToHide() { return Collections.emptyList(); } + + /** + * Some instrumentation need to access JPMS modules that are not accessible by default, this + * method provides a way to access those classes like the "--add-opens" JVM command.
+ * Map key is the name of a "witness class" belonging to the module that will be loaded and used + * to get a reference to the module.
+ * Map value is a list of packages to open in the module + * + * @return map of "witness class" name as key, list of packages as value. + */ + // TODO: copy the javadoc of the original implementation + default Map> jpmsModulesToOpen() { + return Collections.emptyMap(); + } } diff --git a/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java b/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java new file mode 100644 index 000000000000..81ce0e0a1398 --- /dev/null +++ b/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java @@ -0,0 +1,76 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.tooling; + +import static java.util.logging.Level.FINE; +import static java.util.logging.Level.WARNING; + +import java.lang.instrument.Instrumentation; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.logging.Logger; + +public class ModuleOpener { + + private static final Logger logger = Logger.getLogger(ModuleOpener.class.getName()); + + private ModuleOpener() {} + + /** + * Opens JPMS module to a class loader unnamed module + * + * @param classFromTargetModule class from target module + * @param openTo class loader to open module for + * @param packagesToOpen packages to open + */ + public static void open( + Instrumentation instrumentation, + Class classFromTargetModule, + ClassLoader openTo, + Collection packagesToOpen) { + + Module targetModule = classFromTargetModule.getModule(); + Module openToModule = openTo.getUnnamedModule(); + Set openToModuleSet = Collections.singleton(openToModule); + Map> missingOpens = new HashMap<>(); + for (String packageName : packagesToOpen) { + if (!targetModule.isOpen(packageName, openToModule)) { + missingOpens.put(packageName, openToModuleSet); + logger.log( + FINE, + () -> + String.format( + "Exposing package '%s' in module '%s' to module '%s'", + packageName, targetModule, openToModule)); + } + } + if (missingOpens.isEmpty()) { + return; + } + + if (!instrumentation.isModifiableModule(targetModule)) { + logger.log(WARNING, "Module '{}' can't be modified", targetModule); + return; + } + + try { + instrumentation.redefineModule( + targetModule, + Collections.emptySet(), // reads + Collections.>emptyMap(), // exports + missingOpens, // opens + Collections.>emptySet(), // uses + Collections., List>>emptyMap() // provides + ); + } catch (Exception e) { + logger.log(WARNING, "unable to redefine module", e); + } + } +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java index 7eaf7f3511b6..94129855cea4 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java @@ -5,9 +5,12 @@ package io.opentelemetry.javaagent.tooling.instrumentation.indy; +import io.opentelemetry.javaagent.bootstrap.InstrumentationHolder; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.instrumentation.internal.ExperimentalInstrumentationModule; +import io.opentelemetry.javaagent.tooling.ModuleOpener; import io.opentelemetry.javaagent.tooling.util.ClassLoaderValue; +import java.lang.instrument.Instrumentation; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import net.bytebuddy.agent.builder.AgentBuilder; @@ -65,6 +68,40 @@ public static InstrumentationModuleClassLoader getInstrumentationClassLoader( + " yet"); } + if (module instanceof ExperimentalInstrumentationModule) { + ExperimentalInstrumentationModule experimentalModule = + (ExperimentalInstrumentationModule) module; + + // Opening JPMS modules requires to use a 'witness class' in the target module to get a + // reference to the module, which means we have to eagerly load the class. + // + // However, this code here triggered when the advice is being executed for the first time so + // this only creates a very small eager loading that is unlikely to have impact on the + // application. + // + // Also, using a class that is already loaded like the one that is being instrumented or a + // related one would increase the likeliness of not having an effect on application class + // loading. + + Instrumentation instrumentation = InstrumentationHolder.getInstrumentation(); + if (instrumentation == null) { + throw new IllegalStateException("global instrumentation not available"); + } + + experimentalModule + .jpmsModulesToOpen() + .forEach( + (className, packages) -> { + Class type; + try { + type = Class.forName(className, false, instrumentedClassLoader); + } catch (ClassNotFoundException e) { + throw new IllegalStateException("missing witness class " + className, e); + } + + ModuleOpener.open(instrumentation, type, loader, packages); + }); + } return loader; } From 31fb5c21cf613d5f7b5beaf93d848247ba6fcdd5 Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Fri, 13 Dec 2024 18:29:32 +0100 Subject: [PATCH 3/6] enhance javadoc --- .../internal/ExperimentalInstrumentationModule.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java index 3c404aa5e2c7..eb5ce6041f8e 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/internal/ExperimentalInstrumentationModule.java @@ -70,9 +70,8 @@ default List agentPackagesToHide() { * to get a reference to the module.
* Map value is a list of packages to open in the module * - * @return map of "witness class" name as key, list of packages as value. + * @return map of "witness class" FQN as key, list of packages as value. */ - // TODO: copy the javadoc of the original implementation default Map> jpmsModulesToOpen() { return Collections.emptyMap(); } From 6e4614052028db70286431b1a304c06c4abc8e3b Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:33:35 +0100 Subject: [PATCH 4/6] add TODO for current module open impl. --- .../rmi/context/jpms/RmiJpmsInstrumentationModule.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/jpms/RmiJpmsInstrumentationModule.java b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/jpms/RmiJpmsInstrumentationModule.java index 6baf3baad0e0..dbcc4ff1271f 100644 --- a/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/jpms/RmiJpmsInstrumentationModule.java +++ b/instrumentation/rmi/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/rmi/context/jpms/RmiJpmsInstrumentationModule.java @@ -21,6 +21,12 @@ @AutoService(InstrumentationModule.class) public class RmiJpmsInstrumentationModule extends InstrumentationModule { + // TODO: this instrumentation is only kept for inlined advices and will be removed once migration + // to indy instrumentation is complete. + // + // For Indy instrumentation, this is replaced with + // ExperimentalInstrumentationModule#jpmsModulesToOpen + public RmiJpmsInstrumentationModule() { super("rmi", "rmi-jpms"); } From 101710d071f3dfd031c095dbf802d93a4add953c Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:39:03 +0100 Subject: [PATCH 5/6] fix logging --- .../io/opentelemetry/javaagent/tooling/ModuleOpener.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java b/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java index 81ce0e0a1398..3278ada40f0b 100644 --- a/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java +++ b/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java @@ -45,10 +45,8 @@ public static void open( missingOpens.put(packageName, openToModuleSet); logger.log( FINE, - () -> - String.format( - "Exposing package '%s' in module '%s' to module '%s'", - packageName, targetModule, openToModule)); + "Exposing package '{0}' in module '{1}' to module '{2}'", + new Object[] {packageName, targetModule, openToModule}); } } if (missingOpens.isEmpty()) { @@ -56,7 +54,7 @@ public static void open( } if (!instrumentation.isModifiableModule(targetModule)) { - logger.log(WARNING, "Module '{}' can't be modified", targetModule); + logger.log(WARNING, "Module '{0}' can't be modified", targetModule); return; } From 9c251f0464045d17dabb4142ca0d3515ae938933 Mon Sep 17 00:00:00 2001 From: Sylvain Juge <763082+SylvainJuge@users.noreply.github.com> Date: Thu, 19 Dec 2024 10:52:49 +0100 Subject: [PATCH 6/6] use module opener only when supported --- .../javaagent/tooling/ModuleOpener.java | 7 +++++ .../indy/IndyModuleRegistry.java | 30 +++++++++++-------- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java b/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java index 3278ada40f0b..af5b311e7d41 100644 --- a/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java +++ b/javaagent-tooling/javaagent-tooling-java9/src/main/java/io/opentelemetry/javaagent/tooling/ModuleOpener.java @@ -17,6 +17,13 @@ import java.util.Set; import java.util.logging.Logger; +/** + * Module opener provides ability to open JPMS modules and allows instrumentation classloader to + * access module contents without requiring JVM arguments modification.
+ * Usage of this class must be guarded with an {@code net.bytebuddy.utility.JavaModule#isSupported} + * check as it's compiled for Java 9+, otherwise an {@link UnsupportedClassVersionError} will be + * thrown for java 8. + */ public class ModuleOpener { private static final Logger logger = Logger.getLogger(ModuleOpener.class.getName()); diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java index 94129855cea4..1f1203d023fa 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/instrumentation/indy/IndyModuleRegistry.java @@ -14,6 +14,7 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import net.bytebuddy.agent.builder.AgentBuilder; +import net.bytebuddy.utility.JavaModule; public class IndyModuleRegistry { @@ -88,19 +89,22 @@ public static InstrumentationModuleClassLoader getInstrumentationClassLoader( throw new IllegalStateException("global instrumentation not available"); } - experimentalModule - .jpmsModulesToOpen() - .forEach( - (className, packages) -> { - Class type; - try { - type = Class.forName(className, false, instrumentedClassLoader); - } catch (ClassNotFoundException e) { - throw new IllegalStateException("missing witness class " + className, e); - } - - ModuleOpener.open(instrumentation, type, loader, packages); - }); + if (JavaModule.isSupported()) { + // module opener only usable for java 9+ + experimentalModule + .jpmsModulesToOpen() + .forEach( + (className, packages) -> { + Class type; + try { + type = Class.forName(className, false, instrumentedClassLoader); + } catch (ClassNotFoundException e) { + throw new IllegalStateException("missing witness class " + className, e); + } + + ModuleOpener.open(instrumentation, type, loader, packages); + }); + } } return loader; }