From 323aa968c3f9b23856df93074a3000de7ba8030e Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Wed, 1 Apr 2026 13:45:23 -0700 Subject: [PATCH] otel: remove the last workspace crate feature (#16469) ## Why `codex-otel` still carried `disable-default-metrics-exporter`, which was the last remaining workspace crate feature. We are removing workspace crate features because they do not fit our current build model well: - our Bazel setup does not honor crate features today, which can let feature-gated issues go unnoticed - they create extra crate build permutations that we want to avoid For this case, the feature was only being used to keep the built-in Statsig metrics exporter off in test and debug-oriented contexts. This repo already treats `debug_assertions` as the practical proxy for that class of behavior, so OTEL should follow the same convention instead of keeping a dedicated crate feature alive. ## What changed - removed `disable-default-metrics-exporter` from `codex-rs/otel/Cargo.toml` - removed the `codex-otel` dev-dependency feature activation from `codex-rs/core/Cargo.toml` - changed `codex-rs/otel/src/config.rs` so the built-in `OtelExporter::Statsig` default resolves to `None` when `debug_assertions` is enabled, with a focused unit test covering that behavior - removed the final feature exceptions from `.github/scripts/verify_cargo_workspace_manifests.py`, so workspace crate features are now hard-banned instead of temporarily allowlisted - expanded the verifier error message to explain the Bazel mismatch and build-permutation cost behind that policy ## How tested - `python3 .github/scripts/verify_cargo_workspace_manifests.py` - `cargo test -p codex-otel` - `cargo test -p codex-core metrics_exporter_defaults_to_statsig_when_missing` - `cargo test -p codex-app-server app_server_default_analytics_` - `just bazel-lock-check` --- .../verify_cargo_workspace_manifests.py | 24 ++++++++----------- codex-rs/core/Cargo.toml | 4 +--- codex-rs/otel/Cargo.toml | 7 ------ codex-rs/otel/src/config.rs | 20 +++++++++++++++- 4 files changed, 30 insertions(+), 25 deletions(-) diff --git a/.github/scripts/verify_cargo_workspace_manifests.py b/.github/scripts/verify_cargo_workspace_manifests.py index 467b6c2ee..6da903ca7 100644 --- a/.github/scripts/verify_cargo_workspace_manifests.py +++ b/.github/scripts/verify_cargo_workspace_manifests.py @@ -26,19 +26,9 @@ TOP_LEVEL_NAME_EXCEPTIONS = { UTILITY_NAME_EXCEPTIONS = { "path-utils": "codex-utils-path", } -MANIFEST_FEATURE_EXCEPTIONS = { - "codex-rs/otel/Cargo.toml": { - "disable-default-metrics-exporter": (), - }, -} +MANIFEST_FEATURE_EXCEPTIONS = {} OPTIONAL_DEPENDENCY_EXCEPTIONS = set() -INTERNAL_DEPENDENCY_FEATURE_EXCEPTIONS = { - ( - "codex-rs/core/Cargo.toml", - "dev-dependencies", - "codex-otel", - ): ("disable-default-metrics-exporter",), -} +INTERNAL_DEPENDENCY_FEATURE_EXCEPTIONS = {} def main() -> int: @@ -73,6 +63,12 @@ def main() -> int: "opt into workspace lints, and avoid introducing new workspace crate " "features." ) + print( + "Workspace crate features are disallowed because our Bazel build setup " + "does not honor them today, which can let issues hidden behind feature " + "gates go unnoticed, and because they add extra crate build " + "permutations we want to avoid." + ) print( "Cargo only applies `codex-rs/Cargo.toml` `[workspace.lints.clippy]` " "entries to a crate when that crate declares:" @@ -91,8 +87,8 @@ def main() -> int: "`codex-rs/utils//Cargo.toml`." ) print( - "Existing workspace crate features are temporarily allowlisted in this " - "script and must shrink as they are removed." + "Workspace crate features are forbidden; add a targeted exception here " + "only if there is a deliberate temporary migration in flight." ) print() for path in sorted(failures_by_path): diff --git a/codex-rs/core/Cargo.toml b/codex-rs/core/Cargo.toml index e40066027..cea836d08 100644 --- a/codex-rs/core/Cargo.toml +++ b/codex-rs/core/Cargo.toml @@ -146,9 +146,7 @@ codex-shell-escalation = { workspace = true } assert_cmd = { workspace = true } assert_matches = { workspace = true } codex-arg0 = { workspace = true } -codex-otel = { workspace = true, features = [ - "disable-default-metrics-exporter", -] } +codex-otel = { workspace = true } codex-utils-cargo-bin = { workspace = true } core_test_support = { workspace = true } ctor = { workspace = true } diff --git a/codex-rs/otel/Cargo.toml b/codex-rs/otel/Cargo.toml index 3e90b8536..937011990 100644 --- a/codex-rs/otel/Cargo.toml +++ b/codex-rs/otel/Cargo.toml @@ -12,13 +12,6 @@ path = "src/lib.rs" [lints] workspace = true -[features] -## Disables the built-in default metrics exporter. -## -## Intended for use from `dev-dependencies` so unit/integration tests never -## attempt to export metrics over the network. -disable-default-metrics-exporter = [] - [dependencies] chrono = { workspace = true } codex-utils-absolute-path = { workspace = true } diff --git a/codex-rs/otel/src/config.rs b/codex-rs/otel/src/config.rs index 1898e4bdb..4f2b82a22 100644 --- a/codex-rs/otel/src/config.rs +++ b/codex-rs/otel/src/config.rs @@ -10,7 +10,11 @@ pub(crate) const STATSIG_API_KEY: &str = "client-MkRuleRQBd6qakfnDYqJVR9JuXcY57L pub(crate) fn resolve_exporter(exporter: &OtelExporter) -> OtelExporter { match exporter { OtelExporter::Statsig => { - if cfg!(test) || cfg!(feature = "disable-default-metrics-exporter") { + // Keep the built-in Statsig default off in debug builds so + // incremental local development and test runs do not emit + // best-effort OTEL traffic unless a test or binary opts into an + // explicit exporter configuration. + if cfg!(debug_assertions) { return OtelExporter::None; } @@ -74,3 +78,17 @@ pub enum OtelExporter { tls: Option, }, } + +#[cfg(test)] +mod tests { + use super::OtelExporter; + use super::resolve_exporter; + + #[test] + fn statsig_default_metrics_exporter_is_disabled_in_debug_builds() { + assert!(matches!( + resolve_exporter(&OtelExporter::Statsig), + OtelExporter::None + )); + } +}