fix and improve manifest resource provider (#10857)

Co-authored-by: Lauri Tulmin <ltulmin@splunk.com>

instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/AttributeResourceProvider.java

@@ -17,7 +17,9 @@ import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 
 /**
  * An easier alternative to {@link io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider}, which
@@ -25,7 +27,6 @@ import java.util.function.Function;
  *
  * <p>An example of how to use this interface can be found in {@link ManifestResourceProvider}.
  */
-@SuppressWarnings({"unchecked", "rawtypes"})
 public abstract class AttributeResourceProvider<D> implements ConditionalResourceProvider {
 
   private final AttributeProvider<D> attributeProvider;
@@ -36,29 +37,31 @@ public abstract class AttributeResourceProvider<D> implements ConditionalResourc
 
     @CanIgnoreReturnValue
     @Override
+    @SuppressWarnings({"unchecked", "rawtypes"})
     public <T> AttributeBuilder add(AttributeKey<T> key, Function<D, Optional<T>> getter) {
       attributeGetters.put((AttributeKey) key, Objects.requireNonNull((Function) getter));
       return this;
     }
   }
 
-  private static final ThreadLocal<Resource> existingResource = new ThreadLocal<>();
+  private Set<AttributeKey<?>> filteredKeys;
 
   private final Map<AttributeKey<Object>, Function<D, Optional<?>>> attributeGetters =
       new HashMap<>();
 
-  public AttributeResourceProvider(AttributeProvider<D> attributeProvider) {
+  AttributeResourceProvider(AttributeProvider<D> attributeProvider) {
     this.attributeProvider = attributeProvider;
     attributeProvider.registerAttributes(new AttributeBuilder());
   }
 
   @Override
   public final boolean shouldApply(ConfigProperties config, Resource existing) {
-    existingResource.set(existing);
+    Map<String, String> resourceAttributes = config.getMap("otel.resource.attributes");
-
+    filteredKeys =
-    Map<String, String> resourceAttributes = getResourceAttributes(config);
+        attributeGetters.keySet().stream()
-    return attributeGetters.keySet().stream()
+            .filter(key -> shouldUpdate(config, existing, key, resourceAttributes))
-        .allMatch(key -> shouldUpdate(config, existing, key, resourceAttributes));
+            .collect(Collectors.toSet());
+    return !filteredKeys.isEmpty();
   }
 
   @Override
@@ -67,17 +70,12 @@ public abstract class AttributeResourceProvider<D> implements ConditionalResourc
         .readData()
         .map(
             data -> {
-              // what should we do here?
+              if (filteredKeys == null) {
-              // we don't have access to the existing resource
+                throw new IllegalStateException("shouldApply should be called first");
-              // if the resource provider produces a single key, we can rely on shouldApply
+              }
-              // i.e. this method won't be called if the key is already present
-              // the thread local is a hack to work around this
-              Resource existing =
-                  Objects.requireNonNull(existingResource.get(), "call shouldApply first");
-              Map<String, String> resourceAttributes = getResourceAttributes(config);
               AttributesBuilder builder = Attributes.builder();
               attributeGetters.entrySet().stream()
-                  .filter(e -> shouldUpdate(config, existing, e.getKey(), resourceAttributes))
+                  .filter(e -> filteredKeys.contains(e.getKey()))
                   .forEach(
                       e ->
                           e.getValue()
@@ -92,10 +90,6 @@ public abstract class AttributeResourceProvider<D> implements ConditionalResourc
     builder.put(key, value);
   }
 
-  private static Map<String, String> getResourceAttributes(ConfigProperties config) {
-    return config.getMap("otel.resource.attributes");
-  }
-
   private static boolean shouldUpdate(
       ConfigProperties config,
       Resource existing,

instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/JarServiceNameDetector.java

@@ -16,6 +16,8 @@ import io.opentelemetry.sdk.resources.Resource;
 import io.opentelemetry.semconv.ResourceAttributes;
 import java.nio.file.Path;
 import java.util.Map;
+import java.util.Optional;
+import java.util.function.Supplier;
 import java.util.logging.Logger;
 
 /**
@@ -26,22 +28,26 @@ public final class JarServiceNameDetector implements ConditionalResourceProvider
 
   private static final Logger logger = Logger.getLogger(JarServiceNameDetector.class.getName());
 
-  private final JarPathFinder jarPathFinder;
+  private final Supplier<Optional<Path>> jarPathSupplier;
 
   @SuppressWarnings("unused") // SPI
   public JarServiceNameDetector() {
-    this(new JarPathFinder());
+    this(MainJarPathHolder::getJarPath);
+  }
+
+  private JarServiceNameDetector(Supplier<Optional<Path>> jarPathSupplier) {
+    this.jarPathSupplier = jarPathSupplier;
   }
 
   // visible for tests
-  JarServiceNameDetector(JarPathFinder jarPathFinder) {
+  JarServiceNameDetector(MainJarPathFinder jarPathFinder) {
-    this.jarPathFinder = jarPathFinder;
+    this(() -> Optional.ofNullable(jarPathFinder.detectJarPath()));
   }
 
   @Override
   public Resource createResource(ConfigProperties config) {
-    return jarPathFinder
+    return jarPathSupplier
-        .getJarPath()
+        .get()
         .map(
             jarPath -> {
               String serviceName = getServiceName(jarPath);

instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/MainJarPathFinder.java

@@ -9,33 +9,22 @@ import java.nio.file.Files;
 import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
-import java.util.Optional;
 import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
-class JarPathFinder {
+class MainJarPathFinder {
   private final Supplier<String[]> getProcessHandleArguments;
   private final Function<String, String> getSystemProperty;
   private final Predicate<Path> fileExists;
 
-  private static class DetectionResult {
+  public MainJarPathFinder() {
-    private final Optional<Path> jarPath;
-
-    private DetectionResult(Optional<Path> jarPath) {
-      this.jarPath = jarPath;
-    }
-  }
-
-  private static Optional<DetectionResult> detectionResult = Optional.empty();
-
-  public JarPathFinder() {
     this(ProcessArguments::getProcessArguments, System::getProperty, Files::isRegularFile);
   }
 
   // visible for tests
-  JarPathFinder(
+  MainJarPathFinder(
       Supplier<String[]> getProcessHandleArguments,
       Function<String, String> getSystemProperty,
       Predicate<Path> fileExists) {
@@ -44,19 +33,7 @@ class JarPathFinder {
     this.fileExists = fileExists;
   }
 
-  // visible for testing
+  Path detectJarPath() {
-  static void resetForTest() {
-    detectionResult = Optional.empty();
-  }
-
-  Optional<Path> getJarPath() {
-    if (!detectionResult.isPresent()) {
-      detectionResult = Optional.of(new DetectionResult(Optional.ofNullable(detectJarPath())));
-    }
-    return detectionResult.get().jarPath;
-  }
-
-  private Path detectJarPath() {
     Path jarPath = getJarPathFromProcessHandle();
     if (jarPath != null) {
       return jarPath;

instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/MainJarPathHolder.java

@@ -0,0 +1,20 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.resources;
+
+import java.nio.file.Path;
+import java.util.Optional;
+
+class MainJarPathHolder {
+  private static final Optional<Path> jarPath =
+      Optional.ofNullable(new MainJarPathFinder().detectJarPath());
+
+  static Optional<Path> getJarPath() {
+    return jarPath;
+  }
+
+  private MainJarPathHolder() {}
+}

instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/ManifestResourceProvider.java

@@ -10,11 +10,12 @@ import static java.util.logging.Level.WARNING;
 import com.google.auto.service.AutoService;
 import io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider;
 import io.opentelemetry.semconv.ResourceAttributes;
-import java.io.InputStream;
+import java.io.IOException;
-import java.net.URL;
 import java.nio.file.Path;
 import java.util.Optional;
 import java.util.function.Function;
+import java.util.function.Supplier;
+import java.util.jar.JarFile;
 import java.util.jar.Manifest;
 import java.util.logging.Logger;
 
@@ -29,17 +30,16 @@ public final class ManifestResourceProvider extends AttributeResourceProvider<Ma
 
   @SuppressWarnings("unused") // SPI
   public ManifestResourceProvider() {
-    this(new JarPathFinder(), ManifestResourceProvider::readManifest);
+    this(MainJarPathHolder::getJarPath, ManifestResourceProvider::readManifest);
   }
 
-  // Visible for testing
+  private ManifestResourceProvider(
-  ManifestResourceProvider(
+      Supplier<Optional<Path>> jarPathSupplier, Function<Path, Optional<Manifest>> manifestReader) {
-      JarPathFinder jarPathFinder, Function<Path, Optional<Manifest>> manifestReader) {
     super(
         new AttributeProvider<Manifest>() {
           @Override
           public Optional<Manifest> readData() {
-            return jarPathFinder.getJarPath().flatMap(manifestReader);
+            return jarPathSupplier.get().flatMap(manifestReader);
           }
 
           @Override
@@ -63,14 +63,17 @@ public final class ManifestResourceProvider extends AttributeResourceProvider<Ma
         });
   }
 
+  // Visible for testing
+  ManifestResourceProvider(
+      MainJarPathFinder jarPathFinder, Function<Path, Optional<Manifest>> manifestReader) {
+    this(() -> Optional.ofNullable(jarPathFinder.detectJarPath()), manifestReader);
+  }
+
   private static Optional<Manifest> readManifest(Path jarPath) {
-    try (InputStream s =
+    try (JarFile jarFile = new JarFile(jarPath.toFile(), false)) {
-        new URL(String.format("jar:%s!/META-INF/MANIFEST.MF", jarPath.toUri())).openStream()) {
+      return Optional.of(jarFile.getManifest());
-      Manifest manifest = new Manifest();
+    } catch (IOException exception) {
-      manifest.read(s);
+      logger.log(WARNING, "Error reading manifest", exception);
-      return Optional.of(manifest);
-    } catch (Exception e) {
-      logger.log(WARNING, "Error reading manifest", e);
       return Optional.empty();
     }
   }

instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/JarServiceNameDetectorTest.java

@@ -16,7 +16,6 @@ import java.nio.file.Paths;
 import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.stream.Stream;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.ExtensionContext;
@@ -33,11 +32,6 @@ class JarServiceNameDetectorTest {
 
   @Mock ConfigProperties config;
 
-  @BeforeEach
-  void setUp() {
-    JarPathFinder.resetForTest();
-  }
-
   @Test
   void createResource_empty() {
     String[] processArgs = new String[0];
@@ -53,7 +47,7 @@ class JarServiceNameDetectorTest {
   private static JarServiceNameDetector getDetector(
       String[] processArgs, Function<String, String> getProperty, Predicate<Path> fileExists) {
     return new JarServiceNameDetector(
-        new JarPathFinder(() -> processArgs, getProperty, fileExists));
+        new MainJarPathFinder(() -> processArgs, getProperty, fileExists));
   }
 
   @Test

instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/ManifestResourceProviderTest.java

@@ -9,6 +9,7 @@ import static io.opentelemetry.semconv.ResourceAttributes.SERVICE_NAME;
 import static io.opentelemetry.semconv.ResourceAttributes.SERVICE_VERSION;
 import static org.assertj.core.api.Assertions.assertThat;
 
+import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
 import io.opentelemetry.sdk.autoconfigure.spi.internal.DefaultConfigProperties;
 import io.opentelemetry.sdk.resources.Resource;
@@ -19,28 +20,29 @@ import java.util.Optional;
 import java.util.jar.Manifest;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DynamicTest;
 import org.junit.jupiter.api.TestFactory;
 
 class ManifestResourceProviderTest {
 
-  @BeforeEach
-  void setUp() {
-    JarPathFinder.resetForTest();
-  }
-
   private static class TestCase {
     private final String name;
     private final String expectedName;
     private final String expectedVersion;
     private final InputStream input;
+    private final Resource existing;
 
-    public TestCase(String name, String expectedName, String expectedVersion, InputStream input) {
+    public TestCase(
+        String name,
+        String expectedName,
+        String expectedVersion,
+        InputStream input,
+        Resource existing) {
       this.name = name;
       this.expectedName = expectedName;
       this.expectedVersion = expectedVersion;
       this.input = input;
+      this.existing = existing;
     }
   }
 
@@ -49,10 +51,25 @@ class ManifestResourceProviderTest {
     ConfigProperties config = DefaultConfigProperties.createFromMap(Collections.emptyMap());
 
     return Stream.of(
-            new TestCase("name ok", "demo", "0.0.1-SNAPSHOT", openClasspathResource("MANIFEST.MF")),
-            new TestCase("name - no resource", null, null, null),
             new TestCase(
-                "name - empty resource", null, null, openClasspathResource("empty-MANIFEST.MF")))
+                "name ok",
+                "demo",
+                "0.0.1-SNAPSHOT",
+                openClasspathResource("MANIFEST.MF"),
+                Resource.getDefault()),
+            new TestCase("name - no resource", null, null, null, Resource.getDefault()),
+            new TestCase(
+                "name - empty resource",
+                null,
+                null,
+                openClasspathResource("empty-MANIFEST.MF"),
+                Resource.getDefault()),
+            new TestCase(
+                "name already detected",
+                null,
+                "0.0.1-SNAPSHOT",
+                openClasspathResource("MANIFEST.MF"),
+                Resource.create(Attributes.of(SERVICE_NAME, "old"))))
         .map(
             t ->
                 DynamicTest.dynamicTest(
@@ -60,7 +77,7 @@ class ManifestResourceProviderTest {
                     () -> {
                       ManifestResourceProvider provider =
                           new ManifestResourceProvider(
-                              new JarPathFinder(
+                              new MainJarPathFinder(
                                   () -> JarServiceNameDetectorTest.getArgs("app.jar"),
                                   prop -> null,
                                   JarServiceNameDetectorTest::failPath),
@@ -73,7 +90,7 @@ class ManifestResourceProviderTest {
                                   return Optional.empty();
                                 }
                               });
-                      provider.shouldApply(config, Resource.getDefault());
+                      provider.shouldApply(config, t.existing);
 
                       Resource resource = provider.createResource(config);
                       assertThat(resource.getAttribute(SERVICE_NAME)).isEqualTo(t.expectedName);