Decode plus sign in resource attributes #8059
Conversation
arnav-dasgupta
force-pushed
the
decode-otel-resource-attributes-correctly
branch
from
bec5b30 to
a9964e9
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8059 +/- ##
=========================================
Coverage 90.23% 90.23%
- Complexity 7602 7608 +6
=========================================
Files 838 838
Lines 22806 22821 +15
Branches 2274 2277 +3
=========================================
+ Hits 20578 20592 +14
Misses 1518 1518
- Partials 710 711 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| assertThat( | ||
| ResourceConfiguration.createEnvironmentResource( | ||
| DefaultConfigProperties.createFromMap(props))) | ||
| .isEqualTo(Resource.create(Attributes.of(stringKey("key"), "hello world"))); |
There was a problem hiding this comment.
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
| bytes[pos++] = (byte) c; | ||
| } | ||
| return new String(bytes, 0, pos, StandardCharsets.UTF_8); | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
What has the potential to throw here?
| // 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 | ||
|
|
There was a problem hiding this comment.
#nit: erroneous line break
| 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); |
There was a problem hiding this comment.
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 🙂
2 participants
#8030