build: OpenTelemetry ABI now defaults to version 2 - #16387
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the default OpenTelemetry C++ ABI version to 2 for both Bazel and CMake builds, adjusting CI configurations and documentation to match. The review feedback recommends simplifying the Bazel configuration by removing the redundant custom otel_abi_version_no flag and directly referencing the upstream @opentelemetry-cpp//api:abi_version_no flag to prevent potential ABI mismatches. This change simplifies BUILD.bazel, .bazelrc, and the changelog instructions. Additionally, it is suggested to rename the ABI test script and its triggers to explicitly reflect that they test ABI version 1, since version 2 is now the default.
| string_flag( | ||
| name = "otel_abi_version_no", | ||
| build_setting_default = "2", | ||
| values = [ | ||
| "1", | ||
| "2", | ||
| ], | ||
| ) | ||
|
|
||
| config_setting( | ||
| name = "otel_abi_v1", | ||
| flag_values = {":otel_abi_version_no": "1"}, | ||
| ) | ||
|
|
||
| config_setting( | ||
| name = "otel_abi_v2", | ||
| flag_values = {":otel_abi_version_no": "2"}, | ||
| ) |
There was a problem hiding this comment.
Defining a custom otel_abi_version_no flag in this repository introduces a risk of ABI mismatch if a downstream consumer configures the upstream --@opentelemetry-cpp//api:abi_version_no flag but forgets to also set --//:otel_abi_version_no.
To ensure a single source of truth and prevent silent ABI mismatches, we can define the config_settings to directly check the upstream @opentelemetry-cpp//api:abi_version_no flag. This allows us to completely remove the redundant otel_abi_version_no string flag.
| string_flag( | |
| name = "otel_abi_version_no", | |
| build_setting_default = "2", | |
| values = [ | |
| "1", | |
| "2", | |
| ], | |
| ) | |
| config_setting( | |
| name = "otel_abi_v1", | |
| flag_values = {":otel_abi_version_no": "1"}, | |
| ) | |
| config_setting( | |
| name = "otel_abi_v2", | |
| flag_values = {":otel_abi_version_no": "2"}, | |
| ) | |
| config_setting( | |
| name = "otel_abi_v1", | |
| flag_values = {"@opentelemetry-cpp//api:abi_version_no": "1"}, | |
| ) | |
| config_setting( | |
| name = "otel_abi_v2", | |
| flag_values = {"@opentelemetry-cpp//api:abi_version_no": "2"}, | |
| ) |
There was a problem hiding this comment.
We want to default to ABI 2 without having to upstream changes to opentelemetry-cpp. We will emit a warning when there is a mismatch between the flags.
| # limitations under the License. | ||
|
|
||
| load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") | ||
| load("@bazel_skylib//rules:common_settings.bzl", "bool_flag", "string_flag") |
There was a problem hiding this comment.
Since we are removing the custom string_flag in favor of directly referencing the upstream OpenTelemetry flag, we no longer need to load string_flag from @bazel_skylib.
| load("@bazel_skylib//rules:common_settings.bzl", "bool_flag", "string_flag") | |
| load("@bazel_skylib//rules:common_settings.bzl", "bool_flag") |
There was a problem hiding this comment.
We're keeping the flag.
| # --config otel1: Open Telemetry ABI version 1 | ||
| build:otel1 --//:otel_abi_version_no=1 | ||
| build:otel1 --@opentelemetry-cpp//api:abi_version_no=1 |
There was a problem hiding this comment.
With the removal of the custom otel_abi_version_no flag, we can simplify the otel1 config to only set the upstream OpenTelemetry flag.
# --config otel1: Open Telemetry ABI version 1
build:otel1 --@opentelemetry-cpp//api:abi_version_no=1
There was a problem hiding this comment.
But we're not removing the custom flag.
| * In Bazel builds, downstream consumers requiring ABI version 1 can configure | ||
| `--//:otel_abi_version_no=1` (or use `--config=otel1` when building in this | ||
| repository). |
There was a problem hiding this comment.
Update the instructions for downstream Bazel consumers to use the standard --@opentelemetry-cpp//api:abi_version_no=1 flag instead of the removed custom flag.
| * In Bazel builds, downstream consumers requiring ABI version 1 can configure | |
| `--//:otel_abi_version_no=1` (or use `--config=otel1` when building in this | |
| repository). | |
| * In Bazel builds, downstream consumers requiring ABI version 1 can configure | |
| `--@opentelemetry-cpp//api:abi_version_no=1` (or use `--config=otel1` when building in this | |
| repository). |
|
|
||
| mapfile -t args < <(bazel::common_args) | ||
| args+=("--config=otel2") | ||
| args+=("--config=otel1") |
There was a problem hiding this comment.
Since OpenTelemetry ABI version 2 is now the default, a generic name like bazel-otel-abi for this script and its triggers is misleading because it actually tests the non-default ABI version 1 (--config=otel1).
To improve maintainability and avoid confusion, please consider renaming the script to bazel-otel-abi-1.sh (or bazel-otel-abi-v1.sh) and updating the corresponding Cloud Build trigger YAML files accordingly.
There was a problem hiding this comment.
Future versions of this test may include multiple ABI versions, so we're keeping the more generic name.
63c5c64 to
55067a3
Compare
4856aa8 to
9bd6fc4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16387 +/- ##
==========================================
- Coverage 92.30% 92.29% -0.01%
==========================================
Files 2245 2245
Lines 211870 211824 -46
==========================================
- Hits 195568 195511 -57
- Misses 16302 16313 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
OTel ABI version 1 is still available for now, via:
--//:otel_abi_version_no=1(or use--config=otel1when building in thisrepository).
-DGOOGLE_CLOUD_CPP_OPENTELEMETRY_ABI_VERSION=1.