diff --git a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/AttributeResourceProvider.java b/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/AttributeResourceProvider.java index e8ab69d2d3..95fbc85208 100644 --- a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/AttributeResourceProvider.java +++ b/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; * *

An example of how to use this interface can be found in {@link ManifestResourceProvider}. */ -@SuppressWarnings({"unchecked", "rawtypes"}) public abstract class AttributeResourceProvider implements ConditionalResourceProvider { private final AttributeProvider attributeProvider; @@ -36,29 +37,31 @@ public abstract class AttributeResourceProvider implements ConditionalResourc @CanIgnoreReturnValue @Override + @SuppressWarnings({"unchecked", "rawtypes"}) public AttributeBuilder add(AttributeKey key, Function> getter) { attributeGetters.put((AttributeKey) key, Objects.requireNonNull((Function) getter)); return this; } } - private static final ThreadLocal existingResource = new ThreadLocal<>(); + private Set> filteredKeys; private final Map, Function>> attributeGetters = new HashMap<>(); - public AttributeResourceProvider(AttributeProvider attributeProvider) { + AttributeResourceProvider(AttributeProvider attributeProvider) { this.attributeProvider = attributeProvider; attributeProvider.registerAttributes(new AttributeBuilder()); } @Override public final boolean shouldApply(ConfigProperties config, Resource existing) { - existingResource.set(existing); - - Map resourceAttributes = getResourceAttributes(config); - return attributeGetters.keySet().stream() - .allMatch(key -> shouldUpdate(config, existing, key, resourceAttributes)); + Map resourceAttributes = config.getMap("otel.resource.attributes"); + filteredKeys = + attributeGetters.keySet().stream() + .filter(key -> shouldUpdate(config, existing, key, resourceAttributes)) + .collect(Collectors.toSet()); + return !filteredKeys.isEmpty(); } @Override @@ -67,17 +70,12 @@ public abstract class AttributeResourceProvider implements ConditionalResourc .readData() .map( data -> { - // what should we do here? - // we don't have access to the existing resource - // 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 resourceAttributes = getResourceAttributes(config); + if (filteredKeys == null) { + throw new IllegalStateException("shouldApply should be called first"); + } 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 implements ConditionalResourc builder.put(key, value); } - private static Map getResourceAttributes(ConfigProperties config) { - return config.getMap("otel.resource.attributes"); - } - private static boolean shouldUpdate( ConfigProperties config, Resource existing, diff --git a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/JarServiceNameDetector.java b/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/JarServiceNameDetector.java index 51b379f292..ab855705e2 100644 --- a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/JarServiceNameDetector.java +++ b/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> jarPathSupplier; @SuppressWarnings("unused") // SPI public JarServiceNameDetector() { - this(new JarPathFinder()); + this(MainJarPathHolder::getJarPath); + } + + private JarServiceNameDetector(Supplier> jarPathSupplier) { + this.jarPathSupplier = jarPathSupplier; } // visible for tests - JarServiceNameDetector(JarPathFinder jarPathFinder) { - this.jarPathFinder = jarPathFinder; + JarServiceNameDetector(MainJarPathFinder jarPathFinder) { + this(() -> Optional.ofNullable(jarPathFinder.detectJarPath())); } @Override public Resource createResource(ConfigProperties config) { - return jarPathFinder - .getJarPath() + return jarPathSupplier + .get() .map( jarPath -> { String serviceName = getServiceName(jarPath); diff --git a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/JarPathFinder.java b/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/MainJarPathFinder.java similarity index 77% rename from instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/JarPathFinder.java rename to instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/MainJarPathFinder.java index 0c25176a67..8f9b9287ac 100644 --- a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/JarPathFinder.java +++ b/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 getProcessHandleArguments; private final Function getSystemProperty; private final Predicate fileExists; - private static class DetectionResult { - private final Optional jarPath; - - private DetectionResult(Optional jarPath) { - this.jarPath = jarPath; - } - } - - private static Optional detectionResult = Optional.empty(); - - public JarPathFinder() { + public MainJarPathFinder() { this(ProcessArguments::getProcessArguments, System::getProperty, Files::isRegularFile); } // visible for tests - JarPathFinder( + MainJarPathFinder( Supplier getProcessHandleArguments, Function getSystemProperty, Predicate fileExists) { @@ -44,19 +33,7 @@ class JarPathFinder { this.fileExists = fileExists; } - // visible for testing - static void resetForTest() { - detectionResult = Optional.empty(); - } - - Optional getJarPath() { - if (!detectionResult.isPresent()) { - detectionResult = Optional.of(new DetectionResult(Optional.ofNullable(detectJarPath()))); - } - return detectionResult.get().jarPath; - } - - private Path detectJarPath() { + Path detectJarPath() { Path jarPath = getJarPathFromProcessHandle(); if (jarPath != null) { return jarPath; diff --git a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/MainJarPathHolder.java b/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/MainJarPathHolder.java new file mode 100644 index 0000000000..c6948d3eca --- /dev/null +++ b/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 jarPath = + Optional.ofNullable(new MainJarPathFinder().detectJarPath()); + + static Optional getJarPath() { + return jarPath; + } + + private MainJarPathHolder() {} +} diff --git a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/ManifestResourceProvider.java b/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/ManifestResourceProvider.java index 461af3cd99..335583d124 100644 --- a/instrumentation/resources/library/src/main/java/io/opentelemetry/instrumentation/resources/ManifestResourceProvider.java +++ b/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.net.URL; +import java.io.IOException; 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> manifestReader) { + private ManifestResourceProvider( + Supplier> jarPathSupplier, Function> manifestReader) { super( new AttributeProvider() { @Override public Optional readData() { - return jarPathFinder.getJarPath().flatMap(manifestReader); + return jarPathSupplier.get().flatMap(manifestReader); } @Override @@ -63,14 +63,17 @@ public final class ManifestResourceProvider extends AttributeResourceProvider> manifestReader) { + this(() -> Optional.ofNullable(jarPathFinder.detectJarPath()), manifestReader); + } + private static Optional readManifest(Path jarPath) { - try (InputStream s = - new URL(String.format("jar:%s!/META-INF/MANIFEST.MF", jarPath.toUri())).openStream()) { - Manifest manifest = new Manifest(); - manifest.read(s); - return Optional.of(manifest); - } catch (Exception e) { - logger.log(WARNING, "Error reading manifest", e); + try (JarFile jarFile = new JarFile(jarPath.toFile(), false)) { + return Optional.of(jarFile.getManifest()); + } catch (IOException exception) { + logger.log(WARNING, "Error reading manifest", exception); return Optional.empty(); } } diff --git a/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/JarServiceNameDetectorTest.java b/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/JarServiceNameDetectorTest.java index dd9562d572..bb4d6c9d16 100644 --- a/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/JarServiceNameDetectorTest.java +++ b/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 getProperty, Predicate fileExists) { return new JarServiceNameDetector( - new JarPathFinder(() -> processArgs, getProperty, fileExists)); + new MainJarPathFinder(() -> processArgs, getProperty, fileExists)); } @Test diff --git a/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/ManifestResourceProviderTest.java b/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/ManifestResourceProviderTest.java index d789dbb0ff..d61b53a43e 100644 --- a/instrumentation/resources/library/src/test/java/io/opentelemetry/instrumentation/resources/ManifestResourceProviderTest.java +++ b/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);