Decode plus sign in resource attributes#8059
Decode plus sign in resource attributes#8059arnav-dasgupta wants to merge 4 commits intoopen-telemetry:mainfrom
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.21% 90.22%
- Complexity 7605 7611 +6
=========================================
Files 841 841
Lines 22923 22936 +13
Branches 2291 2294 +3
=========================================
+ Hits 20681 20695 +14
+ Misses 1525 1523 -2
- Partials 717 718 +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
There was a problem hiding this comment.
Done! Followed the example and changed the test.
| 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?
There was a problem hiding this comment.
Value being null can throw a NullPointer exception but other than that.. nothing. Have removed the try-catch as the method handles most (if not all) edge cases.
...ns/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ResourceConfiguration.java
Outdated
Show resolved
Hide resolved
| 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 🙂
There was a problem hiding this comment.
Commented the details in the method.
…ssary try-catch block + comments and a method description
2 participants
#8030