From f5f2de9511755711621a1137f9882df3f8d7c499 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Thu, 1 Jul 2021 17:26:51 +0200 Subject: [PATCH] Refactor/rename BootstrapPackagesProvider and PropertySource (#3435) * Refactor/rename BootstrapPackagesConfigurer and PropertySource * Update comment --- .../ClassLoaderInstrumentation.java | 3 ++ .../bootstrap/BootstrapPackagesBuilder.java | 33 ++++++++++++++++++ .../BootstrapPackagesConfigurer.java | 28 +++++++++++++++ .../config/ConfigPropertySource.java} | 8 +++-- .../spi/BootstrapPackagesProvider.java | 24 ------------- .../javaagent/tooling/AgentInstaller.java | 32 ++++++++--------- .../BootstrapPackagesBuilderImpl.java | 34 +++++++++++++++++++ .../tooling/config/ConfigInitializer.java | 7 ++-- .../AgentTestingExporterPropertySource.java | 6 ++-- 9 files changed, 126 insertions(+), 49 deletions(-) create mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/bootstrap/BootstrapPackagesBuilder.java create mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/bootstrap/BootstrapPackagesConfigurer.java rename javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/{spi/config/PropertySource.java => extension/config/ConfigPropertySource.java} (66%) delete mode 100644 javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/BootstrapPackagesProvider.java create mode 100644 javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bootstrap/BootstrapPackagesBuilderImpl.java diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentation.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentation.java index c2d0e764d1..5851d6a79f 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentation.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ClassLoaderInstrumentation.java @@ -28,6 +28,7 @@ import java.util.List; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; +import org.slf4j.LoggerFactory; /* * Some class loaders do not delegate to their parent, so classes in those class loaders @@ -90,6 +91,8 @@ public class ClassLoaderInstrumentation implements TypeInstrumentation { //noinspection unchecked return (List) methodHandle.invokeExact(); } catch (Throwable e) { + LoggerFactory.getLogger(Holder.class) + .warn("Unable to load bootstrap package prefixes from the bootstrap CL", e); return Constants.BOOTSTRAP_PACKAGE_PREFIXES; } } diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/bootstrap/BootstrapPackagesBuilder.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/bootstrap/BootstrapPackagesBuilder.java new file mode 100644 index 0000000000..5eaf6acdf9 --- /dev/null +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/bootstrap/BootstrapPackagesBuilder.java @@ -0,0 +1,33 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.extension.bootstrap; + +import java.util.Collection; + +/** + * This interface exposes a way to define which packages/classes are present in the bootstrap + * classloader. + * + *

This interface should not be implemented by the javaagent extension developer - the javaagent + * will provide the implementation. + */ +public interface BootstrapPackagesBuilder { + + /** + * Mark {@code classNameOrPrefix} as one that belongs to the bootstrap classloader. + * + * @return {@code this} + */ + BootstrapPackagesBuilder add(String classNameOrPrefix); + + /** + * Mark all elements of {@code classNamesOrPrefixes} as ones that belongs to the bootstrap + * classloader. + * + * @return {@code this} + */ + BootstrapPackagesBuilder addAll(Collection classNamesOrPrefixes); +} diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/bootstrap/BootstrapPackagesConfigurer.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/bootstrap/BootstrapPackagesConfigurer.java new file mode 100644 index 0000000000..33fa29311d --- /dev/null +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/bootstrap/BootstrapPackagesConfigurer.java @@ -0,0 +1,28 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.extension.bootstrap; + +import io.opentelemetry.instrumentation.api.config.Config; + +/** + * This SPI can be used to define which packages/classes belong to the bootstrap class loader: all + * packages configured here will always be loaded by the bootstrap class loader, even if class + * loader that initiated loading of the class does not normally delegate to bootstrap class loader. + * + *

IMPORTANT: This SPI cannot add new packages to the bootstrap CL, it only defines those + * that are already there - the purpose is to make sure they're loaded by the correct class loader. + * + *

This is a service provider interface that requires implementations to be registered in a + * provider-configuration file stored in the {@code META-INF/services} resource directory. + */ +public interface BootstrapPackagesConfigurer { + + /** + * Configure the passed {@code builder} and define which classes should always be loaded by the + * bootstrap class loader. + */ + void configure(Config config, BootstrapPackagesBuilder builder); +} diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/config/PropertySource.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/config/ConfigPropertySource.java similarity index 66% rename from javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/config/PropertySource.java rename to javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/config/ConfigPropertySource.java index 10881ad9fe..75f2ae20c2 100644 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/config/PropertySource.java +++ b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/config/ConfigPropertySource.java @@ -3,8 +3,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -package io.opentelemetry.javaagent.spi.config; +package io.opentelemetry.javaagent.extension.config; +import io.opentelemetry.javaagent.extension.Ordered; import java.util.Map; /** @@ -12,8 +13,11 @@ import java.util.Map; * by implementations of this interface will be used after the following methods fail to find a * non-empty property value: system properties, environment variables, properties configuration * file. + * + *

This is a service provider interface that requires implementations to be registered in a + * provider-configuration file stored in the {@code META-INF/services} resource directory. */ -public interface PropertySource { +public interface ConfigPropertySource extends Ordered { /** * Returns all properties whose default values are overridden by this property source. Key of the * map is the propertyName (same as system property name, e.g. {@code otel.traces.exporter}), diff --git a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/BootstrapPackagesProvider.java b/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/BootstrapPackagesProvider.java deleted file mode 100644 index e2f067ca6f..0000000000 --- a/javaagent-extension-api/src/main/java/io/opentelemetry/javaagent/spi/BootstrapPackagesProvider.java +++ /dev/null @@ -1,24 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.spi; - -import java.util.List; - -/** - * A service provider to allow adding classes from specified package prefixes to the bootstrap - * classloader. The classes in the bootstrap classloader are available to all instrumentations. This - * is useful if large number of custom instrumentations are using functionality from common - * packages. - */ -public interface BootstrapPackagesProvider { - - /** - * Classes from returned package prefixes will be available in the bootstrap classloader. - * - * @return package prefixes. - */ - List getPackagePrefixes(); -} 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 d44978b915..784b6a069f 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 @@ -6,6 +6,7 @@ package io.opentelemetry.javaagent.tooling; import static io.opentelemetry.javaagent.bootstrap.AgentInitializer.isJavaBefore9; +import static io.opentelemetry.javaagent.tooling.SafeServiceLoader.load; import static io.opentelemetry.javaagent.tooling.SafeServiceLoader.loadOrdered; import static io.opentelemetry.javaagent.tooling.Utils.getResourceName; import static net.bytebuddy.matcher.ElementMatchers.any; @@ -14,12 +15,13 @@ import io.opentelemetry.instrumentation.api.config.Config; import io.opentelemetry.javaagent.bootstrap.AgentClassLoader; import io.opentelemetry.javaagent.extension.AgentExtension; import io.opentelemetry.javaagent.extension.AgentListener; +import io.opentelemetry.javaagent.extension.bootstrap.BootstrapPackagesConfigurer; import io.opentelemetry.javaagent.extension.ignore.IgnoredTypesConfigurer; import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule; import io.opentelemetry.javaagent.instrumentation.api.internal.BootstrapPackagePrefixesHolder; import io.opentelemetry.javaagent.instrumentation.api.internal.InstrumentedTaskClasses; -import io.opentelemetry.javaagent.spi.BootstrapPackagesProvider; import io.opentelemetry.javaagent.tooling.asyncannotationsupport.WeakRefAsyncOperationEndStrategies; +import io.opentelemetry.javaagent.tooling.bootstrap.BootstrapPackagesBuilderImpl; import io.opentelemetry.javaagent.tooling.config.ConfigInitializer; import io.opentelemetry.javaagent.tooling.context.FieldBackedProvider; import io.opentelemetry.javaagent.tooling.ignore.IgnoredClassLoadersMatcher; @@ -70,9 +72,9 @@ public class AgentInstaller { log = LoggerFactory.getLogger(AgentInstaller.class); addByteBuddyRawSetting(); - BootstrapPackagePrefixesHolder.setBoostrapPackagePrefixes(loadBootstrapPackagePrefixes()); // this needs to be done as early as possible - before the first Config.get() call ConfigInitializer.initialize(); + // ensure java.lang.reflect.Proxy is loaded, as transformation code uses it internally // loading java.lang.reflect.Proxy after the bytebuddy transformer is set up causes // the internal-proxy instrumentation module to transform it, and then the bytebuddy @@ -115,6 +117,9 @@ public class AgentInstaller { WeakRefAsyncOperationEndStrategies.initialize(); Config config = Config.get(); + + setBootstrapPackages(config); + runBeforeAgentListeners(agentListeners, config); instrumentation = inst; @@ -169,6 +174,14 @@ public class AgentInstaller { return resettableClassFileTransformer; } + private static void setBootstrapPackages(Config config) { + BootstrapPackagesBuilderImpl builder = new BootstrapPackagesBuilderImpl(); + for (BootstrapPackagesConfigurer configurer : load(BootstrapPackagesConfigurer.class)) { + configurer.configure(config, builder); + } + BootstrapPackagePrefixesHolder.setBoostrapPackagePrefixes(builder.build()); + } + private static void runBeforeAgentListeners( Iterable agentListeners, Config config) { for (AgentListener agentListener : agentListeners) { @@ -239,21 +252,6 @@ public class AgentInstaller { } } - private static List loadBootstrapPackagePrefixes() { - List bootstrapPackages = new ArrayList<>(Constants.BOOTSTRAP_PACKAGE_PREFIXES); - Iterable bootstrapPackagesProviders = - SafeServiceLoader.load(BootstrapPackagesProvider.class); - for (BootstrapPackagesProvider provider : bootstrapPackagesProviders) { - List packagePrefixes = provider.getPackagePrefixes(); - log.debug( - "Loaded bootstrap package prefixes from {}: {}", - provider.getClass().getName(), - packagePrefixes); - bootstrapPackages.addAll(packagePrefixes); - } - return bootstrapPackages; - } - static class RedefinitionLoggingListener implements AgentBuilder.RedefinitionStrategy.Listener { private static final Logger log = LoggerFactory.getLogger(RedefinitionLoggingListener.class); diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bootstrap/BootstrapPackagesBuilderImpl.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bootstrap/BootstrapPackagesBuilderImpl.java new file mode 100644 index 0000000000..588bbaf72e --- /dev/null +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/bootstrap/BootstrapPackagesBuilderImpl.java @@ -0,0 +1,34 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.tooling.bootstrap; + +import io.opentelemetry.javaagent.extension.bootstrap.BootstrapPackagesBuilder; +import io.opentelemetry.javaagent.tooling.Constants; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; + +public class BootstrapPackagesBuilderImpl implements BootstrapPackagesBuilder { + + // TODO: consider using a Trie + private final List packages = new ArrayList<>(Constants.BOOTSTRAP_PACKAGE_PREFIXES); + + @Override + public BootstrapPackagesBuilder add(String classNameOrPrefix) { + packages.add(classNameOrPrefix); + return this; + } + + @Override + public BootstrapPackagesBuilder addAll(Collection classNamesOrPrefixes) { + packages.addAll(classNamesOrPrefixes); + return this; + } + + public List build() { + return packages; + } +} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigInitializer.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigInitializer.java index 06529e4d4a..a6d881a875 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigInitializer.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/config/ConfigInitializer.java @@ -5,9 +5,10 @@ package io.opentelemetry.javaagent.tooling.config; +import static io.opentelemetry.javaagent.tooling.SafeServiceLoader.loadOrdered; + import io.opentelemetry.instrumentation.api.config.Config; -import io.opentelemetry.javaagent.spi.config.PropertySource; -import io.opentelemetry.javaagent.tooling.SafeServiceLoader; +import io.opentelemetry.javaagent.extension.config.ConfigPropertySource; import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -41,7 +42,7 @@ public final class ConfigInitializer { /** Retrieves all default configuration overloads using SPI and initializes Config. */ private static Properties loadSpiConfiguration() { Properties propertiesFromSpi = new Properties(); - for (PropertySource propertySource : SafeServiceLoader.load(PropertySource.class)) { + for (ConfigPropertySource propertySource : loadOrdered(ConfigPropertySource.class)) { propertiesFromSpi.putAll(propertySource.getProperties()); } return propertiesFromSpi; diff --git a/testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/exporter/AgentTestingExporterPropertySource.java b/testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/exporter/AgentTestingExporterPropertySource.java index c792481e0a..464074d774 100644 --- a/testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/exporter/AgentTestingExporterPropertySource.java +++ b/testing/agent-exporter/src/main/java/io/opentelemetry/javaagent/testing/exporter/AgentTestingExporterPropertySource.java @@ -6,12 +6,12 @@ package io.opentelemetry.javaagent.testing.exporter; import com.google.auto.service.AutoService; -import io.opentelemetry.javaagent.spi.config.PropertySource; +import io.opentelemetry.javaagent.extension.config.ConfigPropertySource; import java.util.HashMap; import java.util.Map; -@AutoService(PropertySource.class) -public class AgentTestingExporterPropertySource implements PropertySource { +@AutoService(ConfigPropertySource.class) +public class AgentTestingExporterPropertySource implements ConfigPropertySource { @Override public Map getProperties() { Map properties = new HashMap<>();