Support http/json in otlptracehttp#8273
Support http/json in otlptracehttp#8273sqyang94 wants to merge 14 commits intoopen-telemetry:mainfrom
http/json in otlptracehttp#8273Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8273 +/- ##
======================================
Coverage 82.9% 83.0%
======================================
Files 314 316 +2
Lines 24985 25385 +400
======================================
+ Hits 20729 21081 +352
- Misses 3882 3907 +25
- Partials 374 397 +23
🚀 New features to boost your workflow:
|
http/json in otlptracehttp
MrAlias
left a comment
There was a problem hiding this comment.
This adds the expected http/json configuration path, but I don’t think the wire behavior is correct yet.
The main issue is that the new JSON path is using generic protobuf JSON marshaling/unmarshaling rather than OTLP/JSON’s mapping rules. That can make a spec-compliant collector reject trace exports, and it also makes the response path less forward-compatible than OTLP requires. The current tests mostly validate the new path against the repo’s own mock collector, which uses the same protojson behavior and can hide these interoperability problems.
Assisted-by: Claude Opus 4.7 Signed-off-by: Shiqi Yang <syang482@bloomberg.net>
| } | ||
|
|
||
| // WithProtocol tells the driver to use a specific protocol for the request payload. | ||
| func WithProtocol(protocol Protocol) Option { |
There was a problem hiding this comment.
Now I'm leaning towards WithEncoding in otlptracehttp as opposed to WithProtocol. It feels a bit weird to call it WithProtocol but only provide two options, as grpc definitely doesn't make sense to be here.
Signed-off-by: Shiqi Yang <syang482@bloomberg.net>
Signed-off-by: Shiqi Yang <syang482@bloomberg.net> Assisted-by: Claude Opus 4.7
MrAlias
left a comment
There was a problem hiding this comment.
The OTLP/JSON correctness issues I raised earlier look addressed in the current revision. I just have one remaining doc nit inline.
Signed-off-by: Shiqi Yang <syang482@bloomberg.net>
Signed-off-by: Shiqi Yang <syang482@bloomberg.net>
Resolves #8150
My initial impression to this issue was to just add a new option like
WithEncodingto switch betweenprotobufandjson. But after realizing that users can useOTEL_EXPORTER_OTLP_[TRACES_]*environment variables to control the configuration and the OTLP specification has already definedOTEL_EXPORTER_OTLP_[*_]PROTOCOLto supportgrpc,http/protobufandhttp/json, I decided to add a new option typeProtocolto control the protocol/encoding.Let me know if this is the intended usage of the OTLP protocol.