Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
import io.opentelemetry.sdk.autoconfigure.spi.internal.DefaultConfigProperties;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.resources.ResourceBuilder;
import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.HashSet;
Expand Down Expand Up @@ -67,18 +65,14 @@ public static Resource createEnvironmentResource() {
*/
public static Resource createEnvironmentResource(ConfigProperties config) {
AttributesBuilder resourceAttributes = Attributes.builder();
try {
for (Map.Entry<String, String> entry : config.getMap(ATTRIBUTE_PROPERTY).entrySet()) {
resourceAttributes.put(
entry.getKey(),
// Attributes specified via otel.resource.attributes follow the W3C Baggage spec and
// characters outside the baggage-octet range are percent encoded
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#specifying-resource-information-via-an-environment-variable
URLDecoder.decode(entry.getValue(), StandardCharsets.UTF_8.displayName()));
}
} catch (UnsupportedEncodingException e) {
// Should not happen since always using standard charset
throw new ConfigurationException("Unable to decode resource attributes.", e);
for (Map.Entry<String, String> entry : config.getMap(ATTRIBUTE_PROPERTY).entrySet()) {
resourceAttributes.put(
entry.getKey(),
// Attributes specified via otel.resource.attributes follow the W3C Baggage spec and
// characters outside the baggage-octet range are percent encoded
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#specifying-resource-information-via-an-environment-variable

Copy link
Member

Choose a reason for hiding this comment

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

#nit: erroneous line break

Suggested change

decodeResourceAttributes(entry.getValue()));
}
String serviceName = config.getString(SERVICE_NAME_PROPERTY);
if (serviceName != null) {
Expand Down Expand Up @@ -132,5 +126,35 @@ static Resource filterAttributes(Resource resource, ConfigProperties configPrope
return builder.build();
}

private static String decodeResourceAttributes(String value) {
try {
if (value.indexOf('%') < 0) {
return value;
}

int n = value.length();
byte[] bytes = new byte[n];
int pos = 0;

for (int i = 0; i < n; i++) {
char c = value.charAt(i);
if (c == '%' && i + 2 < n) {
int d1 = Character.digit(value.charAt(i + 1), 16);
int d2 = Character.digit(value.charAt(i + 2), 16);
if (d1 != -1 && d2 != -1) {
bytes[pos++] = (byte) ((d1 << 4) + d2);
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining what's happening here and why it works? I've looked at it closely and have convinced myself its correct, but its a high cognitive load to understand so let's give future maintainers some help 🙂

i += 2;
continue;
}
}
// Keep '+' as '+' and any other non-encoded chars
bytes[pos++] = (byte) c;
}
return new String(bytes, 0, pos, StandardCharsets.UTF_8);
} catch (RuntimeException e) {
Copy link
Member

Choose a reason for hiding this comment

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

What has the potential to throw here?

throw new ConfigurationException("Failed to decode resource attributes: " + value, e);
}
}

private ResourceConfiguration() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,93 @@ void customConfigResourceWithDisabledKeys() {
.build());
}

@Test
void decodePlusSignInCustomConfigResource() {
Map<String, String> props = new HashMap<>();
props.put("otel.service.name", "my-app");
props.put(
"otel.resource.attributes", "food=cheese+cake,drink=juice,animal= ,color=,shape=square");

assertThat(
ResourceConfiguration.configureResource(
DefaultConfigProperties.create(props, componentLoader),
SpiHelper.create(ResourceConfigurationTest.class.getClassLoader()),
(r, c) -> r))
.isEqualTo(
Resource.getDefault().toBuilder()
.put(stringKey("service.name"), "my-app")
.put("food", "cheese+cake")
.put("drink", "juice")
.put("shape", "square")
.build());
}

@Test
void decodePercentEncodedSpace() {
Map<String, String> props = new HashMap<>();
props.put("otel.resource.attributes", "key=hello%20world");

assertThat(
ResourceConfiguration.createEnvironmentResource(
DefaultConfigProperties.createFromMap(props)))
.isEqualTo(Resource.create(Attributes.of(stringKey("key"), "hello world")));
Copy link
Member

Choose a reason for hiding this comment

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

Can you turn these into a parameterized test please? Improves the readability to see all the test cases neatly / compactly arranged next to each other.

Example available here: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/common/src/test/java/io/opentelemetry/sdk/common/internal/GlobUtilTest.java

}

@Test
void decodeInvalidPercentEncodingPreservesLiteral() {
Map<String, String> props = new HashMap<>();
props.put("otel.resource.attributes", "key=abc%2Gdef");

assertThat(
ResourceConfiguration.createEnvironmentResource(
DefaultConfigProperties.createFromMap(props)))
.isEqualTo(Resource.create(Attributes.of(stringKey("key"), "abc%2Gdef")));
}

@Test
void decodeIncompletePercentEncodingPreservesLiteral() {
Map<String, String> props = new HashMap<>();
props.put("otel.resource.attributes", "key=abc%2");

assertThat(
ResourceConfiguration.createEnvironmentResource(
DefaultConfigProperties.createFromMap(props)))
.isEqualTo(Resource.create(Attributes.of(stringKey("key"), "abc%2")));
}

@Test
void decodePercentAtEndPreservesLiteral() {
Map<String, String> props = new HashMap<>();
props.put("otel.resource.attributes", "key=abc%");

assertThat(
ResourceConfiguration.createEnvironmentResource(
DefaultConfigProperties.createFromMap(props)))
.isEqualTo(Resource.create(Attributes.of(stringKey("key"), "abc%")));
}

@Test
void decodeMultiplePercentEncodings() {
Map<String, String> props = new HashMap<>();
props.put("otel.resource.attributes", "key=a%20b%2Bc%3Dd");

assertThat(
ResourceConfiguration.createEnvironmentResource(
DefaultConfigProperties.createFromMap(props)))
.isEqualTo(Resource.create(Attributes.of(stringKey("key"), "a b+c=d")));
}

@Test
void decodeNoPercentEncoding() {
Map<String, String> props = new HashMap<>();
props.put("otel.resource.attributes", "key=plain-value");

assertThat(
ResourceConfiguration.createEnvironmentResource(
DefaultConfigProperties.createFromMap(props)))
.isEqualTo(Resource.create(Attributes.of(stringKey("key"), "plain-value")));
}

@Test
void createEnvironmentResource_Empty() {
Attributes attributes = ResourceConfiguration.createEnvironmentResource().getAttributes();
Expand Down
Loading