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.

Sign up for GitHub

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

Jump to bottom

cleanup(otel): bazel flag is obsolete #14416

Merged
merged 3 commits into from
Jul 3, 2024
Merged

cleanup(otel): bazel flag is obsolete #14416

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91c8407
cleanup(otel): bazel flag is obsolete
dbolduc Jul 1, 2024
dc6f761
checkmate
dbolduc Jul 1, 2024
9c75b62
address review comments
dbolduc Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion .bazelrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ common:macos --repo_env=BAZEL_NO_APPLE_CPP_TOOLCHAIN=1
test --test_env=GTEST_SHUFFLE --test_env=GTEST_RANDOM_SEED

# By default, build the library with OpenTelemetry
build --@io_opentelemetry_cpp//api:with_abseil
build --//:enable_opentelemetry

# Don't show warnings when building external dependencies. This still shows
Expand Down
4 changes: 0 additions & 4 deletions ci/verify_current_targets/.bazelrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,3 @@ common --noenable_bzlmod
# TODO(#13311) - remove once gRPC works with Bazel v7 or when gRPC stops using
# `apple_rules`.
common:macos --repo_env=BAZEL_NO_APPLE_CPP_TOOLCHAIN=1

# Build OpenTelemetry assuming Abseil exists. Otherwise we get symbol conflicts
# between the Abseil implementation and the copy in OTel.
build --@io_opentelemetry_cpp//api:with_abseil
32 changes: 3 additions & 29 deletions google/cloud/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,9 @@ capture_build_info(
)

config_setting(
name = "enable_opentelemetry_valid",
name = "enable_opentelemetry",
flag_values = {
"//:enable_opentelemetry": "true",
"@io_opentelemetry_cpp//api:with_abseil": "true",
},
)

config_setting(
name = "disable_opentelemetry",
flag_values = {
"//:enable_opentelemetry": "false",
},
)

Expand All @@ -70,7 +62,7 @@ cc_library(
srcs = google_cloud_cpp_common_srcs + ["internal/build_info.cc"],
hdrs = google_cloud_cpp_common_hdrs,
defines = select({
":enable_opentelemetry_valid": [
":enable_opentelemetry": [
# Enable OpenTelemetry features in google-cloud-cpp
"GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY",
],
Expand All @@ -89,24 +81,6 @@ cc_library(
],
"//conditions:default": [],
}),
target_compatible_with = select(
{
":enable_opentelemetry_valid": [],
":disable_opentelemetry": [],
},
# else, OpenTelemetry is enabled, but with an invalid configuration.
no_match_error = """


You requested OpenTelemetry support for `google-cloud-cpp`, but OpenTelemetry is
configured without Abseil support. This configuration will result in ODR
violations, and possibly build-time errors, as OpenTelemetry redefines Abseil
symbols in this case, and `google-cloud-cpp` uses Abseil directly too.

To fix this problem, supply the `--@io_opentelemetry_cpp//api:with_abseil` flag
to your build command, or set this value in your `.bazelrc` file(s).
""",
),
visibility = [
":__subpackages__",
"//:__pkg__",
Expand All @@ -121,7 +95,7 @@ to your build command, or set this value in your `.bazelrc` file(s).
"@com_google_absl//absl/types:span",
"@com_google_absl//absl/types:variant",
] + select({
":enable_opentelemetry_valid": [
":enable_opentelemetry": [
"@io_opentelemetry_cpp//api",
],
"//conditions:default": [],
Expand Down
12 changes: 11 additions & 1 deletion google/cloud/opentelemetry/quickstart/.bazelrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,15 @@ build:macos --host_cxxopt=-std=c++14
build --experimental_convenience_symlinks=ignore

# Enable OpenTelemetry tracing instrumentation for google-cloud-cpp.
build --@io_opentelemetry_cpp//api:with_abseil
build --@google_cloud_cpp//:enable_opentelemetry

# OpenTelemetry versions < v1.16.0 are not compatible with Abseil
Copy link
Contributor

Choose a reason for hiding this comment

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

After me, nobody will read this comment block. doc/packaging.md may be a better place, or the CHANGELOG.md file.

Copy link
Member Author

Choose a reason for hiding this comment

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

nobody will read this comment block.

Likely true. I was really banking on people finding the write up in the quickstart's README.

doc/packaging.md may be a better place, or the CHANGELOG.md file.

You are probably right that the more places we say words the better. I wrote up some words in the CHANGELOG. I also added a link (if you click enough times) from the packaging doc to the quickstart README.

# out-of-the-box. If you are using a version of OpenTelemetry < v1.16.0,
# uncomment the following configuration:
#
# ```
# build --@io_opentelemetry_cpp//api:with_abseil
# ```
#
# Note that the flag is not required for OpenTelemetry versions >= v1.16.0. In
# fact, the flag may be removed in future versions of OpenTelemetry.
16 changes: 10 additions & 6 deletions google/cloud/opentelemetry/quickstart/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,22 @@ flags when compiling `opentelemetry-cpp`.

### Details

Note the following two feature flags explicitly set in the `.bazelrc`. Together,
these flags enable OpenTelemetry tracing instrumentation in `google-cloud-cpp`.
Without these flags, the above `bazel build ...` command would fail.
Note the following feature flag explicitly set in the `.bazelrc`. This flag
enables OpenTelemetry tracing instrumentation in `google-cloud-cpp`.

```bash
# Required for OpenTelemetry + Abseil compatibility
build --@io_opentelemetry_cpp//api:with_abseil

# Enables tracing instrumentation in google-cloud-cpp
build --@google_cloud_cpp//:enable_opentelemetry
```

If you are using an OpenTelemetry version \< v1.16.0, you must also supply the
following flag for compatibility with Abseil. Without this flag, the above
`bazel build ...` command will fail.

```bash
build --@io_opentelemetry_cpp//api:with_abseil
```

Also note that we explicitly load OpenTelemetry's dependencies in the
`WORKSPACE.bazel`.

Expand Down
2 changes: 1 addition & 1 deletion google/cloud/testing_util/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ cc_library(
"@com_google_absl//absl/debugging:symbolize",
"@com_google_googletest//:gtest_main",
] + select({
"//google/cloud:enable_opentelemetry_valid": [
"//google/cloud:enable_opentelemetry": [
"@io_opentelemetry_cpp//exporters/memory:in_memory_span_exporter",
"@io_opentelemetry_cpp//sdk/src/trace",
],
Expand Down