[Spring starter] Make sdk bean overridable and cache env calls (#15132)

jaydeluca · web-flow · commit b94cf7da1334 · 2025-11-07T07:30:43.000+02:00
diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/properties/CachedPropertyResolver.java b/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/properties/CachedPropertyResolver.java
@@ -0,0 +1,95 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.spring.autoconfigure.internal.properties;
+
+import java.util.Objects;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
+import javax.annotation.Nullable;
+import org.springframework.core.env.Environment;
+
+/**
+ * A caching wrapper around Spring's {@link Environment} that caches property lookups to avoid
+ * repeated expensive operations and property source traversal.
+ *
+ * <p>Thread-safe for concurrent access. Cached values persist indefinitely and are assumed to be
+ * immutable after the first access. If properties can change at runtime, use {@link #clear()} to
+ * invalidate the cache.
+ *
+ * <p>This class is internal and is hence not for public use. Its APIs are unstable and can change
+ * at any time.
+ */
+final class CachedPropertyResolver {
+  private final Environment environment;
+  private final ConcurrentHashMap<CacheKey, Optional<?>> cache = new ConcurrentHashMap<>();
+
+  CachedPropertyResolver(Environment environment) {
+    this.environment = Objects.requireNonNull(environment, "environment");
+  }
+
+  /**
+   * Gets a property value from the environment, using a cache to avoid repeated lookups.
+   *
+   * @param name the property name
+   * @param targetType the target type to convert to
+   * @return the property value, or null if not found
+   */
+  @Nullable
+  <T> T getProperty(String name, Class<T> targetType) {
+    CacheKey key = new CacheKey(name, targetType);
+    return targetType.cast(
+        cache
+            .computeIfAbsent(
+                key, k -> Optional.ofNullable(environment.getProperty(name, targetType)))
+            .orElse(null));
+  }
+
+  /**
+   * Gets a string property value from the environment.
+   *
+   * @param name the property name
+   * @return the property value, or null if not found
+   */
+  @Nullable
+  String getProperty(String name) {
+    return getProperty(name, String.class);
+  }
+
+  /** Clears all cached property values, forcing fresh lookups on subsequent calls. */
+  void clear() {
+    cache.clear();
+  }
+
+  /** Cache key combining property name and target type. */
+  private static final class CacheKey {
+    private final String name;
+    private final Class<?> targetType;
+    private final int cachedHashCode;
+
+    CacheKey(String name, Class<?> targetType) {
+      this.name = Objects.requireNonNull(name, "name");
+      this.targetType = Objects.requireNonNull(targetType, "targetType");
+      this.cachedHashCode = Objects.hash(name, targetType);
+    }
+
+    @Override
+    public boolean equals(Object obj) {
+      if (this == obj) {
+        return true;
+      }
+      if (!(obj instanceof CacheKey)) {
+        return false;
+      }
+      CacheKey other = (CacheKey) obj;
+      return name.equals(other.name) && targetType.equals(other.targetType);
+    }
+
+    @Override
+    public int hashCode() {
+      return cachedHashCode;
+    }
+  }
+}
diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/properties/SpringConfigProperties.java b/instrumentation/spring/spring-boot-autoconfigure/src/main/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/properties/SpringConfigProperties.java
@@ -25,7 +25,7 @@
  * any time.
  */
 public class SpringConfigProperties implements ConfigProperties {
-  private final Environment environment;
+  private final CachedPropertyResolver environment;
 
   private final ExpressionParser parser;
   private final OtlpExporterProperties otlpExporterProperties;
@@ -44,7 +44,7 @@ public SpringConfigProperties(
       OtelResourceProperties resourceProperties,
       OtelSpringProperties otelSpringProperties,
       ConfigProperties otelSdkProperties) {
-    this.environment = environment;
+    this.environment = new CachedPropertyResolver(environment);
     this.parser = parser;
     this.otlpExporterProperties = otlpExporterProperties;
     this.resourceProperties = resourceProperties;
@@ -154,8 +154,8 @@ public String getString(String name) {
     String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name);
     String value = environment.getProperty(normalizedName, String.class);
     if (value == null && normalizedName.equals("otel.exporter.otlp.protocol")) {
-      // SDK autoconfigure module defaults to `grpc`, but this module aligns with recommendation
-      // in specification to default to `http/protobuf`
+      // SDK autoconfigure module defaults to `grpc`, but this module aligns with
+      // recommendation in specification to default to `http/protobuf`
       return OtlpConfigUtil.PROTOCOL_HTTP_PROTOBUF;
     }
     return or(value, otelSdkProperties.getString(name));
@@ -196,7 +196,6 @@ public Double getDouble(String name) {
   @SuppressWarnings("unchecked")
   @Override
   public List<String> getList(String name) {
-
     String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name);
 
     List<String> list = listPropertyValues.get(normalizedName);
@@ -210,7 +209,8 @@ public List<String> getList(String name) {
       }
     }
 
-    return or(environment.getProperty(normalizedName, List.class), otelSdkProperties.getList(name));
+    List<String> envValue = (List<String>) environment.getProperty(normalizedName, List.class);
+    return or(envValue, otelSdkProperties.getList(name));
   }
 
   @Nullable
@@ -231,6 +231,21 @@ public Map<String, String> getMap(String name) {
 
     String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name);
     // maps from config properties are not supported by Environment, so we have to fake it
+    Map<String, String> specialMap = getSpecialMapProperty(normalizedName, otelSdkMap);
+    if (specialMap != null) {
+      return specialMap;
+    }
+
+    String value = environment.getProperty(normalizedName);
+    if (value == null) {
+      return otelSdkMap;
+    }
+    return (Map<String, String>) parser.parseExpression(value).getValue();
+  }
+
+  @Nullable
+  private Map<String, String> getSpecialMapProperty(
+      String normalizedName, Map<String, String> otelSdkMap) {
     switch (normalizedName) {
       case "otel.resource.attributes":
         return mergeWithOtel(resourceProperties.getAttributes(), otelSdkMap);
@@ -243,14 +258,8 @@ public Map<String, String> getMap(String name) {
       case "otel.exporter.otlp.traces.headers":
         return mergeWithOtel(otlpExporterProperties.getTraces().getHeaders(), otelSdkMap);
       default:
-        break;
-    }
-
-    String value = environment.getProperty(normalizedName);
-    if (value == null) {
-      return otelSdkMap;
+        return null;
     }
-    return (Map<String, String>) parser.parseExpression(value).getValue();
   }
 
   /**
diff --git a/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/properties/SpringConfigPropertiesTest.java b/instrumentation/spring/spring-boot-autoconfigure/src/test/java/io/opentelemetry/instrumentation/spring/autoconfigure/internal/properties/SpringConfigPropertiesTest.java
@@ -5,8 +5,13 @@
 
 package io.opentelemetry.instrumentation.spring.autoconfigure.internal.properties;
 
+import static java.util.Collections.emptyMap;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.entry;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 
 import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.instrumentation.spring.autoconfigure.OpenTelemetryAutoConfiguration;
@@ -17,6 +22,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.function.Consumer;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
@@ -162,8 +168,120 @@ void shouldInitializeAttributesByMap() {
             });
   }
 
+  public static Stream<Arguments> propertyCachingTestCases() {
+    return Stream.of(
+        // property, typeClass, assertion
+        Arguments.of(
+            "otel.service.name=test-service",
+            String.class,
+            (Consumer<SpringConfigProperties>)
+                config ->
+                    assertThat(config.getString("otel.service.name")).isEqualTo("test-service")),
+        Arguments.of(
+            "otel.exporter.otlp.enabled=true",
+            Boolean.class,
+            (Consumer<SpringConfigProperties>)
+                config -> assertThat(config.getBoolean("otel.exporter.otlp.enabled")).isTrue()),
+        Arguments.of(
+            "otel.metric.export.interval=10",
+            Integer.class,
+            (Consumer<SpringConfigProperties>)
+                config -> assertThat(config.getInt("otel.metric.export.interval")).isEqualTo(10)),
+        Arguments.of(
+            "otel.bsp.schedule.delay=5000",
+            Long.class,
+            (Consumer<SpringConfigProperties>)
+                config -> assertThat(config.getLong("otel.bsp.schedule.delay")).isEqualTo(5000L)),
+        Arguments.of(
+            "otel.traces.sampler.arg=0.5",
+            Double.class,
+            (Consumer<SpringConfigProperties>)
+                config -> assertThat(config.getDouble("otel.traces.sampler.arg")).isEqualTo(0.5)),
+        Arguments.of(
+            "otel.bsp.export.timeout=30s",
+            String.class,
+            (Consumer<SpringConfigProperties>)
+                config ->
+                    assertThat(config.getDuration("otel.bsp.export.timeout"))
+                        .isEqualByComparingTo(java.time.Duration.ofSeconds(30))),
+        Arguments.of(
+            "otel.attribute.value.length.limit=256",
+            List.class,
+            (Consumer<SpringConfigProperties>)
+                config ->
+                    assertThat(config.getList("otel.attribute.value.length.limit"))
+                        .containsExactly("256")));
+  }
+
+  @ParameterizedTest
+  @MethodSource("propertyCachingTestCases")
+  @DisplayName("should cache property lookups and call Environment.getProperty() only once")
+  void propertyCaching(
+      String property, Class<?> typeClass, Consumer<SpringConfigProperties> assertion) {
+    String propertyName = property.split("=")[0];
+
+    this.contextRunner
+        .withPropertyValues(property)
+        .run(
+            context -> {
+              Environment realEnvironment = context.getBean("environment", Environment.class);
+              Environment spyEnvironment = spy(realEnvironment);
+
+              SpringConfigProperties config =
+                  new SpringConfigProperties(
+                      spyEnvironment,
+                      new SpelExpressionParser(),
+                      context.getBean(OtlpExporterProperties.class),
+                      context.getBean(OtelResourceProperties.class),
+                      context.getBean(OtelSpringProperties.class),
+                      DefaultConfigProperties.createFromMap(emptyMap()));
+
+              for (int i = 0; i < 100; i++) {
+                assertion.accept(config);
+              }
+
+              verify(spyEnvironment, times(1)).getProperty(eq(propertyName), eq(typeClass));
+            });
+  }
+
+  @Test
+  @DisplayName("should cache map property lookups and call Environment.getProperty() only once")
+  void mapPropertyCaching() {
+    this.contextRunner
+        .withSystemProperties(
+            "otel.instrumentation.common.peer-service-mapping={'host1':'serviceA','host2':'serviceB'}")
+        .run(
+            context -> {
+              Environment realEnvironment = context.getBean("environment", Environment.class);
+              Environment spyEnvironment = spy(realEnvironment);
+
+              SpringConfigProperties config =
+                  new SpringConfigProperties(
+                      spyEnvironment,
+                      new SpelExpressionParser(),
+                      context.getBean(OtlpExporterProperties.class),
+                      context.getBean(OtelResourceProperties.class),
+                      context.getBean(OtelSpringProperties.class),
+                      DefaultConfigProperties.createFromMap(emptyMap()));
+
+              for (int i = 0; i < 100; i++) {
+                Map<String, String> mapping =
+                    config.getMap("otel.instrumentation.common.peer-service-mapping");
+                assertThat(mapping)
+                    .containsEntry("host1", "serviceA")
+                    .containsEntry("host2", "serviceB");
+              }
+
+              // Map properties call getProperty(name) which delegates to getProperty(name,
+              // String.class)
+              verify(spyEnvironment, times(1))
+                  .getProperty(
+                      eq("otel.instrumentation.common.peer-service-mapping"), eq(String.class));
+            });
+  }
+
   private static ConfigProperties getConfig(AssertableApplicationContext context) {
-    return getConfig(context, Collections.emptyMap());
+    return getConfig(context, emptyMap());
   }
 
   private static SpringConfigProperties getConfig(
