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

Clarifications needed around WITH_ABSEIL #3116

Open
marcalff opened this issue Oct 30, 2024 · 5 comments · May be fixed by #3167
Open

Clarifications needed around WITH_ABSEIL #3116

marcalff opened this issue Oct 30, 2024 · 5 comments · May be fixed by #3167
Labels
bug Something isn't working needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@marcalff
Copy link
Member

How to use opentelemetry-cpp with or without ABSEIL is unclear.

This is because:

  • using the opentelemetry-cpp ABI with abseil
  • using an exporter that indirectly depends on abseil (namely, the OTLP GRPC exporter)
  • using abseil in an application that happens to use opentelemetry-cpp header files

all collide into the same flag, WITH_ABSEIL, while each topic is a separate concern.

We need to clarify what is supported, what is not supported, and how to build cleanly a coherent binary while separating each concern.

@marcalff marcalff added the bug Something isn't working label Oct 30, 2024
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 30, 2024
@marcalff
Copy link
Member Author

marcalff commented Oct 30, 2024

Findings, for discussion.

API

Abseil is used for only one thing, which is to implement nostd::variant in some cases.

The class nostd::variant is always provided by opentelemetry-cpp.

See file api/include/opentelemetry/nostd/variant.h

It is implemented using, by order of decreasing priority:

  • C++ 17 std::variant, if available (OPENTELEMETRY_STL_VERSION >= 2017, OPENTELEMETRY_HAVE_STD_VARIANT)
  • Else, using external Abseil absl::variant, if available (HAVE_ABSEIL)
  • Else, using internal Abseil absl::variant, as a last fallback.

It is important to note that when compiling with OPENTELEMETRY_STL_VERSION >= 2017,
the setting WITH_ABSEIL has no effect on the ABI: class nostd::variant is aliased to C++17 std::variant.

External Abseil

Using WITH_ABSEIL causes a dependency on an external Abseil library.

Header files from external abseil uses inline C++ namespaces, so that:

absl::variant is inlined to absl::implementation_defined::variant

For example:

#define ABSL_OPTION_USE_INLINE_NAMESPACE 1
#define ABSL_OPTION_INLINE_NAMESPACE_NAME lts_20240722

Internal Abseil

See directory api/include/opentelemetry/nostd/internal/absl

Per the README.md there,

# Notes on Abseil Variant implementation

This is a snapshot of Abseil Variant `absl::variant` from Abseil
`v2020-03-03#8`.

This code is ancient, unmaintained, and was changed only to address build breaks caused by new compilers or new C++ standards.

The internal abseil implementation is header only.

Internal abseil headers use inline namespaces, so that:

absl::variant is inlined to absl::otel_v1::variant, per:

#define OTABSL_OPTION_INLINE_NAMESPACE_NAME otel_v1

It uses the same namespace absl::variant as the main abseil code base, which is a cause of build breaks when somehow both the internal and external abseil header files are in the same compile unit.

This is because a namespace can be inlined to only 1 place, there can not be 2 inline pointing to 2 different implementations: absl::variant can not point both to the internal and external code at the same time.

Application that uses Abseil

An application may want to use Abseil for its own reasons, independently of the ABI for nostd::variant.

The problem is that a flag named WITH_ABSEIL set by the application makefile will collide with the flag named WITH_ABSEIL that controls how to build the API/ABI.

It should be possible to:

  • a-1) compile an application with abseil

  • a-2) compile an application without abseil

  • b-1) compile nostd::variant using abseil

  • b-2) compile nostd::variant without using abseil

and support combinations like (a-1, b-2) or (a-2, b-1), which is not possible today.

@marcalff
Copy link
Member Author

marcalff commented Oct 30, 2024

Possible changes, for discussion.

OPENTELEMETRY_WITH_ABSEIL

In opentelemetry-cpp, use OPENTELEMETRY_WITH_ABSEIL instead of WITH_ABSEIL

This allows to build an application WITH_ABSEIL=ON/OFF, independently of opentelemetry-cpp OPENTELEMETRY_WITH_ABSEIL=ON/OFF.

Internal abseil

In the internal abseil implementation, do not use the inline form absl::variant but instead use explicitly absl::otel_v1::variant.

See @owent fix in #3041

This is to allow, in the same binary:

  • The ABI using internal abseil, where nostd::variant points to absl::otel_v1::variant
  • GRPC using ABSEIL

so that the internal and external implementation can exist side by side without colliding on symbol names.

With this, we should be able to build:

  • an API that does not use abseil (OPENTELEMETRY_WITH_ABSEIL=NO)
  • an OTLP GRPC exporter that uses abseil

@marcalff
Copy link
Member Author

@lalitb @ThomsonTan @esigo @owent Please take a look, for discussion.

@owent
Copy link
Member

owent commented Nov 4, 2024

There's a conflict between otel_absl and absl due to the inline namespace, which is the last issue in the main branch's code. I eliminated the external Abseil dependency when enabling gRPC or compiling otel-cpp with protobuf 3.22 or higher in #3041. Now, there is no conflict and it won't break the ABI.

I think maybe we can replace all options of WITH_XXX into OPENTELEMETRY_WITH_XXX. Not only abseil. What do you think?

@marcalff
Copy link
Member Author

marcalff commented Nov 4, 2024

There's a conflict between otel_absl and absl due to the inline namespace, which is the last issue in the main branch's code. I eliminated the external Abseil dependency when enabling gRPC or compiling otel-cpp with protobuf 3.22 or higher in #3041. Now, there is no conflict and it won't break the ABI.

Yes, saw that. I agree with the fix.

I think maybe we can replace all options of WITH_XXX into OPENTELEMETRY_WITH_XXX. Not only abseil. What do you think?

Yes, but we need to do that in a separate PR, and make sure we don't break existing users in the transition.
See #3084

@owent owent linked a pull request Nov 25, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants