-
Notifications
You must be signed in to change notification settings - Fork 940
Decode plus sign in resource attributes #8059
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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 | ||
|
|
||
| decodeResourceAttributes(entry.getValue())); | ||
| } | ||
| String serviceName = config.getString(SERVICE_NAME_PROPERTY); | ||
| if (serviceName != null) { | ||
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
|---|---|---|
|
|
@@ -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"))); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#nit: erroneous line break