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

add nginx-plus-module-otel to base image #4508

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

justdan96
Copy link

@justdan96 justdan96 commented Oct 12, 2023

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.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Signed-off-by: Dan <justdan96@gmail.com>
@justdan96 justdan96 requested a review from a team as a code owner October 12, 2023 10:30
@jjngx
Copy link
Contributor

jjngx commented Oct 12, 2023

Hi @justdan96, thank you for opening the PR. Please follow the project contributing guideline described here.

@brianehlert brianehlert added enhancement Pull requests for new features/feature enhancements backlog Pull requests/issues that are backlog items labels Oct 12, 2023
@brianehlert
Copy link
Collaborator

This could benefit from:

  • an Issue that describes the enhancement (this informs release notes and watchers)
  • at least an example showing other users how to use the module

@justdan96
Copy link
Author

justdan96 commented Oct 13, 2023

an Issue that describes the enhancement (this informs release notes and watchers)

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).

at least an example showing other users how to use the module

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.:

MainOpenTracingLoadModule bool

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 \
Copy link
Member

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?

Copy link
Collaborator

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.

@brianehlert
Copy link
Collaborator

brianehlert commented Oct 20, 2023

The key question that we need an answer for before accepting this PR is:

  • can both the OpenTracing and OpenTelemetry modules be installed at the same time and not interact in a negative way with each other?

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.
We totally embrace moving to the new native NGINX OpenTelemetry module but want to be sure to also do the right thing for anyone using OpenTracing and not introduce a release that immediately breaks them if we can avoid it.

Can anyone / has anyone investigated this?

@brianehlert
Copy link
Collaborator

Also, the NGINX OpenTelemetry module should be an option for both NGINX Plus and NGINX open source.

@justdan96
Copy link
Author

  1. We have both modules installed at the same time with no ill effects. We have only one loaded though.
  2. I can amend the Dockerfile to add it to further images.

@brianehlert
Copy link
Collaborator

Since this will be configured exclusively using snippets at this time, it would be great if there was also an example included.
Probably under /shared-examples (not entirely sure though)
That could help us cover a documentation gap.

@justdan96
Copy link
Author

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?

@brianehlert
Copy link
Collaborator

I think the key here is that we need a solution for this for both the free and N+ releases.

@defanator
Copy link

  • can both the OpenTracing and OpenTelemetry modules be installed at the same time and not interact in a negative way with each other?

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.

@brianehlert
Copy link
Collaborator

If we can finish this with the N+ OTEL module that is fine.
While any N OSS module sorts itself out.

@vepatel vepatel linked an issue Jan 18, 2024 that may be closed by this pull request
@brianehlert brianehlert requested a review from a team February 21, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Pull requests/issues that are backlog items enhancement Pull requests for new features/feature enhancements
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Add nginx-plus-module-otel to base image
5 participants