Repository navigation
Conversation
Add optional trace.uid enrichment through a plain sampled-context extractor, with explicit correlation taking precedence on all ten event builders. Register the extractor at gateway and supervisor startup and keep an INFO span active around network and HTTP deny emissions at the default log level. Integrate the Authentication event and current network APIs while retaining sanitization, policy decisions, credential handling and conditional DEBUG parenting. Document per-decision traces and export/retention limits. Signed-off-by: Harry Turner <harry.github@loopfeed.com>
Remove the top-level trace object and Trace profile from the existing 1.1/1.3 projection path, keeping this compatibility contract separate from native trace correlation. Test trace removal for both legacy targets and native OTLP body preservation, including source-event immutability. Document the legacy trace contract without redesigning downgrade behavior or claiming full schema conformance. Signed-off-by: Harry Turner <harry.github@loopfeed.com>
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
|
recheck |
Merge NVIDIA/OpenShell main at d223943. Keep both resource_details and the Trace schemas in the vendored schema README, correct its object count and preserve the refresh instructions. Retain both published correlation commits and the tested source tree 247a27f. Signed-off-by: Harry Turner <harry.github@loopfeed.com>
|
The README conflict is resolved, and the updated branch passes the local checks. Could a maintainer please vet and approve the CI workflows? Fresh Linux/Collector/Tempo validation remains pending. |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
@HarryMoss, please have your agent assess each of these comments and check it against the code before acting on it. Some may turn out not to apply, and I would rather you confirm or refute them than take them at face value.
This review comes from an agent-assisted run with several reviewers (cc-review: seven prompt reviewers plus Codex and CodeRabbit, then a skeptic/calibration pass). The findings and the scenarios in them come from the agents reading the code. Nothing was executed or reproduced, and I have not verified each scenario myself.
What Went Well
crates/openshell-otel/src/correlation.rscurrent_ocsf_trace_correlationonly correlates when the span context is both valid and sampled.crates/openshell-ocsf/src/trace.rskeepsopenshell-ocsffree of OpenTelemetry through aOnceLockextractor, andenrichnever overrides an explicit trace.crates/openshell-ocsf/src/events/base_event.rsset_traceadds thetraceprofile only once.crates/openshell-ocsf/src/format/downgrade.rsstrips both thetracefield and the profile for 1.1 and 1.3, with tests for each target.crates/openshell-supervisor-network/src/telemetry/tests.rsexportedchecks real OTLP/gRPC exports instead of only the JSON shape.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Important | crates/openshell-supervisor-network/src/telemetry.rs:32 |
Paired deny event and finding get different trace IDs | gator |
| Minor | crates/openshell-server/src/cli.rs:669 |
Gateway trace.uid is caller-propagated but documented as trusted |
security |
| Minor | crates/openshell-supervisor-network/src/l7/middleware.rs:466 |
Capacity-exhausted finding marked deny when allowed | correctness |
| Minor | crates/openshell-supervisor-network/src/l7/middleware.rs:487 |
Load shedding traced as policy deny | goal-alignment |
| Minor | crates/openshell-supervisor-network/src/l7/middleware.rs:1004 |
denied: predicates untested |
test-quality |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:1261 |
Transparent middleware deny not parented on connect span (see below) | correctness |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:5677 |
Non-canonical rejection gets no deny span | codex |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:5956 |
CONNECT/FORWARD deny-span wiring untested | test-quality |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:6072 |
Audit-mode L7 failure traced as enforced deny | codex |
| Minor | crates/openshell-supervisor-network/src/proxy.rs:6660 |
parent: is a no-op for non-deny events |
architecture |
| Minor | crates/openshell-supervisor-network/src/proxy/trace_tests.rs:80 |
Debug deny test lacks count and uid checks | test-quality |
| Minor | crates/openshell-supervisor-network/src/telemetry.rs:11 |
Local ocsf_emit! shadows the upstream macro |
architecture |
| Minor | crates/openshell-supervisor-network/src/telemetry.rs:41 |
Deny classification split between event and flag | architecture |
| Minor | crates/openshell-supervisor-network/src/telemetry.rs:55 |
Deny-span policy/host attributes never asserted | test-quality |
| Minor | crates/openshell-supervisor-network/src/telemetry.rs:64 |
One exported root span per deny, no rate gate | production |
| Minor | crates/openshell-ocsf/src/builders/network.rs:121 |
.trace() setter on 10 builders used only by tests |
architecture |
| Minor | crates/openshell-otel/src/lib.rs:49 |
Supervisor filter moved into shared public API | architecture |
| Minor | docs/observability/ocsf-json-export.mdx:281 |
Docs mention the internal builder API | architecture |
Outside the diff: crates/openshell-supervisor-network/src/proxy.rs:1261. The transparent-TCP fail-closed middleware deny calls emit_middleware_uninspectable without the live connect_span, unlike the sibling transparent denies, so at debug level this deny is not linked to the connect/authorize trace even though the docs say it should be. An optional parent parameter, or wrapping the call in connect_span.in_scope(...), would fix it.
Observation: the supervisor.egress.deny span and its openshell.policy.* keys live in telemetry.rs, separate from the supervisor.egress.* spans in proxy/egress.rs, and the attribute values come from string-keyed evidence lookups. Shared constants would keep the two in sync.
Review Details
- Findings posted: 18
- Findings reviewed and not posted: 0
- Gate outcome: FAIL (one Important finding)
- Participating reviewers: architecture, correctness, goal-alignment, production, security, test-quality, gator, codex (CodeRabbit ran with no findings)
| pub fn emit(event: OcsfEvent, parent: Option<&tracing::Span>, denied: bool) { | ||
| let (action, endpoint, policy) = match &event { | ||
| OcsfEvent::NetworkActivity(e) => { | ||
| (e.action, e.dst_endpoint.as_ref(), e.firewall_rule.as_ref()) | ||
| } | ||
| OcsfEvent::HttpActivity(e) => (e.action, e.dst_endpoint.as_ref(), e.firewall_rule.as_ref()), | ||
| OcsfEvent::DetectionFinding(e) => (e.action, None, None), | ||
| _ => (None, None, None), | ||
| }; | ||
| if !denied && action != Some(ActionId::Denied) { | ||
| openshell_ocsf::ocsf_emit!(event); | ||
| return; | ||
| } | ||
|
|
||
| // Finding evidence supplies decision fields, not a trace parent. | ||
| let evidence = |key: &str| match &event { | ||
| OcsfEvent::DetectionFinding(e) => e.evidences.as_ref().and_then(|items| { | ||
| items | ||
| .iter() | ||
| .find_map(|item| item.data.as_ref()?.get(key)?.as_str()) | ||
| }), | ||
| _ => None, | ||
| }; | ||
| let policy = policy | ||
| .map(|rule| rule.name.as_str()) | ||
| .or_else(|| evidence("policy")); | ||
| let address = endpoint | ||
| .and_then(|endpoint| endpoint.domain.as_deref().or(endpoint.ip.as_deref())) | ||
| .or_else(|| evidence("host")); | ||
| let port = endpoint.and_then(|endpoint| endpoint.port); | ||
| let reason = event.base().status_detail.as_deref(); | ||
| let parent = parent.cloned().unwrap_or_else(tracing::Span::current); | ||
| let span = tracing::info_span!( | ||
| parent: &parent, | ||
| "supervisor.egress.deny", | ||
| openshell.policy.decision = "deny", | ||
| openshell.policy.name = tracing::field::Empty, | ||
| openshell.policy.reason = tracing::field::Empty, | ||
| server.address = tracing::field::Empty, | ||
| server.port = tracing::field::Empty, | ||
| ocsf.class_uid = event.class_uid(), | ||
| ); | ||
| if let Some(policy) = policy { | ||
| span.record("openshell.policy.name", policy); | ||
| } | ||
| if let Some(reason) = reason { | ||
| span.record("openshell.policy.reason", reason); | ||
| } | ||
| if let Some(address) = address { | ||
| span.record("server.address", address); | ||
| } | ||
| if let Some(port) = port { | ||
| span.record("server.port", i64::from(port)); | ||
| } | ||
| span.in_scope(|| openshell_ocsf::ocsf_emit!(event)); | ||
| } |
There was a problem hiding this comment.
Important: emit opens a new supervisor.egress.deny span for every OCSF event it handles. A denial is dual-emitted (domain event plus DetectionFinding), so the two records of one incident end up with different trace.uid values in separate root traces.
Why this matters: At the default level there is no active parent, so each emission starts its own trace. Affected sites include l7/rest.rs:1537 and :1551, proxy.rs:2948 together with l7/mod.rs:447 (the finding has no parent even at debug level), l7/relay.rs:290-291, l7/websocket.rs:1388-1389, the loop at l7/middleware.rs:1004 and the finding loops in l7/rest/http_response.rs. Pivoting from the finding's trace.uid then lands on a one-span trace that has nothing to do with the network event's trace, so the correlation is useless there. It also doubles span volume per denial.
Suggested fix: Open one deny span per decision and emit all events of that decision inside it, for example a telemetry::deny_scope(parent, &primary_event, || { ... }) helper, and let emit reuse an already active deny span instead of nesting a new one. A test that emits a network deny plus its finding and asserts one exported deny span and equal trace.uid values would keep this from regressing.
Source: gator agent (also flagged by: production agent, correctness agent)
| openshell_ocsf::register_trace_correlation_extractor( | ||
| openshell_otel::current_ocsf_trace_correlation, | ||
| ); |
There was a problem hiding this comment.
Minor: Registering current_ocsf_trace_correlation in the gateway stamps gateway audit events with the trace ID of the request span, and that span is parented on the caller's inbound traceparent (multiplex.rs:129-136) before authentication runs. On the gateway, trace.uid is therefore chosen by the caller, including on Authentication failure events.
Why this matters: Honoring inbound traceparent on the gateway is intentional (CI trace continuity from the CLI), so this is not a request to drop propagation. The problem is the contract: TraceCorrelation and correlation.rs describe the value as trusted, and #2640 says caller-controlled headers never go into trace. An unauthenticated client could then place its failed logons in another trace, or send flags=00 to keep them out of correlation. This scenario is derived from code reading and was not reproduced.
Suggested fix: Document that gateway trace.uid is caller-propagated (a correlation hint, not proof) and drop the "trusted" wording in trace.rs, base_event.rs and correlation.rs. Optionally, leave correlation off for events emitted before authentication succeeds.
Source: security agent (also flagged by: gator agent, goal-alignment agent)
| .message("Persistent middleware session admission was refused at process capacity") | ||
| .build(); | ||
| ocsf_emit!(event); | ||
| ocsf_emit!(denied: true, event); |
There was a problem hiding this comment.
Minor: emit_middleware_session_capacity_exhausted always passes denied: true, but session_capacity_exhausted() returns allowed: !fail_closed. With an all-FailOpen chain the WebSocket upgrade is allowed and still gets a deny span and deny correlation.
Why this matters: Telemetry then reports a denial for traffic that went through. Enforcement is unaffected.
Suggested fix: Pass the decision in, e.g. emit_middleware_session_capacity_exhausted(ctx, !outcome.allowed) with ocsf_emit!(denied: denied, event).
Source: correctness agent
|
|
||
| fn emit_middleware_admission_exhausted(ctx: &L7EvalContext) { | ||
| ocsf_emit!(middleware_admission_exhausted_event(ctx)); | ||
| ocsf_emit!(denied: true, middleware_admission_exhausted_event(ctx)); |
There was a problem hiding this comment.
Minor: Load-shedding findings (admission_exhausted here, session_capacity_exhausted above) open INFO deny spans with openshell.policy.decision=deny, although no policy made that decision.
Why this matters: The request is refused, so a deny label is defensible. But it adds one exported span per shed request right when the process is saturated, and the "policy" attribute is misleading.
Suggested fix: Either drop denied: true for capacity refusals or document that they are deliberately traced as denies.
Source: goal-alignment agent
| ) { | ||
| for event in middleware_events(ctx, req, outcome) { | ||
| ocsf_emit!(event); | ||
| ocsf_emit!(denied: !outcome.allowed && matches!(event, openshell_ocsf::OcsfEvent::DetectionFinding(_)), event); |
There was a problem hiding this comment.
Minor: The new denied: predicates for the middleware chain and WebSocket finding emissions (here and at lines 182, 267, 410, 430) have no tests for either branch.
Why this matters: Inverting any of these predicates keeps the suite green.
Suggested fix: Add a table-driven exported("warn", ...) test over emit_middleware_events and emit_websocket_preflight_events with an allowed and a denied outcome, asserting which events carry trace.uid and how many deny spans are exported.
Source: test-quality agent
| }), | ||
| _ => None, | ||
| }; | ||
| let policy = policy |
There was a problem hiding this comment.
Minor: No test asserts the values of openshell.policy.name and server.address on the deny span, including the fallback to evidence("policy")/evidence("host").
Why this matters: assert_deny_resolves only checks that the decision and class attributes exist, so replacing the policy expression with None keeps the suite green.
Suggested fix: In the middleware and relay trace tests, assert openshell.policy.name == "middleware-policy" / "rest-policy" and server.address == "blocked.example".
Source: test-quality agent
| let port = endpoint.and_then(|endpoint| endpoint.port); | ||
| let reason = event.base().status_detail.as_deref(); | ||
| let parent = parent.cloned().unwrap_or_else(tracing::Span::current); | ||
| let span = tracing::info_span!( |
There was a problem hiding this comment.
Minor: Each denied event exports its own INFO root span at the default level, without rate limiting or aggregation.
Why this matters: This matches the stated design and scales like the audit records themselves, but a workload retrying a blocked host in a loop produces one trace per attempt and can fill the batch exporter queue.
Suggested fix: Consider a per-destination rate gate before creating the span, or link the existing sampler settings (OTEL_TRACES_SAMPLER) from the deny-trace section of the docs.
Source: production agent
| impl<'a, EndpointState> NetworkActivityBuilder<'a, EndpointState> { | ||
| /// Set trace correlation explicitly instead of using the active span. | ||
| #[must_use] | ||
| pub fn trace(mut self, trace: crate::TraceCorrelation) -> Self { |
There was a problem hiding this comment.
Minor: All ten builders gain a public .trace(TraceCorrelation) setter with typestate plumbing, but only tests call it; production correlation goes through the registered extractor.
Why this matters: That is about 130 near-identical lines of public API, and TraceCorrelation.uid is a public String whose 32-hex invariant is only documented. The PR description lists the setters as intended, so treat this as a design question.
Suggested fix: Either let tests use BaseEventData::set_trace and drop the setters, or keep one validating constructor with a private field.
Source: architecture agent
| /// Export supervisor spans at INFO by default, or DEBUG and TRACE when enabled. | ||
| /// Dependency spans remain at INFO. | ||
| #[must_use] | ||
| pub fn supervisor_span_filter(log_level: &str) -> tracing_subscriber::EnvFilter { |
There was a problem hiding this comment.
Minor: supervisor_span_filter is now public API of the shared openshell-otel crate, while openshell-supervisor/src/main.rs keeps an otlp_span_filter wrapper that only forwards to it. The PR description does not mention the new API.
Why this matters: A crate shared by the gateway and drivers now exports one binary's span policy, and the wrapper duplicates doc and test.
Suggested fix: Inline the call in main.rs and drop the wrapper, or keep the filter in the supervisor and give the tests a generic helper.
Source: architecture agent (also flagged by: goal-alignment agent)
| with 32 lowercase hexadecimal characters. Search for it in your configured | ||
| Jaeger or Tempo backend to inspect the supervisor's handling of the deny. | ||
| Without an active sampled context, automatic correlation adds neither the | ||
| trace object nor its profile, including when OTLP export is disabled. Explicit |
There was a problem hiding this comment.
Minor: "Explicit builder correlation takes precedence" describes an internal Rust builder API that operators cannot use.
Why this matters: The docs guidelines ask to leave out internals that do not affect users.
Suggested fix: Delete the sentence or move it to the openshell-ocsf README.
Source: architecture agent
Integrate upstream snapshot c4688aa while retaining optional trace correlation in the borrowed/shared-context builders. Signed-off-by: Harry Turner <harry.github@loopfeed.com>
Group companion audit records and terminal response findings under one denial span. Keep ordinary transport errors out of enforced-denial spans, retain available HTTP policy context, and export the OCSF class as an integer. Clarify propagated context and the additional Trace-profile applicability, with regression coverage and operator documentation. Signed-off-by: Harry Turner <harry.github@loopfeed.com>
|
Thanks for the review, @rhuss. The follow-up at 910509993e18659404e923a2aa33d772a4ae5655, following the upstream integration, groups companion deny records and terminal response failures under one decision span, corrects caller-propagated correlation wording, and keeps ordinary I/O failures out of enforced-denial spans. Forward evaluation-error spans retain the HTTP policy context, and The final source tree is The docs now explicitly identify Trace applicability on the eight extra classes as an OpenShell extension beyond official OCSF 1.8 HTTP/API applicability. Please confirm whether that extension is acceptable. Fresh Linux and default-level Collector/Tempo validation with allowed/schema controls, NVIDIA CI approval/completion and reviewer reassessment remain pending. This remains Draft; earlier independent deployment results apply only to |
Summary
Adds
trace.uidto audit records emitted within a valid, sampled OpenTelemetry span so operators can find the corresponding trace. Dedicated supervisors with OTLP tracing enabled emit INFO spans for enforced network and HTTP refusals at the default log level. Companion records share one decision scope, including stopped-response diagnostics and the applicable terminal failure finding. Ordinary transport failures do not create enforced-denial spans.This remains a draft pending the validation and maintainer decisions below.
Related Issue
Closes #2640
Changes
openshell-ocsfhas no OpenTelemetry dependency.ocsf.class_uidas an integer for numeric backend queries. Tests exported IDs, available parents, concurrent contexts and allowed/audit controls.L7 evaluation failed: ….c4688aa8715ea75d6c39fe468361dbbc16c691fa, preserving borrowed/shared event contexts and trace propagation through all ten builders.Submitted revision
The upstream integration is 36cc028102d03f73a0c9e836f406fdc9ae7581fb, followed by the review corrections in 910509993e18659404e923a2aa33d772a4ae5655. Both new commits contain DCO sign-offs. The submitted head is
910509993e18659404e923a2aa33d772a4ae5655, with verified source treee231ce833d5999a46d664121858cac22cdc526aa. The existing correlation and legacy-projection commits remain in branch history, and the original3fae097abranch is preserved.Testing
macOS arm64, Rust 1.95.0, locked dependencies and repository default features/profiles. Results are attributed to their executed source below.
e231ce833d5999a46d664121858cac22cdc526aa: 1,561 passed, 0 failed, 2 ignored. The crate'stests/integration suite was not freshly rerun.939f725a8d9973578d0d13dea8b974b5836910c8; reused after verifying subsequent changes are confined to private network Rust source/tests. OCSF/OTel source and configuration are identical.-D warningsand formatting passed. Changed-page Markdown lint, docs navigation and Fern passed on47dde144cbe107b0d265968c87d93c72bca37a63; docs/configuration are unchanged. Fern reported zero errors and three existing warnings.61c776ab16b1f3886d67c0cd8a52e5f140a0f218: supervisor 193 passed, gateway/server 2,372 passed, 21 ignored. Their source is unchanged; these are not fresh reruns.Fresh Linux compilation/tests, deployed default-level deny-to-Collector/Tempo validation with allowed/schema controls, NVIDIA CI approval/completion and reviewer reassessment remain pending for the submitted revision. Local loopback exporter tests do not establish a deployed pass. Kevin's independent review and rhuss's deployed check apply to
3fae097a5d4355eeff77746f3ea90e89228455e3only.Checklist
Shared supervisor hosts currently initialize no OTLP exporters and provide no automatic sandbox trace correlation. Separate native-conformance/catalog work in #4288 and broader downgrade removal in #4284 are outside this PR.
Thanks to @kvnloo for the independent review and pinned compatibility evidence, @rhuss for review, schema/span-lifetime guidance and the earlier deployed check, and @zanetworker for the related native schema-conformance work in #4288. These acknowledgements do not imply approval of this revision.