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
add nginx-plus-module-otel to base image #4508
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Dan <justdan96@gmail.com>
Hi @justdan96, thank you for opening the PR. Please follow the project contributing guideline described here. |
This could benefit from:
|
There isn't a lot to describe, this just avoids users having to compile their own images to add the otel module (or use a custom init container).
It currently isn't possible to use the module (even with this merged) as it needs to be enabled in the template. It would need code similar to that for OpenTracing - i.e.:
I don't have time to do that at the moment so this PR is just the first step. Users can otherwise refer to this page for module configuration parameters: https://docs.nginx.com/nginx/admin-guide/dynamic-modules/opentelemetry/. |
@@ -43,7 +43,7 @@ RUN --mount=type=secret,id=nginx-repo.crt,dst=/etc/apk/cert.pem,mode=0644 \ | |||
--mount=type=bind,from=alpine-opentracing-lib,target=/tmp/ot/ \ | |||
wget -nv -O /etc/apk/keys/nginx_signing.rsa.pub https://cs.nginx.com/static/keys/nginx_signing.rsa.pub \ | |||
&& printf "%s\n" "https://pkgs.nginx.com/plus/${NGINX_PLUS_VERSION}/alpine/v$(grep -E -o '^[0-9]+\.[0-9]+' /etc/alpine-release)/main" >> /etc/apk/repositories \ | |||
&& apk add --no-cache nginx-plus nginx-plus-module-njs nginx-plus-module-opentracing nginx-plus-module-fips-check libcap libcurl \ | |||
&& apk add --no-cache nginx-plus nginx-plus-module-njs nginx-plus-module-opentracing nginx-plus-module-otel nginx-plus-module-fips-check libcap libcurl \ |
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.
do we want to install both OpenTracing and OpenTelemetry?
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.
Is there any problem with doing so?
the project has supported OpenTracing for so long that this would essentially be a breaking change for tracing if we suddenly replaced OpenTracing, and there is not any enablement through YAML - OpenTel tracing would be all snippets based configuration until we can develop the YAML patterns.
The key question that we need an answer for before accepting this PR is:
This is so we can give a path for customers to migrate from OpenTracing to OpenTelemetry Tracing in-place and not introduce an immediately breaking change. Can anyone / has anyone investigated this? |
Also, the NGINX OpenTelemetry module should be an option for both NGINX Plus and NGINX open source. |
|
Since this will be configured exclusively using snippets at this time, it would be great if there was also an example included. |
Actually for point 2 there doesn't seem to be Docker images I could copy the .so files from, so I am assuming we would want to compile from source? |
I think the key here is that we need a solution for this for both the free and N+ releases. |
I believe they can, there's nothing that should prevent having both and using just the one. I'm tagging @dplotnikov-f5 and @thresheek as they could share first-hand feedback. |
If we can finish this with the N+ OTEL module that is fine. |
Proposed changes
This PR is just to add nginx-plus-module-otel to the base images, so that OpenTelemetry can be configured easily.
Checklist
Before creating a PR, run through this checklist and mark each as complete.