Make it possible to register multiple helper resources under the same… (

open-telemetry#5703)

* 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

instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/java/instrumentation/TestInstrumentationModule.java

@@ -32,6 +32,8 @@ public List<TypeInstrumentation> 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 {

instrumentation/internal/internal-class-loader/javaagent-integration-tests/src/main/resources/test-resources/test-resource-2.txt

@@ -0,0 +1 @@
+Hello there

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()
 

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<URL> resources) {
-      URL helper = HelperResources.load(classLoader, name);
-      if (helper == null) {
+      List<URL> helpers = HelperResources.loadAll(classLoader, name);
+      if (helpers.isEmpty()) {
         return;
       }
 
       if (!resources.hasMoreElements()) {
-        resources = Collections.enumeration(Collections.singleton(helper));
+        resources = Collections.enumeration(helpers);
         return;
       }
 
       List<URL> 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);

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<ClassLoader, Map<String, URL>> RESOURCES = Cache.weak();
-  private static final Map<String, URL> ALL_CLASSLOADERS_RESOURCES = new ConcurrentHashMap<>();
+  private static final Cache<ClassLoader, Map<String, List<URL>>> RESOURCES = Cache.weak();
+  private static final Map<String, List<URL>> 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<URL> 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<URL> 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<URL> append(@Nullable List<URL> resources, List<URL> toAdd) {
+    List<URL> 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<URL> 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<String, URL> map = RESOURCES.get(classLoader);
-    URL resource = null;
+  public static URL loadOne(ClassLoader classLoader, String path) {
+    List<URL> 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<URL> loadAll(ClassLoader classLoader, String path) {
+    Map<String, List<URL>> map = RESOURCES.get(classLoader);
+    List<URL> 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() {}

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<URL> 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<URL> 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<URL> result, File file) {
     try {
       URL wrappedUrl = new URL("otel", null, -1, "/", new RemappingUrlStreamHandler(file));

muzzle/src/main/java/io/opentelemetry/javaagent/tooling/HelperInjector.java

@@ -176,30 +176,41 @@ private void injectHelperResources(ClassLoader classLoader) {
         classLoader,
         cl -> {
           for (HelperResource helperResource : helperResources) {
-            URL resource = helpersSource.getResource(helperResource.getAgentPath());
-            if (resource == null) {
+            List<URL> 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;
             }
 
             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);
             }
           }