feat(tracing): correlate business spans with active observability trace - #465
feat(tracing): correlate business spans with active observability trace#465NiteshDhanpal merged 1 commit into
Conversation
Sorry, something went wrong.
| obs = obs_correlation() | ||
| if obs: | ||
| serialized_data = {**(serialized_data or {}), **obs} |
There was a problem hiding this comment.
serialized_data can be a list when data is passed as list[dict[str, Any]] — recursive_model_dump returns a list for that case, and a non-empty list is truthy, so serialized_data or {} evaluates to the list. {**<list>, **obs} then raises TypeError: argument after ** must be a mapping, not list at span-creation time whenever an obs context is active. The same pattern is duplicated in AsyncTrace.start_span (line 243–244). A safe guard would be: if obs and isinstance(serialized_data, dict): serialized_data = {**serialized_data, **obs} (or similarly initialise to {} when None).
This is a comment left during a code review.
Path: src/agentex/lib/core/tracing/trace.py
Line: 86-88
Comment:
`serialized_data` can be a list when `data` is passed as `list[dict[str, Any]]` — `recursive_model_dump` returns a list for that case, and a non-empty list is truthy, so `serialized_data or {}` evaluates to the list. `{**<list>, **obs}` then raises `TypeError: argument after ** must be a mapping, not list` at span-creation time whenever an obs context is active. The same pattern is duplicated in `AsyncTrace.start_span` (line 243–244). A safe guard would be: `if obs and isinstance(serialized_data, dict): serialized_data = {**serialized_data, **obs}` (or similarly initialise to `{}` when `None`).
How can I resolve this? If you propose a fix, please make it concise.
Sorry, something went wrong.
| ctx = tracer.current_trace_context() | ||
| if ctx and ctx.trace_id: | ||
| return format(ctx.trace_id, "032x"), format(ctx.span_id or 0, "016x") | ||
| return None |
There was a problem hiding this comment.
ctx.span_id or 0 falls back to 0 when span_id is None or zero, which formats to "0000000000000000" — the OTel invalid/null sentinel. This produces a correlation pair where obs.trace_id is populated but obs.span_id is the null sentinel, which consumers may interpret as "no active span." If span_id is absent, skipping the correlation entirely is more correct.
| ctx = tracer.current_trace_context() | |
| if ctx and ctx.trace_id: | |
| return format(ctx.trace_id, "032x"), format(ctx.span_id or 0, "016x") | |
| return None | |
| ctx = tracer.current_trace_context() | |
| if ctx and ctx.trace_id and ctx.span_id: | |
| return format(ctx.trace_id, "032x"), format(ctx.span_id, "016x") | |
| return None |
This is a comment left during a code review.
Path: src/agentex/lib/core/tracing/obs_ids.py
Line: 61-64
Comment:
`ctx.span_id or 0` falls back to `0` when `span_id` is `None` or zero, which formats to `"0000000000000000"` — the OTel invalid/null sentinel. This produces a correlation pair where `obs.trace_id` is populated but `obs.span_id` is the null sentinel, which consumers may interpret as "no active span." If `span_id` is absent, skipping the correlation entirely is more correct.
```suggestion
ctx = tracer.current_trace_context()
if ctx and ctx.trace_id and ctx.span_id:
return format(ctx.trace_id, "032x"), format(ctx.span_id, "016x")
return None
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Sorry, something went wrong.
Tag each adk business span at creation time with the active observability trace_id/span_id (OpenTelemetry preferred, ddtrace fallback), selected via SGP_OBS_MODE. The business trace_id stays the run-level task id; the obs ids are attached as span data (obs.trace_id/obs.span_id) using the span-link pattern so a span can be pivoted to its per-turn Tempo/Datadog trace without collapsing the agent-run grouping. Capture is emit-time (in start_span), never at ingest, so batched /v5/spans/batch flushing cannot shatter one run across N obs traces. Returns no tag when no obs context is active (safe no-op; default SGP_OBS_MODE=dd_only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Stack (1/3)
What this PR does
Tags each adk business span, at creation time, with the active observability trace_id/span_id (OpenTelemetry preferred, ddtrace fallback), so a persisted span can be pivoted to its per-turn Tempo/Datadog trace.
Design notes
Verification
py_compile + unit smoke: default mode dd_only, empty dict when no active context, correct id widths (32-hex / 16-hex).
Greptile Summary
This PR introduces obs-correlation tagging: at span creation time, each business span is stamped with the active observability trace_id/span_id (OTel preferred, ddtrace fallback) so a persisted span can be pivoted to its per-turn Tempo/Datadog trace without overwriting the run-level business trace_id.
Confidence Score: 4/5
The obs-correlation logic runs in the hot path of every span creation; an unhandled exception from ddtrace or OTel would silently crash all span creation for that agent run.
Two issues in obs_ids.py directly affect span creation reliability: the narrow except ImportError leaves runtime errors from ddtrace/OTel unhandled, and the span_id or 0 null-sentinel path (flagged in a prior thread). Both live in the critical start_span path shared by every caller of Trace and AsyncTrace.
src/agentex/lib/core/tracing/obs_ids.py and src/agentex/lib/core/tracing/trace.py need attention before merge.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Caller participant Trace/AsyncTrace participant obs_ids participant OTel/ddtrace Caller->>Trace/AsyncTrace: "start_span(name, data=...)" Trace/AsyncTrace->>obs_ids: obs_correlation() obs_ids->>obs_ids: get_obs_mode() reads SGP_OBS_MODE alt "mode == lgtm" obs_ids->>OTel/ddtrace: _lgtm_ids() get_current_span() else "mode == dual" obs_ids->>OTel/ddtrace: _lgtm_ids() or _ddtrace_ids() else "mode == dd_only default" obs_ids->>OTel/ddtrace: _ddtrace_ids() current_trace_context() end OTel/ddtrace-->>obs_ids: (trace_id, span_id) or None obs_ids-->>Trace/AsyncTrace: obs.trace_id and obs.span_id or empty dict Trace/AsyncTrace->>Trace/AsyncTrace: merge obs into serialized_data Trace/AsyncTrace->>Trace/AsyncTrace: create Span with merged data Trace/AsyncTrace-->>Caller: Span%%{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 participant Trace/AsyncTrace participant obs_ids participant OTel/ddtrace Caller->>Trace/AsyncTrace: "start_span(name, data=...)" Trace/AsyncTrace->>obs_ids: obs_correlation() obs_ids->>obs_ids: get_obs_mode() reads SGP_OBS_MODE alt "mode == lgtm" obs_ids->>OTel/ddtrace: _lgtm_ids() get_current_span() else "mode == dual" obs_ids->>OTel/ddtrace: _lgtm_ids() or _ddtrace_ids() else "mode == dd_only default" obs_ids->>OTel/ddtrace: _ddtrace_ids() current_trace_context() end OTel/ddtrace-->>obs_ids: (trace_id, span_id) or None obs_ids-->>Trace/AsyncTrace: obs.trace_id and obs.span_id or empty dict Trace/AsyncTrace->>Trace/AsyncTrace: merge obs into serialized_data Trace/AsyncTrace->>Trace/AsyncTrace: create Span with merged data Trace/AsyncTrace-->>Caller: SpanReviews (2): Last reviewed commit: "feat(tracing): correlate business spans ..." | Re-trigger Greptile