Skip to content
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.

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

[BUILD] Enable building otel-cpp extensions from main repo #1937

Merged
merged 11 commits into from
Jun 28, 2023

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Jan 25, 2023

While we discussed this shortly in earlier community meeting, would like to have wider discussion over the PR and in next community meeting.

Change summary:

  • Add CMake option WITH_CONTRIB to allow building contrib repo from main repo.
  • Reading and building external otel-cpp extension from local directory path set in OPENTELEMETRY_CONTRIB_PATH env-variable,

The motivation behind the change is to

  • Validate in future the build compatibility between otel-cpp and otel-cpp-contrib. This change will allow us to add an optional CI job to build otel-cpp-contrib against the otel-cpp repo and thus know right-away if any of main repo change breaks contrib repo.
  • Enable building the vendor specific extensions ( e.g., exporters, span-processors, samplers, propagators) as part of otel-cpp build process - This will also allow vendors to creating a single distributable package (deb/rpm/NuGet) containing both SDK + Extension. And for Windows, once we support creating single otel-cpp DLL (Build OpenTelemetry C++ SDK and exporter into DLL #1932), the extension can be part of this too.
  • The changes required to support this are minimal, most probably confined to root-level CMake file. We can keep it simple without adding extension specific cmake config file as part of otel-cpp cmake config. So, to include the both otel-cpp and extensions, the application would still need this:
                         find_package(opentelemetry-cpp)
                         find_package(opentelemetry-cpp-extension)

As an example, the build and distribution process of Geneva/stats exporter as (currently) documented here can become much simpler with this change.

@lalitb lalitb requested a review from a team January 25, 2023 04:48
@codecov
Copy link

codecov bot commented Jan 25, 2023

Codecov Report

Merging #1937 (0fd6d9f) into main (d0452f8) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1937   +/-   ##
=======================================
  Coverage   87.53%   87.53%           
=======================================
  Files         169      169           
  Lines        4889     4889           
=======================================
  Hits         4279     4279           
  Misses        610      610           

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Thanks for extending this discussion to opentelemetry-cpp.

For the motivation, validating the compatibility of a change in opentelemetry-cpp with the
content of opentelemetry-cpp-contrib is very valuable:

  • this will allow to catch unintended breaking changes,
  • to catch breaks early, this build must be done in CI in the main branch
    for opentelemetry-cpp at least, instead of waiting on a release of
    opentelemetry-cpp.

So, I agree something must be done in the opentelemetry-cpp repository to
help, because this is where the code live for the main branch.

For the technical solution, if I understand correctly, there are now
dependencies like this:

  • The opentelemetry-cpp project (a.k.a top level CMakeList.txt)
    • depends (WITH_CONTRIB) on the opentelemetry-cpp-contrib project
    • depends (OPENTELEMETRY_CONTRIB_PATH) on arbitrary code

I have concerns about this structure.

First, a dependency from opentelemetry-cpp to opentelemetry-cpp-contrib,
no matter how optional, brings a whole new set of issues:

  • Legal:

    • what is the license of every dependencies introduced by
      opentelemetry-cpp-contrib ?
    • How about further dependencies, what is the license of NGINX ?
    • Who are the copyright holders ?
    • Are these licenses compatible with Apache v2 ?
  • Technical risk:

    • Are all the components introduced by opentelemetry-cpp-contrib
      maintained ?
    • Is opentelemetry-cpp-contrib using the most up to date version of the
      packages it depends upon ?
    • Are there known security bugs ?
    • How did the attack surface increased with the added dependencies ?

I just went though this exercise internally to get approval to use
opentelemetry-cpp in the first place, and adding more dependencies to more
code will invalidate this.

This is not specific to my employer, I believe that anyone looking
to deploy opentelemetry-cpp in a production environment will (or at least
should) have the same concerns about this.

Second, I am skeptical about how this can work without having at least some
knowledge about the code included as part of contrib, and I doubt a simple
add_subdirectory() will be sufficient.

For example, I would expect that some packaging or install logic will be
adjusted, to pick items provided by opentelemetry-cpp-contrib when building
all together, and I don't see how this can work in a generic way with
different vendors providing unknown content using
OPENTELEMETRY_CONTRIB_PATH.


Alternate proposal.

Define a super_build project, with its own CMakeList.txt

Have dependencies using this structure:

  • super_build
    • depends on opentelemetry-cpp
    • depends on opentelemetry-cpp-contrib

The makefile for this super_build can have any relevant logic,
including logic to package and install, opentelemetry-cpp and
opentelemetry-cpp-contrib, together, with knowledge of the
opentelemetry-cpp-contrib internal structure if needed.

In this layout:

  • opentelemetry-cpp top level makefile will be UNCHANGED,
  • opentelemetry-cpp does not depend on opentelemetry-cpp-contrib,
  • the superbuild makefile can be used as an example for vendors to use.

In terms of file layout in the opentelenetry-cpp repository,
we can have for example:

  • a directory repo root/super_build,
  • a file repo root/super_build/CMakeList.txt.

File super_build/CMakeList.txt can:

  • add the opentelemetry-cpp dependency using ExternalProject_Add() in CMake,
    pointing to repo root/CMakeLists.txt
  • add the opentelemetry-cpp dependency using FetchContent() in CMake

This needs further investigations (not tested obviously).

Assuming this layout works, it will:

  • address the concerns I expressed,
  • test that opentelemetry-cpp can actually be used in a super_build,
  • show an example of super_build.

which I think will be very beneficial to everyone.

CMakeLists.txt Outdated
@@ -534,6 +534,28 @@ if(NOT WITH_API_ONLY)
endif()
endif()

# Add custom vendor directory via environment variable
if(DEFINED ENV{OPENTELEMETRY_CONTRIB_PATH})
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we prefer cmake variable or environment here?

@lalitb
Copy link
Member Author

lalitb commented Jan 25, 2023

@marcalff Thanks for your comments, and the concerns raised. These are indeed valid concerns.

  • Regarding dependencies, I agree that otel-cpp is not directly dependent on otel-cpp-contrib. Infact it is other way around - All the components within the otel-cpp-contrib repo have dependency on the main repo. However, as of now, otel-cpp-contrib repo is not well maintained. There is no way to ensure that all the otel-cpp-contrib components build successfully against the new release of otel-cpp. Adding this option, with little changes in otel-cpp-contrib repo will allow us to validate this in near future. And allow us to remove the stale components from contrib repo.

  • Regarding legal aspects, in-terms of licenses of components within contrib repo. All the repo's within open-telemetry github Org (within CNCF) are supposed to be Apache Licensed, copyright of OpenTelemetry. Even if that is not the case, as long as we are not git-checkin any file violating the license in main repo, we should be good. So in general it should be good in both scenarios:
    - contrib is the OSS components/exporters coming from otel-cpp-contrib repo.
    - contrib is totally different private proprietary vendor repository, containing non-OSS components/exporter.

While we can also make the hardcoded contrib repo github url as configurable, I don't think this is really required.

  • The super_build method suggested is definitely the ideal way of consuming external component with main repo. The contrib build approach provides vendor to build a single libOpenTelemetryAll+contrib.[so|dll], or single rpm/deb/NuGet package, along with build convenience in general.

As discussed in community meeting, to keep the root CMakeLists.txt clean, I have added these changes in separate cmake file. Please let me know if you have more comments/concerns here. Thanks.

@marcalff
Copy link
Member

More comments on this ...

Before introducing more coupling between opentelemetry-cpp and opentelemetry-cpp-contrib (either one way or the other), I think there are preliminary tasks to complete:

Once repository are dependent, moving code around without breaking anything is going to be more complex.

This is actually important to cleanup:

Assuming this is resolved, and on the proposal itself, I don't understand how it would work in practice:

  • The opentelemetry-cpp-contrib repository does not have a top level CMakeLists.txt
  • What will be the effect of add_subdirectory then ?

The contrib repository contains:

  • exporters, which should be compiled against the API + SDK, and the third party code involved
  • instrumentation, which should be compiled against the API only, and the third party code involved

The best place to build each part with the proper dependency on opentelemetry-cpp (API versus API+SDK), and the proper third party dependency (httpd, nginx, fluentd, geneva, prometheus) is makefiles in the contrib-cpp repository, and to setup CI in the -cpp-contrib repository itself.

Even when there is low activity in -cpp-contrib, I think CI can be triggered with a cron (say, once a week), to make sure everything in -cpp-contrib still builds with -cpp, and that tests still pass, on a regular basis.

@github-actions github-actions bot added the Stale label Apr 23, 2023
@github-actions github-actions bot closed this May 1, 2023
@ThomsonTan ThomsonTan removed the Stale label Jun 22, 2023
@ThomsonTan
Copy link
Contributor

ThomsonTan commented Jun 22, 2023

I'd like to reactivate this PR.

The title says enable building otcpp extension, but seems the content then focused on our contrib repo?

I think this should not be limited to contrib repo, instead of it can be any user provided repo. So probably we call it OPENTELEMETRY-EXTRA-EXPORTERS? License should not matter as we don't include any of the code, just provide a mono build experience to user provided exporters. In the meantime, contrib exporters can utilize this to make its build easier.

For dependencies, I think this mono build approach doesn't invert the contrib -> main dependency as this is optional.

The super_build suggestion could also work, but it introduces complexity of an extra build flavor, and which may not be easy to be discovered by the users.

@ThomsonTan ThomsonTan reopened this Jun 22, 2023
@lalitb
Copy link
Member Author

lalitb commented Jun 22, 2023

Thanks @ThomsonTan for reopening this. I have made it more generic, and not fetching the contrib repo anymore:

And either of these env-variables should be set now:

OPENTELEMETRY_EXTERNAL_COMPONENT_PATH - The local path to external component.
OPENTELEMETRY_EXTERNAL_COMPONENT_URL - The link to external component GitHub repo.

@marcalff marcalff self-requested a review June 23, 2023 08:26
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

The original request was to build both opentelemetry-cpp and opentelemetry-cpp-contrib together, to test -cpp-contrib is not broken by -cpp changes.

Are there plans to add a new build in CI (in -cpp) that will use this ?

@marcalff marcalff dismissed their stale review June 23, 2023 08:34

Re evaluate updated proposal

@marcalff
Copy link
Member

I removed my initial request for changes to unblock this, to re evaluate the proposal.

To discuss in SIG meeting, do not merge yet.

@marcalff marcalff added the pr:do-not-merge This PR is not ready to be merged. label Jun 23, 2023
@ThomsonTan
Copy link
Contributor

The original request was to build both opentelemetry-cpp and opentelemetry-cpp-contrib together, to test -cpp-contrib is not broken by -cpp changes.

@marcalff we will discuss this further in SIG meeting on this. But for testing side, I think we don't need -cpp-contrib to our main CI as validation. That can be a separate issue if we need it. For this PR, we provide an extension mechanism to build customer provided extension like exporter. So no need to pull in any contrib exporter for testing, probably we can create a simple exporter example to acts as customer provided exporter. The test will focus on the build extension method instead of any specific customer exporter.

@marcalff marcalff removed the pr:do-not-merge This PR is not ready to be merged. label Jun 28, 2023
@marcalff marcalff changed the title [RFC] Enable building otel-cpp extensions from main repo [BUILD] Enable building otel-cpp extensions from main repo Jun 28, 2023
@marcalff marcalff merged commit efde010 into open-telemetry:main Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants