[BUILD] Transitive dependency issue with the otlp http exporter

[astitcher]

Fixes #2153

Changes

Change nlohmann_json and protobuf dependencies of the otlp http exporter to be private not public so that applications/libraries linking with this exporter don't need to have the cmake config nor the header files for them present in their builds.

##Comments

As far as I can see these don't need to be public or even interface dependencies because they are only used by the exporter itself in its own code. They aren't exposed either by the exporter header files by a transitive include or by the header files mentioning a symbol that is only in one of these libraries.

Using this change allows my builds to proceed.

[marcalff]

@astitcher Thanks for the report and PR.

@owent Could you take a look, and let me know if release 1.9.1 needs to wait for this fix ? (I don't know CMake well enough to assess the fix)

Thanks.

[astitcher]

If this works I'd love to get it into 0.9.1 as we've been trying to release qpid-proton 0.39 and this will depend on some changes to our examples to use the otlp http exporter instead of the jaeger exporter which is now deprecated. But we can't get OT to build and work correctly in our CI builds with qpid-proton yet.

[marcalff]

@astitcher

If this works I'd love to get it into 0.9.1

I assume you meant 1.9.1

Please adjust the formatting for CMakeLists.txt, to fix the CI Format failure.

From the CI log:

The following files have changes:
exporters/otlp/CMakeLists.txt
diff --git a/exporters/otlp/CMakeLists.txt b/exporters/otlp/CMakeLists.txt
index b453b79..77804ab 100644
--- a/exporters/otlp/CMakeLists.txt
+++ b/exporters/otlp/CMakeLists.txt
@@ -113,8 +113,8 @@ if(WITH_OTLP_HTTP)
 
   target_link_libraries(
     opentelemetry_exporter_otlp_http_client
-    PRIVATE opentelemetry_sdk opentelemetry_proto opentelemetry_http_client_curl
-           nlohmann_json::nlohmann_json)
+    PRIVATE opentelemetry_sdk opentelemetry_proto
+            opentelemetry_http_client_curl nlohmann_json::nlohmann_json)
   if(nlohmann_json_clone)
     add_dependencies(opentelemetry_exporter_otlp_http_client
                      nlohmann_json::nlohmann_json)
@@ -197,9 +197,9 @@ endif()
 
 if(BUILD_TESTING)
   add_executable(otlp_recordable_test test/otlp_recordable_test.cc)
