Repository navigation
Conversation
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
|
@krishicks this might be in your wheelhouse for a look |
|
This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. |
github-actions
Bot
added
the
state:stale
|
I believe this bug fix is still ready for review and merge and should not be closed as stale. I've fixed the merge conflict which added a commit. If there's something else I can do to mark this "not stale" let me know! Thanks! |
github-actions
Bot
removed
the
state:stale
|
This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. |
github-actions
Bot
added
the
state:stale
| mod grpc; | ||
| mod propagation; | ||
|
|
||
| pub use grpc::RecordGrpcFailure; |
There was a problem hiding this comment.
Critical (build): This branch does not compile. The "Merge branch 'main' into fix2639" merge left duplicate pub use re-exports here, so cargo build -p openshell-otel fails with 9 E0252 errors (RecordGrpcFailure is re-exported three times at lines 10, 14, and 22; RecordGrpcStatus twice; HeaderMapExtractor/MetadataMapInjector/TraceContextInterceptor twice each). Since openshell-otel is foundational, the whole workspace is broken, so pre-commit cannot be passing on this HEAD.
Fix: drop the duplicated lines 10-15 and fold the new symbol into the existing canonical block at lines 26-28:
pub use propagation::{
EnvTraceContextInterceptor, HeaderMapExtractor, MetadataMapInjector, TraceContextInterceptor,
current_trace_context_carrier,
};then re-confirm cargo build and cargo clippy (the duplicates also emit unused-import warnings).
The feature logic itself reads cleanly: EnvTraceContextInterceptor forwards TRACEPARENT/TRACESTATE as gRPC metadata and is a correct no-op when unset or invalid, with good test coverage. Just needs the merge fixed so it builds.
Source: cc-review correctness agent (build verified locally)
There was a problem hiding this comment.
@rhuss Thanks for the review. I've just pushed another commit that should address the outstanding issues (details in the comment below). Let me know if there's anything else I can do here!
There was a problem hiding this comment.
Confirmed resolved. The rebase dropped the bad merge, so openshell-otel now has a single canonical pub use propagation::{...} block. cargo test -p openshell-otel compiles and passes locally (10 tests).
|
Thanks for this, Chris, and apologies: my earlier "changes requested" review landed with an empty body, so you never got the actual feedback. Here it is. The feature itself is good and matches the accepted design in #2639. One blocking issue and one housekeeping item: 1. The branch doesn't compile. A The fix is to drop the duplicated lines and add only the new symbol to the existing canonical blocks: pub use grpc::{RecordGrpcFailure, RecordGrpcStatus};
pub use propagation::{
EnvTraceContextInterceptor, HeaderMapExtractor, MetadataMapInjector, TraceContextInterceptor,
current_trace_context_carrier,
};2. The branch now conflicts with After that, please confirm If you're short on time, let me know and I'm happy to push the dedupe + conflict resolution onto the branch myself so this doesn't get lost. |
github-actions
Bot
removed
the
state:stale
…tinuity The CLI's gateway gRPC channel now passively forwards the W3C trace context (TRACEPARENT/TRACESTATE) from its environment, so a CI pipeline's trace parents the gateway's spans. This needs no OTel collector or provider on the CLI side - short-lived clients on CI runners and jump hosts just pass through what the CI system already exports. Add EnvTraceContextInterceptor to openshell-otel: it reads the trace context once at construction and injects it as gRPC metadata, and is a no-op when the context is unset or invalid. Compose it with EdgeAuthInterceptor via a new GatewayInterceptor, since tonic allows only one interceptor per channel. Closes issue NVIDIA#2639 Signed-off-by: Chris Moore <cmoore@redhat.com>
|
Thanks for the detailed feedback! Both items are addressed in 2d6be3c. Rather than patch the duplicated re-exports on top of the bad merge, I rebased the feature commit cleanly onto current main, which drops the merge
Verified locally: cargo build clean, cargo clippy --workspace --all-targets clean (only pre-existing openshell-driver-vm build-script env warnings), cargo test -p openshell-otel -p |
There was a problem hiding this comment.
@crm00re thanks for getting the PR back on track!. I've run an additional round of reviews, there are no really blocking issues. feel free to assess and possibly address those issues that my agents found below.
cc-review Summary
What Went Well
- Parse-then-reserialize in
context_from/inject_context(propagation.rs:252-266): raw environment bytes never reach the wire, so header/CRLF injection is structurally impossible rather than filtered. context_fromextracts against a freshContext::new()(propagation.rs:252), making the no-op-on-invalid guarantee deterministic and the tests meaningful.GatewayInterceptor::call(tls.rs:48-49) runs auth first with?short-circuit and writes disjoint metadata keys, so trace forwarding cannot run on an auth failure or alter auth headers.env_interceptor_forwards_valid_traceparent(propagation.rs:431) asserts exact round-trippedtraceparentandtracestatevalues, not mere presence.
Findings (gate: PASS, 0 Critical / 0 Important)
| Severity | File | Description | Source |
|---|---|---|---|
| Minor | propagation.rs:424 | Core read_env/from_env env-reading path is untested |
test-quality |
| Minor | traces.mdx:72 | "Every gateway request" overclaims; term and completions bypass the interceptor |
codex |
| Minor | propagation.rs:198 | Blank uppercase TRACEPARENT shadows a valid lowercase fallback |
correctness |
| Minor | propagation.rs:187 | Hardcoded env-name literals instead of *_ENV constants |
architecture |
| Minor | propagation.rs:195 | read_env vs set_parent_from_environment diverge on env semantics |
architecture |
| Minor | propagation.rs:245 | Invalid present TRACEPARENT dropped with no debug log |
production |
| Minor | tls.rs:43 | GatewayInterceptor auth+trace composition untested |
test-quality |
| Minor | propagation.rs:470 | Invalid-case test cannot verify tracestate is dropped |
test-quality |
Notable (not posted inline): read_env accepting the lowercase spelling is undeclared in the issue/PR goals (though it is declared in the code's doc comment). Benign; covered in substance by the read_env findings above.
Review Details
- Findings posted: 8 inline
- Gate outcome: PASS
- Participating reviewers: architecture, correctness, goal-alignment, production, security, test-quality, gator, codex, coderabbit (security, gator, coderabbit found nothing)
| ); | ||
| } | ||
|
|
||
| fn interceptor_with(extractor: EnvTraceContext) -> EnvTraceContextInterceptor { |
There was a problem hiding this comment.
Minor: The three env-interceptor tests build EnvTraceContext by hand through interceptor_with (this helper), bypassing EnvTraceContext::from_env. So read_env and its lowercase fallback, whitespace trimming, and blank filtering (propagation.rs:195-200) have no coverage, even though reading CI env vars is the feature's whole purpose.
Why this matters: A regression in read_env (dropped lowercase fallback, lost trimming, lost blank filtering) would leave every test green.
Suggested fix: Add a from_env() test guarded by the existing test_lock that sets the process env and asserts the carrier for the valid, lowercase-only, whitespace-padded, and blank cases.
Source: test-quality agent
There was a problem hiding this comment.
Addressed in 369bd0c: added env_trace_context_from_env_reads_both_spellings_and_skips_blanks, guarded by the existing test_lock. It snapshots and restores the four env names, then exercises EnvTraceContext::from_env directly for the valid, whitespace-padded, lowercase-only, blank-uppercase-over-valid-lowercase, and blank-in-both-spellings cases, asserting the captured carrier fields for each.
There was a problem hiding this comment.
Confirmed in 369bd0c0. env_trace_context_from_env_reads_both_spellings_and_skips_blanks exercises from_env/read_env directly (padded upper-case, lower-case-only, and the blank-upper-shadows-lower case), guarded by test_lock. Passes locally.
|
|
||
| The gateway continues a valid W3C `traceparent` sent by the caller, so a client's trace can include the gateway's spans for its requests. Without one, the gateway starts a new trace. | ||
|
|
||
| The CLI forwards `TRACEPARENT` and `TRACESTATE` from its environment on every gateway request. A CI system that exports these variables (GitHub Actions, GitLab CI, Jenkins OpenTelemetry plugins) therefore parents the gateway's spans for the commands it runs, with no collector or provider configuration on the CI runner. |
There was a problem hiding this comment.
Minor: This paragraph says the CLI forwards the trace context "on every gateway request", but openshell term (main.rs around line 4080) and shell completions (completers.rs completion_grpc_client) build their gateway clients with only EdgeAuthInterceptor, bypassing the new GatewayInterceptor.
Why this matters: Those requests start separate, unparented gateway traces, so the "every gateway request" claim is inaccurate. The CI workflow the feature targets (sandbox create, provider, run) all go through grpc_client and are covered; term and completions are interactive paths, not CI steps.
Suggested fix: Either narrow the wording (for example "on gateway requests from standard CLI commands"), or build the term and completion clients through interceptor_from_tls so the claim holds literally.
Source: codex
There was a problem hiding this comment.
Addressed by narrowing the wording rather than changing behavior: the doc now says "on gateway requests from its standard commands," and the GatewayInterceptor doc comment was narrowed the same way. openshell term and the completions are interactive paths, and routing their clients through interceptor_from_tls would be a behavior change beyond this PR's CI-forwarding goal — happy to do that wiring as a follow-up if you'd prefer the claim to hold literally.
There was a problem hiding this comment.
Confirmed. The wording now reads "on gateway requests from its standard commands", which matches the clients built through interceptor_from_tls. Resolving.
| fn read_env(upper: &str) -> Option<String> { | ||
| std::env::var(upper) | ||
| .ok() | ||
| .or_else(|| std::env::var(upper.to_ascii_lowercase()).ok()) |
There was a problem hiding this comment.
Minor: read_env calls std::env::var(upper).ok().or_else(...) and only trims and filters blanks afterwards. When TRACEPARENT is set but blank, .ok() returns Some(""), so the lowercase traceparent fallback on this line never runs; the later trim and filter then yield None.
Why this matters: A present, valid lowercase traceparent is dropped whenever a blank uppercase TRACEPARENT also exists, contradicting the doc comment ("blank values are treated as unset", "both spellings are accepted"). The pipeline trace silently fails to forward.
Suggested fix: Normalize each source before the fallback, for example a read_one(name) doing std::env::var(name).ok().map(|v| v.trim().to_string()).filter(|v| !v.is_empty()), then read_one(upper).or_else(|| read_one(&upper.to_ascii_lowercase())).
Source: correctness (also flagged by: codex)
There was a problem hiding this comment.
Fixed as suggested: read_one now normalizes each spelling (trim, drop blanks) before the fallback, so read_env is read_one(upper).or_else(|| read_one(&upper.to_ascii_lowercase())). A blank upper-case value can no longer shadow a valid lower-case one. The exact regression case (blank TRACEPARENT + valid traceparent) is asserted in the new from_env test.
There was a problem hiding this comment.
Confirmed. read_one now trims and drops blanks per spelling before read_env's or_else, so a blank upper-case value no longer shadows a valid lower-case one. Covered by the new from_env test.
| impl EnvTraceContext { | ||
| fn from_env() -> Self { | ||
| Self { | ||
| traceparent: read_env("TRACEPARENT"), |
There was a problem hiding this comment.
Minor: from_env passes the raw string literals "TRACEPARENT" and "TRACESTATE" to read_env, while the module already exports TRACEPARENT_ENV and TRACESTATE_ENV constants that set_parent_from_environment uses.
Why this matters: Two sources of truth for the same env names. If the canonical constant changes, this path silently keeps reading the old names.
Suggested fix: Pass TRACEPARENT_ENV and TRACESTATE_ENV into read_env.
Source: architecture agent
There was a problem hiding this comment.
Done — EnvTraceContext::from_env now passes TRACEPARENT_ENV/TRACESTATE_ENV, so there is a single source of truth for the names.
There was a problem hiding this comment.
Confirmed. from_env now passes TRACEPARENT_ENV/TRACESTATE_ENV, giving a single source of truth for the names.
|
|
||
| /// Read an environment variable by its upper-case name, falling back to the | ||
| /// lower-case spelling, trimming whitespace and discarding blank values. | ||
| fn read_env(upper: &str) -> Option<String> { |
There was a problem hiding this comment.
Minor: read_env trims, accepts the lowercase spelling, and filters blanks. The pre-existing set_parent_from_environment, which reads the same two variables, uses plain std::env::var(name) with none of that, so the two paths disagree on which values count as set.
Why this matters: A runner exporting lowercase traceparent, or a whitespace-padded value, is honored by this CLI interceptor but ignored by set_parent_from_environment, giving inconsistent parenting for the same environment.
Suggested fix: Extract one shared env-reading helper and use it in both places (this also resolves the hardcoded-literal and blank-shadow items above).
Source: architecture agent
There was a problem hiding this comment.
Done via the shared helper: set_parent_from_environment now reads through read_env, so both paths trim, accept the lower-case spelling, and treat blanks as unset; its doc comment states the shared semantics. The only non-test caller is the supervisor's startup span — blanks previously produced an invalid carrier (no parenting) anyway, so the behavioral delta is limited to valid lowercase/padded values now parenting, which is the intent.
There was a problem hiding this comment.
Confirmed. set_parent_from_environment now reads through read_env, so both paths share the trim, lower-case, and blank handling.
| #[must_use] | ||
| pub fn from_env() -> Self { | ||
| Self { | ||
| context: context_from(&EnvTraceContext::from_env()), |
There was a problem hiding this comment.
Minor: When TRACEPARENT is present but malformed, from_env captures a context with no valid span and every request injects nothing, with no diagnostic anywhere on the invalid-but-present path.
Why this matters: An operator sees the gateway start a fresh trace with no signal distinguishing "TRACEPARENT was never set" from "TRACEPARENT was set but rejected", so debugging the exact scenario this feature enables requires reading source.
Suggested fix: Emit a one-line tracing::debug! (value redacted) when a present traceparent yields an invalid span context. This keeps the passive no-op behavior and adds a grep-able signal.
Source: production-readiness agent
There was a problem hiding this comment.
Done: EnvTraceContextInterceptor::from_env now emits a single tracing::debug!("TRACEPARENT is set but invalid; not forwarding trace context") when a present traceparent yields an invalid span context. The value itself is not logged and the passive no-op behavior is unchanged, so it's a grep-able signal only.
There was a problem hiding this comment.
Confirmed. EnvTraceContextInterceptor::from_env emits tracing::debug!("TRACEPARENT is set but invalid; not forwarding trace context") on the set-but-invalid path, with the value redacted.
| trace: EnvTraceContextInterceptor, | ||
| } | ||
|
|
||
| impl tonic::service::Interceptor for GatewayInterceptor { |
There was a problem hiding this comment.
Minor: GatewayInterceptor::call composes auth then trace, and interceptor_from_tls wires EnvTraceContextInterceptor::from_env into it, but no test exercises this composition; the tls.rs tests cover only routing and TLS helpers.
Why this matters: This is the integration point that makes CLI trace forwarding actually happen. If trace.call were dropped or the two interceptors swapped, the feature would silently no-op and no test would catch it.
Suggested fix: Add a test that sets both an edge token and a TRACEPARENT, runs the composed interceptor, and asserts both the auth header and the traceparent metadata are present.
Source: test-quality agent
There was a problem hiding this comment.
Addressed with gateway_interceptor_composes_auth_and_trace_forwarding: it builds the interceptor through the real interceptor_from_tls with an OIDC token plus TRACEPARENT/TRACESTATE set, runs the composed call, and asserts authorization, traceparent, and tracestate all land in the request metadata — so a dropped or swapped trace.call now fails the test.
One deviation from the suggestion: the CLI crate is #![forbid(unsafe_code)], so the test can't call std::env::set_var directly. It sets the env via the crate's existing temp-env dev-dependency under TEST_ENV_LOCK (the same pattern completers.rs uses), which adds no new dependencies or public API.
There was a problem hiding this comment.
Confirmed. gateway_interceptor_composes_auth_and_trace_forwarding builds through the real interceptor_from_tls and asserts the auth header plus traceparent/tracestate are all present. Passes locally.
| fn env_interceptor_is_noop_for_invalid_traceparent() { | ||
| let mut interceptor = interceptor_with(EnvTraceContext { | ||
| traceparent: Some("not-a-valid-traceparent".to_string()), | ||
| tracestate: None, |
There was a problem hiding this comment.
Minor: env_interceptor_is_noop_for_invalid_traceparent sets tracestate: None, so it only asserts traceparent is absent and cannot verify that a present tracestate is dropped when traceparent is invalid (a W3C requirement).
Why this matters: A regression that injected a stray tracestate alongside an invalid or absent traceparent would go uncaught.
Suggested fix: Set tracestate: Some("vendor=value") here and add assert!(request.metadata().get("tracestate").is_none());.
Source: test-quality agent
There was a problem hiding this comment.
Done: env_interceptor_is_noop_for_invalid_traceparent now sets tracestate: Some("vendor=value") alongside the invalid traceparent and asserts tracestate is absent from the request metadata, covering the W3C requirement.
There was a problem hiding this comment.
Confirmed. The invalid-case test now sets tracestate: Some("vendor=value") alongside the invalid traceparent and asserts both traceparent and tracestate are dropped.
…omposition Review follow-ups on the trace-context forwarding feature: - Normalize each TRACEPARENT/TRACESTATE spelling (trim, drop blanks) before the lower-case fallback so a blank upper-case value no longer shadows a valid lower-case one - Share one env-reading helper between EnvTraceContextInterceptor and set_parent_from_environment so both paths agree on what counts as set - Pass the TRACEPARENT_ENV/TRACESTATE_ENV constants instead of duplicated string literals - Log at debug level when a present TRACEPARENT is invalid, keeping the passive no-op behavior but leaving a grep-able signal - Cover EnvTraceContext::from_env (both spellings, padding, blanks) and the GatewayInterceptor auth+trace composition with tests - Drop tracestate when traceparent is invalid, and assert it in tests - Narrow the traces.mdx claim to standard CLI commands, since term and completions build clients without the trace interceptor Signed-off-by: Chris Moore <cmoore@redhat.com>
|
@rhuss I have pushed a commit that addresses the follow up items. There are comments above with explanations of the approach for each one. Thanks! |
This branch has not been deployed
3 participants
Summary
The CLI's gateway gRPC channel now passively forwards the W3C trace context (TRACEPARENT/TRACESTATE) from its environment, so a CI pipeline's trace parents the gateway's spans. This needs no OTel collector or provider on the CLI side - short-lived clients on CI runners and jump hosts just pass through what the CI system already exports.
Add EnvTraceContextInterceptor to openshell-otel: it reads the trace context once at construction and injects it as gRPC metadata, and is a no-op when the context is unset or invalid. Compose it with EdgeAuthInterceptor via a new GatewayInterceptor, since tonic allows only one interceptor per channel.
Related Issue
Closes #2639
Changes
Testing
mise run pre-commitpassesChecklist