optimize resource detection by skipping providers that would add no n…

…ew keys
open-telemetry · Feb 13, 2024 · a8b30ea · a8b30ea
1 parent 48d41d3
commit a8b30ea
Show file tree

Hide file tree

Showing 8 changed files with 112 additions and 30 deletions.
diff --git a/docs/apidiffs/current_vs_latest/opentelemetry-sdk-common.txt b/docs/apidiffs/current_vs_latest/opentelemetry-sdk-common.txt
@@ -1,2 +1,4 @@
 Comparing source compatibility of  against 
-No changes.
+***  MODIFIED CLASS: PUBLIC ABSTRACT io.opentelemetry.sdk.resources.Resource  (not serializable)
+	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+	+++  NEW METHOD: PUBLIC(+) io.opentelemetry.sdk.resources.Resource mergeReverse(io.opentelemetry.sdk.resources.Resource)
diff --git a/docs/apidiffs/current_vs_latest/opentelemetry-sdk-extension-autoconfigure-spi.txt b/docs/apidiffs/current_vs_latest/opentelemetry-sdk-extension-autoconfigure-spi.txt
@@ -1,2 +1,4 @@
 Comparing source compatibility of  against 
-No changes.
+***  MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider  (not serializable)
+	===  CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+	+++  NEW METHOD: PUBLIC(+) java.util.Set<io.opentelemetry.api.common.AttributeKey<?>> supportedKeys()
diff --git a/...oconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/ResourceProvider.java b/...oconfigure-spi/src/main/java/io/opentelemetry/sdk/autoconfigure/spi/ResourceProvider.java
@@ -5,7 +5,10 @@
 
 package io.opentelemetry.sdk.autoconfigure.spi;
 
+import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.sdk.resources.Resource;
+import java.util.Collections;
+import java.util.Set;
 
 /**
  * A service provider interface (SPI) for providing a {@link Resource} that is merged into the
@@ -14,4 +17,15 @@
 public interface ResourceProvider extends Ordered {
 
   Resource createResource(ConfigProperties config);
+
+  /**
+   * Returns the set of attribute keys that this provider supports. This is used to determine if a
+   * provider should be used to create a resource.
+   *
+   * @return the set of attribute keys that this provider supports - an empty set indicates that the
+   *     provider should always be used
+   */
+  default Set<AttributeKey<?>> supportedKeys() {
+    return Collections.emptySet();
+  }
 }
diff --git a/...autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ResourceConfiguration.java b/...autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ResourceConfiguration.java
@@ -21,6 +21,7 @@
 import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.BiFunction;
