From 11a0931e6c39e4431b5c07629ce5341058ef57a1 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Thu, 31 Mar 2022 11:51:46 +0200 Subject: [PATCH] =?UTF-8?q?Make=20it=20possible=20to=20register=20multiple?= =?UTF-8?q?=20helper=20resources=20under=20the=20same=E2=80=A6=20(#5703)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Make it possible to register multiple helper resources under the same name * go back to using the old property in tests after all * code review comments --- .../TestInstrumentationModule.java | 2 + .../test-resources/test-resource-2.txt | 1 + .../test/groovy/ResourceInjectionTest.groovy | 7 +- .../ResourceInjectionInstrumentation.java | 27 ++++--- .../javaagent/bootstrap/HelperResources.java | 74 +++++++++++++++---- .../tooling/ExtensionClassLoader.java | 14 ++-- .../javaagent/tooling/HelperInjector.java | 25 +++++-- 7 files changed, 104 insertions(+), 46 deletions(-) create mode 100644 instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/resources/test-resources/test-resource-2.txt diff --git a/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java index 8bc20d18cf73..e4044ababaa7 100644 --- a/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java +++ b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java @@ -32,6 +32,8 @@ public List typeInstrumentations() { @Override public void registerHelperResources(HelperResourceBuilder helperResourceBuilder) { helperResourceBuilder.register("test-resources/test-resource.txt"); + helperResourceBuilder.register( + "test-resources/test-resource.txt", "test-resources/test-resource-2.txt"); } public static class TestTypeInstrumentation implements TypeInstrumentation { diff --git a/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/resources/test-resources/test-resource-2.txt b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/resources/test-resources/test-resource-2.txt new file mode 100644 index 000000000000..d6613f5f8b58 --- /dev/null +++ b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/resources/test-resources/test-resource-2.txt @@ -0,0 +1 @@ +Hello there diff --git a/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/test/groovy/ResourceInjectionTest.groovy b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/test/groovy/ResourceInjectionTest.groovy index 565c89eabfb4..5636b490b4b3 100644 --- a/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/test/groovy/ResourceInjectionTest.groovy +++ b/instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/test/groovy/ResourceInjectionTest.groovy @@ -30,11 +30,12 @@ class ResourceInjectionTest extends AgentInstrumentationSpecification { // this triggers resource injection emptyLoader.get().loadClass(SystemUtils.getName()) - resourceUrls = emptyLoader.get().getResources(resourceName) + resourceUrls = Collections.list(emptyLoader.get().getResources(resourceName)) then: - resourceUrls.hasMoreElements() - resourceUrls.nextElement().openStream().text.trim() == 'Hello world!' + resourceUrls.size() == 2 + resourceUrls.get(0).openStream().text.trim() == 'Hello world!' + resourceUrls.get(1).openStream().text.trim() == 'Hello there' !notInjectedLoader.getResources(resourceName).hasMoreElements() diff --git a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ResourceInjectionInstrumentation.java b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ResourceInjectionInstrumentation.java index 23e24520f3b0..cd2404c62712 100644 --- a/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ResourceInjectionInstrumentation.java +++ b/instrumentation/internal/internal-class-loader/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/internal/classloader/ResourceInjectionInstrumentation.java @@ -58,7 +58,7 @@ public static void onExit( @Advice.Argument(0) String name, @Advice.Return(readOnly = false) URL resource) { - URL helper = HelperResources.load(classLoader, name); + URL helper = HelperResources.loadOne(classLoader, name); if (helper != null) { resource = helper; } @@ -73,26 +73,29 @@ public static void onExit( @Advice.This ClassLoader classLoader, @Advice.Argument(0) String name, @Advice.Return(readOnly = false) Enumeration resources) { - URL helper = HelperResources.load(classLoader, name); - if (helper == null) { + List helpers = HelperResources.loadAll(classLoader, name); + if (helpers.isEmpty()) { return; } if (!resources.hasMoreElements()) { - resources = Collections.enumeration(Collections.singleton(helper)); + resources = Collections.enumeration(helpers); return; } List result = Collections.list(resources); - boolean duplicate = false; - for (URL loadedUrl : result) { - if (helper.sameFile(loadedUrl)) { - duplicate = true; - break; + + for (URL helperUrl : helpers) { + boolean duplicate = false; + for (URL loadedUrl : result) { + if (helperUrl.sameFile(loadedUrl)) { + duplicate = true; + break; + } + } + if (!duplicate) { + result.add(helperUrl); } - } - if (!duplicate) { - result.add(helper); } resources = Collections.enumeration(result); diff --git a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/HelperResources.java b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/HelperResources.java index 56dbf8f749f1..c79b50d27a04 100644 --- a/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/HelperResources.java +++ b/javaagent-bootstrap/src/main/java/io/opentelemetry/javaagent/bootstrap/HelperResources.java @@ -5,10 +5,16 @@ package io.opentelemetry.javaagent.bootstrap; +import static java.util.Collections.emptyList; +import static java.util.Collections.unmodifiableList; + import io.opentelemetry.instrumentation.api.cache.Cache; import java.net.URL; +import java.util.ArrayList; +import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import javax.annotation.Nullable; /** * A holder of resources needed by instrumentation. We store them in the bootstrap classloader so @@ -17,36 +23,72 @@ */ public final class HelperResources { - private static final Cache> RESOURCES = Cache.weak(); - private static final Map ALL_CLASSLOADERS_RESOURCES = new ConcurrentHashMap<>(); + private static final Cache>> RESOURCES = Cache.weak(); + private static final Map> ALL_CLASSLOADERS_RESOURCES = + new ConcurrentHashMap<>(); /** - * Registers the {@code url} to be available to instrumentation at {@code path}, when given {@code - * classLoader} attempts to load that resource. + * Registers the {@code urls} to be available to instrumentation at {@code path}, when given + * {@code classLoader} attempts to load that resource. */ - public static void register(ClassLoader classLoader, String path, URL url) { - RESOURCES.computeIfAbsent(classLoader, unused -> new ConcurrentHashMap<>()).put(path, url); + public static void register(ClassLoader classLoader, String path, List urls) { + RESOURCES + .computeIfAbsent(classLoader, unused -> new ConcurrentHashMap<>()) + .compute(path, (k, v) -> append(v, urls)); + } + + /** Registers the {@code urls} to be available to instrumentation at {@code path}. */ + public static void registerForAllClassLoaders(String path, List urls) { + ALL_CLASSLOADERS_RESOURCES.compute(path, (k, v) -> append(v, urls)); } - /** Registers the {@code url} to be available to instrumentation at {@code path}. */ - public static void registerForAllClassLoaders(String path, URL url) { - ALL_CLASSLOADERS_RESOURCES.putIfAbsent(path, url); + private static List append(@Nullable List resources, List toAdd) { + List newResources = resources == null ? new ArrayList<>() : new ArrayList<>(resources); + for (URL newResource : toAdd) { + // make sure to de-dupe resources - each extension classloader has the agent classloader as + // its parent, and the MultipleParentClassLoader (that every individual extension CL gets put + // into) concatenates all found resources on getResources(); this means that if you ask for a + // built-in agent resource, each extension CL will also return URL pointing to it, thus the + // final collection will have (no of extension CLs) + 1 duplicates of the same URL + if (!containsSameFile(newResources, newResource)) { + newResources.add(newResource); + } + } + return unmodifiableList(newResources); + } + + private static boolean containsSameFile(List haystack, URL needle) { + for (URL r : haystack) { + if (r.sameFile(needle)) { + return true; + } + } + return false; } /** * Returns a {@link URL} that can be used to retrieve the content of the resource at {@code path}, * or {@code null} if no resource could be found at {@code path}. */ - public static URL load(ClassLoader classLoader, String path) { - Map map = RESOURCES.get(classLoader); - URL resource = null; + public static URL loadOne(ClassLoader classLoader, String path) { + List resources = loadAll(classLoader, path); + return resources.isEmpty() ? null : resources.get(0); + } + + /** + * Returns all {@link URL}s that can be used to retrieve the content of the resource at {@code + * path}. + */ + public static List loadAll(ClassLoader classLoader, String path) { + Map> map = RESOURCES.get(classLoader); + List resources = null; if (map != null) { - resource = map.get(path); + resources = map.get(path); } - if (resource == null) { - resource = ALL_CLASSLOADERS_RESOURCES.get(path); + if (resources == null) { + resources = ALL_CLASSLOADERS_RESOURCES.get(path); } - return resource; + return resources == null ? emptyList() : resources; } private HelperResources() {} diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java index 658782bcec2b..fac4a862a361 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExtensionClassLoader.java @@ -61,12 +61,6 @@ public static ClassLoader getInstance(ClassLoader parent, File javaagentFile) { System.getenv("OTEL_JAVAAGENT_EXPERIMENTAL_EXTENSIONS")), javaagentFile)); - extensions.addAll( - parseLocation( - System.getProperty( - "otel.javaagent.experimental.initializer.jar", - System.getenv("OTEL_JAVAAGENT_EXPERIMENTAL_INITIALIZER_JAR")), - javaagentFile)); // TODO when logging is configured add warning about deprecated property if (extensions.isEmpty()) { @@ -128,10 +122,10 @@ private static List parseLocation(String locationName, File javaagentFile) } File location = new File(locationName); - if (location.isFile()) { + if (isJar(location)) { addFileUrl(result, location); } else if (location.isDirectory()) { - File[] files = location.listFiles(f -> f.isFile() && f.getName().endsWith(".jar")); + File[] files = location.listFiles(ExtensionClassLoader::isJar); if (files != null) { for (File file : files) { if (!file.getAbsolutePath().equals(javaagentFile.getAbsolutePath())) { @@ -143,6 +137,10 @@ private static List parseLocation(String locationName, File javaagentFile) return result; } + private static boolean isJar(File f) { + return f.isFile() && f.getName().endsWith(".jar"); + } + private static void addFileUrl(List result, File file) { try { URL wrappedUrl = new URL("otel", null, -1, "/", new RemappingUrlStreamHandler(file)); diff --git a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java index 0571f90b5c9f..92d1fc73d73f 100644 --- a/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java +++ b/muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java @@ -176,11 +176,22 @@ private void injectHelperResources(ClassLoader classLoader) { classLoader, cl -> { for (HelperResource helperResource : helperResources) { - URL resource = helpersSource.getResource(helperResource.getAgentPath()); - if (resource == null) { + List resources; + try { + resources = + Collections.list(helpersSource.getResources(helperResource.getAgentPath())); + } catch (IOException e) { + logger.log( + SEVERE, + "Unexpected exception occurred when loading resources {}; skipping", + new Object[] {helperResource.getAgentPath()}, + e); + continue; + } + if (resources.isEmpty()) { logger.log( FINE, - "Helper resource {0} requested but not found.", + "Helper resources {0} requested but not found.", helperResource.getAgentPath()); continue; } @@ -188,18 +199,18 @@ private void injectHelperResources(ClassLoader classLoader) { if (helperResource.allClassLoaders()) { logger.log( FINE, - "Injecting resource onto all classloaders: {0}", + "Injecting resources onto all classloaders: {0}", helperResource.getApplicationPath()); HelperResources.registerForAllClassLoaders( - helperResource.getApplicationPath(), resource); + helperResource.getApplicationPath(), resources); } else { if (logger.isLoggable(FINE)) { logger.log( FINE, - "Injecting resource onto classloader {0} -> {1}", + "Injecting resources onto classloader {0} -> {1}", new Object[] {classLoader, helperResource.getApplicationPath()}); } - HelperResources.register(classLoader, helperResource.getApplicationPath(), resource); + HelperResources.register(classLoader, helperResource.getApplicationPath(), resources); } }