Skip to content

Adding GRPC support for HTTP headers - #76

Closed
mag415 wants to merge 2 commits into
instana:masterfrom
mag415:master
Closed

Adding GRPC support for HTTP headers#76
mag415 wants to merge 2 commits into
instana:masterfrom
mag415:master

Conversation

@mag415

@mag415 mag415 commented May 25, 2018

Copy link
Copy Markdown

I'm trying to use the opentracing grpc library with the instana tracer.
However, the HTTP headers created by instana tracer are considered to be invalid metadata by the grpc lib since it contains upper case characters.

https://github.com/grpc/grpc/blob/master/src/core/lib/surface/validate_metadata.cc
validate_metadata: {"created":"@ ... ","description":"Illegal header key","file":"src/core/lib/surface/validate_metadata.cc","file_line":43,"offset":0,"raw_bytes":" ... 'X-Instana-T'"}

The fix here is to add an optional type of lower case http headers.

@pglombardo

Copy link
Copy Markdown
Contributor

Hi @mag415 - thanks for the PR and pointing this out. Honestly I'm undecided on how to best handle this. If another special case comes up, do we add another if case? Or do we instead split these variations out into separate propagators?

I'm going to give this some thought and post back soon. Thanks!

@pglombardo

Copy link
Copy Markdown
Contributor

One downside I see with this, is that to enable this, it's set via environment variable which makes this take affect globally for the application. But what do we do if we have a mix of clients - both HTTP and GRPC? We are stuck using one form or another.

Additionally, since OpenTracing doesn't define a specific GRPC format type, we can't easily spin of a dedicated propagator for this.

And finally, it crossed my mind that we could provide a default parameter (use_grpc_safe_headers=false) to the inject method but that is veering off from the OpenTracing APIs.

So still undecided but progressing :-)

@pglombardo

Copy link
Copy Markdown
Contributor

Hi @mag415 after much personal debating on this issue, it boils down to a limitation in OpenTracing that it does not yet provide support for this use case.

The core problem with this PR is that if you set the environment variable and enable GRPC headers, it breaks HTTP context passing (no tracing of distributed HTTP calls).

I do have an alternative though that may work for you in the interim: If you subclass InstanaTracer, you can set the HTTPPropagator to one of your own with GRPC safe headers. The InstanaTracer is declared here.

I was hoping for a better outcome for this use case but for the above reasons, we can't support this use case in this fashion.

@pglombardo pglombardo closed this Jun 20, 2018
@pglombardo

Copy link
Copy Markdown
Contributor

Also for the record, we're soliciting input from the OpenTracing community with this issue: opentracing/opentracing-python#87

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants