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

Fix context propagation #37

Merged
merged 86 commits into from
Jun 3, 2018
Merged

Conversation

rnburn
Copy link
Collaborator

@rnburn rnburn commented May 17, 2018

Summary:
nginx-opentracing was propagating span context by modifying the headers of the active request. This isn't supported by NGINX. While it works most of the time, it can cause NGINX to crash kubernetes/ingress-nginx#2222 (comment).

Since there doesn't seem to be any way for a module to add arbitrary headers to upstream requests, this fix adds a directive opentracing_propagate_context that gets translated to several proxy_set_header that will set the headers for the active span context. Since these directives have to be set up when the configuration is loaded and the header keys have to be fixed, the module creates a dummy span at startup to try to determine what header keys a tracer uses for propagation.

Any header keys a tracer might use later for propagation that weren't included in the dummy span will be dropped. (Note: this will break baggage propagation for LightStep and Jaeger since they don't use fixed keys).

Also, as part of the reorganization for the fix, I dropped the vendor-specific modules. Tracer's should now be specified with the opentracing_load_tracer directive. The vendor modules didn't work well with the order of operations that need to happen for the header keys to be determined at startup.

For more background on the fix, see the discussion at
http://mailman.nginx.org/pipermail/nginx-devel/2018-March/010987.html

And since opentracing_propagate_context and proxy_set_header are specific to NGINX's proxy module, I'll also need to add a directive opentracing_fastcgi_propagate_context to handle context propagate for the fastcgi module.

@rnburn
Copy link
Collaborator Author

rnburn commented May 17, 2018

cc @isaachier - this change means dropping the vendor modules for loading a tracer in favor of dynamic loading from a JSON specification of their configuration.

You might consider adding some documentation for your configuration format to https://github.com/jaegertracing/jaeger-client-cpp and improving the error handling for misconfigurations to give a more helpful message.

@rnburn rnburn mentioned this pull request May 29, 2018
@rnburn rnburn merged commit 2619608 into opentracing-contrib:master Jun 3, 2018
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