JSON/HTTP support in OTLP receiver #1177
Comments
|
I fail to see the problem. This is how JSON encoding in grpc-gateway is supposed to work. Why do we not want to support it? |
Yes, but it has issue with encoding trace/span IDs and JSON is not officially supported yet AFAIK. See https://github.com/open-telemetry/opentelemetry-proto/issues/129 @tigrannajaryan see your own comment in the discussion thread https://github.com/open-telemetry/opentelemetry-proto/issues/129#issuecomment-622443175: JSON support is not documented by the spec. @yurishkuro has previously shared that in Jaeger they use a custom serialization to solve the problem with base64 vs hex encoding. The consensus seems to be that JSON is not supported, but in fact it is technically supported by collector. We should either declare it as supported, or disable it to make the documentation and implementation consistent. Please correct me if I'm wrong. |
|
@tigrannajaryan @bogdandrutu @yurishkuro Alternatively, if we all agree that supporting JSON is important, I can look into addressing https://github.com/open-telemetry/opentelemetry-proto/issues/129 via custom marshaler instead. Most likely, this will require defining new (internal) Go types specifically for unmarshalling trace spans, or using |
|
@nilebox I am not convinced https://github.com/open-telemetry/opentelemetry-proto/issues/129 is a problem that needs a solution either :-) It seems to me base64 is an acceptable way to encode trace id and span id in JSON. |
|
@tigrannajaryan I am personally conflicted about that. I often catch myself looking at raw trace data in JSON, maybe because UI doesn't show something, or there's some other information/correlation I am looking for. Having the IDs in JSON base64 encoded makes it difficult to correlate. The tracing UI could be smart to recognize both encodings, but if I tag my logs with trace/span IDs and that's done in hex encoding, then the user needs to jump through hoops to correlate the data (not everything can be solved via UI integrations). I tend to agree with @Oberon00 (https://github.com/open-telemetry/opentelemetry-proto/issues/129#issuecomment-599686909), it could be better if we always used base64 instead of hex, including log fields. |
|
If we do want to support JSON and fine with base64 encoding, then no code changes are required (apart from making sure that we also use base64 in logs), but we need to document |
nilebox
changed the title
|
Rephrased the issue to discuss the path to official JSON support rather than just disabling it. |
tigrannajaryan
added
the
help wanted
|
Spec was updated to require hex instead of base64 for trace id and span id: open-telemetry/opentelemetry-specification#911 |
The TraceID type uses custom data type so that JSON serialization is in hex format instead of base64 (which is the default Protobuf JSON format). Hex format is required by OTLP spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/protocol/otlp.md#request SpanID must be also modified similarly. Will be done in a future PR to avoid creating a huge PR. TraceID data is not yet used in the codebase. This is just an introductory commit that adds the type and tests but does not yet use them for actual serialization of OTLP messages. It will be done in a future PR. Contributes to #1177
Contributes to open-telemetry#1177 1. The TraceID type uses custom data type so that JSON serialization is in hex format instead of base64 (which is the default Protobuf JSON format). Hex format is required by OTLP spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/protocol/otlp.md#request SpanID must be also modified similarly. Will be done in a future PR to avoid creating a huge PR. 2. Moved pdata.TraceID to its own file. Note that there is pdata.TraceID which is different from otlp TraceID custom data type. Due to the way packages are structured we need both to keep OTLP generated data types decoupled from pdata data types. The majority of the changes in this commit are simply type changes from []byte to TraceID.
Contributes to #1177 1. The TraceID type uses custom data type so that JSON serialization is in hex format instead of base64 (which is the default Protobuf JSON format). Hex format is required by OTLP spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/protocol/otlp.md#request SpanID must be also modified similarly. Will be done in a future PR to avoid creating a huge PR. 2. Moved pdata.TraceID to its own file. Note that there is pdata.TraceID which is different from otlp TraceID custom data type. Due to the way packages are structured we need both to keep OTLP generated data types decoupled from pdata data types. The majority of the changes in this commit are simply type changes from []byte to TraceID.
In preparation to SpanID type that is very similar to TraceID I extracted bytesID as a separate that implements the common functionality. Contribytes to open-telemetry#1177
This is step 2 in adding SpanID custom data type. This only introduces the type but does not yet use it in Protobuf messages. It will added in a future PR. Contributes to open-telemetry#1177
This enables usage of SpanID data type. Now span id is hex encoded as it is required by OTLP specification. Contributes to: open-telemetry#1177
In preparation to SpanID type that is very similar to TraceID I extracted bytesID as a separate that implements the common functionality. Contribytes to #1177
This is step 2 in adding SpanID custom data type. This only introduces the type but does not yet use it in Protobuf messages. It will added in a future PR. Contributes to open-telemetry#1177
This enables usage of SpanID data type. Now span id is hex encoded as it is required by OTLP specification. Contributes to: open-telemetry#1177
This is step 2 in adding SpanID custom data type. This only introduces the type but does not yet use it in Protobuf messages. It will added in a future PR. Contributes to #1177
This enables usage of SpanID data type. Now span id is hex encoded as it is required by OTLP specification. Contributes to: open-telemetry#1177
This enables usage of SpanID data type. Now span id is hex encoded as it is required by OTLP specification. Contributes to: open-telemetry#1177
This enables usage of SpanID data type. Now span id is hex encoded as it is required by OTLP specification. Contributes to: open-telemetry#1177
This enables usage of SpanID data type. Now span id is hex encoded as it is required by OTLP specification. Contributes to: #1177
|
This is now done and should confirm to OTLP/JSON spec. Closing. Submit separate bugs if any problems are found. |
We need to decide if the currently supported JSON via gRPC Gateway should be made official, or whether we need to address the issue https://github.com/open-telemetry/opentelemetry-proto/issues/129 first.
Additional context
We still need to keep gRPC Gateway integration to support "Protobuf over HTTP" protocol introduced in #1021.
We just need to disable the default "wildcard" handler, which accepts JSON by default: https://github.com/grpc-ecosystem/grpc-gateway/blob/ee3ef70b7777cde4e61e4e224cb11e92beecee6a/runtime/marshaler_registry.go#L78
The text was updated successfully, but these errors were encountered: