Merge pull request #44494 from nosan

* pr/44494:
  Polish "Refine the handling of OpenTelemetry resource attributes"
  Refine the handling of OpenTelemetry resource attributes

Closes gh-44494

spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/export/otlp/OtlpMetricsPropertiesConfigAdapter.java

@@ -17,6 +17,7 @@
 package org.springframework.boot.actuate.autoconfigure.metrics.export.otlp;
 
 import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 
@@ -28,7 +29,6 @@ import org.springframework.boot.actuate.autoconfigure.metrics.export.properties.
 import org.springframework.boot.actuate.autoconfigure.opentelemetry.OpenTelemetryProperties;
 import org.springframework.boot.actuate.autoconfigure.opentelemetry.OpenTelemetryResourceAttributes;
 import org.springframework.core.env.Environment;
-import org.springframework.util.StringUtils;
 
 /**
  * Adapter to convert {@link OtlpMetricsProperties} to an {@link OtlpConfig}.
@@ -40,11 +40,6 @@ import org.springframework.util.StringUtils;
 class OtlpMetricsPropertiesConfigAdapter extends StepRegistryPropertiesConfigAdapter<OtlpMetricsProperties>
 		implements OtlpConfig {
 
-	/**
-	 * Default value for application name if {@code spring.application.name} is not set.
-	 */
-	private static final String DEFAULT_APPLICATION_NAME = "unknown_service";
-
 	private final OpenTelemetryProperties openTelemetryProperties;
 
 	private final OtlpMetricsConnectionDetails connectionDetails;
@@ -77,21 +72,10 @@ class OtlpMetricsPropertiesConfigAdapter extends StepRegistryPropertiesConfigAda
 
 	@Override
 	public Map<String, String> resourceAttributes() {
-		Map<String, String> attributes = new OpenTelemetryResourceAttributes(
+		Map<String, String> resourceAttributes = new LinkedHashMap<>();
-				this.openTelemetryProperties.getResourceAttributes())
+		new OpenTelemetryResourceAttributes(this.environment, this.openTelemetryProperties.getResourceAttributes())
-			.asMap();
+			.applyTo(resourceAttributes::put);
-		attributes.computeIfAbsent("service.name", (key) -> getApplicationName());
+		return Collections.unmodifiableMap(resourceAttributes);
-		attributes.computeIfAbsent("service.group", (key) -> getApplicationGroup());
-		return Collections.unmodifiableMap(attributes);
-	}
-
-	private String getApplicationName() {
-		return this.environment.getProperty("spring.application.name", DEFAULT_APPLICATION_NAME);
-	}
-
-	private String getApplicationGroup() {
-		String applicationGroup = this.environment.getProperty("spring.application.group");
-		return (StringUtils.hasLength(applicationGroup)) ? applicationGroup : null;
 	}
 
 	@Override

spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/opentelemetry/OpenTelemetryAutoConfiguration.java

@@ -16,8 +16,6 @@
 
 package org.springframework.boot.actuate.autoconfigure.opentelemetry;
 
-import java.util.Map;
-
 import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.context.propagation.ContextPropagators;
 import io.opentelemetry.sdk.OpenTelemetrySdk;
@@ -36,7 +34,6 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
 import org.springframework.context.annotation.Bean;
 import org.springframework.core.env.Environment;