@@ -85,13 +86,18 @@ static Resource configureResource(
       ConfigProperties config,
       SpiHelper spiHelper,
       BiFunction<? super Resource, ConfigProperties, ? extends Resource> resourceCustomizer) {
-    Resource result = Resource.getDefault();
+
+    // don't add the default service name yet
+    Resource result =
+        Resource.getDefault().toBuilder().removeIf(key -> key.equals(SERVICE_NAME)).build();
 
     Set<String> enabledProviders =
         new HashSet<>(config.getList("otel.java.enabled.resource.providers"));
     Set<String> disabledProviders =
         new HashSet<>(config.getList("otel.java.disabled.resource.providers"));
-    for (ResourceProvider resourceProvider : spiHelper.loadOrdered(ResourceProvider.class)) {
+    List<ResourceProvider> providers = spiHelper.loadOrdered(ResourceProvider.class);
+    Collections.reverse(providers);
+    for (ResourceProvider resourceProvider : providers) {
       if (!enabledProviders.isEmpty()
           && !enabledProviders.contains(resourceProvider.getClass().getName())) {
         continue;
@@ -103,9 +109,21 @@ static Resource configureResource(
           && !((ConditionalResourceProvider) resourceProvider).shouldApply(config, result)) {
         continue;
       }
-      result = result.merge(resourceProvider.createResource(config));
+
+      Set<AttributeKey<?>> supportedKeys = resourceProvider.supportedKeys();
+      if (!supportedKeys.isEmpty()) {
+        // empty means it always applies
+        Map<AttributeKey<?>, Object> attributes = result.getAttributes().asMap();
+        if (supportedKeys.stream().allMatch(attributes::containsKey)) {
+          continue;
+        }
+      }
+      result = result.mergeReverse(resourceProvider.createResource(config));
     }
 
+    // now add the default service name
+    result = result.mergeReverse(Resource.getDefault());
+
     result = filterAttributes(result, config);
 
     return resourceCustomizer.apply(result, config);

diff --git a/...urceProvider/java/io/opentelemetry/sdk/autoconfigure/ConditionalResourceProviderTest.java b/...urceProvider/java/io/opentelemetry/sdk/autoconfigure/ConditionalResourceProviderTest.java
@@ -5,31 +5,31 @@
 
 package io.opentelemetry.sdk.autoconfigure;
 
-import static io.opentelemetry.api.common.AttributeKey.stringKey;
 import static java.util.Collections.singletonMap;
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.entry;
 
-import org.junit.jupiter.api.Test;
+import io.opentelemetry.sdk.autoconfigure.provider.FirstResourceProvider;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 
 class ConditionalResourceProviderTest {
 
-  @Test
-  void shouldConditionallyProvideResourceAttributes_skipBasedOnPreviousResource() {
-    AutoConfiguredOpenTelemetrySdk sdk = AutoConfiguredOpenTelemetrySdk.builder().build();
-
-    assertThat(sdk.getResource().getAttributes().asMap())
-        .contains(entry(stringKey("service.name"), "test-service"));
-  }
-
-  @Test
-  void shouldConditionallyProvideResourceAttributes_skipBasedOnConfig() {
+  @ParameterizedTest
+  @CsvSource({
+    "false, test-service-2, 0",
+    "true, test-service, 1",
+  })
+  void shouldConditionallyProvideResourceAttributes_skipBasedOnConfig(
+      boolean skipSecond, String expectedServiceName, int expectedCallsToFirst) {
     AutoConfiguredOpenTelemetrySdk sdk =
         AutoConfiguredOpenTelemetrySdk.builder()
-            .addPropertiesSupplier(() -> singletonMap("skip-first-resource-provider", "true"))
+            .addPropertiesSupplier(
+                () -> singletonMap("skip-second-resource-provider", String.valueOf(skipSecond)))
             .build();
 
     assertThat(sdk.getResource().getAttributes().asMap())
-        .contains(entry(stringKey("service.name"), "test-service-2"));
+        .containsEntry(FirstResourceProvider.KEY, expectedServiceName);
+
+    assertThat(FirstResourceProvider.calls).isEqualTo(expectedCallsToFirst);
   }
 }
diff --git a/...ourceProvider/java/io/opentelemetry/sdk/autoconfigure/provider/FirstResourceProvider.java b/...ourceProvider/java/io/opentelemetry/sdk/autoconfigure/provider/FirstResourceProvider.java
@@ -7,16 +7,25 @@
 
 import static io.opentelemetry.api.common.AttributeKey.stringKey;
 
+import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
-import io.opentelemetry.sdk.autoconfigure.spi.internal.ConditionalResourceProvider;
+import io.opentelemetry.sdk.autoconfigure.spi.ResourceProvider;
 import io.opentelemetry.sdk.resources.Resource;
+import java.util.Collections;
+import java.util.Set;
 
-public class FirstResourceProvider implements ConditionalResourceProvider {
+public class FirstResourceProvider implements ResourceProvider {
+
+  @SuppressWarnings("NonFinalStaticField")
+  public static int calls = 0;
+
+  public static final AttributeKey<String> KEY = stringKey("service.name");
 
   @Override
   public Resource createResource(ConfigProperties config) {
-    return Resource.create(Attributes.of(stringKey("service.name"), "test-service"));
+    calls++;
+    return Resource.create(Attributes.of(KEY, "test-service"));
   }
 
   @Override
@@ -25,7 +34,7 @@ public int order() {
   }
 
   @Override
-  public boolean shouldApply(ConfigProperties config, Resource existing) {
-    return !config.getBoolean("skip-first-resource-provider", false);
+  public Set<AttributeKey<?>> supportedKeys() {
+    return Collections.singleton(KEY);
   }
 }
diff --git a/...urceProvider/java/io/opentelemetry/sdk/autoconfigure/provider/SecondResourceProvider.java b/...urceProvider/java/io/opentelemetry/sdk/autoconfigure/provider/SecondResourceProvider.java
@@ -7,26 +7,35 @@
 
 import static io.opentelemetry.api.common.AttributeKey.stringKey;
 
+import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
 import io.opentelemetry.sdk.autoconfigure.spi.internal.ConditionalResourceProvider;
 import io.opentelemetry.sdk.resources.Resource;
+import java.util.Collections;
+import java.util.Set;
 
 public class SecondResourceProvider implements ConditionalResourceProvider {
 
+  public static final AttributeKey<String> KEY = stringKey("service.name");
+
   @Override
   public Resource createResource(ConfigProperties config) {
-    return Resource.create(Attributes.of(stringKey("service.name"), "test-service-2"));
+    return Resource.create(Attributes.of(KEY, "test-service-2"));
   }
 
   @Override
   public int order() {
     return 200;
   }
 
+  @Override
+  public Set<AttributeKey<?>> supportedKeys() {
+    return Collections.singleton(KEY);
+  }
+
   @Override
   public boolean shouldApply(ConfigProperties config, Resource existing) {
-    String serviceName = existing.getAttribute(stringKey("service.name"));
-    return serviceName == null || "unknown_service:java".equals(serviceName);
+    return !config.getBoolean("skip-second-resource-provider", false);
   }
 }
diff --git a/sdk/common/src/main/java/io/opentelemetry/sdk/resources/Resource.java b/sdk/common/src/main/java/io/opentelemetry/sdk/resources/Resource.java
@@ -63,6 +63,11 @@ public abstract class Resource {
 
   private static final Resource DEFAULT = MANDATORY.merge(TELEMETRY_SDK);
 
+  private enum MergePrecedence {
+    OTHER_TAKES_PRECEDENCE,
+    THIS_TAKES_PRECEDENCE
+  }
+
   /**
    * Returns the default {@link Resource}. This resource contains the default attributes provided by
    * the SDK.
@@ -146,13 +151,36 @@ public <T> T getAttribute(AttributeKey<T> key) {
    * @return the newly merged {@code Resource}.
    */
   public Resource merge(@Nullable Resource other) {
+    return doMerge(other, MergePrecedence.OTHER_TAKES_PRECEDENCE);
+  }
+
+  /**
+   * Returns a new, merged {@link Resource} by merging the current {@code Resource} with the {@code
+   * other} {@code Resource}. In case of a collision, the "this" {@code Resource} takes precedence.
+   *
+   * @param other the {@code Resource} that will be merged with {@code this}.
+   * @return the newly merged {@code Resource}.
+   */
+  public Resource mergeReverse(@Nullable Resource other) {
+    return doMerge(other, MergePrecedence.THIS_TAKES_PRECEDENCE);
+  }
+
+  private Resource doMerge(@Nullable Resource other, MergePrecedence precedence) {
     if (other == null || other == EMPTY) {
       return this;
     }
 
     AttributesBuilder attrBuilder = Attributes.builder();
-    attrBuilder.putAll(this.getAttributes());
-    attrBuilder.putAll(other.getAttributes());
+    switch (precedence) {
+      case OTHER_TAKES_PRECEDENCE:
+        attrBuilder.putAll(this.getAttributes());
+        attrBuilder.putAll(other.getAttributes());
+        break;
+      case THIS_TAKES_PRECEDENCE:
+        attrBuilder.putAll(other.getAttributes());
+        attrBuilder.putAll(this.getAttributes());
+        break;
+    }
 
     if (other.getSchemaUrl() == null) {
       return create(attrBuilder.build(), getSchemaUrl());