feat(tracing): establish W3C trace context at FastACP ingress#466
Open
NiteshDhanpal wants to merge 1 commit into
Open
feat(tracing): establish W3C trace context at FastACP ingress#466NiteshDhanpal wants to merge 1 commit into
NiteshDhanpal wants to merge 1 commit into
Conversation
Extend RequestIDMiddleware to extract the inbound W3C trace context (traceparent/tracestate) and attach it as the current OpenTelemetry context for the lifetime of the request. This gives obs_correlation() (added in the previous change) a real emit-time context, so business spans created while handling an RPC -- and any Temporal workflow started from it -- adopt the caller's observability trace_id. Best-effort and lazy-imported: no traceparent header, or OTel unavailable, is a safe no-op and never breaks a request. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
NiteshDhanpal
force-pushed
the
obs/01-span-obs-correlation
branch
from
July 21, 2026 20:46
a3a7ae1 to
cd91ba7
Compare
NiteshDhanpal
force-pushed
the
obs/02-fastacp-w3c-ingress
branch
from
July 21, 2026 20:46
2bfde1c to
24f0835
Compare
Comment on lines
+62
to
+67
| try: | ||
| carrier = { | ||
| k.decode("latin-1"): v.decode("latin-1") | ||
| for k, v in scope.get("headers", []) | ||
| } | ||
| ctx = extract(carrier) |
There was a problem hiding this comment.
Duplicate
tracestate headers silently dropped
The plain-dict carrier overwrites duplicate keys, so if an intermediary forwards multiple tracestate headers (which the W3C Trace Context spec explicitly permits), all but the last entry are silently discarded. The OTel DefaultGetter calls carrier.get(key) on a plain dict, which can only return a single string value, so combined trace-state from several systems in the propagation chain would be partially lost.
To handle multi-value headers correctly, the carrier should concatenate repeated values with a comma separator before inserting them into the dict, or use a multi-value mapping with a custom Getter.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agentex/lib/sdk/fastacp/base/base_acp_server.py
Line: 62-67
Comment:
**Duplicate `tracestate` headers silently dropped**
The plain-dict carrier overwrites duplicate keys, so if an intermediary forwards multiple `tracestate` headers (which the W3C Trace Context spec explicitly permits), all but the last entry are silently discarded. The OTel `DefaultGetter` calls `carrier.get(key)` on a plain dict, which can only return a single string value, so combined trace-state from several systems in the propagation chain would be partially lost.
To handle multi-value headers correctly, the carrier should concatenate repeated values with a comma separator before inserting them into the dict, or use a multi-value mapping with a custom `Getter`.
How can I resolve this? If you propose a fix, please make it concise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack (2/3)
What this PR does
Extends
RequestIDMiddlewareinbase_acp_server.pyto extract the inbound W3C trace context (traceparent/tracestate) and attach it as the current OpenTelemetry context for the lifetime of the request.This gives
obs_correlation()(PR 1) a real emit-time context: business spans created while handling an RPC — and any Temporal workflow started from that request (PR 3) — now adopt the caller's observabilitytrace_id.Why it's needed
PR 1 only reads the active obs context; without this, there is no context to read on the ACP request path (the SGP
/v5/spans/batchtransport doesn't carry it). This is the client-side propagation half.Safety
Best-effort and lazy-imported: no
traceparentheader, or OTel unavailable, is a safe no-op and never breaks a request. Context is detached in afinally.Verification
py_compile+ functional smoke: atraceparentheader yields a valid current span context with the expectedtrace_id; no header → invalid context (no-op).Greptile Summary
This PR extends
RequestIDMiddlewareinbase_acp_server.pyto extract the inbound W3C trace context (traceparent/tracestate) and attach it as the active OpenTelemetry context for the lifetime of each HTTP request, enabling downstream business spans and Temporal workflows to share the caller's observability trace ID._attach_incoming_trace_context,_detach_trace_context) are added; both are best-effort with lazy OTel imports so a missingopentelemetrypackage or a missing header is always a safe no-op.RequestIDMiddleware.__call__is refactored from a guard-and-fall-through pattern to an early-return for non-HTTP scope types, and the trace context attach/detach is wrapped in atry/finallyblock alongside the existing request-ID logic.Confidence Score: 4/5
Safe to merge — the change is additive, never raises, and the existing request-handling path is functionally unchanged in the absence of OTel.
The attach/detach lifecycle is correct and properly guarded with try/finally; asyncio.create_task inherits the contextvars copy at creation time so background tasks correctly retain the trace context. The one imprecision is the plain-dict carrier that silently drops all but the last value when a caller forwards multiple tracestate headers, which is spec-legal but uncommon in practice.
base_acp_server.py — the multi-value tracestate edge case in _attach_incoming_trace_context is the only area that warrants a second look.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Caller as Caller (SGP/upstream) participant MW as RequestIDMiddleware participant OTel as OpenTelemetry participant App as FastAPI App participant BG as Background Task Caller->>MW: HTTP Request (traceparent header) MW->>MW: Extract x-request-id → ctx_var_request_id.set() MW->>OTel: extract(carrier) → W3C context OTel-->>MW: ctx (with trace_id from traceparent) MW->>OTel: context.attach(ctx) → token MW->>App: await self.app(scope, receive, send) App->>App: asyncio.create_task(_process_request) copies current context App-->>MW: response sent BG->>BG: runs with copied context (trace_id still active) MW->>OTel: context.detach(token) [finally] Note over MW,OTel: original context restored%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Caller as Caller (SGP/upstream) participant MW as RequestIDMiddleware participant OTel as OpenTelemetry participant App as FastAPI App participant BG as Background Task Caller->>MW: HTTP Request (traceparent header) MW->>MW: Extract x-request-id → ctx_var_request_id.set() MW->>OTel: extract(carrier) → W3C context OTel-->>MW: ctx (with trace_id from traceparent) MW->>OTel: context.attach(ctx) → token MW->>App: await self.app(scope, receive, send) App->>App: asyncio.create_task(_process_request) copies current context App-->>MW: response sent BG->>BG: runs with copied context (trace_id still active) MW->>OTel: context.detach(token) [finally] Note over MW,OTel: original context restoredPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(tracing): establish W3C trace conte..." | Re-trigger Greptile