[RFC] Poc config yaml #2518
[RFC] Poc config yaml #2518
Conversation
|
/easycla |
Added CODE-REVIEW-PR2518.txt
| if(WITH_OTLP_GRPC) | ||
| include_directories(${CMAKE_SOURCE_DIR}/exporters/otlp/include) | ||
| add_definitions(-DOTEL_HAVE_OTLP_GRPC) | ||
| target_link_libraries(example_yaml opentelemetry_otlp_grpc_builder) |
There was a problem hiding this comment.
Hi @marcalff, I took a look today and wanted to share some thoughts and initial observations of the cmake changes to help spark some discussion about how to fit this pr into the new cmake components introduced by #3220:
Initial Observations:
This PR introduces some new cmake targets. These targets include theopentelemetry-cpp::configuration target from sdk/src/configuration and opentelemetry-cpp::init target from sdk/src/init as well as opentelemetry-cpp::*_builder targets (ie: opentelemetry-cpp::otlp_grpc_builder) created in the respective exporter directories.
The example_yaml shows how the new targets must be linked to an executable independently (ie: the opentelemetry-cpp::init target and the opentelemetry-cpp::*_builder targets must be explicitly linked to the example).
This PR also introduces the WITH_CONFIGURATION cmake option which determines if the opentelemetry-cpp::configuration and opentelemetry-cpp::init targets are created along with the main example and tests. The opentelemetry-cpp::*_builder targets are created independent of this option and are created if the corresponding WITH_* option is on (ie: WITH_OTLP_GRPC=ON creates the opentelemetry-cpp::otlp_grpc_builder target).
Summary of the cmake component approach so far:
In the new cmake component approach from #3220, each optional component is tied to a WITH_* option and has a fixed/required set of cmake targets defined by cmake/component_definitions.cmake. This creates a strong guarantee that if a component is successfully found with find_package(opentelemetry-cpp COMPONENTS <component>), then all cmake targets associated with the component have been imported and can be linked. This enables failing fast at cmake configure time - if a required cmake target was not installed then the user’s build will fail on cmake configure instead of failing at build or run time.
Initial thoughts on options to associated new targets with components:
- Option 1: Create a single “configuration” component and assign all new targets to it
- Guard all configuration related targets with the WITH_CONFIGURATION option
- Allow
opentelemetry-cpp::*_buildertargets to be optional based on theWITH_*options, but link them to a commonopentelemetry-cpp::configuration_builderstarget that will be the primary interface for users - Add all related targets to a single component called “configuration”
- Mark the
rymldependency as required by theconfigurationcomponent. - Set the
opentelemetry-cpp::init,opentelemetry-cpp::configuration, andopentelemetry-cpp::configuration_builderscmake targets as required for the component.
- Option 2: Assign the new targets to existing components and allow them to be optional
- Guard all configuration related targets with the
WITH_CONFIGURATIONoption - Add
initandconfigurationtargets to thesdkcomponent - Set the
rymldependency as optional for thesdkcomponent. - Add the
opentelemetry-cpp::*_buildertargets to the components associated with theWITH_*options that guard them now. - Update the cmake components framework to support marking targets as optional for each component and provide some other mechanism to inform the user which targets are imported after calling find_package(opentelemetry-cpp..).
- Guard all configuration related targets with the
Just some initial thoughts after a quick look at the PR - there are definitely other options to consider. What do you think?
There was a problem hiding this comment.
Another option:
- Option 3: Create a
configuration_commoncomponent and optional dependent builder components- Add a
configuration_commoncomponent that requires therymldependency andinitandconfigurationtargets - Add builder components that depend on the common component (ie:
configuration_otlp_grpc) that require the builder targets
- Add a
This would allow cmake usage like this:
find_package(opentelemetry-cpp COMPONENTS configuration_otlp_grpc configuration_otlp_file)
add_library(my_lib my_lib.cc)
target_link_libraries(my_lib PRIVATE
opentelemetry-cpp::init
opentelemetry-cpp::configuration
opentelemetry-cpp::otlp_grpc_builder
opentelemetry-cpp::otlp_file_builder) 
Fixes #2481
Request For Comments (Proof of concept code for now)
Start with examples/yaml/main.cc
Features implemented:
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changesCode review
Due to the size, code review to be done block by block.
See file
CODE-REVIEW-PR2518.txtfor details.REVIEW BLOCK 1: Trace Model
Files to review: