Uh oh!
There was an error while loading. Please reload this page.
feat(tracing): correlate business spans with obs via dedicated wrapper span - #484
feat(tracing): correlate business spans with obs via dedicated wrapper span#484NiteshDhanpal wants to merge 8 commits into
Conversation
| # span is never .end()ed -> never exported (Simple/Batch processors only emit on | ||
| # end). A module-level dict keyed by the unique span id survives across instances; | ||
| # uuid4 span ids cannot collide across concurrent traces. | ||
| _OBS_HANDLES: dict[str, ObsSpanHandle] = {} |
There was a problem hiding this comment.
The registry assumes start_span and end_span run in the same process, but the Temporal path dispatches START_SPAN and END_SPAN as separate activities (adk/_modules/tracing.py), and AsyncTrace runs inside those activities. With more than one worker replica, a span's END routinely lands on a different worker than its START: the handle on the START worker is never popped, so this dict grows without bound, and the wrapper span is never ended. In lgtm mode an unended span is never exported, so the persisted obs_span_id points at a span that does not exist in Tempo. Even on a single worker the two activities run in different asyncio tasks, so context.detach logs a "Failed to detach context" traceback per span.
Suggestion: skip the wrapper inside Temporal activities (temporalio.activity.in_activity()) and fall back to obs_correlation(). With #485's TracingInterceptor the ambient context inside the activity carries the turn's propagated trace, so temporal still gets trace level correlation, just without the named per step wrapper, which cannot work across activities anyway since the business work runs elsewhere.
| task_id=task_id, | ||
| ) | ||
| if obs_handle is not None: | ||
| _OBS_HANDLES[span.id] = obs_handle |
There was a problem hiding this comment.
Same failure class as the sync path issue Greptile flagged: if model_copy(deep=True) raises here, the handle leaks in _OBS_HANDLES and the wrapper's context stays attached in the caller's task, so later spans in the task would parent under a stale wrapper. Worth covering both paths with the same try/except when you fix the sync one.
…r span Model the business<->observability correlation edge on the emit side, with no schema migration (rides the existing operation_metadata JSONB). - obs_ids: standardize the correlation keys to obs_trace_id/obs_span_id (underscored, JSON-path friendly); remove the non-working `dual` mode (it required an in-process ddtrace<->OTel bridge that can't exist -- you can't run ddtrace-run and the OTel operator together, and DD_TRACE_OTEL_ENABLED is a single tracer). `dual` now safely degrades to dd_only. Harden obs_correlation to never raise. - obs_span (new): when the SDK creates a business span it opens a dedicated obs span named for that step and makes it active, so obs_span_id is stable and meaningful (a named span with its httpx call nested underneath) instead of an arbitrary innermost instrumentation span. Backends: OTel in lgtm; ddtrace in dd_only but only when a request trace is already active (avoids orphan root traces in un-instrumented agents). Reverse tag: stamps agentex.business_span_id / agentex.business_trace_id onto the obs span so the pivot is bidirectional. - trace: wire the wrapper into start_span/end_span (sync + async). Observability can never fail an app call -- every path is guarded and is a no-op when the tracer isn't configured. - tests: obs_ids (mode degrade + keys), obs_span (both backends, non-interference, never-fails, reverse tag), and the 3-turn mortgage Turn-2 example pinned as an executable contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Observability review (P1): the dedicated wrapper obs span was ended/finished without recording failure, so a failed business step (e.g. chat_completion) showed green in Tempo/DD -- violating "observe both success and failure" and undercutting the meaningful-obs_span_id goal. close_obs_span now takes the business span's error (from get_span_error) and marks the obs span before closing: - OTel: span.set_status(Status(ERROR, msg)) + error.type attribute - ddtrace: span.error = 1 + error.type / error.message tags end_span passes error=get_span_error(span) on both sync and async paths. Success path is unchanged (no status set). Guarded so error-marking can never break the close. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ddtrace start_span does not auto-parent (unlike OTel): start_span(name) mints a new ROOT trace every call, so a turn's business spans scattered across N Datadog traces (verified live: 52 spans -> 52 distinct obs_trace_ids). Pass child_of=current_trace_context() so wrappers nest under the request/turn trace and roll up into one trace; obs_span_id stays distinct per step. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The obs wrapper span (opened in start_span, ended in end_span) was tracked in an INSTANCE dict (self._obs_handles). But TracingService creates a fresh Trace object for every call -- self._tracer.trace(trace_id) in BOTH start_span and end_span -- so end_span ran on a different instance with an empty dict: the handle was never found, close_obs_span(None) was a no-op, and the OTel/ddtrace wrapper span was never .end()ed. Consequence in lgtm mode: the wrapper span records and its ids are written to Postgres (read at start), but since Simple/Batch span processors only export on span end, the span never reaches Tempo -- the turn trace was silently missing while everything looked correct (provider ours, sampler ALWAYS_ON, recording=True, ids stored). Fix: move the handle registry to module level, keyed by the uuid4 span id, so it survives across Trace instances. Adds a regression test that starts a span on one Trace instance and ends it on another. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes the failing lint job on this PR (ruff I001 import ordering + format) on the obs_ids / obs_span / trace correlation-edge files and their tests. No logic change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Annotate fake ModuleType stubs as Any (pyright rejects attribute assignment on ModuleType), widen the mock 'record' dicts to dict[str, Any], assert the Optional resolver returns before unpacking, and narrow span.data with isinstance before subscripting. Clears the pyright errors failing the lint job. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
start_span/end_span run as SEPARATE Temporal activities (START_SPAN/END_SPAN) that Temporal can route to different worker processes. The obs wrapper handle is stored in a process-local module dict, so on a multi-replica fleet the END lands on a different worker than the START: the handle is never popped (leak / OOM risk) and the wrapper span is never ended (dangling obs_span_id in Tempo). Inside a Temporal activity, skip opening our own wrapper and instead stamp the reverse tag onto the interceptor-propagated ambient span (tag_ambient_obs_span) and read forward ids via obs_correlation(). Trace-level correlation is preserved via the Temporal OTel TracingInterceptor (#485); the per-step named wrapper and TurnTrace RETRY/ASYNC roll-up are deferred (see TODO(obs-followup)). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9466b3b to
1159d23Compare…races The Temporal OTel interceptor propagates trace context by injecting the active span into the Temporal message headers on start_workflow/signal, so the worker roots the workflow+activity under it. But the ACP server dispatches from a bare async handler with no active span -> nothing injected -> the workflow's activity becomes a detached trace root, disconnected from the task/create / event/send that triggered it. Wrap submit_task and send_event in an OTel span (agentex.acp) so the interceptor has a context to inject. Child of the ingress request span when one is active (front-of-request propagation), else a per-turn root. Fail-open. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What & why
Model the business ↔ observability correlation edge on the emit side, so a persisted business span can pivot to its Tempo/Datadog trace and back. No schema migration — the ids ride the existing
operation_metadataJSONB (→ ClickHousemetadata_raw).Two worlds today: business spans (
trace_id = task id, stored inapplication_trace_span+ CH) and obs traces (OTel/ddtrace in Tempo/Datadog) share no id. This adds the link.Changes
obs_ids.py— correlation keys standardized toobs_trace_id/obs_span_id(underscored → Postgres JSON-path addressable); removed the non-workingdualmode (it needed an in-process ddtrace↔OTel bridge that can't exist; degrades todd_only);obs_correlation()hardened to never raise.obs_span.py(new) — dedicated per-business-span wrapper obs span:obs_span_idis stable/meaningful (not an arbitrary innermost httpx span); nested instrumentation parents under it.lgtm) / ddtrace (dd_only, only when a request trace is active — avoids orphan roots).agentex.business_span_id/agentex.business_trace_idon the obs span → bidirectional pivot.child_ofnesting (ddtrace): ddtrace'sstart_spandoes not auto-parent — passeschild_of=current_trace_context()so a turn's spans roll up into ONE trace instead of N roots.trace.py— wrapsstart_span/end_span(sync + async). Observability can never fail an app call (guarded; no-op when unconfigured); the two backends never interfere.Behavior (Turn 2 of the 3-turn example)
obs_span_id)Distinct, named
obs_span_idper step; all under the one turn obs trace B.✅ Live validation — infra-staging,
dd_only, real rocket-mock turn (52 spans)Deployed this branch (git-dep) to
sgp-rocket-mock-agenton sgp-infra-staging, fired a realmessage/send, and read the spans straight out of egpapplication_trace_span.operation_metadata:Edge populates: 52/52 business spans carry
obs_trace_id+obs_span_idinoperation_metadata— no migration.Wrapper works:
obs_span_idis distinct per step (52 distinct) — a dedicated named span per business step, not the coarse request span.child_offix — before/after:obs_trace_idobs_span_idstart_spannochild_of)child_of=current_ctx)→ all 52 spans now share the one request/distributed trace id (
ca5e90d9…), each a distinct named wrapper span nested under it — the per-turn (TurnTrace) shape.Sample
operation_metadata:{"turn":0,"agent":"rocket_mock_agent","__source__":"agentex","obs_trace_id":"ca5e90d9…","obs_span_id":"…","__agent_id__":"c7a9692f…"}Tests
test_obs_ids.py+test_obs_span.py: mode degrade + keys, both backends, non-interference, never-fails, reverse tag, error status,child_ofnesting, and the Turn-2 example. Full tracing suite green.Not in this PR (follow-ups)
lgtm/Tempo population needs an OTel TracerProvider in the agent (operator auto-instrumentation or app-level) — SDK side is complete and backend-agnostic.sgp-obs-tracing-middlewarelibrary still carriesdual/bridge.py— separate cleanup.🤖 Generated with Claude Code
Greptile Summary
Adds bidirectional correlation between business spans and observability traces.
Confidence Score: 4/5
The PR is not yet safe to merge because synchronous processor failures can still leak wrapper spans and leave their observability context active.
The wrapper handle is registered before synchronous processor callbacks, but an exception from a callback escapes start_span without removing or closing that handle; because no Span is returned, callers cannot reach end_span to perform cleanup.
Files Needing Attention: src/agentex/lib/core/tracing/trace.py
Important Files Changed
Reviews (4): Last reviewed commit: "feat(tracing): span the ACP->Temporal di..." | Re-trigger Greptile