Skip to content

Build gRPC with CMake for OLTP exporter #369

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

Build gRPC with CMake for OLTP exporter #369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions exporters/otlp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
include_directories(include)

add_library(opentelemetry_exporter_otprotocol src/recordable.cc)
target_include_directories(
opentelemetry_exporter_otprotocol
PUBLIC $<TARGET_PROPERTY:opentelemetry_proto,INTERFACE_INCLUDE_DIRECTORIES>)
target_link_libraries(opentelemetry_exporter_otprotocol
$<TARGET_OBJECTS:opentelemetry_proto>)

Copy link
Copy Markdown
Contributor

@pyohannes pyohannes Oct 19, 2020

Choose a reason for hiding this comment

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

For the tests below, could you please add otlp_exporter_test in addition to recordable_test? That should now link properly with the gRPC dependency available.

Copy link
Copy Markdown
Author

@ahaeber ahaeber Nov 8, 2020

Choose a reason for hiding this comment

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

Sorry for the long delay. I've tried to add the tests, but I found that gRPC actually uses the same flag to enable tests as in this project. That gave me a problem since one of gRPC's tests didn't build correctly. I currently workaround it by renaming the OT flag to BUILD_TESTING_OT. Next it turned out that need to generate more gRPC code for testing, but unfortunately I haven't had time to complete that yet due to lot's of other work. Hopefully will get better time in the coming weeks to get this completed.

Any thoughts on the flag to enable tests since it clashes with gRPC? Since it slows down the build having to also test gRPC and its dependencies it certainly has disadvantages. But it can also be considered as a good thing to know that gRPC works correctly.

Copy link
Copy Markdown
Contributor

@pyohannes pyohannes Nov 18, 2020

Choose a reason for hiding this comment

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

I think gRPC uses grpc_BUILD_TEST. In any case, we should avoid running gRPC tests.

If it's necessary, I have no qualms with renaming BUILD_TESTING and adding a OT specific prefix or postfix.

Copy link
Copy Markdown
Author

@ahaeber ahaeber Nov 26, 2020

Choose a reason for hiding this comment

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

I think you're right, @pyohannes. It's a Abseil, a dependency to gRPC, that uses BUILD_TESTING too.
Unfortunately, I still can't tests to be built. It appears to be a clash between which protobuf compiler I've got installed on my system and what is included in gRPC added with CMake.

At the top of the generated header files for gRPC it adds:

#include <google/protobuf/port_def.inc>
#if PROTOBUF_VERSION < 3011000
#error This file was generated by a newer version of protoc which is
#error incompatible with your Protocol Buffer headers. Please update
#error your headers.
#endif
#if 3011002 < PROTOBUF_MIN_PROTOC_VERSION
#error This file was generated by an older version of protoc which is
#error incompatible with your Protocol Buffer headers. Please
#error regenerate this file with a newer version of protoc.
#endif

One workaround is to also add Protobuf with FetchContent, but I found a project that struggled to make that approach work google/or-tools#2219 which links to a CMake issue at https://gitlab.kitware.com/cmake/cmake/-/issues/17735

Copy link
Copy Markdown
Contributor

@pyohannes pyohannes Nov 26, 2020

Choose a reason for hiding this comment

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

I wonder how this clash between protobuf versions comes about. In such a scenario, we should always use the one added with gRPC via CMake (by adding local paths, include paths and library paths before system paths).

Expand Down
82 changes: 51 additions & 31 deletions third_party/opentelemetry-proto/Protobuf.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,68 @@
include(FetchContent)
FetchContent_Declare(
gRPC
GIT_REPOSITORY https://github.com/grpc/grpc
GIT_TAG v1.32.0)
FetchContent_MakeAvailable(gRPC)
Copy link
Copy Markdown
Contributor

@pyohannes pyohannes Oct 19, 2020

Choose a reason for hiding this comment

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

As far as I know this requires CMake 3.14+, we currently have a minimum requirement of 3.1.

Could you please increase the required CMake version in the global CMakeLists.txt file? Then we can review this together with this PR.

Copy link
Copy Markdown
Contributor

@ThomsonTan ThomsonTan Oct 21, 2020

Choose a reason for hiding this comment

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

Could upgrade CMake to 3.14 causes compatibility issue? In my Ubuntu 18.04 LTS, the default installed one is 3.10.

Copy link
Copy Markdown
Contributor

@maxgolov maxgolov Dec 18, 2020

Choose a reason for hiding this comment

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

It'll take 15 minutes of CI build time to build latest CMake from source for Ubuntu 14.xx and Ubuntu 18.xx

Copy link
Copy Markdown
Contributor

@maxgolov maxgolov Dec 18, 2020

Choose a reason for hiding this comment

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

I added this script here: https://github.com/open-telemetry/opentelemetry-cpp/blob/master/tools/setup-cmake.sh and the best scenario to run it while creating docker image, to subsequently reuse that docker image (that's how I build locally). We need to be creative for CI. Either publishing docker image or using GitHub cache action (5GB cache) to pre-cache an image that is built once, then we save 15 mins of CMake buld time.... I assume we do not want to use some Ubuntu PPA to host the latest CMake compiled for old Ubuntu..


set(PROTO_PATH "${CMAKE_SOURCE_DIR}/third_party/opentelemetry-proto")

