Fix http json example #1835
Fix http json example #1835
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1835 +/- ##
=======================================
- Coverage 75.0% 75.0% -0.1%
=======================================
Files 122 122
Lines 20279 20279
=======================================
- Hits 15212 15211 -1
- Misses 5067 5068 +1 ☔ View full report in Codecov by Sentry. |
|
@lalitb @TommyCpp please take a look at the changes when you get a chance. I had to disable the call to serialize My editor picked up some formatting changes in addition. |
opentelemetry-proto/src/proto.rs
Outdated
| { | ||
| // Serialize any_value::Value using its own implementation | ||
| // If value is None, it will be serialized as such | ||
| match value { | ||
| Some(any_value) => match &any_value.value { | ||
| Some(Value::IntValue(i)) => serialize_i64_to_string(i, serializer), | ||
| //Some(Value::IntValue(i)) => serialize_i64_to_string(i, serializer), |
There was a problem hiding this comment.
@ramgdev does commenting it out resolve the export issue? From my understanding, proto i64/u64 should be converted to JSON strings."
There was a problem hiding this comment.
@lalitb yes, this change fixes the exporter issue in the example. I see this in the JSON mapping doc JSON value will be a decimal string. Either numbers or strings are accepted. Since using numbers in serialization help with deserializing back into either standalone value or value wrapped in opentelemetry::Value enum, this should work imho.
There was a problem hiding this comment.
Not sure if I am looking at the wrong place. Looks like the attribute spec is asking int64 to be encoded as number, not string. https://opentelemetry.io/docs/specs/otel/common/#attribute
There was a problem hiding this comment.
Yes, the int64 type is supported as number for attributes, however when it is converted to JSON, it should string as per the proto-JSON mapping doc.
There was a problem hiding this comment.
Doesn't that introduce ambiguity for deserializer without the explicit use of Value::String during serialization?
Fixes #1763
Design discussion issue (if applicable) #
Changes
Use string value for event attribute
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes