Skip to content

Align SpringConfigProperties with DefaultConfigProperties #12398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 7, 2024
Merged

Align SpringConfigProperties with DefaultConfigProperties #12398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73c31cd
Align SpringConfigProperties with DefaultConfigProperties
jeanbisutti Oct 4, 2024
a28b1d7
Fix checkstyle
jeanbisutti Oct 4, 2024
5f5f6d0
Fix
jeanbisutti Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions ...etry/instrumentation/spring/autoconfigure/internal/properties/SpringConfigProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package io.opentelemetry.instrumentation.spring.autoconfigure.internal.properties;

import io.opentelemetry.api.internal.ConfigUtil;
import io.opentelemetry.exporter.otlp.internal.OtlpConfigUtil;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import io.opentelemetry.sdk.autoconfigure.spi.internal.DefaultConfigProperties;
Expand Down Expand Up @@ -65,8 +66,9 @@ public static ConfigProperties create(
@Nullable
@Override
public String getString(String name) {
String value = environment.getProperty(name, String.class);
if (value == null && name.equals("otel.exporter.otlp.protocol")) {
String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name);
String value = environment.getProperty(normalizedName, String.class);
if (value == null && normalizedName.equals("otel.exporter.otlp.protocol")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (value == null && normalizedName.equals("otel.exporter.otlp.protocol")) {
if (value == null && "otel.exporter.otlp.protocol".equals(normalizedName) {

Maybe this is better, it can avoid the influence from upstream API. The line 116 is same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, normalizedName can't be null at this point.

I would prefer to limit the changes in this PR.

I personally agree with your suggestion.

Perhaps this change could be discussed in another PR. Feel free to create it.

// SDK autoconfigure module defaults to `grpc`, but this module aligns with recommendation
// in specification to default to `http/protobuf`
return OtlpConfigUtil.PROTOCOL_HTTP_PROTOBUF;
Expand All @@ -77,35 +79,46 @@ public String getString(String name) {
@Nullable
@Override
public Boolean getBoolean(String name) {
return or(environment.getProperty(name, Boolean.class), otelSdkProperties.getBoolean(name));
return or(
environment.getProperty(ConfigUtil.normalizeEnvironmentVariableKey(name), Boolean.class),
otelSdkProperties.getBoolean(name));
}

@Nullable
@Override
public Integer getInt(String name) {
return or(environment.getProperty(name, Integer.class), otelSdkProperties.getInt(name));
return or(
environment.getProperty(ConfigUtil.normalizeEnvironmentVariableKey(name), Integer.class),
otelSdkProperties.getInt(name));
}

@Nullable
@Override
public Long getLong(String name) {
return or(environment.getProperty(name, Long.class), otelSdkProperties.getLong(name));
return or(
environment.getProperty(ConfigUtil.normalizeEnvironmentVariableKey(name), Long.class),
otelSdkProperties.getLong(name));
}

@Nullable
@Override
public Double getDouble(String name) {
return or(environment.getProperty(name, Double.class), otelSdkProperties.getDouble(name));
return or(
environment.getProperty(ConfigUtil.normalizeEnvironmentVariableKey(name), Double.class),
otelSdkProperties.getDouble(name));
}

@SuppressWarnings("unchecked")
@Override
public List<String> getList(String name) {
if (name.equals("otel.propagators")) {

String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name);

if (normalizedName.equals("otel.propagators")) {
return propagationProperties.getPropagators();
}

return or(environment.getProperty(name, List.class), otelSdkProperties.getList(name));
return or(environment.getProperty(normalizedName, List.class), otelSdkProperties.getList(name));
}

@Nullable
Expand All @@ -123,8 +136,10 @@ public Duration getDuration(String name) {
@Override
public Map<String, String> getMap(String name) {
Map<String, String> otelSdkMap = otelSdkProperties.getMap(name);

String normalizedName = ConfigUtil.normalizeEnvironmentVariableKey(name);
// maps from config properties are not supported by Environment, so we have to fake it
switch (name) {
switch (normalizedName) {
case "otel.resource.attributes":
return mergeWithOtel(resourceProperties.getAttributes(), otelSdkMap);
case "otel.exporter.otlp.headers":
Expand All @@ -139,7 +154,7 @@ public Map<String, String> getMap(String name) {
break;
}

String value = environment.getProperty(name);
String value = environment.getProperty(normalizedName);
if (value == null) {
return otelSdkMap;
}
Expand Down
15 changes: 15 additions & 0 deletions ...n/src/main/java/io/opentelemetry/spring/smoketest/AbstractOtelSpringStarterSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,21 @@ AutoConfigurationCustomizerProvider propagatorCustomizer() {
Attributes.of(
AttributeKey.booleanKey("keyFromResourceCustomizer"), true))));
}

@Bean
AutoConfigurationCustomizerProvider customizerUsingPropertyDefinedInaSpringFile() {
return customizer ->
customizer.addResourceCustomizer(
(resource, config) -> {
String valueForKeyDeclaredZsEnvVariable = config.getString("APPLICATION_PROP");
assertThat(valueForKeyDeclaredZsEnvVariable).isNotEmpty();

String valueForKeyWithDash = config.getString("application.prop-with-dash");
assertThat(valueForKeyWithDash).isNotEmpty();

return resource;
});
}
}

@Test
Expand Down
4 changes: 4 additions & 0 deletions smoke-tests-otel-starter/spring-boot-common/src/main/resources/application.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ otel:
attributes:
attributeFromYaml: true # boolean will be automatically converted to string by spring

application:
prop: propValue
prop-with-dash: provWithDashValue

spring:
kafka:
bootstrap-servers: localhost:9094
Expand Down
Loading