set(COMMON_PROTO "${PROTO_PATH}/opentelemetry/proto/common/v1/common.proto")
set(RESOURCE_PROTO "${PROTO_PATH}/opentelemetry/proto/resource/v1/resource.proto")
set(RESOURCE_PROTO
"${PROTO_PATH}/opentelemetry/proto/resource/v1/resource.proto")
set(TRACE_PROTO "${PROTO_PATH}/opentelemetry/proto/trace/v1/trace.proto")

set(GENERATED_PROTOBUF_PATH "${CMAKE_BINARY_DIR}/generated/third_party/opentelemetry-proto")
set(GENERATED_PROTOBUF_PATH
"${CMAKE_BINARY_DIR}/generated/third_party/opentelemetry-proto")

file(MAKE_DIRECTORY "${GENERATED_PROTOBUF_PATH}")

set(COMMON_PB_CPP_FILE "${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/common/v1/common.pb.cc")
set(COMMON_PB_H_FILE "${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/common/v1/common.pb.h")
set(RESOURCE_PB_CPP_FILE "${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/resource/v1/resource.pb.cc")
set(RESOURCE_PB_H_FILE "${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/resource/v1/resource.pb.h")
set(TRACE_PB_CPP_FILE "${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/trace/v1/trace.pb.cc")
set(TRACE_PB_H_FILE "${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/trace/v1/trace.pb.h")
set(COMMON_PB_CPP_FILE
"${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/common/v1/common.pb.cc")
set(COMMON_PB_H_FILE
"${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/common/v1/common.pb.h")
set(RESOURCE_PB_CPP_FILE
"${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/resource/v1/resource.pb.cc")
set(RESOURCE_PB_H_FILE
"${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/resource/v1/resource.pb.h")
set(TRACE_PB_CPP_FILE
"${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/trace/v1/trace.pb.cc")
set(TRACE_PB_H_FILE
"${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/trace/v1/trace.pb.h")
set(TRACE_GRPC_PB_CPP_FILE
"${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/trace/v1/trace.grpc.pb.cc")
set(TRACE_GRPC_PB_H_FILE
"${GENERATED_PROTOBUF_PATH}/opentelemetry/proto/trace/v1/trace.grpc.pb.h")

foreach(IMPORT_DIR ${PROTOBUF_IMPORT_DIRS})
list(APPEND PROTOBUF_INCLUDE_FLAGS "-I${IMPORT_DIR}")
endforeach()

add_custom_command(
Copy link
Copy Markdown
Contributor

@maxgolov maxgolov Dec 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally this file kinda belongs to this repo: https://github.com/open-telemetry/opentelemetry-proto - there is a makefile in there that handles the protobufs for all languages except C++ and CMake :) .. Logical reasoning: if mainline opentelemetry-proto maintains build scripts for other languages and build systems, then the CMake portion has to be contributed in there as well. Then we'd consume it from there using submodule.

OUTPUT
${COMMON_PB_H_FILE}
${COMMON_PB_CPP_FILE}
${RESOURCE_PB_H_FILE}
${RESOURCE_PB_CPP_FILE}
${TRACE_PB_H_FILE}
${TRACE_PB_CPP_FILE}
COMMAND ${PROTOBUF_PROTOC_EXECUTABLE}
ARGS
"--proto_path=${PROTO_PATH}"
${PROTOBUF_INCLUDE_FLAGS}
OUTPUT ${COMMON_PB_H_FILE}
${COMMON_PB_CPP_FILE}
${RESOURCE_PB_H_FILE}
${RESOURCE_PB_CPP_FILE}
${TRACE_PB_H_FILE}
${TRACE_PB_CPP_FILE}
${TRACE_GRPC_PB_H_FILE}
${TRACE_GRPC_PB_CPP_FILE}
COMMAND
${PROTOBUF_PROTOC_EXECUTABLE} ARGS "--grpc_out=${GENERATED_PROTOBUF_PATH}"
"--proto_path=${PROTO_PATH}" ${PROTOBUF_INCLUDE_FLAGS}
"--cpp_out=${GENERATED_PROTOBUF_PATH}"
${COMMON_PROTO}
${RESOURCE_PROTO}
${TRACE_PROTO}
)

include_directories(SYSTEM "${CMAKE_BINARY_DIR}/generated/third_party/opentelemetry-proto")

add_library(opentelemetry_proto OBJECT
${COMMON_PB_CPP_FILE}
${RESOURCE_PB_CPP_FILE}
${TRACE_PB_CPP_FILE})
if (BUILD_SHARED_LIBS)
--plugin=protoc-gen-grpc=$<TARGET_FILE:grpc_cpp_plugin> ${COMMON_PROTO}
${RESOURCE_PROTO} ${TRACE_PROTO})

include_directories(${GENERATED_PROTOBUF_PATH})

add_library(
opentelemetry_proto OBJECT ${COMMON_PB_CPP_FILE} ${RESOURCE_PB_CPP_FILE}
${TRACE_PB_CPP_FILE} ${TRACE_GRPC_PB_CPP_FILE})
set_target_properties(
opentelemetry_proto
PROPERTIES INTERFACE_INCLUDE_DIRECTORIES ${GENERATED_PROTOBUF_PATH}
INTERFACE_LINK_LIBRARIES gRPC::grpc)
if(BUILD_SHARED_LIBS)
set_property(TARGET opentelemetry_proto PROPERTY POSITION_INDEPENDENT_CODE ON)
endif()