Allow to build SDK + contrib repo via BUILD_CONTRIB build option

[maxgolov]

Is your feature request related to a problem? Not really a problem...

OpenTelemetry C++ SDK contrib repo:

https://github.com/open-telemetry/opentelemetry-cpp-contrib

may contain additional "non-standard" exporters that are maintained by community, e.g. fluentd exporter, for example.

Describe the solution you'd like

I would like to implement a build option for the main repo that allows to build contrib - BUILD_CONTRIB :

• by turning on a custom build option; OR
• by specifying the path to local checked out contrib snapshot, OR to alternate "contrib" overlay.

Describe alternatives you've considered

We could have added contrib repo as a submodule. Adding it as submodule presents other maintenance challenges.

Additional context

• CMake : when the SDK is built BUILD_CONTRIB, it should include the CMakeLists.txt from contrib path using add_subdirectory(/path/to/contrib) , that way the entire build tree of contrib repo is also included in the build.

• Bazel : contrib may be added as symlink (Unix) or directory juniction (Windows) at /contrib path. Bazeil would pick it up.
  What it gives is ability to build a distro of SDK with additional exporters.

Note that the contrib:

• could be our contrib with community provided OSS exporters (mentioned above); or
• could be just totally different PRIVATE CUSTOM vendor contrib repository, with private closed-source non-standard vendor exporters.

What we achieve is a single build that builds the SDK with custom additions?

• Continuous CI of contrib against the main repository.
• Running all tests of contrib plus the main repository.
• Opportunity to customize the build targets and provide your own combined build target.
• Contrib knows the main repo targets; it can cover extra steps, such as custom binary packaging (e.g. nuget)

I'd like to illustrate how that would work for the following example:

• fluentd exporter lives in contrib repo.
• build option in the main allows to build with a snapshot of contrib repo overlaid on top of the main SDK repo.

Bonus points

Once the change is merged, we could have added a CI that implements an OPTIONAL build check: verify that refactor in the main is not breaking anything significant in contrib. By building the main BUILD_CONTRIB against the latest https://github.com/open-telemetry/opentelemetry-cpp-contrib (main) branch snapshot of contrib. That way if refactor happens in the main that breaks any of the contrib exporters, we would know right away.

[eyjohn]

@maxgolov I'm curious about the expected usage of this, based on your summary above it seems like the main purpose is to bundle the build of a "contrib" or "vendor" package library while building otel itself. However, when we do this, do we still expect that the contrib/vendor builds still produce their own build targets (e.g. exporter/fluentd or exporter/some_vendor), by that I mean at the point of the binary build artefacts of .a/.so/.dll ...

If we are generating separate build targets, I expect that the user still has awareness so as to include the target, either includes, or linkage. I'm unfamiliar with how OpenCV does this, but I think "ffmpeg" does something similar, but in their case, they don't actually expose the additional build targets to users as they are abstracted through the built library that has the dependencies on optional libraries. As I understand, in this case, there wouldn't be this type of abstraction where we only provide libOpenTelemetryALL which would hide the contrib modules and the user still needs to know what to include and how to link it.

If that's the case, then this would effectively become a convenience helper to avoid checking out and building contrib, or vendor repo, but still requires the user to explicitly depend on the parts of contrib/vendor that they need. In this case, I wonder whether simply asking users to pull-in both otel and otel-contrib into their projects wouldn't be simpler?

[lalitb]

I second with @eyjohn. otel-cpp has no dependency on the otel-contrib repo, so we shouldn't be adding the option to build that.

[maxgolov]

@lalitb @eyjohn

It's not about otel-cpp having a dependency on otel-contrib, indeed it does not have a dependency on optional contrib.

It's the other way around: otel-contrib at any given moment of time has a dependency on a given version of otel-cpp. In order to verify that the otel-contrib build is not broken, or to precisely catch the moment when it gets broken, it is invaluable -- from otel-contrib module owner perspective, to have a build setup / CI in place to build the main+contrib baseline together.

i.e. the main otel-cpp has to be aware that it may be built with otel-contrib, not in the interest of the main, but in the interest of contrib. It sees them in separate folders. But runs them all in one combined run. You can add +100 more test cases by overlaying contrib and building with contrib, to verify that contrib modules remain all sane for every change in the main.

do we still expect that the contrib/vendor builds still produce their own build targets

Yes. They may do so. Why not? Build artifacts from contrib go into separate folder. It could be an additional set of test binaries that is unique to contrib repo, e.g. functional tests. CMake CTest then sees all binaries combined, the main + contrib tests in one run.

