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

Setup nginx config file for dynamic module building #52

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Setup nginx config file for dynamic module building #52

merged 1 commit into from
Sep 20, 2021

Conversation

dmathieu
Copy link
Member

This sets up a config file so nginx knows how to use this module as a dynamic one.
See https://www.nginx.com/resources/wiki/extending/new_config/

With this change, passing the option

--add-dynamic-module=/path/to/opentelemetry-cpp-contrib/instrumentation/nginx"

to when configuring nginx allows building this plugin as well.

@dmathieu dmathieu requested a review from seemk as a code owner September 10, 2021 08:48
@dmathieu dmathieu requested a review from a team September 10, 2021 08:48
@dmathieu
Copy link
Member Author

Hi @TomRoSystems @seemk,
Is there anything I can do to help you review this PR?

@seemk
Copy link
Contributor

seemk commented Sep 20, 2021

Hi @TomRoSystems @seemk,
Is there anything I can do to help you review this PR?

Hey, thanks for the ping :) I was away for 2 weeks and just came back today.

With this config file, how are for example OpenTelemetry CPP include paths handled? Are they passed to the nginx config command line?

@dmathieu
Copy link
Member Author

Yes, the include paths are passed to the nginx config command line.
As an example, you can see my draft PR adding this module to ingress-nginx: https://github.com/kubernetes/ingress-nginx/pull/7621/files#diff-d96b4dbdd33ef5b9f1b1c7ce45ef0a515f55a55f130318c1af397e731a42d232R674

@seemk
Copy link
Contributor

seemk commented Sep 20, 2021

If I understand correctly currently it's looking for the include paths in the system path? So if Otel CPP is installed elsewhere, ngx_module_incs would need to be set in the config file? And the same goes for linking?

Not that it matters much, could very well live with this for now, just trying to understand it 😄

@dmathieu
Copy link
Member Author

I'm not really an expert at C compilation. I've adapted this config file from the one in opentracing: https://github.com/opentracing-contrib/nginx-opentracing/blob/master/opentracing/config
I'm happy to add any config that you will think would be better.

@seemk
Copy link
Contributor

seemk commented Sep 20, 2021

In your ingress-nginx PR does it work like this? My hunch is that all the OpenTelemetry libs and libstdc++ would need to be linked to the module at the config file as well: ngx_module_libs="-lstdc++ -lopentelemetry_trace ...".

However OpenTelemetry CPP consists of a huge list of libraries that all need to be linked, all the libabsl_*, libopentelemetry_*.
This was something I had issues with when initially trying to build it via nginx's build system and opted for a CMake build with a workaround (e.g. https://github.com/open-telemetry/opentelemetry-cpp-contrib/blob/main/instrumentation/nginx/src/otel_ngx_module_modules.c) that mimics what the nginx build system does.

@dmathieu
Copy link
Member Author

I haven't been able to do extensive testing of the feature, and sending data yet. But yes, with the library being built in my ingress-nginx, I am able to build the library, and start nginx with the module configured.

@seemk
Copy link
Contributor

seemk commented Sep 20, 2021

Ok, weird. It might be because of system paths then, let's merge it for now if it helps with ingress-nginx, would love to see it used there. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants