Skip to content
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.

Sign up for GitHub

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

Jump to bottom

status of the inconsistency between otlp http json format and protocol specification #1924

Closed
Frefreak opened this issue May 15, 2021 · 7 comments
Closed

status of the inconsistency between otlp http json format and protocol specification #1924

Frefreak opened this issue May 15, 2021 · 7 comments
Assignees
@paivagustavo
Labels
bug Something isn't working
Milestone
RC1

Comments

@Frefreak
Copy link
Member

Description

For httpjson format, the specification states that trace_id and span_id should not use base64 encoding and should use case insensitive hex encoding. However the current implementation seems to use default protobuf json encoding (which base64-ed trace_id and span_id).
I see http json is experimental currently so is the specification about this part subject to change? or should we fix the json encoding part? Currently the sdk encode trace_id/span_id using base64 so it actually can not send span to the otlp receiver.

Environment

  • OS: Linux
  • Architecture: amd64
  • Go Version: go 1.16.3
  • opentelemetry-go version: master

Steps To Reproduce

  1. modify example/otel-collector/main.go to use otlphttp instead of otlpgrpc, remove grpc specific option, add marshal json option
  2. run a collector, run the compiled binary of step 1

Expected behavior

span send successfully

Actual behavior

log shows status 400, manually check reveals that its because of "invald length for ID".

patch

patch of the modified example file:

diff --git a/example/otel-collector/main.go b/example/otel-collector/main.go
index 2560c04a..05b7524b 100644
--- a/example/otel-collector/main.go
+++ b/example/otel-collector/main.go
@@ -23,12 +23,10 @@ import (
        "log"
        "time"

-       "google.golang.org/grpc"
-
        "go.opentelemetry.io/otel"
        "go.opentelemetry.io/otel/attribute"
        "go.opentelemetry.io/otel/exporters/otlp"
-       "go.opentelemetry.io/otel/exporters/otlp/otlpgrpc"
+       "go.opentelemetry.io/otel/exporters/otlp/otlphttp"
        "go.opentelemetry.io/otel/metric"
        "go.opentelemetry.io/otel/metric/global"
        "go.opentelemetry.io/otel/propagation"
@@ -51,10 +49,10 @@ func initProvider() func() {
        // `localhost:30080` endpoint. Otherwise, replace `localhost` with the
        // endpoint of your cluster. If you run the app inside k8s, then you can
        // probably connect directly to the service through dns
-       driver := otlpgrpc.NewDriver(
-               otlpgrpc.WithInsecure(),
-               otlpgrpc.WithEndpoint("localhost:30080"),
-               otlpgrpc.WithDialOption(grpc.WithBlock()), // useful for testing
+       driver := otlphttp.NewDriver(
+               otlphttp.WithInsecure(),
+               otlphttp.WithEndpoint("localhost:55681"),
+               otlphttp.WithMarshal(otlp.MarshalJSON),
        )
        exp, err := otlp.NewExporter(ctx, driver)
        handleErr(err, "failed to create exporter")
@Frefreak Frefreak added the bug Something isn't working label May 15, 2021
@MadVikingGod
Copy link
Contributor

So looking into this, there doesn't seem to be a way to override the output of a field with the google jsonPB. We would have to explore other marshalers to be able to fix this.

I kinda think that we should just drop support of http/josn, and maybe try to get a more comprehensive fix by changing the spec.

@Frefreak
Copy link
Member Author

explore other marshalers to be able to fix this

indeed. opentelemetry-collector uses gogo which let you defined custom type. Not sure if its applicable to opentelemetry-go too though. (which seems like a big modification). The non-standard (protobuf vice) encoding of traceid/spanid can make most other language's implementation more problematic too.

As for dropping support of http/json, I'm not sure about this either. Maybe in some scenario the users would want http/json even if in most of the cases they should be able to http/protobuf instead. (one advantage of json is that its easier to debug/poke around and getting started). For completeness sake I think if the specification permits it, the sdk should try its best to support it. If the decision is to not support http/json, maybe in the docs we can clearly state this (currently there's a JSON marshal in otlphttp which will make the users think this is supported, which can cause some confusion)

@MadVikingGod
Copy link
Contributor

It looks like Google's GRPC for Go doesn't allow custom types. So we can't use that and change TraceID and SpanID to put out the hex string that the collector (and other implementations) expect.
It looks like this started with an OTEPusing base64 oteps/0122.
Then the JS SIG and others asked to be changed to hex, opentelemetry-specification/#768. With no strong objections, this was written into the spec.

Options:

  1. We drop http/json, and document that we won't do this until:
    • either the Sig changes, maybe to accept both base64 and hex,
    • google allows for custom types; this seems unlikely as this kind of discussion is shut down.
  2. We move to a non-google grpc generated like gogo. We should have a good long discussion about moving to an unsupported, as of right now, grpc generator.

@MrAlias
Copy link
Contributor

MrAlias commented May 20, 2021

With regards to dropping support for http/json, implementing at least one of the supported formats is required, but implementing more than one format is optional. Meaning we can drop this support and still satisfy the specification. I am in favor of doing this if we cannot comply with the format.

@tigrannajaryan do you have any insight about how we can solve this issue?

@tigrannajaryan
Copy link
Member

With regards to dropping support for http/json, implementing at least one of the supported formats is required, but implementing more than one format is optional. Meaning we can drop this support and still satisfy the specification. I am in favor of doing this if we cannot comply with the format.

This is a valid approach, especially given that otlp/json is not even declared stable yet. For those who really need it they can use an intermediary Collector to receive/export otlp/json for now.

Long-term I think it would still be useful to support otlp/json in SDKs. I don't know if we will make it a mandatory requirement in the spec (probably not). It is probably not very complicated to just to do the json encoding/decoding manually via https://golang.org/pkg/encoding/json/ without relying on the Protobuf library.

@MrAlias MrAlias added this to the RC1 milestone May 20, 2021
@MrAlias
Copy link
Contributor

MrAlias commented May 20, 2021 •
edited
Loading

Notes from SIG meeting:

  • We should remove http/json
  • The interfaces that are being created in the OTLP exporter refactor may make it possible for external sources to build custom support for this if they want.
  • If the specification requires this at a later point we can see about changing the ID formats in the process
  • There are alternate solutions (as mentioned above) for those wanting this support.
  • We should remove it after the OTLP exporter refactor Remove unstable metrics dependency from the OTLP exporter #1827.

@Aneurysm9 Aneurysm9 mentioned this issue May 21, 2021
Closed
@Aneurysm9
Copy link
Member

The old OTLP exporter was removed in #1990 and the new HTTP OTLP clients will not support JSON serialization unless and until we have a resolution for this issue. Since that support is optional and there is no longer an inconsistency, I am closing this issue.

@MrAlias MrAlias mentioned this issue Jun 21, 2021
Merged
@Frefreak Frefreak mentioned this issue Jan 14, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@paivagustavo paivagustavo

Labels
bug Something isn't working
Projects
None yet
Milestone
RC1
Development

No branches or pull requests
6 participants
@Aneurysm9 @MadVikingGod @tigrannajaryan @MrAlias @Frefreak @paivagustavo