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

Add WITH_API_ONLY CMake option #778

Merged
merged 5 commits into from
May 21, 2021
Merged

Conversation

lidavidm
Copy link
Contributor

Fixes #751

Changes

This adds a WITH_API_ONLY CMake option so downstream projects can easily build/use OpenTelemetry as a header-only library. When enabled, SDK/EXT/examples are not built. Bazel is not supported, and flags for individual subcomponents are not exposed (as they have interlinked dependencies and it's likely not as useful/common to separate them out that way).

For significant contributions please make sure you have completed the following items:

  • N/A: CHANGELOG.md updated for non-trivial changes (not a major change)
  • N/A: Unit tests have been added (no unit tests for CMake)
  • N/A: Changes in public API reviewed (no changes in public API)

This enables easily building/using OpenTelemetry as a header-only
library. When enabled, SDK/EXT/examples are not built.

Fixes open-telemetry#751
@lidavidm lidavidm requested a review from a team May 20, 2021 15:41
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM . Thank you for change

Copy link
Contributor

@ThomsonTan ThomsonTan left a comment

Choose a reason for hiding this comment

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

Do we need add a bazel target in parity with WITH_API_ONLY?

@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #778 (f773e8f) into main (227d400) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #778   +/-   ##
=======================================
  Coverage   95.95%   95.95%           
=======================================
  Files         176      176           
  Lines        7172     7172           
=======================================
  Hits         6882     6882           
  Misses        290      290           

@lalitb
Copy link
Member

lalitb commented May 20, 2021

Do we need add a bazel target in parity with WITH_API_ONLY?

Good point. I thought we should be able to build a single target just by specifying that target in bazel build command line, but need to check whether it stills build all the dependencies which are not required for API (grpc, google test, Prometheus, curl and so on). Better to have a github issue for that.

Copy link
Contributor

@maxgolov maxgolov left a comment

Choose a reason for hiding this comment

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

I have a question here. Have you tried:

cmake --build . --target opentelemetry_api

instead?

@lidavidm
Copy link
Contributor Author

I have a question here. Have you tried:

cmake --build . --target opentelemetry_api

instead?

In #751 I tried both cmake --build . --target api/install and cmake --build api --target install. Neither works consistently, because it depends on the generator used (Ninja works with the former, Makefiles works with the latter). The suggested CMake command gives 'target not found' for both Ninja and Makefiles.

@maxgolov
Copy link
Contributor

Do we need add a bazel target in parity with WITH_API_ONLY?

@ThomsonTan

Bazel would be problematic. In Bazel build - you can exclude the directories from build at command line using --. Not sure what that means if only api directory is kept?

I think even for CMake - it's possible to specify just the opentelemetry_api target. So adding a new build option should not be necessary. Instead of using new option it might be more practical to just pass the --target flag?

@maxgolov
Copy link
Contributor

maxgolov commented May 20, 2021

@lidavidm - I have a similar setup outside of CMake and Bazel with MSBuild, where just recursively cloning the contents of /api directory, and adding it to the child project is largely sufficient. There are actually a few /sdk classes that you are gonna be needing anyways , even if you re-implement the SDK following the API - it'd be worthwhile to borrow a few things from SDK. Just API may not eventually satisfy your needs.

You could have also used FetchContent in your project, then added the paths to the folders using include_directories in your project.

You'd definitely need #include "opentelemetry/sdk/trace/exporter.h" in addition to API headers, in order to implement your own TracerProvider and Exporter, if you choose to do so. Means you'd probably need to have a build target that not only covers API, but borrows a few things from SDK headers too.

@lidavidm
Copy link
Contributor Author

We intend to only use the Span/Tracer, essentially, as we are a library and plan to be embedded into other applications. Linking all of OpenTelemetry isn't desirable there, and we don't even want to configure an exporter. (As far as I can see, this should be ok - the downstream application can link to the exporter and configure things.)

Also, even if we do pull in SDK, we probably do not want all the exporters, ext, etc. which are also unavoidable right now. But as much as possible, we'd like to avoid building anything, so that we can easily support our platforms (this came up while I was trying to get OpenTelemetry into our CI). And I'd like to avoid adding to the build time as much as possible.

If OpenTelemetry is guaranteeing that the contents of the API directory are basically usable as-is, then that's fine too.

@lalitb
Copy link
Member

lalitb commented May 20, 2021

I think even for CMake - it's possible to specify just the opentelemetry_api target. So adding a new build option should not be necessary. Instead of using new option it might be more practical to just pass the --target flag?

@maxgolov Strangely I don't see both opentelemetry_sdk and opentelemetry_api targets while listing all the available targets using cmake --build . --target help. Not sure if it is because both these are INTERFACE type, and so can't be built from command line ?

@maxgolov
Copy link
Contributor

I don't see both opentelemetry_sdk and opentelemetry_api targets while listing all the available targets

We should fix it.

Copy link
Contributor

@maxgolov maxgolov left a comment

Choose a reason for hiding this comment

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

I think it's fine. Perhaps, long-term we need to fix the opentelemetry_api target to just use that. API target is also expected to give you detail of what types are used on API surface...

I think for DLL on Windows that'd be absl::variant and nostd::span ; for statically linked lib, and/or for header-only alternate SDK implementations this may be std::variant and std::span. If you are not intaking those defines from opentelemetry_api target, then your objects won't link against the SDK.

opentelemetry_api CMake target is expected to populate the necessary build flags for dependent projects. Maybe we need to get to the point when we provide dynamic linking interface stubs instead, such as .a and .lib files for Unix and Windows respectively. Plus the set of API headers with that, in a package. That'd be in Release v1.1 or later.

@lalitb
Copy link
Member

lalitb commented May 20, 2021

Thanks @maxgolov .

@lidavidm - CI/ format action is failing. Once you fix it, we can merge the changes.

@maxgolov
Copy link
Contributor

maxgolov commented May 20, 2021

@lidavidm - please rerun ./ci/do_ci.sh format locally - to format your CMake changes and check in back to solve the CI / Format failure. I usually use Ubuntu 20.04 with the matching tooling, as installed by our CI to do that. Physical box, Docker or WSL2 all work great for that.

@lidavidm
Copy link
Contributor Author

Thanks for pointing me there, I've fixed the formatting.

@lalitb lalitb merged commit af9cd15 into open-telemetry:main May 21, 2021
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.

Allow building as a header-only library with CMake
4 participants