From 9308f871a8f0d26ea7ea11fa9b51a6b5ba648942 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Fri, 26 Mar 2021 13:12:43 +0100 Subject: [PATCH 1/3] Make muzzle reference creation package(s) configurable (#2615) * Make muzzle reference creation package(s) configurable * Code review comments --- .../v3_1/CouchbaseInstrumentationModule.java | 7 +- .../tooling/InstrumentationModule.java | 26 ++++++ .../muzzle/InstrumentationClassPredicate.java | 20 ++++- .../muzzle/collector/MuzzleCodeGenerator.java | 25 ++++-- .../ReferenceCollectingClassVisitor.java | 13 +-- .../muzzle/collector/ReferenceCollector.java | 15 +++- .../muzzle/matcher/ReferenceMatcher.java | 17 ++-- .../InstrumentationClassPredicateTest.groovy | 17 ++-- .../muzzle/ReferenceCollectorTest.groovy | 40 ++++++--- .../groovy/muzzle/ReferenceMatcherTest.groovy | 81 +++++++++++++------ .../java/external/NotInstrumentation.java | 10 +++ .../instrumentation/ExternalHelper.java | 14 ++++ .../java/muzzle/MuzzleWeakReferenceTest.java | 6 +- .../src/test/java/muzzle/TestClasses.java | 7 ++ 14 files changed, 222 insertions(+), 76 deletions(-) create mode 100644 testing-common/src/test/java/external/NotInstrumentation.java create mode 100644 testing-common/src/test/java/external/instrumentation/ExternalHelper.java diff --git a/instrumentation/couchbase/couchbase-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v3_1/CouchbaseInstrumentationModule.java b/instrumentation/couchbase/couchbase-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v3_1/CouchbaseInstrumentationModule.java index 4948c06f69a..7c602975d4b 100644 --- a/instrumentation/couchbase/couchbase-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v3_1/CouchbaseInstrumentationModule.java +++ b/instrumentation/couchbase/couchbase-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/couchbase/v3_1/CouchbaseInstrumentationModule.java @@ -21,11 +21,8 @@ public CouchbaseInstrumentationModule() { } @Override - protected String[] additionalHelperClassNames() { - return new String[] { - "com.couchbase.client.tracing.opentelemetry.OpenTelemetryRequestSpan", - "com.couchbase.client.tracing.opentelemetry.OpenTelemetryRequestTracer" - }; + public boolean isHelperClass(String className) { + return className.startsWith("com.couchbase.client.tracing.opentelemetry"); } @Override diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/InstrumentationModule.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/InstrumentationModule.java index 567b9122282..13f11ff48ef 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/InstrumentationModule.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/InstrumentationModule.java @@ -28,6 +28,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.description.annotation.AnnotationSource; import net.bytebuddy.description.method.MethodDescription; @@ -251,6 +252,31 @@ private String mainInstrumentationName() { return instrumentationNames.iterator().next(); } + /** + * This is an internal helper method for muzzle code generation: generating {@code invokedynamic} + * instructions in ASM is so painful that it's much simpler and readable to just have a plain old + * Java helper function here. + */ + @SuppressWarnings("unused") + protected final Predicate additionalLibraryInstrumentationPackage() { + return this::isHelperClass; + } + + /** + * Instrumentation modules can override this method to specify additional packages (or classes) + * that should be treated as "library instrumentation" packages. Classes from those packages will + * be treated by muzzle as instrumentation helper classes: they will be scanned for references and + * automatically injected into the application classloader if they're used in any type + * instrumentation. The classes for which this predicate returns {@code true} will be treated as + * helper classes, in addition to the default ones defined in {@link + * InstrumentationClassPredicate}. + * + * @param className The name of the class that may or may not be a helper class. + */ + public boolean isHelperClass(String className) { + return false; + } + /** * The actual implementation of this method is generated automatically during compilation by the * {@link io.opentelemetry.javaagent.tooling.muzzle.collector.MuzzleCodeGenerationPlugin} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/InstrumentationClassPredicate.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/InstrumentationClassPredicate.java index f8982c5497d..2e825167ba3 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/InstrumentationClassPredicate.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/InstrumentationClassPredicate.java @@ -5,6 +5,8 @@ package io.opentelemetry.javaagent.tooling.muzzle; +import java.util.function.Predicate; + public final class InstrumentationClassPredicate { // javaagent instrumentation packages private static final String JAVAAGENT_INSTRUMENTATION_PACKAGE = @@ -16,6 +18,13 @@ public final class InstrumentationClassPredicate { private static final String LIBRARY_INSTRUMENTATION_PACKAGE = "io.opentelemetry.instrumentation."; private static final String INSTRUMENTATION_API_PACKAGE = "io.opentelemetry.instrumentation.api."; + private final Predicate additionalLibraryInstrumentationPredicate; + + public InstrumentationClassPredicate( + Predicate additionalLibraryInstrumentationPredicate) { + this.additionalLibraryInstrumentationPredicate = additionalLibraryInstrumentationPredicate; + } + /** * Defines which classes are treated by muzzle as "internal", "helper" instrumentation classes. * @@ -23,9 +32,14 @@ public final class InstrumentationClassPredicate { * instrumentation classes are treated as "helper" classes and are subjected to the reference * collection process. All others (including {@code instrumentation-api} and {@code javaagent-api} * modules are not scanned for references (but references to them are collected). + * + *

Aside from "standard" instrumentation helper class packages, instrumentation modules can + * pass an additional predicate to include instrumentation helper classes from 3rd party packages. */ - public static boolean isInstrumentationClass(String className) { - return isJavaagentInstrumentationClass(className) || isLibraryInstrumentationClass(className); + public boolean isInstrumentationClass(String className) { + return isJavaagentInstrumentationClass(className) + || isLibraryInstrumentationClass(className) + || additionalLibraryInstrumentationPredicate.test(className); } private static boolean isJavaagentInstrumentationClass(String className) { @@ -37,6 +51,4 @@ private static boolean isLibraryInstrumentationClass(String className) { return className.startsWith(LIBRARY_INSTRUMENTATION_PACKAGE) && !className.startsWith(INSTRUMENTATION_API_PACKAGE); } - - private InstrumentationClassPredicate() {} } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java index 0296e36ded8..79c6aeb46a4 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/MuzzleCodeGenerator.java @@ -73,7 +73,7 @@ private static class GenerateMuzzleReferenceMatcherMethodAndField extends ClassV private final boolean frames; private String instrumentationClassName; - private InstrumentationModule instrumenter; + private InstrumentationModule instrumentationModule; public GenerateMuzzleReferenceMatcherMethodAndField(ClassVisitor classVisitor, boolean frames) { super(Opcodes.ASM7, classVisitor); @@ -90,7 +90,7 @@ public void visit( String[] interfaces) { this.instrumentationClassName = name; try { - instrumenter = + instrumentationModule = (InstrumentationModule) MuzzleCodeGenerator.class .getClassLoader() @@ -143,15 +143,15 @@ public void visitEnd() { private ReferenceCollector collectReferences() { Set adviceClassNames = - instrumenter.typeInstrumentations().stream() + instrumentationModule.typeInstrumentations().stream() .flatMap(typeInstrumentation -> typeInstrumentation.transformers().values().stream()) .collect(Collectors.toSet()); - ReferenceCollector collector = new ReferenceCollector(); + ReferenceCollector collector = new ReferenceCollector(instrumentationModule::isHelperClass); for (String adviceClass : adviceClassNames) { collector.collectReferencesFromAdvice(adviceClass); } - for (String resource : instrumenter.helperResourceNames()) { + for (String resource : instrumentationModule.helperResourceNames()) { collector.collectReferencesFromResource(resource); } return collector; @@ -204,8 +204,9 @@ private void generateMuzzleReferenceMatcherMethod(ReferenceCollector collector) * if (null == this.muzzleReferenceMatcher) { * this.muzzleReferenceMatcher = new ReferenceMatcher(this.getAllHelperClassNames(), * new Reference[]{ - * //reference builders - * }); + * // reference builders + * }, + * this.additionalLibraryInstrumentationPackage()); * } * return this.muzzleReferenceMatcher; * } @@ -467,11 +468,19 @@ private void generateMuzzleReferenceMatcherMethod(ReferenceCollector collector) mv.visitInsn(Opcodes.AASTORE); } + mv.visitVarInsn(Opcodes.ALOAD, 0); + mv.visitMethodInsn( + Opcodes.INVOKEVIRTUAL, + instrumentationClassName, + "additionalLibraryInstrumentationPackage", + "()Ljava/util/function/Predicate;", + false); + mv.visitMethodInsn( Opcodes.INVOKESPECIAL, "io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher", "", - "(Ljava/util/List;[Lio/opentelemetry/javaagent/tooling/muzzle/Reference;)V", + "(Ljava/util/List;[Lio/opentelemetry/javaagent/tooling/muzzle/Reference;Ljava/util/function/Predicate;)V", false); mv.visitFieldInsn( Opcodes.PUTFIELD, diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java index bf2c036ea4f..614c21c4157 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollectingClassVisitor.java @@ -5,9 +5,8 @@ package io.opentelemetry.javaagent.tooling.muzzle.collector; -import static io.opentelemetry.javaagent.tooling.muzzle.InstrumentationClassPredicate.isInstrumentationClass; - import io.opentelemetry.javaagent.tooling.Utils; +import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationClassPredicate; import io.opentelemetry.javaagent.tooling.muzzle.Reference; import io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag; import io.opentelemetry.javaagent.tooling.muzzle.Reference.Flag.ManifestationFlag; @@ -108,7 +107,9 @@ private static Type underlyingType(Type type) { return type; } + private final InstrumentationClassPredicate instrumentationClassPredicate; private final boolean isAdviceClass; + private final Map references = new LinkedHashMap<>(); private final Set helperClasses = new HashSet<>(); // helper super classes which are themselves also helpers @@ -117,8 +118,10 @@ private static Type underlyingType(Type type) { private String refSourceClassName; private Type refSourceType; - ReferenceCollectingClassVisitor(boolean isAdviceClass) { + ReferenceCollectingClassVisitor( + InstrumentationClassPredicate instrumentationClassPredicate, boolean isAdviceClass) { super(Opcodes.ASM7); + this.instrumentationClassPredicate = instrumentationClassPredicate; this.isAdviceClass = isAdviceClass; } @@ -136,7 +139,7 @@ Set getHelperSuperClasses() { private void addExtendsReference(Reference ref) { addReference(ref); - if (isInstrumentationClass(ref.getClassName())) { + if (instrumentationClassPredicate.isInstrumentationClass(ref.getClassName())) { helperSuperClasses.add(ref.getClassName()); } } @@ -150,7 +153,7 @@ private void addReference(Reference ref) { references.put(ref.getClassName(), reference.merge(ref)); } } - if (isInstrumentationClass(ref.getClassName())) { + if (instrumentationClassPredicate.isInstrumentationClass(ref.getClassName())) { helperClasses.add(ref.getClassName()); } } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollector.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollector.java index ddd47ac3b30..6875a9d1cea 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollector.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/collector/ReferenceCollector.java @@ -6,7 +6,6 @@ package io.opentelemetry.javaagent.tooling.muzzle.collector; import static com.google.common.base.Preconditions.checkNotNull; -import static io.opentelemetry.javaagent.tooling.muzzle.InstrumentationClassPredicate.isInstrumentationClass; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.Collections.singleton; @@ -16,6 +15,7 @@ import com.google.common.graph.Graphs; import com.google.common.graph.MutableGraph; import io.opentelemetry.javaagent.tooling.Utils; +import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationClassPredicate; import io.opentelemetry.javaagent.tooling.muzzle.Reference; import java.io.BufferedReader; import java.io.IOException; @@ -32,6 +32,7 @@ import java.util.Map; import java.util.Queue; import java.util.Set; +import java.util.function.Predicate; import java.util.regex.Pattern; import net.bytebuddy.jar.asm.ClassReader; @@ -46,6 +47,12 @@ public class ReferenceCollector { private final Map references = new LinkedHashMap<>(); private final MutableGraph helperSuperClassGraph = GraphBuilder.directed().build(); private final Set visitedClasses = new HashSet<>(); + private final InstrumentationClassPredicate instrumentationClassPredicate; + + public ReferenceCollector(Predicate libraryInstrumentationPredicate) { + this.instrumentationClassPredicate = + new InstrumentationClassPredicate(libraryInstrumentationPredicate); + } /** * If passed {@code resource} path points to an SPI file (either Java {@link @@ -116,7 +123,8 @@ private void visitClassesAndCollectReferences( try (InputStream in = getClassFileStream(visitedClassName)) { // only start from method bodies for the advice class (skips class/method references) - ReferenceCollectingClassVisitor cv = new ReferenceCollectingClassVisitor(isAdviceClass); + ReferenceCollectingClassVisitor cv = + new ReferenceCollectingClassVisitor(instrumentationClassPredicate, isAdviceClass); ClassReader reader = new ClassReader(in); reader.accept(cv, ClassReader.SKIP_FRAMES); @@ -125,7 +133,8 @@ private void visitClassesAndCollectReferences( Reference reference = entry.getValue(); // Don't generate references created outside of the instrumentation package. - if (!visitedClasses.contains(refClassName) && isInstrumentationClass(refClassName)) { + if (!visitedClasses.contains(refClassName) + && instrumentationClassPredicate.isInstrumentationClass(refClassName)) { instrumentationQueue.add(refClassName); } addReference(refClassName, reference); diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java index 21ea2c99c6b..9cfe525a503 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/ReferenceMatcher.java @@ -5,13 +5,13 @@ package io.opentelemetry.javaagent.tooling.muzzle.matcher; -import static io.opentelemetry.javaagent.tooling.muzzle.InstrumentationClassPredicate.isInstrumentationClass; import static java.util.Collections.emptyList; import static net.bytebuddy.dynamic.loading.ClassLoadingStrategy.BOOTSTRAP_LOADER; import io.opentelemetry.javaagent.bootstrap.WeakCache; import io.opentelemetry.javaagent.tooling.AgentTooling; import io.opentelemetry.javaagent.tooling.Utils; +import io.opentelemetry.javaagent.tooling.muzzle.InstrumentationClassPredicate; import io.opentelemetry.javaagent.tooling.muzzle.Reference; import io.opentelemetry.javaagent.tooling.muzzle.Reference.Source; import io.opentelemetry.javaagent.tooling.muzzle.matcher.HelperReferenceWrapper.Factory; @@ -24,6 +24,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; import net.bytebuddy.description.field.FieldDescription; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; @@ -35,17 +36,19 @@ public final class ReferenceMatcher { private final WeakCache mismatchCache = AgentTooling.newWeakCache(); private final Map references; private final Set helperClassNames; + private final InstrumentationClassPredicate instrumentationClassPredicate; - public ReferenceMatcher(Reference... references) { - this(emptyList(), references); - } - - public ReferenceMatcher(List helperClassNames, Reference[] references) { + public ReferenceMatcher( + List helperClassNames, + Reference[] references, + Predicate libraryInstrumentationPredicate) { this.references = new HashMap<>(references.length); for (Reference reference : references) { this.references.put(reference.getClassName(), reference); } this.helperClassNames = new HashSet<>(helperClassNames); + this.instrumentationClassPredicate = + new InstrumentationClassPredicate(libraryInstrumentationPredicate); } Collection getReferences() { @@ -106,7 +109,7 @@ private List checkMatch(Reference reference, ClassLoader loader) { AgentTooling.poolStrategy() .typePool(AgentTooling.locationStrategy().classFileLocator(loader), loader); try { - if (isInstrumentationClass(reference.getClassName())) { + if (instrumentationClassPredicate.isInstrumentationClass(reference.getClassName())) { // make sure helper class is registered if (!helperClassNames.contains(reference.getClassName())) { return Collections.singletonList( diff --git a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/InstrumentationClassPredicateTest.groovy b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/InstrumentationClassPredicateTest.groovy index fbfe31b2ac2..8a6adb7abc8 100644 --- a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/InstrumentationClassPredicateTest.groovy +++ b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/muzzle/InstrumentationClassPredicateTest.groovy @@ -11,19 +11,26 @@ import spock.lang.Unroll class InstrumentationClassPredicateTest extends Specification { @Unroll def "should collect references for #desc"() { + setup: + def predicate = new InstrumentationClassPredicate({ it.startsWith("com.example.instrumentation.library") }) + expect: - InstrumentationClassPredicate.isInstrumentationClass(className) + predicate.isInstrumentationClass(className) where: - desc | className - "javaagent instrumentation class" | "io.opentelemetry.javaagent.instrumentation.some_instrumentation.Advice" - "library instrumentation class" | "io.opentelemetry.instrumentation.LibraryClass" + desc | className + "javaagent instrumentation class" | "io.opentelemetry.javaagent.instrumentation.some_instrumentation.Advice" + "library instrumentation class" | "io.opentelemetry.instrumentation.LibraryClass" + "additional library instrumentation class" | "com.example.instrumentation.library.ThirdPartyExternalInstrumentation" } @Unroll def "should not collect references for #desc"() { + setup: + def predicate = new InstrumentationClassPredicate({ false }) + expect: - !InstrumentationClassPredicate.isInstrumentationClass(className) + !predicate.isInstrumentationClass(className) where: desc | className diff --git a/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy index 23562b7d087..99b0cccf1b9 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceCollectorTest.groovy @@ -13,6 +13,7 @@ import static muzzle.TestClasses.HelperAdvice import static muzzle.TestClasses.LdcAdvice import static muzzle.TestClasses.MethodBodyAdvice +import external.instrumentation.ExternalHelper import io.opentelemetry.instrumentation.OtherTestHelperClasses import io.opentelemetry.instrumentation.TestHelperClasses import io.opentelemetry.javaagent.tooling.muzzle.Reference @@ -23,7 +24,7 @@ import spock.lang.Unroll class ReferenceCollectorTest extends Specification { def "method body creates references"() { setup: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(MethodBodyAdvice.name) def references = collector.getReferences() @@ -69,7 +70,7 @@ class ReferenceCollectorTest extends Specification { def "protected ref test"() { setup: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(MethodBodyAdvice.B2.name) def references = collector.getReferences() @@ -81,7 +82,7 @@ class ReferenceCollectorTest extends Specification { def "ldc creates references"() { setup: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(LdcAdvice.name) def references = collector.getReferences() @@ -91,7 +92,7 @@ class ReferenceCollectorTest extends Specification { def "instanceof creates references"() { setup: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(TestClasses.InstanceofAdvice.name) def references = collector.getReferences() @@ -101,7 +102,7 @@ class ReferenceCollectorTest extends Specification { def "invokedynamic creates references"() { setup: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(TestClasses.InvokeDynamicAdvice.name) def references = collector.getReferences() @@ -112,7 +113,7 @@ class ReferenceCollectorTest extends Specification { def "should create references for helper classes"() { when: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(HelperAdvice.name) def references = collector.getReferences() @@ -146,7 +147,7 @@ class ReferenceCollectorTest extends Specification { def "should find all helper classes"() { when: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(HelperAdvice.name) def helperClasses = collector.getSortedHelperClasses() @@ -163,7 +164,7 @@ class ReferenceCollectorTest extends Specification { def "should correctly find helper classes from multiple advice classes"() { when: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(TestClasses.HelperAdvice.name) collector.collectReferencesFromAdvice(TestClasses.HelperOtherAdvice.name) def helperClasses = collector.getSortedHelperClasses() @@ -193,10 +194,25 @@ class ReferenceCollectorTest extends Specification { ]) } + def "should correctly find external instrumentation classes"() { + when: + def collector = new ReferenceCollector({ it.startsWith("external.instrumentation") }) + collector.collectReferencesFromAdvice(TestClasses.ExternalInstrumentationAdvice.name) + + then: "should collect references" + def references = collector.getReferences() + references['external.instrumentation.ExternalHelper'] != null + references['external.NotInstrumentation'] != null + + then: "should collect helper classes" + def helperClasses = collector.getSortedHelperClasses() + helperClasses == [ExternalHelper.name] + } + @Unroll def "should collect helper classes from resource file #desc"() { when: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromResource(resource) then: "SPI classes are collected as references" @@ -219,8 +235,8 @@ class ReferenceCollectorTest extends Specification { ] where: - desc | resource - "Java SPI" | "META-INF/services/test.resource.file" + desc | resource + "Java SPI" | "META-INF/services/test.resource.file" "AWS SDK v2 global interceptors file" | "software/amazon/awssdk/global/handlers/execution.interceptors" "AWS SDK v2 service interceptors file" | "software/amazon/awssdk/services/testservice/execution.interceptors" "AWS SDK v2 service (second level) interceptors file" | "software/amazon/awssdk/services/testservice/testsubservice/execution.interceptors" @@ -231,7 +247,7 @@ class ReferenceCollectorTest extends Specification { def "should ignore arbitrary resource file"() { when: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromResource("application.properties") then: diff --git a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy index 681bb30dde1..6e5f503f182 100644 --- a/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy +++ b/testing-common/src/test/groovy/muzzle/ReferenceMatcherTest.groovy @@ -28,8 +28,11 @@ import io.opentelemetry.javaagent.tooling.muzzle.matcher.ReferenceMatcher import net.bytebuddy.jar.asm.Type import spock.lang.Shared import spock.lang.Specification +import spock.lang.Unroll +@Unroll class ReferenceMatcherTest extends Specification { + static final TEST_EXTERNAL_INSTRUMENTATION_PACKAGE = "com.external.otel.instrumentation" @Shared ClassLoader safeClasspath = new URLClassLoader([ClasspathUtils.createJarWithClasses(MethodBodyAdvice.A, @@ -46,10 +49,9 @@ class ReferenceMatcherTest extends Specification { def "match safe classpaths"() { setup: - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(MethodBodyAdvice.name) - Reference[] refs = collector.getReferences().values().toArray(new Reference[0]) - def refMatcher = new ReferenceMatcher(refs) + def refMatcher = createMatcher(collector.getReferences().values()) expect: getMismatchClassSet(refMatcher.getMismatchedReferenceSources(safeClasspath)).empty @@ -84,12 +86,11 @@ class ReferenceMatcherTest extends Specification { MethodBodyAdvice.SomeImplementation)] as URL[], (ClassLoader) null) - def collector = new ReferenceCollector() + def collector = new ReferenceCollector({ false }) collector.collectReferencesFromAdvice(MethodBodyAdvice.name) - Reference[] refs = collector.getReferences().values().toArray(new Reference[0]) - def refMatcher1 = new ReferenceMatcher(refs) - def refMatcher2 = new ReferenceMatcher(refs) + def refMatcher1 = createMatcher(collector.getReferences().values()) + def refMatcher2 = createMatcher(collector.getReferences().values()) assert getMismatchClassSet(refMatcher1.getMismatchedReferenceSources(cl)).empty int countAfterFirstMatch = cl.count // the second matcher should be able to used cached type descriptions from the first @@ -106,7 +107,7 @@ class ReferenceMatcherTest extends Specification { .build() when: - def mismatches = new ReferenceMatcher(ref).getMismatchedReferenceSources(this.class.classLoader) + def mismatches = createMatcher([ref]).getMismatchedReferenceSources(this.class.classLoader) then: getMismatchClassSet(mismatches) == expectedMismatches as Set @@ -125,7 +126,7 @@ class ReferenceMatcherTest extends Specification { .build() when: - def mismatches = new ReferenceMatcher(reference) + def mismatches = createMatcher([reference]) .getMismatchedReferenceSources(this.class.classLoader) then: @@ -149,7 +150,7 @@ class ReferenceMatcherTest extends Specification { .build() when: - def mismatches = new ReferenceMatcher(reference) + def mismatches = createMatcher([reference]) .getMismatchedReferenceSources(this.class.classLoader) then: @@ -172,14 +173,14 @@ class ReferenceMatcherTest extends Specification { .build() when: - def mismatches = new ReferenceMatcher([reference.className], [reference] as Reference[]) + def mismatches = createMatcher([reference], [reference.className]) .getMismatchedReferenceSources(emptyClassLoader) then: mismatches.empty } - def "should not check abstract helper classes"() { + def "should not check abstract #desc helper classes"() { given: def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper") .withSuperName(TestHelperClasses.HelperSuperClass.name) @@ -188,61 +189,81 @@ class ReferenceMatcherTest extends Specification { .build() when: - def mismatches = new ReferenceMatcher([reference.className], [reference] as Reference[]) + def mismatches = createMatcher([reference], [reference.className]) .getMismatchedReferenceSources(this.class.classLoader) then: mismatches.empty + + where: + desc | className + "internal" | "com.external.otel.instrumentation.Helper" + "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" } - def "should not check helper classes with no supertypes"() { + def "should not check #desc helper classes with no supertypes"() { given: - def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + def reference = new Reference.Builder(className) .withSuperName(Object.name) .withMethod(new Source[0], [] as Reference.Flag[], "someMethod", Type.VOID_TYPE) .build() when: - def mismatches = new ReferenceMatcher([reference.className], [reference] as Reference[]) + def mismatches = createMatcher([reference], [reference.className]) .getMismatchedReferenceSources(this.class.classLoader) then: mismatches.empty + + where: + desc | className + "internal" | "com.external.otel.instrumentation.Helper" + "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" } - def "should fail helper classes that does not implement all abstract methods"() { + def "should fail #desc helper classes that does not implement all abstract methods"() { given: - def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + def reference = new Reference.Builder(className) .withSuperName(TestHelperClasses.HelperSuperClass.name) .withMethod(new Source[0], [] as Reference.Flag[], "someMethod", Type.VOID_TYPE) .build() when: - def mismatches = new ReferenceMatcher([reference.className], [reference] as Reference[]) + def mismatches = createMatcher([reference], [reference.className]) .getMismatchedReferenceSources(this.class.classLoader) then: getMismatchClassSet(mismatches) == [MissingMethod] as Set + + where: + desc | className + "internal" | "com.external.otel.instrumentation.Helper" + "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" } - def "should fail helper classes that does not implement all abstract methods - even if empty abstract class reference exists"() { + def "should fail #desc helper classes that do not implement all abstract methods - even if empty abstract class reference exists"() { given: def emptySuperClassRef = new Reference.Builder(TestHelperClasses.HelperSuperClass.name) .build() - def reference = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + def reference = new Reference.Builder(className) .withSuperName(TestHelperClasses.HelperSuperClass.name) .withMethod(new Source[0], [] as Reference.Flag[], "someMethod", Type.VOID_TYPE) .build() when: - def mismatches = new ReferenceMatcher([reference.className, emptySuperClassRef.className], [reference, emptySuperClassRef] as Reference[]) + def mismatches = createMatcher([reference, emptySuperClassRef], [reference.className, emptySuperClassRef.className]) .getMismatchedReferenceSources(this.class.classLoader) then: getMismatchClassSet(mismatches) == [MissingMethod] as Set + + where: + desc | className + "internal" | "com.external.otel.instrumentation.Helper" + "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" } - def "should check whether interface methods are implemented in the super class"() { + def "should check #desc helper class whether interface methods are implemented in the super class"() { given: def baseHelper = new Reference.Builder("io.opentelemetry.instrumentation.BaseHelper") .withSuperName(Object.name) @@ -250,18 +271,28 @@ class ReferenceMatcherTest extends Specification { .withMethod(new Source[0], [] as Reference.Flag[], "foo", Type.VOID_TYPE) .build() // abstract HelperInterface#foo() is implemented by BaseHelper - def helper = new Reference.Builder("io.opentelemetry.instrumentation.Helper") + def helper = new Reference.Builder(className) .withSuperName(baseHelper.className) .withInterface(TestHelperClasses.AnotherHelperInterface.name) .withMethod(new Source[0], [] as Reference.Flag[], "bar", Type.VOID_TYPE) .build() when: - def mismatches = new ReferenceMatcher([helper.className, baseHelper.className], [helper, baseHelper] as Reference[]) + def mismatches = createMatcher([helper, baseHelper], [helper.className, baseHelper.className]) .getMismatchedReferenceSources(this.class.classLoader) then: mismatches.empty + + where: + desc | className + "internal" | "com.external.otel.instrumentation.Helper" + "external" | "${TEST_EXTERNAL_INSTRUMENTATION_PACKAGE}.Helper" + } + + private static ReferenceMatcher createMatcher(Collection references = [], + List helperClasses = []) { + new ReferenceMatcher(helperClasses, references as Reference[], { it.startsWith(TEST_EXTERNAL_INSTRUMENTATION_PACKAGE) }) } private static Set getMismatchClassSet(List mismatches) { diff --git a/testing-common/src/test/java/external/NotInstrumentation.java b/testing-common/src/test/java/external/NotInstrumentation.java new file mode 100644 index 00000000000..df5c51d2979 --- /dev/null +++ b/testing-common/src/test/java/external/NotInstrumentation.java @@ -0,0 +1,10 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package external; + +public class NotInstrumentation { + public void someLibraryCode() {} +} diff --git a/testing-common/src/test/java/external/instrumentation/ExternalHelper.java b/testing-common/src/test/java/external/instrumentation/ExternalHelper.java new file mode 100644 index 00000000000..65e987e85b0 --- /dev/null +++ b/testing-common/src/test/java/external/instrumentation/ExternalHelper.java @@ -0,0 +1,14 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package external.instrumentation; + +import external.NotInstrumentation; + +public class ExternalHelper { + public void instrument() { + new NotInstrumentation().someLibraryCode(); + } +} diff --git a/testing-common/src/test/java/muzzle/MuzzleWeakReferenceTest.java b/testing-common/src/test/java/muzzle/MuzzleWeakReferenceTest.java index 988331ba9ee..4aeabd8ff85 100644 --- a/testing-common/src/test/java/muzzle/MuzzleWeakReferenceTest.java +++ b/testing-common/src/test/java/muzzle/MuzzleWeakReferenceTest.java @@ -12,6 +12,7 @@ import java.lang.ref.WeakReference; import java.net.URL; import java.net.URLClassLoader; +import java.util.Collections; public class MuzzleWeakReferenceTest { /* @@ -22,10 +23,11 @@ public class MuzzleWeakReferenceTest { public static boolean classLoaderRefIsGarbageCollected() throws InterruptedException { ClassLoader loader = new URLClassLoader(new URL[0], null); WeakReference clRef = new WeakReference<>(loader); - ReferenceCollector collector = new ReferenceCollector(); + ReferenceCollector collector = new ReferenceCollector(className -> false); collector.collectReferencesFromAdvice(TestClasses.MethodBodyAdvice.class.getName()); Reference[] refs = collector.getReferences().values().toArray(new Reference[0]); - ReferenceMatcher refMatcher = new ReferenceMatcher(refs); + ReferenceMatcher refMatcher = + new ReferenceMatcher(Collections.emptyList(), refs, className -> false); refMatcher.getMismatchedReferenceSources(loader); loader = null; GcUtils.awaitGc(clRef); diff --git a/testing-common/src/test/java/muzzle/TestClasses.java b/testing-common/src/test/java/muzzle/TestClasses.java index 3ce23108066..bf55b3426ee 100644 --- a/testing-common/src/test/java/muzzle/TestClasses.java +++ b/testing-common/src/test/java/muzzle/TestClasses.java @@ -5,6 +5,7 @@ package muzzle; +import external.instrumentation.ExternalHelper; import io.opentelemetry.instrumentation.OtherTestHelperClasses; import io.opentelemetry.instrumentation.TestHelperClasses.Helper; import net.bytebuddy.asm.Advice; @@ -107,4 +108,10 @@ public static void adviceMethod() { new OtherTestHelperClasses.Bar().doSomething(); } } + + public static class ExternalInstrumentationAdvice { + public static void adviceMethod() { + new ExternalHelper().instrument(); + } + } } From cf7f2e0a8be0f32b1ed89a2c5274a5ff21c4a140 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 27 Mar 2021 11:21:42 -0700 Subject: [PATCH 2/3] Add Azure SDK instrumentation --- .../javaagent/azure-sdk-javaagent.gradle | 13 ++++ .../AzureSdkInstrumentationModule.java | 69 +++++++++++++++++++ .../src/test/groovy/AzureSdkTest.groovy | 15 ++++ settings.gradle | 1 + 4 files changed, 98 insertions(+) create mode 100644 instrumentation/azure-sdk/javaagent/azure-sdk-javaagent.gradle create mode 100644 instrumentation/azure-sdk/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/azuresdk/AzureSdkInstrumentationModule.java create mode 100644 instrumentation/azure-sdk/javaagent/src/test/groovy/AzureSdkTest.groovy diff --git a/instrumentation/azure-sdk/javaagent/azure-sdk-javaagent.gradle b/instrumentation/azure-sdk/javaagent/azure-sdk-javaagent.gradle new file mode 100644 index 00000000000..9f77d81abcc --- /dev/null +++ b/instrumentation/azure-sdk/javaagent/azure-sdk-javaagent.gradle @@ -0,0 +1,13 @@ +apply from: "$rootDir/gradle/instrumentation.gradle" + +configurations { + testRuntime.exclude group: "com.azure", module: "azure-core-tracing-opentelemetry" +} + +dependencies { + implementation(group: "com.azure", name: "azure-core-tracing-opentelemetry", version: "1.0.0-beta.8") { + exclude(group: "com.azure", module: "azure-core") + } + + library group: "com.azure", name: "azure-core", version: "1.14.0" +} diff --git a/instrumentation/azure-sdk/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/azuresdk/AzureSdkInstrumentationModule.java b/instrumentation/azure-sdk/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/azuresdk/AzureSdkInstrumentationModule.java new file mode 100644 index 00000000000..1f1177f8fb8 --- /dev/null +++ b/instrumentation/azure-sdk/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/azuresdk/AzureSdkInstrumentationModule.java @@ -0,0 +1,69 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.azuresdk; + +import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher.hasClassesNamed; +import static java.util.Collections.emptyMap; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; + +import com.google.auto.service.AutoService; +import io.opentelemetry.javaagent.tooling.InstrumentationModule; +import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(InstrumentationModule.class) +public class AzureSdkInstrumentationModule extends InstrumentationModule { + public AzureSdkInstrumentationModule() { + super("azure-sdk"); + } + + @Override + public boolean isHelperClass(String className) { + return className.startsWith("com.azure.core.tracing.opentelemetry."); + } + + @Override + public String[] helperResourceNames() { + return new String[] { + "META-INF/services/com.azure.core.http.policy.AfterRetryPolicyProvider", + "META-INF/services/com.azure.core.util.tracing.Tracer" + }; + } + + @Override + public ElementMatcher.Junction classLoaderMatcher() { + return hasClassesNamed("com.azure.core.util.tracing.Tracer") + .and(not(hasClassesNamed("com.azure.core.tracing.opentelemetry.OpenTelemetryTracer"))); + } + + @Override + public List typeInstrumentations() { + return Collections.singletonList(new EmptyTypeInstrumentation()); + } + + public static class EmptyTypeInstrumentation implements TypeInstrumentation { + @Override + public ElementMatcher typeMatcher() { + // we cannot use com.azure.core.util.tracing.Tracer here because one of the classes that we + // inject implements this interface, causing the interface to be loaded while it's being + // transformed, which leads to duplicate class definition error after the interface is + // transformed and the triggering class loader tries to load it. + return named("com.azure.core.util.tracing.TracerProxy"); + } + + @Override + public Map, String> transformers() { + // Nothing to instrument, no methods to match + return emptyMap(); + } + } +} diff --git a/instrumentation/azure-sdk/javaagent/src/test/groovy/AzureSdkTest.groovy b/instrumentation/azure-sdk/javaagent/src/test/groovy/AzureSdkTest.groovy new file mode 100644 index 00000000000..35137f6c349 --- /dev/null +++ b/instrumentation/azure-sdk/javaagent/src/test/groovy/AzureSdkTest.groovy @@ -0,0 +1,15 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +import com.azure.core.util.tracing.TracerProxy +import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification + +class AzureSdkTest extends AgentInstrumentationSpecification { + + def "test helper classes injected"() { + expect: + TracerProxy.isTracingEnabled() + } +} diff --git a/settings.gradle b/settings.gradle index 5cc9cd966fd..fc1a614072a 100644 --- a/settings.gradle +++ b/settings.gradle @@ -83,6 +83,7 @@ include ':instrumentation:aws-sdk:aws-sdk-2.2:library' include ':instrumentation:aws-sdk:aws-sdk-2.2:testing' include ':instrumentation:cassandra:cassandra-3.0:javaagent' include ':instrumentation:cassandra:cassandra-4.0:javaagent' +include ':instrumentation:azure-sdk:javaagent' // out of order to avoid merge conflict with azure-functions instrumentation include ':instrumentation:cdi-testing' include ':instrumentation:classloaders:javaagent' include ':instrumentation:classloaders:javaagent:jboss-testing' From 099f9a347df11631a2c9198a877a049b9e367d52 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 27 Mar 2021 19:11:18 -0700 Subject: [PATCH 3/3] Fix webflux client filter subscribe --- .../spring/webflux/client/WebClientTracingFilter.java | 1 + 1 file changed, 1 insertion(+) diff --git a/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/WebClientTracingFilter.java b/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/WebClientTracingFilter.java index b1ca7d3f6a1..6c354870e53 100644 --- a/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/WebClientTracingFilter.java +++ b/instrumentation/spring/spring-webflux-5.0/library/src/main/java/io/opentelemetry/instrumentation/spring/webflux/client/WebClientTracingFilter.java @@ -51,6 +51,7 @@ public MonoWebClientTrace(ClientRequest request, ExchangeFunction next) { public void subscribe(CoreSubscriber subscriber) { Context parentContext = Context.current(); if (!tracer().shouldStartSpan(parentContext)) { + next.exchange(request).subscribe(subscriber); return; }