-import org.springframework.util.StringUtils;
 
 /**
  * {@link EnableAutoConfiguration Auto-configuration} for OpenTelemetry.
@@ -49,11 +46,6 @@ import org.springframework.util.StringUtils;
 @EnableConfigurationProperties(OpenTelemetryProperties.class)
 public class OpenTelemetryAutoConfiguration {
 
-	/**
-	 * Default value for application name if {@code spring.application.name} is not set.
-	 */
-	private static final String DEFAULT_APPLICATION_NAME = "unknown_service";
-
 	@Bean
 	@ConditionalOnMissingBean(OpenTelemetry.class)
 	OpenTelemetrySdk openTelemetry(ObjectProvider<SdkTracerProvider> tracerProvider,
@@ -76,21 +68,8 @@ public class OpenTelemetryAutoConfiguration {
 
 	private Resource toResource(Environment environment, OpenTelemetryProperties properties) {
 		ResourceBuilder builder = Resource.builder();
-		Map<String, String> attributes = new OpenTelemetryResourceAttributes(properties.getResourceAttributes())
+		new OpenTelemetryResourceAttributes(environment, properties.getResourceAttributes()).applyTo(builder::put);
-			.asMap();
-		attributes.computeIfAbsent("service.name", (key) -> getApplicationName(environment));
-		attributes.computeIfAbsent("service.group", (key) -> getApplicationGroup(environment));
-		attributes.forEach(builder::put);
 		return builder.build();
 	}
 
-	private String getApplicationName(Environment environment) {
-		return environment.getProperty("spring.application.name", DEFAULT_APPLICATION_NAME);
-	}
-
-	private String getApplicationGroup(Environment environment) {
-		String applicationGroup = environment.getProperty("spring.application.group");
-		return (StringUtils.hasLength(applicationGroup)) ? applicationGroup : null;
-	}
-
 }

spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/opentelemetry/OpenTelemetryResourceAttributes.java

@@ -21,16 +21,19 @@ import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.function.BiConsumer;
 import java.util.function.Function;
 
+import org.springframework.core.env.Environment;
+import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
 
 /**
- * OpenTelemetryResourceAttributes retrieves information from the
+ * {@link OpenTelemetryResourceAttributes} retrieves information from the
  * {@code OTEL_RESOURCE_ATTRIBUTES} and {@code OTEL_SERVICE_NAME} environment variables
- * and merges it with the resource attributes provided by the user.
+ * and merges it with the resource attributes provided by the user. User-provided resource
- * <p>
+ * attributes take precedence. Additionally, {@code spring.application.*} related
- * <b>User-provided resource attributes take precedence.</b>
+ * properties can be applied as defaults.
  * <p>
  * <a href= "https://opentelemetry.io/docs/specs/otel/resource/sdk/">OpenTelemetry
  * Resource Specification</a>
@@ -40,47 +43,72 @@ import org.springframework.util.StringUtils;
  */
 public final class OpenTelemetryResourceAttributes {
 
+	/**
+	 * Default value for service name if {@code service.name} is not set.
+	 */
+	private static final String DEFAULT_SERVICE_NAME = "unknown_service";
+
+	private final Environment environment;
+
 	private final Map<String, String> resourceAttributes;
 
 	private final Function<String, String> getEnv;
 
 	/**
 	 * Creates a new instance of {@link OpenTelemetryResourceAttributes}.
+	 * @param environment the environment
 	 * @param resourceAttributes user provided resource attributes to be used
 	 */
-	public OpenTelemetryResourceAttributes(Map<String, String> resourceAttributes) {
+	public OpenTelemetryResourceAttributes(Environment environment, Map<String, String> resourceAttributes) {
-		this(resourceAttributes, null);
+		this(environment, resourceAttributes, null);
 	}
 
 	/**
 	 * Creates a new {@link OpenTelemetryResourceAttributes} instance.
+	 * @param environment the environment
 	 * @param resourceAttributes user provided resource attributes to be used
 	 * @param getEnv a function to retrieve environment variables by name
 	 */
-	OpenTelemetryResourceAttributes(Map<String, String> resourceAttributes, Function<String, String> getEnv) {
+	OpenTelemetryResourceAttributes(Environment environment, Map<String, String> resourceAttributes,
+			Function<String, String> getEnv) {
+		Assert.notNull(environment, "'environment' must not be null");
+		this.environment = environment;
 		this.resourceAttributes = (resourceAttributes != null) ? resourceAttributes : Collections.emptyMap();
 		this.getEnv = (getEnv != null) ? getEnv : System::getenv;
 	}
 
 	/**
-	 * Returns resource attributes by combining attributes from environment variables and
+	 * Applies resource attributes to the provided BiConsumer after being combined from
-	 * user-defined resource attributes. The final resource contains all attributes from
+	 * environment variables and user-defined resource attributes.
-	 * both sources.
 	 * <p>
 	 * If a key exists in both environment variables and user-defined resources, the value
 	 * from the user-defined resource takes precedence, even if it is empty.
 	 * <p>
-	 * <b>Null keys and values are ignored.</b>
+	 * Additionally, {@code spring.application.name} or {@code unknown_service} will be
-	 * @return the resource attributes
+	 * used as the default for {@code service.name}, and {@code spring.application.group}
+	 * will serve as the default for {@code service.group}.
+	 * @param consumer the {@link BiConsumer} to apply
 	 */
-	public Map<String, String> asMap() {
+	public void applyTo(BiConsumer<String, String> consumer) {
+		Assert.notNull(consumer, "'consumer' must not be null");
 		Map<String, String> attributes = getResourceAttributesFromEnv();
 		this.resourceAttributes.forEach((name, value) -> {
-			if (name != null && value != null) {
+			if (StringUtils.hasLength(name) && value != null) {
 				attributes.put(name, value);
 			}
 		});
-		return attributes;
+		attributes.computeIfAbsent("service.name", (k) -> getApplicationName());
+		attributes.computeIfAbsent("service.group", (k) -> getApplicationGroup());
+		attributes.forEach(consumer);
+	}
+
+	private String getApplicationName() {
+		return this.environment.getProperty("spring.application.name", DEFAULT_SERVICE_NAME);
+	}
+
+	private String getApplicationGroup() {
+		String applicationGroup = this.environment.getProperty("spring.application.group");
+		return (StringUtils.hasLength(applicationGroup)) ? applicationGroup : null;
 	}
 
 	/**
@@ -122,7 +150,7 @@ public final class OpenTelemetryResourceAttributes {
 	 * @param value value to decode
 	 * @return the decoded string
 	 */
-	public static String decode(String value) {
+	private static String decode(String value) {
 		if (value.indexOf('%') < 0) {
 			return value;
 		}

spring-boot-project/spring-boot-actuator-autoconfigure/src/test/java/org/springframework/boot/actuate/autoconfigure/opentelemetry/OpenTelemetryResourceAttributesTests.java

@@ -27,6 +27,8 @@ import org.assertj.core.api.InstanceOfAssertFactories;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
+import org.springframework.mock.env.MockEnvironment;
+
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 
@@ -41,6 +43,8 @@ class OpenTelemetryResourceAttributesTests {
 
 	private static final PercentEscaper escaper = PercentEscaper.create();
 
+	private final MockEnvironment environment = new MockEnvironment();
+
 	private final Map<String, String> environmentVariables = new LinkedHashMap<>();
 
 	private final Map<String, String> resourceAttributes = new LinkedHashMap<>();
@@ -48,7 +52,7 @@ class OpenTelemetryResourceAttributesTests {
 	@BeforeAll
 	static void beforeAll() {
 		long seed = new Random().nextLong();
-		System.out.println("Seed: " + seed);
+		System.out.println(OpenTelemetryResourceAttributesTests.class.getSimpleName() + " seed: " + seed);
 		random = new Random(seed);
 	}
 
@@ -56,40 +60,37 @@ class OpenTelemetryResourceAttributesTests {
 	void otelServiceNameShouldTakePrecedenceOverOtelResourceAttributes() {
 		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "service.name=ignored");
 		this.environmentVariables.put("OTEL_SERVICE_NAME", "otel-service");
-		OpenTelemetryResourceAttributes attributes = getAttributes();
+		assertThat(getAttributes()).hasSize(1).containsEntry("service.name", "otel-service");
-		assertThat(attributes.asMap()).hasSize(1).containsEntry("service.name", "otel-service");
 	}
 
 	@Test
 	void otelServiceNameWhenEmptyShouldTakePrecedenceOverOtelResourceAttributes() {
 		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "service.name=ignored");
 		this.environmentVariables.put("OTEL_SERVICE_NAME", "");
-		OpenTelemetryResourceAttributes attributes = getAttributes();
+		assertThat(getAttributes()).hasSize(1).containsEntry("service.name", "");
-		assertThat(attributes.asMap()).hasSize(1).containsEntry("service.name", "");
 	}
 
 	@Test
-	void otelResourceAttributesShouldBeUsed() {
+	void otelResourceAttributes() {
 		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES",
 				", ,,key1=value1,key2= value2, key3=value3,key4=,=value5,key6,=,key7=spring+boot,key8=ś");
-		OpenTelemetryResourceAttributes attributes = getAttributes();
+		assertThat(getAttributes()).hasSize(7)
-		assertThat(attributes.asMap()).hasSize(6)
 			.containsEntry("key1", "value1")
 			.containsEntry("key2", "value2")
 			.containsEntry("key3", "value3")
 			.containsEntry("key4", "")
 			.containsEntry("key7", "spring+boot")
-			.containsEntry("key8", "ś");
+			.containsEntry("key8", "ś")
+			.containsEntry("service.name", "unknown_service");
 	}
 
 	@Test
-	void resourceAttributesShouldBeMergedWithEnvironmentVariables() {
+	void resourceAttributesShouldBeMergedWithEnvironmentVariablesAndTakePrecedence() {
 		this.resourceAttributes.put("service.group", "custom-group");
 		this.resourceAttributes.put("key2", "");
 		this.environmentVariables.put("OTEL_SERVICE_NAME", "custom-service");
 		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "key1=value1,key2=value2");
-		OpenTelemetryResourceAttributes attributes = getAttributes();
+		assertThat(getAttributes()).hasSize(4)
-		assertThat(attributes.asMap()).hasSize(4)
 			.containsEntry("service.name", "custom-service")
 			.containsEntry("service.group", "custom-group")
 			.containsEntry("key1", "value1")
@@ -97,27 +98,20 @@ class OpenTelemetryResourceAttributesTests {
 	}
 
 	@Test
-	void resourceAttributesWithNullKeyOrValueShouldBeIgnored() {
+	void invalidResourceAttributesShouldBeIgnored() {
-		this.resourceAttributes.put("service.group", null);
+		this.resourceAttributes.put("", "empty-key");
-		this.resourceAttributes.put("service.name", null);
+		this.resourceAttributes.put(null, "null-key");
-		this.resourceAttributes.put(null, "value");
+		this.resourceAttributes.put("null-value", null);
-		this.environmentVariables.put("OTEL_SERVICE_NAME", "custom-service");
+		this.resourceAttributes.put("empty-value", "");
-		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "key1=value1,key2=value2");
+		assertThat(getAttributes()).hasSize(2)
-		OpenTelemetryResourceAttributes attributes = getAttributes();
+			.containsEntry("service.name", "unknown_service")
-		assertThat(attributes.asMap()).hasSize(3)
+			.containsEntry("empty-value", "");
-			.containsEntry("service.name", "custom-service")
-			.containsEntry("key1", "value1")
-			.containsEntry("key2", "value2");
 	}
 
 	@Test
 	@SuppressWarnings("unchecked")
-	void systemGetEnvShouldBeUsedAsDefaultEnvFunctionAndResourceAttributesAreEmpty() {
+	void systemGetEnvShouldBeUsedAsDefaultEnvFunction() {
-		OpenTelemetryResourceAttributes attributes = new OpenTelemetryResourceAttributes(null);
+		OpenTelemetryResourceAttributes attributes = new OpenTelemetryResourceAttributes(this.environment, null);
-		assertThat(attributes).extracting("resourceAttributes")
-			.asInstanceOf(InstanceOfAssertFactories.MAP)
-			.isNotNull()
-			.isEmpty();
 		Function<String, String> getEnv = assertThat(attributes).extracting("getEnv")
 			.asInstanceOf(InstanceOfAssertFactories.type(Function.class))
 			.actual();
@@ -125,38 +119,98 @@ class OpenTelemetryResourceAttributesTests {
 	}
 
 	@Test
-	void shouldDecodeOtelResourceAttributeValues() {
+	void otelResourceAttributeValuesShouldBePercentDecoded() {
 		Stream.generate(this::generateRandomString).limit(10000).forEach((value) -> {
-			String key = "key";
+			this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "key=" + escaper.escape(value));
-			this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", key + "=" + escaper.escape(value));
+			assertThat(getAttributes()).hasSize(2)
-			OpenTelemetryResourceAttributes attributes = getAttributes();
+				.containsEntry("service.name", "unknown_service")
-			assertThat(attributes.asMap()).hasSize(1).containsEntry(key, value);
+				.containsEntry("key", value);
 		});
 	}
 
 	@Test
-	void shouldThrowIllegalArgumentExceptionWhenDecodingPercentIllegalHexChar() {
+	void illegalArgumentExceptionShouldBeThrownWhenDecodingIllegalHexCharPercentEncodedValue() {
 		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "key=abc%ß");
-		assertThatIllegalArgumentException().isThrownBy(() -> getAttributes().asMap())
+		assertThatIllegalArgumentException().isThrownBy(this::getAttributes)
 			.withMessage("Failed to decode percent-encoded characters at index 3 in the value: 'abc%ß'");
 	}
 
 	@Test
-	void shouldUseReplacementCharWhenDecodingNonUtf8Character() {
+	void replacementCharShouldBeUsedWhenDecodingNonUtf8Character() {
 		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "key=%a3%3e");
-		OpenTelemetryResourceAttributes attributes = getAttributes();
+		assertThat(getAttributes()).containsEntry("key", "\ufffd>");
-		assertThat(attributes.asMap()).containsEntry("key", "\ufffd>");
 	}
 
 	@Test
-	void shouldThrowIllegalArgumentExceptionWhenDecodingPercent() {
+	void illegalArgumentExceptionShouldBeThrownWhenDecodingInvalidPercentEncodedValue() {
 		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "key=%");
-		assertThatIllegalArgumentException().isThrownBy(() -> getAttributes().asMap())
+		assertThatIllegalArgumentException().isThrownBy(this::getAttributes)
 			.withMessage("Failed to decode percent-encoded characters at index 0 in the value: '%'");
 	}
 
-	private OpenTelemetryResourceAttributes getAttributes() {
+	@Test
-		return new OpenTelemetryResourceAttributes(this.resourceAttributes, this.environmentVariables::get);
+	void unknownServiceShouldBeUsedAsDefaultServiceName() {
+		assertThat(getAttributes()).hasSize(1).containsEntry("service.name", "unknown_service");
+	}
+
+	@Test
+	void springApplicationGroupNameShouldBeUsedAsDefaultServiceGroup() {
+		this.environment.setProperty("spring.application.group", "spring-boot");
+		assertThat(getAttributes()).hasSize(2)
+			.containsEntry("service.name", "unknown_service")
+			.containsEntry("service.group", "spring-boot");
+	}
+
+	@Test
+	void springApplicationNameShouldBeUsedAsDefaultServiceName() {
+		this.environment.setProperty("spring.application.name", "spring-boot-app");
+		assertThat(getAttributes()).hasSize(1).containsEntry("service.name", "spring-boot-app");
+	}
+
+	@Test
+	void resourceAttributesShouldTakePrecedenceOverSpringApplicationName() {
+		this.resourceAttributes.put("service.name", "spring-boot");
+		this.environment.setProperty("spring.application.name", "spring-boot-app");
+		assertThat(getAttributes()).hasSize(1).containsEntry("service.name", "spring-boot");
+	}
+
+	@Test
+	void otelResourceAttributesShouldTakePrecedenceOverSpringApplicationName() {
+		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "service.name=spring-boot");
+		this.environment.setProperty("spring.application.name", "spring-boot-app");
+		assertThat(getAttributes()).hasSize(1).containsEntry("service.name", "spring-boot");
+	}
+
+	@Test
+	void otelServiceNameShouldTakePrecedenceOverSpringApplicationName() {
+		this.environmentVariables.put("OTEL_SERVICE_NAME", "spring-boot");
+		this.environment.setProperty("spring.application.name", "spring-boot-app");
+		assertThat(getAttributes()).hasSize(1).containsEntry("service.name", "spring-boot");
+	}
+
+	@Test
+	void resourceAttributesShouldTakePrecedenceOverSpringApplicationGroupName() {
+		this.resourceAttributes.put("service.group", "spring-boot-app");
+		this.environment.setProperty("spring.application.group", "spring-boot");
+		assertThat(getAttributes()).hasSize(2)
+			.containsEntry("service.name", "unknown_service")
+			.containsEntry("service.group", "spring-boot-app");
+	}
+
+	@Test
+	void otelResourceAttributesShouldTakePrecedenceOverSpringApplicationGroupName() {
+		this.environmentVariables.put("OTEL_RESOURCE_ATTRIBUTES", "service.group=spring-boot");
+		this.environment.setProperty("spring.application.group", "spring-boot-app");
+		assertThat(getAttributes()).hasSize(2)
+			.containsEntry("service.name", "unknown_service")
+			.containsEntry("service.group", "spring-boot");
+	}
+
+	private Map<String, String> getAttributes() {
+		Map<String, String> attributes = new LinkedHashMap<>();
+		new OpenTelemetryResourceAttributes(this.environment, this.resourceAttributes, this.environmentVariables::get)
+			.applyTo(attributes::put);
+		return attributes;
 	}
 
 	private String generateRandomString() {