Propagate task OTel trace context through IPC and into execution API requests#66151
Conversation
xBis7
left a comment
There was a problem hiding this comment.
I ran a simple dag that has 1 deferred task with 1 task sub span.
The basic spans that we would expect would be
DR [---------]
\_ task1 [----]
\_ sub_span [-]
I think we are getting too much info and it depends on the user and the use-case whether they are useful or not.
There is a config flag otel_debug_traces_on which was used for exporting spans for internal functions of the scheduler. The code was all cleaned up and the flag isn't currently used but I think it would be suitable for providing the option to turn on/off the extra spans.
eb4783d to
6bef27f
Compare
|
Oh yes, I didn't mean to include the supervisor.start and client spans, those were me just testing things. Removed. |
potiuk
left a comment
There was a problem hiding this comment.
Approving — implementation looks correct, design is sound, and the _FrameMixin extraction is a nice cleanup. Inline I've left two larger asks (test coverage for the new propagation chain, and a small structural tweak on the supervisor's try/finally) plus three nit: items that are non-blocking polish. None of them need to gate merge if you'd rather pick them up in a follow-up.
Verified the pygtrie removal is safe (no usage anywhere under shared/observability/).
Drafted-by: Claude Opus 4.7; reviewed by @potiuk before posting
xBis7
left a comment
There was a problem hiding this comment.
As already suggested, some tests would be nice. Apart from that, the context propagation logic LGTM!
…requests The supervisor makes HTTP calls (XCom pushes, RTIF writes, connection lookups) on behalf of the task process via a Unix socket IPC channel. Without explicit propagation, those calls either float under the supervisor's own span or are unparented entirely — the task's trace context never crosses the process boundary. This commit wires the full chain: IPC leg (task process → supervisor): - Add traceparent: str | None = None field to _RequestFrame - _make_frame() injects the task runner's active W3C traceparent into every outgoing IPC frame via TraceContextTextMapPropagator.inject() - handle_requests() extracts the traceparent and calls otel_context.attach() before dispatching each request, restoring the task's trace context in the supervisor process for that request's lifetime - TriggerCommsDecoder.asend() now calls _make_frame() instead of constructing _RequestFrame directly, so trigger IPC frames carry the active span's traceparent too HTTP leg (supervisor → execution API): - inject_trace_context event hook on the httpx Client propagates the currently-active span's traceparent header on every outgoing request, linking server-side spans to the correct task span - _log_and_trace_retry records http.retry events on the active span alongside the existing log warning Dependency cleanup: - Move opentelemetry-api>=1.27.0 from [otel] optional extras to base [dependencies] in shared/observability — it flows unconditionally into task-sdk via shared_distributions, the same way airflow-core already has it unconditionally - Replace try/except ImportError guards and _NoOpTracer fallbacks in comms.py, supervisor.py, and client.py with direct imports; inject() and get_current_span() are no-ops when no TracerProvider is configured, so the guards were only testing "is OTel installed?" not "is it enabled?" - Introduce _FrameMixin (plain Python mixin, not a msgspec.Struct) to share _encoder and as_bytes() between _RequestFrame and _ResponseFrame
6bef27f to
864e559
Compare
…side spans (#67904) Trace propogation is a useful tool to let us trace execution across distrubted systems -- exactly what we have with the API server and the workers. We already propogated the trace context all the way from the task code in #66151, this continues it to any spans emitted on the API server side. The mode of trace propagation is set to "only-authenticated" by default to defend against data polution (i.e. it's not a security risk): - Sampling-flag manipulation: they set the sampled flag (`-01` trailer) on every request to force your tracing backend to record all their reconnaissance/probe traffic. If you pay per ingested span or have constrained trace storage, this has a real cost. - Trace ID pollution: attacker-controlled trace IDs appear in your backend. With 128-bit random IDs, collision with a legitimate trace is negligible, but it clutters dashboards.
The supervisor makes HTTP calls (XCom pushes, RTIF writes, connection lookups) on behalf of the task process via a Unix socket IPC channel. Without explicit propagation, those calls either float under the supervisor's own span or are unparented entirely — the task's trace context never crosses the process boundary.
This commit wires the full chain:
IPC leg (task process → supervisor):
HTTP leg (supervisor → execution API):
Dependency cleanup: