-
Notifications
You must be signed in to change notification settings - Fork 936
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.15% 90.19% +0.03%
- Complexity 7476 7590 +114
============================================
Files 836 837 +1
Lines 22550 22769 +219
Branches 2224 2268 +44
============================================
+ Hits 20331 20536 +205
- Misses 1515 1519 +4
- Partials 704 714 +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
| // TODO(jack-berg): Should this be a JSON encoding? | ||
| // TODO deprecate in favor of toString() or toProtoJson()? | ||
| String asString(); |
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.
open question (also could consider keeping asString() and having it emit proto json and removing toProtoJson())
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.
Can you talk about toProtoJson() a little bit? I see it used the prometheus and zipkin exporters. Does the spec say that these exporters should encode using protobuf JSON in for extended attribute cases?
Do we have to add this functionality in the first stable release of extended attributes or can it be added later?
Also having thoughts about how we have JSON value serialization in two places: Value#toProtoJson() and AnyValueMarshaler. They're of course used in different contexts. There's probably a way to incorporate AnyValueMarshaler into ValueToProtoJsonTest to verify they're equivalent.
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.
open question (also could consider keeping asString() and having it emit proto json and removing toProtoJson())
Yes let's do that. See my comment to the same effect here: https://github.com/open-telemetry/opentelemetry-java/pull/7973/changes#r2696282790
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.
Can you talk about toProtoJson() a little bit? I see it used the prometheus and zipkin exporters. Does the spec say that these exporters should encode using protobuf JSON in for extended attribute cases?
it's unclear to me, I've added topic to discuss in spec meeting
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.
so I'm not sure I love asString emitting proto json, because it feels a little weird for a method named asString to emit the quotes around a string for a simple string value
I think my current preference might be your other suggestion #7973 (comment) which has the other benefit of hiding proto json representation a bit more as an exporter/sdk concern
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.
I think my current preference might be your other suggestion #7973 (comment) which has the other benefit of hiding proto json representation a bit more as an exporter/sdk concern
If we go with the other direction we only need a simplified JSON encoding, even for exporters, right? And so we could get away with adjusting asString to return the simplified JSON encoding.
api/all/src/main/java/io/opentelemetry/api/common/ValueEmpty.java
Outdated
Show resolved
Hide resolved
| .putTag("bytes", "\"AQID\"") | ||
| .putTag("map", "{\"nested\":\"value\"}") | ||
| .putTag("heterogeneousArray", "[\"string\",123]") | ||
| .putTag("empty", "null") |
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.
same questionable choices:
"\"AQID\""vs"AQID""null"vs""vs"{}"
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.
""AQID"" vs "AQID"
Should be base64 encoded string https://protobuf.dev/programming-guides/json/
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.
"null" vs "" vs "{}"
Should be JSON null literal
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 confirming, so you agree with the choices I made in the implementation above?
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.
In open-telemetry/opentelemetry-specification#4848, I'm proposing that top-level attributes be more naturally encoded (instead of JSON encoded), so
| .putTag("bytes", "\"AQID\"") | |
| .putTag("map", "{\"nested\":\"value\"}") | |
| .putTag("heterogeneousArray", "[\"string\",123]") | |
| .putTag("empty", "null") | |
| .putTag("bytes", "AQID") | |
| .putTag("map", "{\"nested\":\"value\"}") | |
| .putTag("heterogeneousArray", "[\"string\",123]") | |
| .putTag("empty", "") |
|
|
||
| @Override | ||
| public String asString() { | ||
| return ""; |
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.
another option would be return the string "null"
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.
I'm fine with this for asString().
Geez I don't think we can justify having asString(), toString(), and toProtoJson() 😅. I think probably we change asString() to be the proto json encoder. I warned in the javadoc that no guarantees are made so we should be able to do this:
* <p>WARNING: No guarantees are made about the encoding of this string response. It MAY change in
* a future minor release. If you need a reliable string encoding, write your own serializer.
| * @return a JSON encoding of this value | ||
| */ | ||
| default String toProtoJson() { | ||
| return "\"unimplemented\""; |
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.
alternatively could throw UnsupportedOperationException
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.
Another alternative would be to have a static String asProtoJson(Value<?>) utility method. Is there anything in the implementations of toProtoJson that benefits from accessing the private fields?
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.
This type of utility method could also packaged somewhere like opentelemetry-exporter-common or opentelemetry-sdk-common so opentelemetry-api doesn't need to have a class called ProtoJson in it, which is a smell.
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.
This type of utility method could also packaged somewhere like
opentelemetry-exporter-commonoropentelemetry-sdk-commonsoopentelemetry-apidoesn't need to have a class calledProtoJsonin it, which is a smell.
just confirming you prefer to ignore this comment since it contradicts #7973 (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.
having second thoughts and may prefer this suggestion: #7973 (comment)
|
|
||
| @Test | ||
| void valueString_basic() { | ||
| assertThat(Value.of("hello").toProtoJson()).isEqualTo("\"hello\""); |
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.
Slight preference for @ParameterizedTest here, but won't split hairs over it because there's plenty of examples in this repo similar to what you have 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.
| Value.of(false), | ||
| Value.empty()) | ||
| .toProtoJson()) | ||
| .isEqualTo("[\"string\",42,3.14,true,false,null]"); |
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.
If longs are supposed to be stringified, then how come in an array context they aren't?
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.
longs are stringified everywhere, but they aren't surrounded by (extra) quotes since they aren't json strings
This reverts commit c133eea.
7dabd12 to
2a8fd46
Compare
No description provided.