-  target_link_libraries(otlp_recordable_test ${GTEST_BOTH_LIBRARIES}
-                        ${CMAKE_THREAD_LIBS_INIT} opentelemetry_otlp_recordable
-		        protobuf::libprotobuf)
+  target_link_libraries(
+    otlp_recordable_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
+    opentelemetry_otlp_recordable protobuf::libprotobuf)
   gtest_add_tests(
     TARGET otlp_recordable_test
     TEST_PREFIX exporter.otlp.
@@ -218,9 +218,10 @@ if(BUILD_TESTING)
 
   add_executable(otlp_metrics_serialization_test
                  test/otlp_metrics_serialization_test.cc)
-  target_link_libraries(otlp_metrics_serialization_test ${GTEST_BOTH_LIBRARIES}
-                        ${CMAKE_THREAD_LIBS_INIT} opentelemetry_otlp_recordable
-		        protobuf::libprotobuf)
+  target_link_libraries(
+    otlp_metrics_serialization_test ${GTEST_BOTH_LIBRARIES}
+    ${CMAKE_THREAD_LIBS_INIT} opentelemetry_otlp_recordable
+    protobuf::libprotobuf)
   gtest_add_tests(
     TARGET otlp_metrics_serialization_test
     TEST_PREFIX exporter.otlp.
@@ -309,8 +310,11 @@ if(BUILD_TESTING)
   if(WITH_OTLP_HTTP)
     add_executable(otlp_http_exporter_test test/otlp_http_exporter_test.cc)
     target_link_libraries(
-      otlp_http_exporter_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
-      ${GMOCK_LIB} opentelemetry_exporter_otlp_http
+      otlp_http_exporter_test
+      ${GTEST_BOTH_LIBRARIES}
+      ${CMAKE_THREAD_LIBS_INIT}
+      ${GMOCK_LIB}
+      opentelemetry_exporter_otlp_http
       opentelemetry_http_client_nosend
       nlohmann_json::nlohmann_json
       protobuf::libprotobuf)

[codecov]

Codecov Report

Merging #2154 (0247534) into main (61e8741) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2154   +/-   ##
=======================================
  Coverage   87.17%   87.17%           
=======================================
  Files         166      166           
  Lines        4777     4777           
=======================================
  Hits         4164     4164           
  Misses        613      613           

[astitcher]

@astitcher

If this works I'd love to get it into 0.9.1

I assume you meant 1.9.1

Oops, yes - confused by the qpid-proton current version numbering!

[astitcher]

Please adjust the formatting for CMakeLists.txt, to fix the CI Format failure.

Will do and force push.

However I'm a little confused because I'm looking at the diff for my change and it has the identical leading spaces as the previous lines - has the CI format checker changed since the original lines went in?

[astitcher]

The misspell failure seems to be a CI failure not related to the change in the PR

[owent]

opentelemetry_exporter_otlp_http_client use the headers of opentelemetry_sdk and opentelemetry_ext.I believe it would be better to keep public link opentelemetry_sdk and opentelemetry_ext and private link opentelemetry_proto, opentelemetry_http_client_curl and nlohmann_json.

[astitcher]

This makes sense - I think I lazily just made everything private instead of looking more carefully. Thanks for the review.

[astitcher]

I've made the requested changes and repushed the PR

[owent]

I think it would be better to public link to opentelemetry_ext, because opentelemetry_exporter_otlp_http_client use the headers of http client.

[astitcher]

But is that relevant to my change as it wasn't there in the first place?
Also looking at the installed ot libraries there doesn't seem to be a libopentelemetry_ext.so library, so what does this public linkage mean? Without an actual library surely the most this could be would be an INTERFACE dependency?
But maybe I'm misunderstanding what you mean

[owent]

Yes, opentelemetry_ext is a interface target.It's created in https://github.com/open-telemetry/opentelemetry-cpp/blob/main/ext/CMakeLists.txt and can be used to tell other targets where to find the headers of ext.Especially when users use add_subdirectory to import optelemetry.

[astitcher]

So you'd like me to add it as a dependency even though it wasn't there before I made my change - I can do that, but it seems a bit unfair to hold up my PR because of a previously missing dependency.

[owent]

Sorry for that, but we did not previously miss this dependency. The reason for this is that opentelemetry_ext is publicly linked to opentelemetry_http_client_curl in https://github.com/open-telemetry/opentelemetry-cpp/blob/main/ext/src/http/client/curl/CMakeLists.txt. Additionally, opentelemetry_ext will be implicitly publicly linked by target_link_libraries(opentelemetry_exporter_otlp_http_client PUBLIC opentelemetry_http_client_curl). However, after we privately link opentelemetry_http_client_curl to opentelemetry_exporter_otlp_http_client, the dependency will become invisible to other targets that use target_link_libraries(<TARGET_NAME> PUBLIC|PRIVATE opentelemetry_exporter_otlp_http_client ).
This change may break certain use cases.
Currently, we lack unit tests for this type of scenario. I think we can add these tests in the future and in other PRs and issues.

[astitcher]

Ok - that's fair enough.
Although I'm a little curious about the fine grained internal parts of OT because doing it this way implies that you could have only some of them instlled on a system, but the extensive internal dependencies seems to me to mean that the options for this are actually much more limited and OT actually in practice tends to be more of a monolith. Albeit with some exceptions around the exporters.
I do understand that the intent is that the API itself is entirely header file and standalone, but in practice you always need an implementation and some exporters anyway, and these seem much more monolithic.
Sorry if this is a bit ranty but I'm just finding that working with the OT internal dependencies to be very unintuitive and difficult seeing that it is a large project with lots of internal dependencies that are hiding external ones that are hard to deal with.

[owent]

Yes, we use cmake CONFIG and modern cmake targets to resolve dependencies.Many components are optional but some of them depend each other. We hope users just concerns the components they use and the depended component can be added automatically.We use target_link_libraries(A PUBLIC B) in cmake and deps setting in bazel.They are similar.

[astitcher]

Another force push to fix the cmake format lint complaint (that linter is annoyingly picky and unpredictable)

[marcalff]

@astitcher

Please avoid using force push if you can, it makes the review harder.

Multiple commits will all be squashed at the end when merging to main, it is not an issue.

For spurious CI failures like spell check, no need to worry about them, I will restart the CI to clear them, as I have done before.

@owent

Thanks for the review comments, you are the expert in this area.

Once a revised patch is both:

• approved by owent
• verified to work by andrew

I will happily merge it to main and make a release right away for 1.9.1

[astitcher]

Just to confirm that this version of the change works for me.

[astitcher]

Just to confirm that this version of the change works for me.

Actually I was mistaken - it turns out that this new version of the change does not work for me, and I think that the header file structure of the otlp http exporters can never work with the packaged version of protobuf for any ubuntu lts or released version of Red Hat.

This is because the opentelemetry_otlp_recordable library/target has header files that include protobuf header files and this library has to be a public dependency because other targets opentelemetry_otlp_http_exporter for example use opentelemetry::exporter::otlp::GetOtlpDefaultHttpTracesEndpoint() in it's own header file so that applications need this symbol which is exported by opentelemetry_otlp_recordable.

It is not clear to me if there is a need to actually be exporting the protobuf types out of ...recordable or whether it is just accidental complexity, but at this point I'm stuck - I can remove the nlohmann_json dependency because that is purely internal and nicely encapsulated not leaking in the header files. But the protobuf dependency leaks and the packages of protobuf available on most deployed linux systems do not have the cmake config files installed necessary to make a cmake build of the exporters work.
@owent Do you have any insight that might help me here? We should probably move this to its own issue, as it seems to me to somewhat serious to deploying the otlp http code.

[owent]

Just to confirm that this version of the change works for me.

Actually I was mistaken - it turns out that this new version of the change does not work for me, and I think that the header file structure of the otlp http exporters can never work with the packaged version of protobuf for any ubuntu lts or released version of Red Hat.

This is because the opentelemetry_otlp_recordable library/target has header files that include protobuf header files and this library has to be a public dependency because other targets opentelemetry_otlp_http_exporter for example use opentelemetry::exporter::otlp::GetOtlpDefaultHttpTracesEndpoint() in it's own header file so that applications need this symbol which is exported by opentelemetry_otlp_recordable.

It is not clear to me if there is a need to actually be exporting the protobuf types out of ...recordable or whether it is just accidental complexity, but at this point I'm stuck - I can remove the nlohmann_json dependency because that is purely internal and nicely encapsulated not leaking in the header files. But the protobuf dependency leaks and the packages of protobuf available on most deployed linux systems do not have the cmake config files installed necessary to make a cmake build of the exporters work. @owent Do you have any insight that might help me here? We should probably move this to its own issue, as it seems to me to somewhat serious to deploying the otlp http code.

We can use OtlpHttpExporterFactory and OtlpGrpcExporterFactory to create exporters, and then we do not need include headers of protobuf.
In cmake, The FindProtobuf module can be used to find prebuilt protobuf that installed in system, and it will create all necessary IMPORTED targets and sett all variables we need, just like CONFIG file installed by protobuf.

[astitcher]

We can use OtlpHttpExporterFactory and OtlpGrpcExporterFactory to create exporters, and then we do not need include headers of protobuf.

I think even in this case the cmake configure process can't know what you will use just what it is possible for you to use! So it still needs to make sure that the dependencies are there and are linked into the final executable.

In cmake, The FindProtobuf module can be used to find prebuilt protobuf that installed in system, and it will create all necessary IMPORTED targets and sett all variables we need, just like CONFIG file installed by protobuf.

The issue here is that the application author needs to know that they need to use find_package(Protobuf) even though although all they were trying to do should be covered by find_package(opentelemetry_cpp) and they don't care about protobuf at all.

With the very most recent versions of protobuf this seems to be fixed as protobuf itself now provides cmake config without requiring the explicit find_package(). This is my current understanding, but I may have missed something.

[astitcher]

As a workaround I'm going to try adding find_package(Protobuf) to the appropriate part of the qpid_proton build and try building again with this patch which is still useful to get the nlohmann_json dependency otu of the way. I'll update the PR if this succeeds as I don't want to hold up 1.9.1 any longer than necessary.

[astitcher]

@marcalff I can now confirm that with my workaround to qpid-proton this PR does now build correctly for me under ubuntu 22.04. So merging this and releasing 1.9.1 would be a great help to us!

[marcalff]

@marcalff I can now confirm that with my workaround to qpid-proton this PR does now build correctly for me under ubuntu 22.04. So merging this and releasing 1.9.1 would be a great help to us!

Thanks @astitcher

In my understanding, @owent comments have been addressed, so I expect he will be able to approve this PR.

Due to the time zone difference, looking to merge most likely tomorrow.

[owent]

LGTM and thanks

[marcalff]

LGTM, thanks for the fix and testing.

[owent]

I think even in this case the cmake configure process can't know what you will use just what it is possible for you to use! So it still needs to make sure that the dependencies are there and are linked into the final executable.

If we build otel-cpp as shared libraries, and link opentelemetry_proto, opentelemetry_http_client_curl and nlohmann_json privately into opentelemetry_exporter_otlp_http_client, these components and protobuf will not be exported as INTERFACE_LINK_LIBRARIES of opentelemetry_exporter_otlp_http_client, so users do not need find_package(Protobuf) when only use opentelemetry_exporter_otlp_http_client.
I find opentelemetry_proto is also public linked into opentelemetry_otlp_recordable and opentelemetry_exporter_otlp_grpc_client, and opentelemetry_otlp_recordable is public depended by all OTLP exporters, That may be why you still need find_package(Protobuf) if you are using any OTLP exporter.

It is not clear to me if there is a need to actually be exporting the protobuf types out of ...recordable or whether it is just accidental complexity, but at this point I'm stuck - I can remove the nlohmann_json dependency because that is purely internal and nicely encapsulated not leaking in the header files. But the protobuf dependency leaks and the packages of protobuf available on most deployed linux systems do not have the cmake config files installed necessary to make a cmake build of the exporters work.

Agree and thanks. I raise a new issue to track this optimization.We hope users only use factories to create OTLP exporters, so all the dependencies in headers should be public.We have already used static_assert to ensure the headers of factory and options do not depend protobuf, and I think we can move the link relationship into private in the next step.
There is one problem that the factories are compiled into exporter targets.If we set these dependencies private and if users want to use the underground components, there will be no easy way for them to import all dependencies.Do you think we can add some interface targets which keep these dependencies public for these cases?
Thanks again and I have a new raise issue (#2159 ). @marcalff could you please help to assign it to @astitcher ?

With the very most recent versions of protobuf this seems to be fixed as protobuf itself now provides cmake config without requiring the explicit find_package(). This is my current understanding, but I may have missed something.

The lateset version of protobuf also need users to use find_package(Protobuf), it provide a cmake CONFIG file to replace the legacy FindProtobuf.cmake, but they all need use find_package(Protobuf) to import.There are some flags can be used to tell cmake prefer the new CONFIG mode or legacy MODULE mode.