From 871f9a0d24759b0595e9fd4f16fb41a08b2580e8 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 11 May 2021 18:24:31 +0200 Subject: [PATCH] InstrumentationModule cleanup (#2925) --- .../methods/MethodInstrumentationModule.java | 2 +- .../InstrumentationModule.java | 141 +++++++----------- .../matcher/AgentElementMatchers.java | 4 +- .../extension/matcher/NameMatchers.java | 2 + .../spi/ByteBuddyAgentCustomizer.java | 34 ----- ...nstrumentationExtensionImplementation.java | 13 +- .../javaagent/tooling/AgentInstaller.java | 16 -- .../muzzle/collector/MuzzleCodeGenerator.java | 12 +- .../matcher/MuzzleGradlePluginUtil.java | 6 +- ...ustomizer.java => TestAgentExtension.java} | 14 +- 10 files changed, 85 insertions(+), 159 deletions(-) delete mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/ByteBuddyAgentCustomizer.java rename testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/bytebuddy/{TestByteBuddyAgentCustomizer.java => TestAgentExtension.java} (51%) diff --git a/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentationModule.java b/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentationModule.java index b525166d11..bd545d73f5 100644 --- a/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentationModule.java +++ b/instrumentation/methods/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/methods/MethodInstrumentationModule.java @@ -55,7 +55,7 @@ public class MethodInstrumentationModule extends InstrumentationModule { // the default configuration has empty "otel.instrumentation.methods.include", and so doesn't // generate any TypeInstrumentation for muzzle to analyze @Override - protected String[] getMuzzleHelperClassNames() { + public String[] getMuzzleHelperClassNames() { return typeInstrumentations.isEmpty() ? new String[0] : new String[] {"io.opentelemetry.javaagent.instrumentation.methods.MethodTracer"}; diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java index c91719292a..48c078d0dc 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/InstrumentationModule.java @@ -33,12 +33,12 @@ import net.bytebuddy.matcher.ElementMatcher; */ public abstract class InstrumentationModule implements AgentExtension { private static final String[] EMPTY = new String[0]; + private static final Reference[] EMPTY_REFS = new Reference[0]; private static final boolean DEFAULT_ENABLED = Config.get().getBooleanProperty("otel.instrumentation.common.default-enabled", true); private final Set instrumentationNames; - final boolean enabled; /** * Creates an instrumentation module. Note that all implementations of {@link @@ -75,7 +75,6 @@ public abstract class InstrumentationModule implements AgentExtension { throw new IllegalArgumentException("InstrumentationModules must be named"); } this.instrumentationNames = new LinkedHashSet<>(instrumentationNames); - this.enabled = Config.get().isInstrumentationEnabled(instrumentationNames, defaultEnabled()); } private static List toList(String first, String[] rest) { @@ -101,15 +100,17 @@ public abstract class InstrumentationModule implements AgentExtension { return instrumentationNames.iterator().next(); } + /** Returns true if this instrumentation module should be installed. */ + public final boolean isEnabled() { + return Config.get().isInstrumentationEnabled(instrumentationNames, defaultEnabled()); + } + /** - * Returns all helper classes that will be injected into the application classloader, both ones - * provided by the implementation and ones that were collected by muzzle during compilation. + * Allows instrumentation modules to disable themselves by default, or to additionally disable + * themselves on some other condition. */ - public final List getAllHelperClassNames() { - List helperClassNames = new ArrayList<>(); - helperClassNames.addAll(asList(additionalHelperClassNames())); - helperClassNames.addAll(asList(getMuzzleHelperClassNames())); - return helperClassNames; + protected boolean defaultEnabled() { + return DEFAULT_ENABLED; } /** @@ -127,83 +128,6 @@ public abstract class InstrumentationModule implements AgentExtension { return false; } - /** - * Returns a list of references to helper and library classes used in this module's type - * instrumentation advices. - * - *

The actual implementation of this method is generated automatically during compilation by - * the {@code io.opentelemetry.javaagent.tooling.muzzle.collector.MuzzleCodeGenerationPlugin} - * ByteBuddy plugin. - * - *

This method is generated automatically: if you override it, the muzzle compile plugin - * will not generate a new implementation, it will leave the existing one. - */ - public Reference[] getMuzzleReferences() { - return new Reference[0]; - } - - /** - * Returns a list of instrumentation helper classes, automatically detected by muzzle during - * compilation. Those helpers will be injected into the application classloader. - * - *

The actual implementation of this method is generated automatically during compilation by - * the {@code io.opentelemetry.javaagent.tooling.muzzle.collector.MuzzleCodeGenerationPlugin} - * ByteBuddy plugin. - * - *

This method is generated automatically: if you override it, the muzzle compile plugin - * will not generate a new implementation, it will leave the existing one. - */ - protected String[] getMuzzleHelperClassNames() { - return EMPTY; - } - - /** - * Returns a map of {@code class-name to context-class-name}. Keys (and their subclasses) will be - * associated with a context class stored in the value. - * - *

The actual implementation of this method is generated automatically during compilation by - * the {@code io.opentelemetry.javaagent.tooling.muzzle.collector.MuzzleCodeGenerationPlugin} - * ByteBuddy plugin. - * - *

This method is generated automatically: if you override it, the muzzle compile plugin - * will not generate a new implementation, it will leave the existing one. - */ - protected Map getMuzzleContextStoreClasses() { - return Collections.emptyMap(); - } - - /** - * Instrumentation modules can override this method to provide additional helper classes that are - * not located in instrumentation packages described in the {@code InstrumentationClassPredicate} - * class and {@link #isHelperClass(String)} (and not automatically detected by muzzle). These - * additional classes will be injected into the application classloader first. - * - *

Implementing {@link #isHelperClass(String)} is generally simpler and less error-prone - * compared to implementing this method. - * - * @deprecated Use {@link #isHelperClass(String)} instead. - */ - @Deprecated - protected String[] additionalHelperClassNames() { - return EMPTY; - } - - /** - * Same as {@link #order()}. - * - * @deprecated use {@link #order()} instead. - */ - @Deprecated - public int getOrder() { - return 0; - } - - /** {@inheritDoc} */ - @Override - public int order() { - return getOrder(); - } - /** Returns resource names to inject into the user's classloader. */ public String[] helperResourceNames() { return EMPTY; @@ -228,10 +152,47 @@ public abstract class InstrumentationModule implements AgentExtension { public abstract List typeInstrumentations(); /** - * Allows instrumentation modules to disable themselves by default, or to additionally disable - * themselves on some other condition. + * Returns a list of references to helper and library classes used in this module's type + * instrumentation advices. + * + *

The actual implementation of this method is generated automatically during compilation by + * the {@code io.opentelemetry.javaagent.tooling.muzzle.collector.MuzzleCodeGenerationPlugin} + * ByteBuddy plugin. + * + *

This method is generated automatically: if you override it, the muzzle compile plugin + * will not generate a new implementation, it will leave the existing one. */ - protected boolean defaultEnabled() { - return DEFAULT_ENABLED; + public Reference[] getMuzzleReferences() { + return EMPTY_REFS; + } + + /** + * Returns a list of instrumentation helper classes, automatically detected by muzzle during + * compilation. Those helpers will be injected into the application classloader. + * + *

The actual implementation of this method is generated automatically during compilation by + * the {@code io.opentelemetry.javaagent.tooling.muzzle.collector.MuzzleCodeGenerationPlugin} + * ByteBuddy plugin. + * + *

This method is generated automatically: if you override it, the muzzle compile plugin + * will not generate a new implementation, it will leave the existing one. + */ + public String[] getMuzzleHelperClassNames() { + return EMPTY; + } + + /** + * Returns a map of {@code class-name to context-class-name}. Keys (and their subclasses) will be + * associated with a context class stored in the value. + * + *

The actual implementation of this method is generated automatically during compilation by + * the {@code io.opentelemetry.javaagent.tooling.muzzle.collector.MuzzleCodeGenerationPlugin} + * ByteBuddy plugin. + * + *

This method is generated automatically: if you override it, the muzzle compile plugin + * will not generate a new implementation, it will leave the existing one. + */ + public Map getMuzzleContextStoreClasses() { + return Collections.emptyMap(); } } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/AgentElementMatchers.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/AgentElementMatchers.java index 949b57e736..e57e0e1473 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/AgentElementMatchers.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/AgentElementMatchers.java @@ -16,7 +16,7 @@ import net.bytebuddy.matcher.ElementMatcher; /** * This class provides some custom ByteBuddy element matchers to use when applying instrumentation. */ -public class AgentElementMatchers { +public final class AgentElementMatchers { public static ElementMatcher.Junction extendsClass( ElementMatcher matcher) { @@ -92,4 +92,6 @@ public class AgentElementMatchers { } } } + + private AgentElementMatchers() {} } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/NameMatchers.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/NameMatchers.java index 9254eb2cbf..139c799209 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/NameMatchers.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/matcher/NameMatchers.java @@ -74,4 +74,6 @@ public final class NameMatchers { return (include && contained) || (!include && !contained); } } + + private NameMatchers() {} } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/ByteBuddyAgentCustomizer.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/ByteBuddyAgentCustomizer.java deleted file mode 100644 index 03a30e394b..0000000000 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/ByteBuddyAgentCustomizer.java +++ /dev/null @@ -1,34 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.spi; - -import io.opentelemetry.javaagent.extension.spi.AgentExtension; -import java.lang.instrument.Instrumentation; -import net.bytebuddy.agent.builder.AgentBuilder; - -/** - * {@link ByteBuddyAgentCustomizer} customizes ByteBuddy agent builder right before the agent is - * installed - {@link AgentBuilder#installOn(Instrumentation)}. This SPI can be used to customize - * {@link AgentBuilder} for vendor specific needs. For example install custom listeners or exclude - * classes. Use this SPI carefully because it can change {@link net.bytebuddy.ByteBuddy} behaviour. - * - *

This is a service provider interface that requires implementations to be registered in {@code - * META-INF/services} folder. - * - * @deprecated Use {@link AgentExtension} with correct {@link AgentExtension#order()} instead. - */ -@Deprecated -public interface ByteBuddyAgentCustomizer { - - /** - * Customize ByteBuddy agent builder before {@link AgentBuilder#installOn(Instrumentation)} is - * called. - * - * @param agentBuilder ByteBuddy agent from {@code AgentInstaller}. - * @return customized agent builder. - */ - AgentBuilder customize(AgentBuilder agentBuilder); -} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/ActualInstrumentationExtensionImplementation.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/ActualInstrumentationExtensionImplementation.java index 414099972e..3d42d05821 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/ActualInstrumentationExtensionImplementation.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/extension/instrumentation/ActualInstrumentationExtensionImplementation.java @@ -49,11 +49,11 @@ public final class ActualInstrumentationExtensionImplementation @Override AgentBuilder extend( InstrumentationModule instrumentationModule, AgentBuilder parentAgentBuilder) { - if (!instrumentationModule.enabled) { + if (!instrumentationModule.isEnabled()) { log.debug("Instrumentation {} is disabled", instrumentationModule.extensionName()); return parentAgentBuilder; } - List helperClassNames = instrumentationModule.getAllHelperClassNames(); + List helperClassNames = asList(instrumentationModule.getMuzzleHelperClassNames()); List helperResourceNames = asList(instrumentationModule.helperResourceNames()); List typeInstrumentations = instrumentationModule.typeInstrumentations(); if (typeInstrumentations.isEmpty()) { @@ -68,7 +68,7 @@ public final class ActualInstrumentationExtensionImplementation ElementMatcher.Junction moduleClassLoaderMatcher = instrumentationModule.classLoaderMatcher(); - MuzzleMatcher muzzleMatcher = new MuzzleMatcher(instrumentationModule); + MuzzleMatcher muzzleMatcher = new MuzzleMatcher(instrumentationModule, helperClassNames); AgentBuilder.Transformer helperInjector = new HelperInjector( instrumentationModule.extensionName(), helperClassNames, helperResourceNames); @@ -135,11 +135,14 @@ public final class ActualInstrumentationExtensionImplementation */ private static class MuzzleMatcher implements AgentBuilder.RawMatcher { private final InstrumentationModule instrumentationModule; + private final List helperClassNames; private final AtomicBoolean initialized = new AtomicBoolean(false); private volatile ReferenceMatcher referenceMatcher; - private MuzzleMatcher(InstrumentationModule instrumentationModule) { + private MuzzleMatcher( + InstrumentationModule instrumentationModule, List helperClassNames) { this.instrumentationModule = instrumentationModule; + this.helperClassNames = helperClassNames; } @Override @@ -185,7 +188,7 @@ public final class ActualInstrumentationExtensionImplementation if (initialized.compareAndSet(false, true)) { referenceMatcher = new ReferenceMatcher( - instrumentationModule.getAllHelperClassNames(), + helperClassNames, instrumentationModule.getMuzzleReferences(), instrumentationModule::isHelperClass); } diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java index a3c01a42fb..316d10dedb 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java @@ -20,7 +20,6 @@ import io.opentelemetry.javaagent.extension.spi.AgentExtension; import io.opentelemetry.javaagent.instrumentation.api.SafeServiceLoader; import io.opentelemetry.javaagent.instrumentation.api.internal.BootstrapPackagePrefixesHolder; import io.opentelemetry.javaagent.spi.BootstrapPackagesProvider; -import io.opentelemetry.javaagent.spi.ByteBuddyAgentCustomizer; import io.opentelemetry.javaagent.spi.ComponentInstaller; import io.opentelemetry.javaagent.spi.IgnoreMatcherProvider; import io.opentelemetry.javaagent.tooling.config.ConfigInitializer; @@ -170,7 +169,6 @@ public class AgentInstaller { } } - agentBuilder = customizeByteBuddyAgent(agentBuilder); log.debug("Installed {} instrumenter(s)", numInstrumenters); ResettableClassFileTransformer resettableClassFileTransformer = agentBuilder.installOn(inst); installComponentsAfterByteBuddy(componentInstallers, config); @@ -221,15 +219,6 @@ public class AgentInstaller { } } - private static AgentBuilder customizeByteBuddyAgent(AgentBuilder agentBuilder) { - Iterable agentCustomizers = loadByteBuddyAgentCustomizers(); - for (ByteBuddyAgentCustomizer agentCustomizer : agentCustomizers) { - log.debug("Applying agent builder customizer {}", agentCustomizer.getClass().getName()); - agentBuilder = agentCustomizer.customize(agentBuilder); - } - return agentBuilder; - } - private static Iterable loadComponentProviders() { return ServiceLoader.load(ComponentInstaller.class, AgentInstaller.class.getClassLoader()); } @@ -245,11 +234,6 @@ public class AgentInstaller { return new NoopIgnoreMatcherProvider(); } - private static Iterable loadByteBuddyAgentCustomizers() { - return ServiceLoader.load( - ByteBuddyAgentCustomizer.class, AgentInstaller.class.getClassLoader()); - } - private static List loadAgentExtensions() { // TODO: InstrumentationModule should no longer be an SPI Stream extensions = 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 5636ed6475..e0d2968516 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 @@ -195,7 +195,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { private void generateMuzzleHelperClassNamesMethod(ReferenceCollector collector) { /* - * protected String[] getMuzzleHelperClassNames() { + * public String[] getMuzzleHelperClassNames() { * return new String[] { * // sorted helper class names * }; @@ -203,7 +203,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { */ MethodVisitor mv = super.visitMethod( - Opcodes.ACC_PROTECTED, + Opcodes.ACC_PUBLIC, MUZZLE_HELPER_CLASSES_METHOD_NAME, "()[Ljava/lang/String;", null, @@ -236,7 +236,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { private void generateMuzzleReferencesMethod(ReferenceCollector collector) { /* - * protected synchronized Reference[] getMuzzleReferences() { + * public synchronized Reference[] getMuzzleReferences() { * if (null == this.muzzleReferences) { * this.muzzleReferences = new Reference[] { * // reference builders @@ -248,7 +248,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { try { MethodVisitor mv = super.visitMethod( - Opcodes.ACC_PROTECTED + Opcodes.ACC_SYNCHRONIZED, + Opcodes.ACC_PUBLIC + Opcodes.ACC_SYNCHRONIZED, MUZZLE_REFERENCES_METHOD_NAME, "()[Lio/opentelemetry/javaagent/extension/muzzle/Reference;", null, @@ -534,7 +534,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { private void generateMuzzleContextStoreClassesMethod(ReferenceCollector collector) { /* - * protected Map getMuzzleContextStoreClasses() { + * public Map getMuzzleContextStoreClasses() { * Map contextStore = new HashMap(); * contextStore.put(..., ...); * return contextStore; @@ -542,7 +542,7 @@ class MuzzleCodeGenerator implements AsmVisitorWrapper { */ MethodVisitor mv = super.visitMethod( - Opcodes.ACC_PROTECTED, + Opcodes.ACC_PUBLIC, MUZZLE_CONTEXT_STORE_CLASSES_METHOD_NAME, "()Ljava/util/Map;", null, diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/MuzzleGradlePluginUtil.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/MuzzleGradlePluginUtil.java index fb5a7be511..c62ebe9970 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/MuzzleGradlePluginUtil.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle/matcher/MuzzleGradlePluginUtil.java @@ -5,6 +5,8 @@ package io.opentelemetry.javaagent.tooling.muzzle.matcher; +import static java.util.Arrays.asList; + import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.extension.muzzle.Reference; import io.opentelemetry.javaagent.tooling.HelperInjector; @@ -49,7 +51,7 @@ public final class MuzzleGradlePluginUtil { ServiceLoader.load(InstrumentationModule.class, agentClassLoader)) { ReferenceMatcher muzzle = new ReferenceMatcher( - instrumentationModule.getAllHelperClassNames(), + asList(instrumentationModule.getMuzzleHelperClassNames()), instrumentationModule.getMuzzleReferences(), instrumentationModule::isHelperClass); List mismatches = muzzle.getMismatchedReferenceSources(userClassLoader); @@ -88,7 +90,7 @@ public final class MuzzleGradlePluginUtil { ServiceLoader.load(InstrumentationModule.class, agentClassLoader)) { try { // verify helper injector works - List allHelperClasses = instrumentationModule.getAllHelperClassNames(); + List allHelperClasses = asList(instrumentationModule.getMuzzleHelperClassNames()); if (!allHelperClasses.isEmpty()) { new HelperInjector( MuzzleGradlePluginUtil.class.getSimpleName(), diff --git a/testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/bytebuddy/TestByteBuddyAgentCustomizer.java b/testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/bytebuddy/TestAgentExtension.java similarity index 51% rename from testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/bytebuddy/TestByteBuddyAgentCustomizer.java rename to testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/bytebuddy/TestAgentExtension.java index cf37549daf..a40d53af8f 100644 --- a/testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/bytebuddy/TestByteBuddyAgentCustomizer.java +++ b/testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/bytebuddy/TestAgentExtension.java @@ -6,13 +6,19 @@ package io.opentelemetry.javaagent.testing.bytebuddy; import com.google.auto.service.AutoService; -import io.opentelemetry.javaagent.spi.ByteBuddyAgentCustomizer; +import io.opentelemetry.javaagent.extension.spi.AgentExtension; import net.bytebuddy.agent.builder.AgentBuilder; -@AutoService(ByteBuddyAgentCustomizer.class) -public class TestByteBuddyAgentCustomizer implements ByteBuddyAgentCustomizer { +@AutoService(AgentExtension.class) +public class TestAgentExtension implements AgentExtension { + @Override - public AgentBuilder customize(AgentBuilder agentBuilder) { + public AgentBuilder extend(AgentBuilder agentBuilder) { return agentBuilder.with(TestAgentListener.INSTANCE); } + + @Override + public String extensionName() { + return "test"; + } }