-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Upgrade to use the latest version of nginx-opentracing. #1580
Conversation
@rnburn please squash the commits |
@rnburn thank you for doing this 👍 |
Happy to squash the commits, but couldn't we just use Squash and merge. |
@rnburn please squash, is not related to the merge but in case of rollback the change |
--add-module=$BUILD_PATH/nginx-opentracing-$NGINX_OPENTRACING/opentracing \ | ||
--add-module=$BUILD_PATH/nginx-opentracing-$NGINX_OPENTRACING/zipkin" | ||
--add-dynamic-module=$BUILD_PATH/nginx-opentracing-$NGINX_OPENTRACING_VERSION/opentracing | ||
--add-dynamic-module=$BUILD_PATH/nginx-opentracing-$NGINX_OPENTRACING_VERSION/zipkin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check https://github.com/rnburn/ingress-nginx/blob/c26af267c5a3d2546ace345122b0fc007a287204/images/nginx-slim/build.sh#L243
you need to copy the opentracing and zipkin modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make install
does copy the modules by default into whatever --modules-path
is set to or /usr/share/nginx/modules
by default.
If you want the modules installed to /etc/nginx/modules/
, any reason you don't just set --with-modules-path=/etc/nginx/modules
? If you look at the image, you can see you have ngx_http_modsecurity_module.so
installed twice:
root@cb1282564228:/# ls /usr/share/nginx/modules/
ngx_http_modsecurity_module.so ngx_http_opentracing_module.so ngx_http_zipkin_module.so
root@cb1282564228:/# ls /etc/nginx/modules/
ngx_http_modsecurity_module.so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnburn you are right. Please change --with-modules-path=/etc/nginx/modules and remove the cp command
@aledbf I changed to use |
/lgtm |
@rnburn thanks! |
This PR updates to build in the latest version of opentracing-contrib/nginx-opentracing, which switches to use dynamic-modules. This provides more isolation if opentracing is disabled and furthermore opens the door to supporting additional tracers in the future. (For example, Jaeger can be added in once their C++ tracer is available).
It also updates the example to demonstrate how traces can join together spans from separate services.
Additionally, it updates the zipkin library so that it can report traces to Jaeger; though propagation won't work out-of-the-box unless Jaeger clients are explicitly configured to work with zipkin.