...there wouldn't be this type of abstraction where we only provide libOpenTelemetryALL which would hide the contrib modules.

Why not? We can enable that as well, where single build of a DLL is built with custom vendor contrib.. That'd be a binary build of libOpenTelemetryAll+contrib. This is also how it's managed in Android, for example. Android OS build would pull in optional vendor / contrib binaries and drivers into its main build tree. Albeit they are doing it with repo tool. In our case we can definitely do that pull with CMake. We can also come up with a similar setup for Bazel too.

Main benefit is that you are testing a baseline of main+contrib in one build, in one run. And run combined set of tests to verify that the baseline is sane. Something that can be added to contrib repo CI then, i.e. it'd pull in main and build with contrib from given commit id or from given directory. We can also set up a hook when main changes, to run a CI for contrib - just f.y.i. to notify the contrib owners that there have been breaking changes in the main. That way we immediately catch what needs fixing in contrib whenever API or SDK implementation changes.

I wonder whether simply asking users to pull-in both otel and otel-contrib

The main issue here is that in order to build the full set in one shot. You'd need to have an extension point in the main build to the optional / vendor components. I am illustrating how it works in the PR. You can either fetch contrib from git, or you can build with custom overlay by specifying either WITH_CONTRIB (pull from git) OR environment variable (use overlay from local snapshot). This way we do not need a separate CMakeLists.txt that would have aggregated the two at higher level elsewhere.. My point is that I'd like it to be here, not elsewhere. Because both projects are under open-telemetry umbrella, why not introduce explicit binding - an ability to build one with another in our main repo, not elsewhere ? It's not required. It's optional. Who would help us to setup a CI to validate that contrib remains sane, if we do not do it ourselves?

Just to recap the benefits:

• I am not excluding the ability to build libOpenTelemetryAll+contrib.[so|dll] in one run.
• all tests are running in one loop.
• we easily catch any regressions in contrib caused by API/SDK changes in the main (my selfish interest here as owner of something in contrib I would like to immediately see if it got broken by some change in the main, so that I can react and fix it accordingly).

[eyjohn]

Thanks for the recap. Perhaps I can reframe the motivations here

1. Validating build compatibility between otel and otel-contrib
2. Extensible build process that can be used to build either with contrib OR other vendor libs
3. The build process yields a single build and test step
4. OPTIONALLY Provide a bundled build to create a single libOtelWithMyCustomDeps.so/dll

