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.

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

FR: Transcode X-Cloud-Trace-Context to grpc-trace-bin #416

Closed
ashi009 opened this issue Jul 17, 2018 · 8 comments
Closed

FR: Transcode X-Cloud-Trace-Context to grpc-trace-bin #416

ashi009 opened this issue Jul 17, 2018 · 8 comments

Comments

@ashi009
Copy link

ashi009 commented Jul 17, 2018

X-Cloud-Trace-Context from HTTP request is forwarded to gRPC backend as-it. However, gRPC expects a trace token in binary format from grpc-trace-bin.

The two formats are interchangeable, so it would be nice if this is done in the ESP instead of the gRPC backend, as this is where the grpc-web to grpc transcoding happens.

(or should ESP use the binary format as well and make it a flag?)

The same issue was reported at oc-go as well: census-instrumentation/opencensus-go#594

@qiwzhang
Copy link
Contributor

ESP could translate from X-Cloud-Trace-Context to grpc-trace-bin for HTTP/gRPC transcoding case. We welcome contribution to add this translation.

@ashi009
Copy link
Author

ashi009 commented Jul 17, 2018

After giving it a second thought, this is more difficult than I expected. For trace data passed within a cluster, an ESP must understand grpc-trace-bin sent from a gRPC backend to another which is also behind an ESP, or the trace won't be complete.

So why ESP is using the old format, given the comment by @rakyll?

@qiwzhang
Copy link
Contributor

Cloud trace document https://cloud.google.com/trace/docs/support still uses X-Cloud-Trace-Context. There are still many tools using this header.

@rakyll
Copy link

rakyll commented Jul 17, 2018

/cc @bogdandrutu

We should do a better job explaining where grpc-trace-bin is supported but this is our new way of propagating trace context for gRPC right now all across GCP. Our load balancers are already supporting this format.

@marwan-at-work
Copy link

I just stumbled upon this issue for CE with gRPC. Is there a current workaround? I'm thinking of having an interceptor that does the conversion and pass it a long. But I was wondering if that already exists and I can't find it. Thanks!

@qiwzhang
Copy link
Contributor

ESP can do followings: use grpc-trace-bin if backend is grpc for trace enabled by ESP. For grpc transcoding from http to grpc, ESP will translate from X-Cloud-Trace-Context to grpc-trace-bin.

What do you think?

I will see if I can find someone to work on this. We welcome any contributions.

@marwan-at-work
Copy link

marwan-at-work commented Aug 16, 2018

That sounds good! However, it seems that gRPC calls the StatsHandler before it calls the UnaryInterceptor. Therefore, I ended up wrapping StatsHandler. Here's a gist of my current workaround in case anyone stumbles upon this issue before it's fixed (could use a review): https://gist.github.com/marwan-at-work/5ac1e6f3106d4e4f064249ea0d7085df

@qiwzhang
Copy link
Contributor

It is added by #503

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants