-
Notifications
You must be signed in to change notification settings - Fork 937
Stabilize complex attributes #7973
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?
Stabilize complex attributes #7973
Conversation
3bc8e76 to
8c951bd
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7973 +/- ##
============================================
+ Coverage 90.17% 90.21% +0.04%
- Complexity 7479 7600 +121
============================================
Files 834 838 +4
Lines 22540 22787 +247
Branches 2236 2271 +35
============================================
+ Hits 20326 20558 +232
- Misses 1511 1516 +5
- Partials 703 713 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
30fb180 to
0e2af07
Compare
55d9121 to
23f5d22
Compare
23f5d22 to
0e19885
Compare
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.
changes copy-pasted from ExtendedArrayBackedAttributes.java
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.
changes copy-pasted from ExtendedArrayBackedAttributesBuilder.java
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.
changes copy-pasted from ExtendedAttributeKey.java
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.
changes copy-pasted from ExtendedAttributes.java
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.
changes copy-pasted from ExtendedAttributesBuilder.java
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.
Is your plan to keeep ExtendedAttributes around for a couple of releases to give folks a chance to migrate gracefully?
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.
yeah, good point, deprecated them in c133eea
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.
oh no, that spun out of control due to suppressing usages, reverted, I'd suggest that I do that as a follow-up PR
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.
leaving this discussion open so I don't forget
api/all/src/main/java/io/opentelemetry/api/common/ValueEmpty.java
Outdated
Show resolved
Hide resolved
...s/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/OtelToZipkinSpanTransformerTest.java
Outdated
Show resolved
Hide resolved
api/all/src/test/java/io/opentelemetry/api/common/ValueToProtoJsonTest.java
Outdated
Show resolved
Hide resolved
api/all/src/test/java/io/opentelemetry/api/common/ValueToProtoJsonTest.java
Outdated
Show resolved
Hide resolved
This reverts commit c133eea.
7dabd12 to
2a8fd46
Compare
9f18eb0 to
a98199a
Compare
api/all/src/main/java/io/opentelemetry/api/common/ProtoJson.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private static void appendString(StringBuilder sb, String value) { |
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.
Is the logic here based on any sort of reference or specification you can link to? Trying to get a sense of how to evaluate it for completeness / correctness.
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.
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.
Thanks. Here's the relevant section: https://datatracker.ietf.org/doc/html/rfc8259#section-7
|
|
||
| private static void appendBytes(StringBuilder sb, ByteBuffer value) { | ||
| byte[] bytes = new byte[value.remaining()]; | ||
| value.duplicate().get(bytes); |
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.
We shouldn't need to duplicate this since ValueBytes.create already copies the array, so it should be strictly read only / immutable here.
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.
| * Return a string encoding of this {@link Value}. This is intended to be a fallback serialized | ||
| * representation in case there is no suitable encoding that can utilize {@link #getType()} / | ||
| * {@link #getValue()} to serialize specific types. | ||
| * Returns a string representation of this {@link Value}. |
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.
Nice javadoc here!
| return value.stream().map(Value::asString).collect(joining(", ", "[", "]")); | ||
| StringBuilder sb = new StringBuilder(); | ||
| ProtoJson.append(sb, this); | ||
| return sb.toString(); |
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.
Could add a helper to ProtoJson to avoid duplicating logic in the Value implementation classes:
static String asString(Value<?> value) {
StringBuilder sb = new StringBuilder();
append(sb, value);
return sb.toString();
}
Then every asString() implementation just looks like:
@Override
public String asString() {
return ProtoJson.asString(this);
}
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.
Oh wait, I see. Its the difference between the top level representation. I still think you could centralize the logic all in ProtoJson by adding a int level parameter to append and changing the logic based on whether the level was 0 or non-0. Something like the following, demonstrated on DOUBLE but applicable to all the types:
static void append(StringBuilder sb, Value<?> value, int level) {
switch (value.getType()) {
case DOUBLE:
appendDouble(sb, (Double) value.getValue(), /*wrapWithQuotes*/ level > 0);
break;
// other cases omitted for brevity
}
}
private static void appendDouble(StringBuilder sb, double value, boolean wrapWithQuotes) {
if (wrapWithQuotes) {
sb.append('"');
}
if (Double.isNaN(value)) {
sb.append("NaN");
} else if (Double.isInfinite(value)) {
sb.append(value > 0 ? "Infinity" : "-Infinity");
} else {
sb.append(value);
}
if (wrapWithQuotes) {
sb.append('"');
}
}
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.
seems more complicated to me, but can apply this pattern if you prefer
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.
Nah you can ignore this if you like the way it is. I don't like the duplicate logic, but my suggestion is crufty. Maybe I'll go back and see if there's a cleaner way to DRY it up post merge.
jack-berg
left a comment
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.
Just minor comments at this point that don't affect the public API
No description provided.