-
Notifications
You must be signed in to change notification settings - Fork 9
[WIP] feat(tracing): correlate business spans with active observability trace #465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| """Correlate adk business spans with the active observability trace. | ||
|
|
||
| The adk business ``trace_id`` is the agent **task id** (run-level: it spans the | ||
| whole agent run across many requests -- task/create, then each message/send turn), | ||
| so we must NOT overwrite it with a per-request observability trace_id. Doing so | ||
| would collapse the run-level grouping. | ||
|
|
||
| Instead, each business span is *tagged* with the active observability | ||
| trace_id/span_id (this is the OpenTelemetry "span link" pattern -- correlate | ||
| across trace granularities rather than merging them). You can then pivot from a | ||
| persisted business span to the Tempo/Datadog trace for the turn that produced it, | ||
| while the business trace still groups the entire run by task id. | ||
|
|
||
| Source selection follows SGP_OBS_MODE, matching egp-api-backend: | ||
| - unset / "dd_only": ddtrace context (current stack) | ||
| - "dual": OTel/LGTM preferred, ddtrace fallback | ||
| - "lgtm": OTel/LGTM only | ||
|
|
||
| This never fabricates ids -- if no observability context is active, it returns | ||
| an empty dict and the span is simply not tagged. | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| from typing import Dict, Optional, Tuple | ||
|
|
||
| __all__ = ("get_obs_mode", "obs_correlation") | ||
|
|
||
| DD_ONLY = "dd_only" | ||
| DUAL = "dual" | ||
| LGTM = "lgtm" | ||
| _DEFAULT_MODE = DD_ONLY | ||
| _VALID_MODES = (DD_ONLY, DUAL, LGTM) | ||
|
|
||
|
|
||
| def get_obs_mode() -> str: | ||
| """Unset/empty/unrecognized -> ``dd_only`` (current behavior).""" | ||
| raw = os.getenv("SGP_OBS_MODE") | ||
| if not raw: | ||
| return _DEFAULT_MODE | ||
| mode = raw.strip().lower() | ||
| return mode if mode in _VALID_MODES else _DEFAULT_MODE | ||
|
|
||
|
|
||
| def _lgtm_ids() -> Optional[Tuple[str, str]]: | ||
| try: | ||
| from opentelemetry import trace | ||
| except ImportError: | ||
| return None | ||
| ctx = trace.get_current_span().get_span_context() | ||
| if ctx and ctx.is_valid: | ||
| return format(ctx.trace_id, "032x"), format(ctx.span_id, "016x") | ||
| return None | ||
|
|
||
|
|
||
| def _ddtrace_ids() -> Optional[Tuple[str, str]]: | ||
| try: | ||
| from ddtrace import tracer | ||
| except ImportError: | ||
| return None | ||
| 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 | ||
|
|
||
|
|
||
| def obs_correlation() -> Dict[str, str]: | ||
| """Return ``{"obs.trace_id": ..., "obs.span_id": ...}`` for the active | ||
| observability context, or ``{}`` if none is active. | ||
|
|
||
| Never fabricates ids -- this is a correlation tag, not the span's id. | ||
| """ | ||
| mode = get_obs_mode() | ||
| if mode == LGTM: | ||
| ids = _lgtm_ids() | ||
| elif mode == DUAL: | ||
| ids = _lgtm_ids() or _ddtrace_ids() | ||
| else: # dd_only | ||
| ids = _ddtrace_ids() | ||
|
|
||
| if not ids: | ||
| return {} | ||
| return {"obs.trace_id": ids[0], "obs.span_id": ids[1]} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| from agentex.types.span import Span | ||
| from agentex.lib.utils.logging import make_logger | ||
| from agentex.lib.utils.model_utils import recursive_model_dump | ||
| from agentex.lib.core.tracing.obs_ids import obs_correlation | ||
| from agentex.lib.core.tracing.span_error import set_span_error | ||
| from agentex.lib.core.tracing.span_queue import ( | ||
| SpanEventType, | ||
|
|
@@ -79,6 +80,12 @@ def start_span( | |
|
|
||
| serialized_input = recursive_model_dump(input) if input else None | ||
| serialized_data = recursive_model_dump(data) if data else None | ||
| # Tag the business span with the active observability trace_id/span_id | ||
| # (OTel/ddtrace) so it can be correlated to the per-turn obs trace. The | ||
| # business trace_id stays the run-level task id -- see obs_ids.py. | ||
| obs = obs_correlation() | ||
| if obs: | ||
| serialized_data = {**(serialized_data or {}), **obs} | ||
|
Comment on lines
+86
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Prompt To Fix With AIThis 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. |
||
| id = str(uuid.uuid4()) | ||
|
|
||
| span = Span( | ||
|
|
@@ -229,6 +236,12 @@ async def start_span( | |
|
|
||
| serialized_input = recursive_model_dump(input) if input else None | ||
| serialized_data = recursive_model_dump(data) if data else None | ||
| # Tag the business span with the active observability trace_id/span_id | ||
| # (OTel/ddtrace) so it can be correlated to the per-turn obs trace. The | ||
| # business trace_id stays the run-level task id -- see obs_ids.py. | ||
| obs = obs_correlation() | ||
| if obs: | ||
| serialized_data = {**(serialized_data or {}), **obs} | ||
| id = str(uuid.uuid4()) | ||
|
|
||
| span = Span( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx.span_id or 0falls back to0whenspan_idisNoneor zero, which formats to"0000000000000000"— the OTel invalid/null sentinel. This produces a correlation pair whereobs.trace_idis populated butobs.span_idis the null sentinel, which consumers may interpret as "no active span." Ifspan_idis absent, skipping the correlation entirely is more correct.Prompt To Fix With AI
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!