Skip to content

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

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

Closed
wants to merge 1 commit into 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

@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.: https://github.com/nginxinc/kubernetes-ingress/blob/2641fbb237352112912e2698994015286863d21b/internal/configs/config_params.go#L32
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/.

@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
Contributor

  • 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
Copy link

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Pull requests/issues with no activity label May 22, 2024
Copy link

github-actions bot commented Jun 1, 2024

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this Jun 1, 2024
@justdan96
Copy link
Author

I need to look at this again at some point.

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 stale Pull requests/issues with no activity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

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