Skip to content

[BUILD] fix build of otlp_grpc_exporter_test with modern grpc #3307

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.

Sign up for GitHub

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 3 commits into from
+22 −34
Closed

[BUILD] fix build of otlp_grpc_exporter_test with modern grpc #3307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ed22ee
remove otlp_grpc_exporter_test link to gRPC::grpc++
dbarker Mar 21, 2025
7f325ce
Merge branch 'main' into fix_link_error_with_latest_grpc
dbarker Mar 21, 2025
1c36168
make grpc's link public to the grpc client. make ext's link private t…
dbarker Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 15 additions & 32 deletions exporters/otlp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,11 @@ if(WITH_OTLP_GRPC)
# targets that depend on opentelemetry_proto_grpc.
target_link_libraries(
opentelemetry_exporter_otlp_grpc_client
PUBLIC opentelemetry_sdk opentelemetry_common opentelemetry_ext
PUBLIC opentelemetry_sdk opentelemetry_common
# gRPC::grpc++ must be linked before opentelemetry_proto_grpc.
opentelemetry_proto_grpc
PRIVATE gRPC::grpc++)

get_target_property(GRPC_INCLUDE_DIRECTORY gRPC::grpc++
INTERFACE_INCLUDE_DIRECTORIES)
if(GRPC_INCLUDE_DIRECTORY)
target_include_directories(
opentelemetry_exporter_otlp_grpc_client BEFORE
PUBLIC "$<BUILD_INTERFACE:${GRPC_INCLUDE_DIRECTORY}>")
endif()
opentelemetry_proto_grpc gRPC::grpc++
Copy link
Member

@owent owent Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gRPC client definitions are not exposed to users, so it's fine to keep the gRPC dependency private. Alternatively, we could use PUBLIC $<BUILD_INTERFACE:gRPC::grpc++> PRIVATE $<INSTALL_INTERFACE:gRPC::grpc++> to expose the include directories internally. Symbol conflicts don't only cause issues during compilation or linking but also will result in runtime crashes.

More details can be found at #1603 , #1891 , #1998

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details and appreciate the feedback. It is definitely a challenge to get cmake linking everything under the various build configurations we have.

The feature to create and share a grpc client among exporters is a really nice one and will require users to include the grpc headers brought in by OtlpGrpcClient, which is included by the OtlpGrpcClientFactory. At the moment they would need to explicitly link in gRPC::grpc++ to their project to get those headers. I'll propose a change in a separate PR to add those headers to the install interface of the grpc client target.

Copy link
Member

@owent owent Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we can move OtlpGrpcClientReferenceGuard out of otlp_grpc_client.h, so we don't need to include otlp_grpc_client.h in headers of OtlpGrpcClientFactory?

I raised #3321 , could you please have a look when you have time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'd need to remove all grpc from otlp_grpc_client.h using pmpl and some generic status codes. Alternatively we could give the client a base class and use it in the constructors of the exporters and return from the factory - then deprecate the constructors and create method with the OtlpGrpcClient shard ptr.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually #3321 is a good solution as long as the client methods are accessed outside the scope of the exporters. good call.

PRIVATE opentelemetry_ext)

target_include_directories(
opentelemetry_exporter_otlp_grpc_client
PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include>"
Expand Down Expand Up @@ -290,9 +283,8 @@ 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)
gtest_add_tests(
TARGET otlp_recordable_test
TEST_PREFIX exporter.otlp.
Expand All @@ -308,10 +300,8 @@ 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)
gtest_add_tests(
TARGET otlp_metrics_serialization_test
TEST_PREFIX exporter.otlp.
Expand All @@ -321,7 +311,7 @@ if(BUILD_TESTING)
add_executable(otlp_grpc_exporter_test test/otlp_grpc_exporter_test.cc)
target_link_libraries(
otlp_grpc_exporter_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
${GMOCK_LIB} opentelemetry_exporter_otlp_grpc gRPC::grpc++)
${GMOCK_LIB} opentelemetry_exporter_otlp_grpc)
gtest_add_tests(
TARGET otlp_grpc_exporter_test
TEST_PREFIX exporter.otlp.
Expand Down Expand Up @@ -396,8 +386,7 @@ if(BUILD_TESTING)
${GMOCK_LIB}
opentelemetry_exporter_otlp_http
opentelemetry_http_client_nosend
nlohmann_json::nlohmann_json
protobuf::libprotobuf)
nlohmann_json::nlohmann_json)
gtest_add_tests(
TARGET otlp_http_exporter_test
TEST_PREFIX exporter.otlp.
Expand Down Expand Up @@ -450,8 +439,7 @@ if(BUILD_TESTING)
opentelemetry_exporter_otlp_http_metric
opentelemetry_metrics
opentelemetry_http_client_nosend
nlohmann_json::nlohmann_json
protobuf::libprotobuf)
nlohmann_json::nlohmann_json)
gtest_add_tests(
TARGET otlp_http_metric_exporter_test
TEST_PREFIX exporter.otlp.
Expand Down Expand Up @@ -486,13 +474,9 @@ if(BUILD_TESTING)

add_executable(otlp_file_exporter_test test/otlp_file_exporter_test.cc)
target_link_libraries(
otlp_file_exporter_test
${GTEST_BOTH_LIBRARIES}
${CMAKE_THREAD_LIBS_INIT}
${GMOCK_LIB}
opentelemetry_exporter_otlp_file
nlohmann_json::nlohmann_json
protobuf::libprotobuf)
otlp_file_exporter_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
${GMOCK_LIB} opentelemetry_exporter_otlp_file
nlohmann_json::nlohmann_json)
gtest_add_tests(
TARGET otlp_file_exporter_test
TEST_PREFIX exporter.otlp.
Expand Down Expand Up @@ -543,8 +527,7 @@ if(BUILD_TESTING)
${GMOCK_LIB}
opentelemetry_exporter_otlp_file_metric
opentelemetry_metrics
nlohmann_json::nlohmann_json
protobuf::libprotobuf)
nlohmann_json::nlohmann_json)
gtest_add_tests(
TARGET otlp_file_metric_exporter_test
TEST_PREFIX exporter.otlp.
Expand Down
9 changes: 7 additions & 2 deletions exporters/zipkin/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,13 @@ if(BUILD_TESTING)
add_executable(zipkin_exporter_test test/zipkin_exporter_test.cc)

target_link_libraries(
zipkin_exporter_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
${GMOCK_LIB} opentelemetry_exporter_zipkin_trace opentelemetry_resources)
zipkin_exporter_test
${GTEST_BOTH_LIBRARIES}
${CMAKE_THREAD_LIBS_INIT}
${GMOCK_LIB}
opentelemetry_exporter_zipkin_trace
opentelemetry_resources
${CURL_LIBRARIES})

gtest_add_tests(
TARGET zipkin_exporter_test
Expand Down
Loading