Repository navigation
feat(observability): correlate HTTP receipts, OCSF and request traces - #5214
afourniernv wants to merge 1 commit into
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Hey, thanks for working on this. The separation between correlation context and authorization identity is important. I've been testing Keel as an external pre-execution authorization service through OpenShell's Supervisor Middleware, and this connects directly to an issue we encountered during our v0.1.3 qualification. In that experiment, the adapter could associate I see this PR introduces For that next integration step, would the intended approach be to propagate the supervisor-generated That would let an external authorization provider join:
It would also be useful to preserve that identifier across request and response evaluation, including denials and middleware failures. I'm not suggesting that correlation alone proves exact-request binding or downstream effects. Those remain separate guarantees. Happy to help validate the eventual runtime integration against our existing sanitized test cases if helpful. |
afourniernv
changed the title
|
Written by Claude Code, reviewed by @krishicks
A question about span volume. Every inspected request gets We see supervisor traces as a tool for latency and errors, sampled like any other service, rather than a full record of every request. OCSF already provides that record, and request rates belong in metrics. Sampling for supervisors is tracked in #5362, and supervisor metrics in #5364. In the meantime, we'd like these spans not to assume they're always exported. In particular, could you confirm that the request ID join through |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. api-behavior — No explicit risk acceptance foundSuccessful HTTP FORWARD requests without an L7 route now emit a fallback HTTP Activity record instead of the existing Allowed/Success record with the requested path and process.
2. api-behavior — No explicit risk acceptance foundx-openshell-request-id previously passed through HTTP forwarding and allowed middleware mutations; the candidate strips incoming values, replaces upstream values and rejects mutations.
3. api-behavior — No explicit risk acceptance foundInspected HTTP denials replace bounded supervisor.egress.deny spans with unbounded openshell.http.request and policy spans that can inherit workload traceparent.
Review details
Advisory review; publication gates are unchanged. |
krishicks
left a comment
krishicks
left a comment
There was a problem hiding this comment.
Written by Claude Code, reviewed by @krishicks
Reviewed at 7e8f7ae. Each point starts with what a user would see; the cause and file references follow. Line numbers refer to that commit.
Audit records
- A SIEM shows requests the supervisor blocked as allowed. A request blocked by a middleware failure (403), admission exhaustion (503), a WebSocket preflight error (502), or a credential preparation failure (502) gets one HTTP Activity record that says
Allowed, with no failure status.- Cause:
emit_primaryreclassifies the record only when the outcome isdenied(l7/correlation.rs:212-220). Theplatform_errorandupstream_erroroutcomes keep the allow record (relay.rs:900-904,relay.rs:1823-1855).
- Cause:
- Allowed plain-HTTP proxy requests no longer say which process made them. On main, every allowed FORWARD request without L7 inspection is recorded with its path, binary, PID, and cmdline. With this PR, it gets a placeholder record: "HTTP request ended before a response was selected", URL
/, no action, no process.- Cause: the correlation is created up front (
proxy.rs:5921-5926), so the empty slot is emitted at the response head. The real allow record arrives after the relay (proxy.rs:7228) and is dropped (correlation.rs:174).
- Cause: the correlation is created up front (
- Requests rejected before policy runs are reported as policy denials. An
Upgraderefusal, a malformed request target, or an invalid GraphQL or JSON-RPC body produces "HTTP request blocked after policy admission", URL/, and a span withpolicy.decision=deny. Policy never ran on these requests, so an operator investigating policy is sent to the wrong place.- Cause: these paths emit only a Network Activity record, so the placeholder record is emitted and relabelled as a denial (
relay.rs:1310-1335,relay.rs:1550-1665,relay.rs:3344-3348).
- Cause: these paths emit only a Network Activity record, so the placeholder record is emitted and relabelled as a denial (
- Some requests still produce extra HTTP Activity records that can't be joined by request ID, and one produces two records that disagree. This happens to any request with response middleware, a request-middleware denial, or a token-grant refusal. A token-grant refusal gives one record saying Denied and another saying Allowed with a 502. Denials on these paths also open a separate
supervisor.egress.denyspan next to the request span.- Cause: these paths still emit through
telemetry::emit_decisionwithoutcorrelation_uid(http_response.rs:698-1251,middleware.rs:1023,token_grant_injection.rs:344). The request ID is already available at the call sites (relay.rs:1810,relay.rs:2655,relay.rs:3138).
- Cause: these paths still emit through
- When credential resolution or MCP reinspection rejects a request, the record no longer shows which rule matched. The surviving record loses the request path, policy rule, and reason. A problem after the response headers are sent produces no correlated record at all.
- Cause:
emit()replaces the primary record with any later HTTP Activity, and silently drops one that arrives after the head is written (correlation.rs:166-176).
- Cause:
Trust and sampling
- A sandboxed workload decides which trace ID appears on supervisor audit records. If it sends a sampled
traceparent, the HTTP Activity record'strace.uidis the workload's trace ID. A workload could point its denials at an unrelated trace. Once #5362 adds a sampling ratio, it could also force its request spans to be exported regardless of that ratio.- Cause: a sampled workload context becomes the parent of
openshell.http.request(propagation.rs:169-174). #4789 documents that workload headers never supply supervisor correlation, and #2640 keeps workload context in a separate field. - Linking sampled context the way unsampled context is already linked would keep the connection to the agent's trace without either effect.
- Cause: a sampled workload context becomes the parent of
Behavior changes beyond correlation
These change what clients see. Could they move to a separate PR, or at least be listed in the PR description?
- Agent HTTP clients stop retrying, so a routine connection close becomes a failed request. When an upstream closes an idle keep-alive connection, main closes the client connection. Go, urllib3, and curl retry that automatically. With this PR the client gets a 502 and surfaces the error.
- Cause: a synthetic 502 on upstream EOF before headers (
http_response.rs:100-110).
- Cause: a synthetic 502 on upstream EOF before headers (
- Some upstream APIs that work today stop working. For example, a server that sends
HTTP/1.1 200without a reason phrase now has its response rejected, and the agent sees the connection close. The audit record still reports a 502.- Cause: new strict response-head validation that applies whether or not a request is correlated (
http_response.rs:425-440).httparseaccepts a missing reason phrase.
- Cause: new strict response-head validation that applies whether or not a request is correlated (
- Agents get a 500 response where the connection used to close. This happens on a missing credential or a stale credential generation.
- Cause:
relay.rs:1064-1080,proxy.rs:6851-6880.
- Cause:
Spans
- Traces don't show where an inspected request's time went. Only policy evaluation appears under
openshell.http.request. Middleware calls, credential preparation, and the upstream dial are not children of the request span, and remote middleware receives the connection's trace context instead of the request's. - WebSocket requests show up as a single span lasting the whole session. The root span stays open until the upgraded connection closes.
- The span marks an error but not which stage failed.
record_error_stageignores its stage argument (correlation.rs:162).
Tests
Most of the record problems above would have been caught by a test asserting exactly one HTTP Activity per request. The dispatch tests use .find() instead. These paths have no coverage:
- placeholder records and rejections before policy runs
- the content of a non-L7 FORWARD record
- token-grant refusal
- request-middleware denial
- upstream EOF before headers
- a second request on a keep-alive connection, which should get a new ID
Process
The branch conflicts with main.
|
This is what the robot drew when I asked for a before/after with this PR: 
|
|
Written by Claude Code, reviewed by @krishicks
A follow-up connecting the review with my earlier comment on span volume. The compatibility review's item 3 raises the same concern: span volume. Review point 10 asks for a child span per stage, which adds spans. Nothing bounds that until #5362 adds supervisor sampling, and the #4789 deny-span limiter isn't the right tool, since #5362 removes it. Could the request spans land with or after #5362? This PR would keep the request ID, Then the after diagram would look like this: |
|
Drafted with Codex; reviewed and approved by @HarryMoss. Hi @afourniernv, I’ve read Kris’s latest review and the proposed split between the correlation core and request spans. My local #2640 connection-startup prototype is based on #5214 at Once you’ve settled the split, which revision should I track for those shared paths so I can avoid duplicating your fixes? Context: #2640 (comment) |
afourniernv
changed the title
|
Drafted by 🤖, signed off by @krishicks A correction to my last comment: the request spans don't need to wait for #5362. They can stay in this PR, along with per-stage children (review point 10). The concern is spans that exist only because a denial happened. A request span that records every outcome is the right shape. Two requirements apply whether or not sampling exists:
The updated diagram: 
|
afourniernv
changed the title
|
Drafted by Codex; checked by @HarryMoss. Thanks @afourniernv for the update. I reviewed b595d70 against @krishicks’s latest guidance. The supervisor-owned roots, untrusted workload links and sampling-independent receipt joins address those design points. Thirty-five focused controls passed locally. Two additional regression tests reproduced cases that may be worth checking:
|
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. api-behavior — No explicit risk acceptance foundInspected HTTP requests previously emitted per-middleware HTTP Activity records; consolidation now drops routine invocation records and their details.
3. api-behavior — No explicit risk acceptance foundInspected denials replace bounded supervisor.egress.deny spans and policy attributes with openshell.http.request spans outside the legacy limits.
Review details
Advisory review; publication gates are unchanged. |
|
Drafted by Codex; @HarryMoss reviewed Thanks @afourniernv for the updates. I reran the two regressions from my earlier review on
I have the small test-only patch ready to share. This is local macOS evidence, rather than a fresh deployed validation. The compatibility advisory also matches what I see in the successful middleware merge path: routine invocation details are dropped rather than retained on the primary record. Would you prefer retaining those audit details, or documenting that loss as an explicit compatibility decision? That question follows Kris's guidance that audit records and joins work independently of trace export. |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. api-behavior — No explicit risk acceptance foundSuccessful token grants previously emitted one HTTP Activity record per credential; inspected requests now merge these records into the existing request decision without retaining their messages.
Review details
Advisory review; publication gates are unchanged. |
Closes NVIDIA#5393 Refs NVIDIA#5210 Signed-off-by: Alex Fournier <[email protected]>
3f46a50 to
11030e5
Compare
Summary
Join inspected HTTP responses, supervisor request traces and OCSF records using
the existing OpenShell-generated request UUID. Preserve middleware audit detail
independently of tracing.
Related Issue
Closes #5393. Tracking: #5210. Request-stage tracing is included here;
operator-controlled supervisor sampling remains separate in #5362.
Changes
X-OpenShell-Request-IDreceipts,OCSF
metadata.correlation_uid, and the exported log attribute.an early-termination fallback. Tag primary and middleware invocation records
separately; retain original invocation fields, phase and late observations.
the late-failure budget.
context is an untrusted link, not a parent. Inspected decisions do not create
additional denial-only spans.
raw-header handling and receipt protection. Request timing ends after owned
stages; HTTP upgrade timing ends before the upgraded session.
The core adds one Rust
OcsfEvent::set_correlation_uidmethod. No gateway RPC,protobuf, configuration or Relay dependency is added. Receipts are join keys,
not authorization or proof of completed delivery. L4 and uninspected passthrough
remain outside the protected-receipt contract.
Release Notes
Middleware invocation records remain available with additive record-kind and
phase tags. Request-count queries should select the tagged primary record.
Inspected request traces replace the legacy denial span name and attributes,
cover allowed traffic at INFO level, and do not use the old denial-span budgets.
The docs include query and capacity migration guidance. Export queues are
bounded, but generation is not rate-limited by this PR. Maintainer acceptance
of the release/versioning implications remains pending; no sampling setting or
denial-only span is introduced as a workaround.
Testing
network ignores. Two initial DNS lookup failures passed isolated and full
reruns without source, policy or resolver changes.
plus all six newer upstream commits, including private-IP enforcement/tests.
fields, late observations, upstream-200/client-403 isolation and tracing-off
behavior remain covered.
Pre-rebase integrated source
967d7d04passed fullmise run testand eightmatching-source native Hermes/Relay workflows across nine actual conversations.
Those runs covered protected GitHub, MCP, redaction, request/response denials,
interrupted delivery and tracing-disabled logging. One failed agent recovery
attempt and verifier corrections were retained. Four credential values were
absent from 167 telemetry/evidence/log files. This is retained pre-rebase evidence,
not a new live run of the rebased commit.
Checklist
11030e5d4on main576d08fe6;historical merge commits removed without changing the feature's code/tests.