(Do let me know if I've captured this incorrectly)

I strongly agree with 1. and I'd go as far as to suggest that both otel and otel-contrib should run builds of both to validate compatibility. However, I think this is best validated as part of CI step which would execute both build steps consequitevly as well as tests. I would suggest that this be a non-blocking validation unless we really want to force strict builds between the two. To achieve this we don't really need an extensible build as we can simply define the consecutive build steps in CI.

While I see the value of 2. I think its a tricky one to achieve, ultimately you're trying to enforce some consistency of build both for otel and otel-contrib and possibly any vendor libs by enforcing a build contract. I would suggest that instead of adding this as an extension to otel build that, we define a standard build convention (i.e. CMake). Presuming every repo (otel, otel-contrib, vendor) all follow the same convention we can then provide instructions/helpers for how to kick off this combined build. IMO this is external to the core otel build steps (i.e. not in the main CMakeLists.txt, perhaps a separate one), but I guess could be bundled in the otel repo for convenience.

I think the convenience of 3 isn't that significant but this may be a trivial benefit based on how we do 2.

I'd be interested to know the use-cases for 4. I'm curious about the reusability of such a bundled dynamically linkable library, I guess otel+contrib is probably a common use-case but otel+vendor? I struggle to see other libraries depending on this, and I imagine only the end application binaries would use this, in which case why not simply statically link this into the application.

Perhaps this might be a good topic to raise during the next SIG meeting?

[maxgolov]

ultimately you're trying to enforce some consistency of build both for otel and otel-contrib

Yes. Absolutely. If we are expecting that more than one contrib exporter would be created, then it'd make sense to define a standard structure how to build all of them. And the process that allows to build all of these exporters via both standard supported build systems:

• CMake
• Bazel

You don't need custom build steps CI here for what can be done with CMake. Bazel already allows you to build all, i.e. you can overlay a custom directory into build tree. But for CMake - you need to explicitly add_directory to overlay all extras. Thus, it'd make sense to (optionally) fetch extra content, and allow building with that content.

I guess could be bundled in the otel repo for convenience.

Convenience is the main point. The change needed to achieve that convenience is rather small. I am showing how to do it in my PR. There is no need to create a separate CMakeLists.txt when the main repo provides an extension point for that in its own build system:

# Add custom vendor directory via environment variable
if(DEFINED ENV{OPENTELEMETRY_CONTRIB_PATH})
  # Add custom contrib path to build tree and consolidate binary artifacts in current project binary output directory.
  add_subdirectory($ENV{OPENTELEMETRY_CONTRIB_PATH} ${PROJECT_BINARY_DIR}/contrib)
endif()
# OR Add opentelemetry-cpp-contrib from GitHub
if(WITH_CONTRIB)
  # This option requires CMake 3.11+: add standard remote contrib to build tree.
  include(FetchContent)
  FetchContent_Declare(
    contrib
    GIT_REPOSITORY "https://github.com/open-telemetry/opentelemetry-cpp-contrib"
    GIT_TAG "main")
  FetchContent_GetProperties(contrib)
  if(NOT contrib_POPULATED)
    FetchContent_Populate(contrib)
    add_subdirectory(${contrib_SOURCE_DIR})
  endif()
endif()

Subsequently, the contrib repo should have its own CMakeLists.txt that can be plugged into the main build.

There is also no extra maintenance cost to the main repo maintainers. You add this extension point once.

Provide a bundled build to create a single libOtelWithMyCustomDeps

This is intended for end applications manually instrumented with OTel API, that require a custom contrib exporter (e.g. fluentd forward exporter or any other custom exporter for that matter).

The target does not have to be just dynamic library. It could be both: static and dynamic, as a single combined binary artifact. Adding single binary artifact is trivial. It does not matter what is in it, standard exporters or an extended set of exporters in one big fat library. Your instrumentation is also agnostic of what exporter you decide to use, or switch to. It is easier to link against one single big fat library knowing that all individual pieces are in it.

Let's discuss during the meeting.

[eyjohn]

Thanks for taking the time to explain this, unfortunately, I couldn't catch the last meeting but will try to attend the next.

In the meantime, some minor follow up thoughts. I do agree with the rationale but a few points stand out.

Nomenclature

WITH_ prefix is a bit ambiguous it describes:

• Which libraries to depend on (ABSL/STD20+/nostd) i.e.
• What parts of the otel library to build (mostly exporters)
• Enabling Tests
• You are adding another category for external things to build AFTER the library.

I feel like these options are getting overloaded and confusing and I might suggest a different prefix for this, BUNDLE_WITH, EXTEND_WITH, BUILD_EXTRA, to throw some ideas.

Entry-point

Given the "after" nature of the downstream build steps, we could have a different entry point altogether, by that I mean NOT using the main CMakeLists.txt, but perhaps a separate subdirectory called "bundle_build" or something or even a different repo for that matter.

Customization

Whichever name we go with I guess the two use cases are "CONTRIB" as a convenience that points to a specific git branch/tag but optionally an existing path. I can see use-cases for also enabling customizations against specific git repos/branches but also potentially building contrib but not from git but a local dir. I'm not against the current interface but worth while thinking about this and consider some use-cases for possible users of this (e.g. what would a vendor do?)

[maxgolov]

@eyjohn - I agree with your logical reasoning. It makes sense to rename it.

In my proposal I'm trying to cover both scenarios:

• OPENTELEMETRY_CONTRIB_PATH - environment variable path to already checked out tree with contrib. I believe this name is already reasonable. Similar (precisely identical in its feature operation, in fact) option in OpenCV is called:
  OPENCV_EXTRA_MODULES_PATH=<opencv_contrib>/modules .

• re. avoiding WITH_FEATURE_NAME prefix.... Would BUILD_CONTRIB possibly work as a better name?

I followed-up offline with @lalitb , showing that the first option - OPENTELEMETRY_CONTRIB_PATH - is identical to how OpenCV project handles their contrib "extension point" in their main project CMake build.

[github-actions]

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.

[github-actions]

Closed as inactive. Feel free to reopen if this is still an issue.

[lalitb]

Reopening for discussion.

[lalitb]

This was discussed again in today's community meeting, and it was agreed that as otel-cpp has no dependency on the external repo, the build option shouldn't be added.

[github-actions]

This issue was marked as stale due to lack of activity.

[github-actions]

This issue was marked as stale due to lack of activity.