open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
860 stars 410 forks source link

Add conditions to the build of the ext subdirectory #1895

Closed gabswb closed 1 year ago

gabswb commented 1 year ago

Is your feature request related to a problem? Currently there is no condition on the add_subdirectory(ext) in the root CMakeLists.txt except the WITH_API_ONLY option. So it adds dependencies like CURL while I only specify the use of OTLP/GRPC in the CMake build options.

Could it be possible to add the ext subdirectory conditionally to the WITH_OTLP_HTTP option and other options that require this subdirectory.

lalitb commented 1 year ago

So it adds dependencies like CURL while I only specify the use of OTLP/GRPC in the CMake build options.

Can you please elaborate this. Even though ext subdirectory is always included, the CURL dependency would only be added to the exporters using it (i.e., Zipkin, Elastic, and OTLP/HTTP). Maybe I am missing something :)

gabswb commented 1 year ago

Maybe I am missing something too but I have a lib opentelemetry_http_client_curl which is build from the ext directory and I don't see a way to disable it

lalitb commented 1 year ago

You are right, this will create the library opentelemetry_http_client_curl if libcurl is present in build system, but this library will not be added as dependency for OTLP/gRPC.

$ ldd libopentelemetry_exporter_otlp_grpc.so | grep curl
$ ldd libopentelemetry_exporter_otlp_http.so | grep curl
    libopentelemetry_http_client_curl.so => /home/labhas/obs/ot/opentelemetry-cpp/build/ext/src/http/client/curl/libopentelemetry_http_client_curl.so (0x00007fc45201c000)
    libcurl.so.4 => /lib/x86_64-linux-gnu/libcurl.so.4 (0x00007fc451e3a000)
gabswb commented 1 year ago

Maybe I'm not using CMake well but it forces me to call find_package(CURL) in the CMakeLists.txt that link ${OPENTELEMETRY_CPP_LIBRARIES}. And this seems to me to be a bad thing ...

lalitb commented 1 year ago

Maybe I'm not using CMake well but it forces me to call find_package(CURL) in the CMakeLists.txt that link ${OPENTELEMETRY_CPP_LIBRARIES}. And this seems to me to be a bad thing ...

Got it, yeah it can be made to check for CURL package quietly in ext/src/http/client/curl/CMakelists.txt, something like -

find_package(CURL QUIET)

Said that, I don't see issue in adding ext subdirectory conditionally. CURL is used for W3C TraceContext test, HTTP example, in addition to Zipkin, Elastic, OTLP/HTTP exporter, so need to include all these in condition. Also need to take care of ZPages server.

Thanks for adding the issue.