From ed08c30e6d1f57254303b1ca2c295b3cc0b710ac Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 7 Mar 2021 11:37:28 -0800 Subject: [PATCH 1/3] Fix app failure under Eclipse OSGi --- .../eclipse-osgi-3.6-javaagent.gradle | 22 ++++++ .../EclipseOsgiInstrumentationModule.java | 76 +++++++++++++++++++ .../api/InClassLoaderMatcher.java | 59 ++++++++++++++ .../bytebuddy/matcher/ClassLoaderMatcher.java | 12 ++- settings.gradle | 1 + 5 files changed, 167 insertions(+), 3 deletions(-) create mode 100644 instrumentation/eclipse-osgi-3.6/javaagent/eclipse-osgi-3.6-javaagent.gradle create mode 100644 instrumentation/eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/osgi/EclipseOsgiInstrumentationModule.java create mode 100644 javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/InClassLoaderMatcher.java diff --git a/instrumentation/eclipse-osgi-3.6/javaagent/eclipse-osgi-3.6-javaagent.gradle b/instrumentation/eclipse-osgi-3.6/javaagent/eclipse-osgi-3.6-javaagent.gradle new file mode 100644 index 000000000000..c1b86a5e4372 --- /dev/null +++ b/instrumentation/eclipse-osgi-3.6/javaagent/eclipse-osgi-3.6-javaagent.gradle @@ -0,0 +1,22 @@ +apply from: "$rootDir/gradle/instrumentation.gradle" + +// this instrumentation applies to the class 'org.eclipse.osgi.internal.loader.BundleLoader' +// which is present in the following artifacts dating back to version 3.6 (2010): +// +// * 'org.eclipse.platform:org.eclipse.osgi' +// * 'org.eclipse.tycho:org.eclipse.osgi' +// * 'org.eclipse.osgi:org.eclipse.osgi' + +// TODO write a smoke test that does the following: +// +// docker run --mount 'type=bind,src=$AGENT_PATH,dst=/opentelemetry-javaagent-all.jar' +// -e JAVA_TOOL_OPTIONS=-javaagent:/opentelemetry-javaagent-all.jar +// wso2/wso2ei-business-process:6.5.0 +// +// without this instrumentation, the following error will appear in the docker logs: +// java.lang.ClassNotFoundException: org.wso2.carbon.humantask.ui.fileupload.HumanTaskUploadExecutor +// cannot be found by org.wso2.carbon.ui_4.4.36 +// +// ... or even better, write a standalone OSGi application that exhibits similar issue, +// so we can run against arbitrary (e.g. latest) Eclipse OSGi release, especially since +// this instrumentation patches a private method which could be renamed at any time diff --git a/instrumentation/eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/osgi/EclipseOsgiInstrumentationModule.java b/instrumentation/eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/osgi/EclipseOsgiInstrumentationModule.java new file mode 100644 index 000000000000..d4b8edeab94c --- /dev/null +++ b/instrumentation/eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/osgi/EclipseOsgiInstrumentationModule.java @@ -0,0 +1,76 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.osgi; + +import static java.util.Collections.singletonList; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.returns; + +import com.google.auto.service.AutoService; +import io.opentelemetry.javaagent.instrumentation.api.InClassLoaderMatcher; +import io.opentelemetry.javaagent.tooling.InstrumentationModule; +import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.util.List; +import java.util.Map; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +/** + * The ClassLoaderMatcher's call to ClassLoader.getResource() causes the Eclipse OSGi class loader + * to "dynamically import" a bundle for the package if such a bundle is not found, which can lead to + * application failure later on due to the bundle hierarchy no longer being "consistent". + * + *

Any side-effect of the ClassLoaderMatcher's call to ClassLoader.getResource() is generally + * undesirable, and so this instrumentation patches the behavior and suppresses the "dynamic import" + * of the missing package/bundle when the call is originating from ClassLoaderMatcher.. + */ +@AutoService(InstrumentationModule.class) +public class EclipseOsgiInstrumentationModule extends InstrumentationModule { + public EclipseOsgiInstrumentationModule() { + super("eclipse-osgi"); + } + + @Override + public List typeInstrumentations() { + return singletonList(new EclipseOsgiInstrumentation()); + } + + private static class EclipseOsgiInstrumentation implements TypeInstrumentation { + + @Override + public ElementMatcher typeMatcher() { + return named("org.eclipse.osgi.internal.loader.BundleLoader"); + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod().and(named("isDynamicallyImported")).and(returns(boolean.class)), + EclipseOsgiInstrumentation.class.getName() + "$IsDynamicallyImportedAdvice"); + } + + public static class IsDynamicallyImportedAdvice { + + @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class, suppress = Throwable.class) + public static boolean onEnter() { + return InClassLoaderMatcher.get(); + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void onExit( + @Advice.Return(readOnly = false) boolean result, + @Advice.Enter boolean inClassLoaderMatcher) { + if (inClassLoaderMatcher) { + result = false; + } + } + } + } +} diff --git a/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/InClassLoaderMatcher.java b/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/InClassLoaderMatcher.java new file mode 100644 index 000000000000..e99d9c3fe650 --- /dev/null +++ b/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/InClassLoaderMatcher.java @@ -0,0 +1,59 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.api; + +public final class InClassLoaderMatcher { + + private static final ThreadLocal inClassLoaderMatcher = + ThreadLocal.withInitial(MutableBoolean::new); + + /** + * Returns whether the ClassLoaderMatcher is currently executing. + * + *

This is used (at least) by the {@code eclipse-osgi} instrumentation in order to suppress a + * side effect in the Eclipse OSGi class loader that occurs when ClassLoaderMatcher calls + * ClassLoader.getResource(). See {@code EclipseOsgiInstrumentationModule} for more details. + */ + public static boolean get() { + return inClassLoaderMatcher.get().value; + } + + /** + * WARNING This should not be used by instrumentation. It should only be used by + * io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher. + * + *

The reason it can't be (easily) hidden is that this class needs to live in the bootstrap + * class loader to be accessible to instrumentation, while the ClassLoaderMatcher lives in the + * agent class loader. + */ + public static boolean getAndSet(boolean value) { + return inClassLoaderMatcher.get().getAndSet(value); + } + + /** + * WARNING This should not be used by instrumentation. It should only be used by + * io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher. + * + *

The reason it can't be (easily) hidden is that this class needs to live in the bootstrap + * class loader to be accessible to instrumentation, while the ClassLoaderMatcher lives in the + * agent class loader. + */ + public static void set(boolean value) { + inClassLoaderMatcher.get().value = value; + } + + // using MutableBoolean to avoid an extra thread local lookup for getAndSet() + private static class MutableBoolean { + + private boolean value; + + private boolean getAndSet(boolean value) { + boolean oldValue = this.value; + this.value = value; + return oldValue; + } + } +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bytebuddy/matcher/ClassLoaderMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bytebuddy/matcher/ClassLoaderMatcher.java index 515e293cff5a..7d93dda85a48 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bytebuddy/matcher/ClassLoaderMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bytebuddy/matcher/ClassLoaderMatcher.java @@ -6,6 +6,7 @@ package io.opentelemetry.javaagent.tooling.bytebuddy.matcher; import io.opentelemetry.javaagent.bootstrap.WeakCache; +import io.opentelemetry.javaagent.instrumentation.api.InClassLoaderMatcher; import io.opentelemetry.javaagent.tooling.AgentTooling; import net.bytebuddy.matcher.ElementMatcher; @@ -45,10 +46,15 @@ private ClassLoaderHasClassesNamedMatcher(String... classNames) { } private boolean hasResources(ClassLoader cl) { - for (String resource : resources) { - if (cl.getResource(resource) == null) { - return false; + boolean priorValue = InClassLoaderMatcher.getAndSet(true); + try { + for (String resource : resources) { + if (cl.getResource(resource) == null) { + return false; + } } + } finally { + InClassLoaderMatcher.getAndSet(priorValue); } return true; } diff --git a/settings.gradle b/settings.gradle index 3c49bab31ee2..64c454dbee69 100644 --- a/settings.gradle +++ b/settings.gradle @@ -191,6 +191,7 @@ include ':instrumentation:opentelemetry-api-1.0:javaagent' include ':instrumentation:opentelemetry-api-metrics-1.0:javaagent' include ':instrumentation:oshi:javaagent' include ':instrumentation:oshi:library' +include ':instrumentation:eclipse-osgi-3.6:javaagent' include ':instrumentation:play:play-2.3:javaagent' include ':instrumentation:play:play-2.4:javaagent' include ':instrumentation:play:play-2.6:javaagent' From 10bf097329aec2e1c19c810e55c35a50fae761be Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Mon, 8 Mar 2021 10:21:26 -0800 Subject: [PATCH 2/3] Feedback --- .../osgi/EclipseOsgiInstrumentationModule.java | 4 +++- .../api/{ => internal}/InClassLoaderMatcher.java | 4 +++- .../tooling/bytebuddy/matcher/ClassLoaderMatcher.java | 4 ++-- 3 files changed, 8 insertions(+), 4 deletions(-) rename javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/{ => internal}/InClassLoaderMatcher.java (95%) diff --git a/instrumentation/eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/osgi/EclipseOsgiInstrumentationModule.java b/instrumentation/eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/osgi/EclipseOsgiInstrumentationModule.java index d4b8edeab94c..f61e5a558897 100644 --- a/instrumentation/eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/osgi/EclipseOsgiInstrumentationModule.java +++ b/instrumentation/eclipse-osgi-3.6/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/osgi/EclipseOsgiInstrumentationModule.java @@ -12,7 +12,7 @@ import static net.bytebuddy.matcher.ElementMatchers.returns; import com.google.auto.service.AutoService; -import io.opentelemetry.javaagent.instrumentation.api.InClassLoaderMatcher; +import io.opentelemetry.javaagent.instrumentation.api.internal.InClassLoaderMatcher; import io.opentelemetry.javaagent.tooling.InstrumentationModule; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; import java.util.List; @@ -58,6 +58,8 @@ public Map, String> transfor public static class IsDynamicallyImportedAdvice { + // "skipOn" is used to skip execution of the instrumented method when a ClassLoaderMatcher is + // currently executing, since we will be returning false regardless in onExit below @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class, suppress = Throwable.class) public static boolean onEnter() { return InClassLoaderMatcher.get(); diff --git a/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/InClassLoaderMatcher.java b/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/internal/InClassLoaderMatcher.java similarity index 95% rename from javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/InClassLoaderMatcher.java rename to javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/internal/InClassLoaderMatcher.java index e99d9c3fe650..525b74c6b55c 100644 --- a/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/InClassLoaderMatcher.java +++ b/javaagent-api/src/main/java/io/opentelemetry/javaagent/instrumentation/api/internal/InClassLoaderMatcher.java @@ -3,13 +3,15 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.javaagent.instrumentation.api; +package io.opentelemetry.javaagent.instrumentation.api.internal; public final class InClassLoaderMatcher { private static final ThreadLocal inClassLoaderMatcher = ThreadLocal.withInitial(MutableBoolean::new); + private InClassLoaderMatcher() {} + /** * Returns whether the ClassLoaderMatcher is currently executing. * diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bytebuddy/matcher/ClassLoaderMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bytebuddy/matcher/ClassLoaderMatcher.java index 7d93dda85a48..e5c5d3cda0e7 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bytebuddy/matcher/ClassLoaderMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bytebuddy/matcher/ClassLoaderMatcher.java @@ -6,7 +6,7 @@ package io.opentelemetry.javaagent.tooling.bytebuddy.matcher; import io.opentelemetry.javaagent.bootstrap.WeakCache; -import io.opentelemetry.javaagent.instrumentation.api.InClassLoaderMatcher; +import io.opentelemetry.javaagent.instrumentation.api.internal.InClassLoaderMatcher; import io.opentelemetry.javaagent.tooling.AgentTooling; import net.bytebuddy.matcher.ElementMatcher; @@ -54,7 +54,7 @@ private boolean hasResources(ClassLoader cl) { } } } finally { - InClassLoaderMatcher.getAndSet(priorValue); + InClassLoaderMatcher.set(priorValue); } return true; } From df55464a27e02e0322413cc2c0499f7277ac8e61 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Mon, 8 Mar 2021 13:57:40 -0800 Subject: [PATCH 3/3] Alphabetical --- settings.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/settings.gradle b/settings.gradle index 64c454dbee69..281633996702 100644 --- a/settings.gradle +++ b/settings.gradle @@ -92,6 +92,7 @@ include ':instrumentation:dropwizard-testing' include ':instrumentation:apache-dubbo:apache-dubbo-2.7:library' include ':instrumentation:apache-dubbo:apache-dubbo-2.7:testing' include ':instrumentation:apache-dubbo:apache-dubbo-2.7:javaagent' +include ':instrumentation:eclipse-osgi-3.6:javaagent' include ':instrumentation:elasticsearch:elasticsearch-rest-common:javaagent' include ':instrumentation:elasticsearch:elasticsearch-rest-5.0:javaagent' include ':instrumentation:elasticsearch:elasticsearch-rest-6.4:javaagent' @@ -191,7 +192,6 @@ include ':instrumentation:opentelemetry-api-1.0:javaagent' include ':instrumentation:opentelemetry-api-metrics-1.0:javaagent' include ':instrumentation:oshi:javaagent' include ':instrumentation:oshi:library' -include ':instrumentation:eclipse-osgi-3.6:javaagent' include ':instrumentation:play:play-2.3:javaagent' include ':instrumentation:play:play-2.4:javaagent' include ':instrumentation:play:play-2.6:javaagent'