fix(tracing): address PR #484 review — error status, obs fallback, handle leak, dd_only Temporal backend

Four review-comment fixes for the business<->obs correlation edge, each with unit
coverage:

- ADK span() CM recorded no error on a failing step, so a failed agent step closed
  a green obs span. Now sets span error (guarded so obs can't shadow the app
  exception) and re-raises. (tests/test_adk_tracing_span_error.py)

- In lgtm mode with no TracerProvider, open_obs_span returned a handle with empty
  correlation, suppressing the ambient obs_correlation() fallback -> business span
  got no obs_* ids. Now bails to None so the caller falls back.
  (tests/test_obs_span_fallback.py)

- Obs-handle registry could leak (registration-order + start-without-end via public
  API). Processor hooks now swallow (obs must never crash the app path) and the
  registry is a bounded OrderedDict that evicts+closes the oldest.
  (tests/test_obs_handle_registry.py)

- On the Temporal path the ambient span is the OTel interceptor span regardless of
  SGP_OBS_MODE, but tag/read branched on mode -> in the default dd_only they
  tagged/read an unrelated ddtrace span. Added prefer_otel (OTel-first, ddtrace
  fallback) via a shared _begin_obs helper used by both start_span paths.
  (tests/test_temporal_obs_backend.py)

Also: ObsSpanHandle.close() instead of reaching into _close; Iterator from
collections.abc.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: NiteshDhanpal

src/agentex/lib/adk/_modules/tracing.py

@@ -20,6 +20,7 @@
     TracingActivityName,
 )
 from agentex.lib.core.tracing.tracer import AsyncTracer
+from agentex.lib.core.tracing.span_error import set_span_error
 from agentex.lib.core.harness.types import TurnUsage
 from agentex.types.span import Span
 from agentex.lib.utils.logging import make_logger
@@ -236,6 +237,24 @@ async def span(
         )
         try:
             yield span
+        except Exception as exc:
+            # Record the failure on the span so the obs span reflects the error
+            # instead of a false green. Agents use THIS context manager (not
+            # AsyncTrace.span, which is the only other place set_span_error is
+            # called), so without this a failed step closes green. end_span (in
+            # finally) reads it via get_span_error and propagates it to
+            # close_obs_span. Stored on span.data, so it round-trips through the
+            # END_SPAN activity on the Temporal path too.
+            #
+            # Guard set_span_error itself: it's obs work and must never replace
+            # the app's exception on the way out. We always re-raise the ORIGINAL
+            # exc regardless.
+            if span:
+                try:
+                    set_span_error(span, exc)
+                except Exception:  # pragma: no cover - obs must not break app path
+                    pass
+            raise
         finally:
             if span:
                 await self.end_span(

src/agentex/lib/core/temporal/services/temporal_task_service.py

@@ -1,8 +1,9 @@
 from __future__ import annotations
 
-from typing import Any, Iterator
+from typing import Any
 from datetime import timedelta
 from contextlib import contextmanager
+from collections.abc import Iterator
 
 from agentex.types.task import Task
 from agentex.types.agent import Agent

src/agentex/lib/core/tracing/obs_ids.py

@@ -69,7 +69,7 @@ def _ddtrace_ids() -> Optional[Tuple[str, str]]:
     return None
 
 
-def obs_correlation() -> Dict[str, str]:
+def obs_correlation(prefer_otel: bool = False) -> Dict[str, str]:
     """Return ``{"obs_trace_id": ..., "obs_span_id": ...}`` for the active
     observability context, or ``{}`` if none is active.
 
@@ -79,10 +79,18 @@ def obs_correlation() -> Dict[str, str]:
     dotted) keep them addressable via Postgres JSON paths
     (``operation_metadata->>'obs_trace_id'``).
 
+    ``prefer_otel``: on the Temporal path the active span is the temporalio OTel
+    ``TracingInterceptor`` span regardless of ``SGP_OBS_MODE``, so callers there
+    read OTel first (falling back to ddtrace) -- otherwise the default ``dd_only``
+    mode would read ids for an unrelated ddtrace trace, not the activity span.
+
     Never fabricates ids -- this is a correlation tag, not the span's id.
     """
     try:
-        ids = _lgtm_ids() if get_obs_mode() == LGTM else _ddtrace_ids()
+        if prefer_otel:
+            ids = _lgtm_ids() or _ddtrace_ids()
+        else:
+            ids = _lgtm_ids() if get_obs_mode() == LGTM else _ddtrace_ids()
     except Exception:  # obs must never fail an app call
         return {}
 

src/agentex/lib/core/tracing/obs_span.py

@@ -56,6 +56,11 @@ def __init__(
         self.correlation = correlation
         self._close = close
 
+    def close(self, error: Optional[Dict[str, str]] = None) -> None:
+        """Run the backend-specific closer (detach+end for OTel, finish for
+        ddtrace). ``error`` marks the obs span failed so it isn't a false green."""
+        self._close(error)
+
 
 def _hex_ids(trace_id: int, span_id: int) -> Dict[str, str]:
     """W3C-hex form: 32-hex trace, 16-hex span."""
@@ -82,7 +87,19 @@ def _open_otel_span(
             span.set_attribute(_ATTR_BUSINESS_TRACE_ID, business_trace_id)
         token = context.attach(trace.set_span_in_context(span))
         sc = span.get_span_context()
-        correlation = _hex_ids(sc.trace_id, sc.span_id) if (sc and sc.is_valid) else {}
+        if not (sc and sc.is_valid):
+            # No real TracerProvider installed (lgtm mode but the agent has no
+            # OTel provider yet): the proxy tracer hands back a NonRecordingSpan
+            # with an invalid context. Returning a handle with empty correlation
+            # here would make the caller (trace.py) take obs_handle.correlation
+            # == {} and NEVER consult the obs_correlation() ambient fallback --
+            # so the business span would get no obs_* ids at all, strictly worse
+            # than falling back. Detach the useless context, end the no-op span,
+            # and return None so the caller uses the ambient ids instead.
+            context.detach(token)
+            span.end()
+            return None
+        correlation = _hex_ids(sc.trace_id, sc.span_id)
 
         def _close(error: Optional[Dict[str, str]] = None) -> None:
             try:
@@ -124,11 +141,18 @@ def _open_ddtrace_span(
         # Datadog traces. Parenting to the active request/turn context rolls them
         # into one trace while obs_span_id stays distinct per step.
         span = tracer.start_span(name, child_of=ctx, activate=True)
+        if not span.trace_id:
+            # Symmetry with the OTel path: a handle carrying empty correlation
+            # would suppress the ambient obs_correlation() fallback in trace.py.
+            # (child_of=ctx normally guarantees a real trace_id, so this is
+            # belt-and-braces.) Finish the span and fall back to ambient ids.
+            span.finish()
+            return None
         if business_span_id:
             span.set_tag(_ATTR_BUSINESS_SPAN_ID, business_span_id)
         if business_trace_id:
             span.set_tag(_ATTR_BUSINESS_TRACE_ID, business_trace_id)
-        correlation = _hex_ids(span.trace_id, span.span_id) if span.trace_id else {}
+        correlation = _hex_ids(span.trace_id, span.span_id)
 
         def _close(error: Optional[Dict[str, str]] = None) -> None:
             try:
@@ -175,9 +199,44 @@ def open_obs_span(
         return None
 
 
+def _tag_otel_ambient(business_span_id: Optional[str], business_trace_id: Optional[str]) -> bool:
+    """Stamp the reverse tag onto the active OTel span. Returns True iff a valid
+    OTel span was found and tagged."""
+    try:
+        from opentelemetry import trace
+    except ImportError:
+        return False
+    span = trace.get_current_span()
+    if span is not None and span.get_span_context().is_valid:
+        if business_span_id:
+            span.set_attribute(_ATTR_BUSINESS_SPAN_ID, business_span_id)
+        if business_trace_id:
+            span.set_attribute(_ATTR_BUSINESS_TRACE_ID, business_trace_id)
+        return True
+    return False
+
+
+def _tag_ddtrace_ambient(business_span_id: Optional[str], business_trace_id: Optional[str]) -> bool:
+    """Stamp the reverse tag onto the active ddtrace span. Returns True iff a
+    ddtrace span was found and tagged."""
+    try:
+        from ddtrace.trace import tracer
+    except ImportError:
+        return False
+    span = tracer.current_span()
+    if span is not None:
+        if business_span_id:
+            span.set_tag(_ATTR_BUSINESS_SPAN_ID, business_span_id)
+        if business_trace_id:
+            span.set_tag(_ATTR_BUSINESS_TRACE_ID, business_trace_id)
+        return True
+    return False
+
+
 def tag_ambient_obs_span(
     business_span_id: Optional[str] = None,
     business_trace_id: Optional[str] = None,
+    prefer_otel: bool = False,
 ) -> None:
     """Stamp the reverse tag onto the CURRENTLY ACTIVE obs span -- without opening
     a new one.
@@ -188,26 +247,23 @@ def tag_ambient_obs_span(
     closed. Instead we lean on the span the Temporal OTel ``TracingInterceptor``
     already made active for this activity and just add
     ``agentex.business_span_id`` / ``agentex.business_trace_id`` so the obs -> business
-    pivot still works. Best-effort; never raises."""
+    pivot still works. Best-effort; never raises.
+
+    ``prefer_otel``: on the Temporal path the ambient span is the temporalio OTel
+    ``TracingInterceptor`` span REGARDLESS of ``SGP_OBS_MODE`` -- so callers there
+    pass ``prefer_otel=True`` to tag OTel first (falling back to ddtrace only if
+    no valid OTel span is active). Without this, the default ``dd_only`` mode would
+    tag an unrelated ddtrace span (or nothing) instead of the real activity span."""
     try:
+        if prefer_otel:
+            if _tag_otel_ambient(business_span_id, business_trace_id):
+                return
+            _tag_ddtrace_ambient(business_span_id, business_trace_id)
+            return
         if get_obs_mode() == LGTM:
-            from opentelemetry import trace
-
-            span = trace.get_current_span()
-            if span is not None and span.get_span_context().is_valid:
-                if business_span_id:
-                    span.set_attribute(_ATTR_BUSINESS_SPAN_ID, business_span_id)
-                if business_trace_id:
-                    span.set_attribute(_ATTR_BUSINESS_TRACE_ID, business_trace_id)
+            _tag_otel_ambient(business_span_id, business_trace_id)
         else:
-            from ddtrace.trace import tracer
-
-            span = tracer.current_span()
-            if span is not None:
-                if business_span_id:
-                    span.set_tag(_ATTR_BUSINESS_SPAN_ID, business_span_id)
-                if business_trace_id:
-                    span.set_tag(_ATTR_BUSINESS_TRACE_ID, business_trace_id)
+            _tag_ddtrace_ambient(business_span_id, business_trace_id)
     except Exception:  # pragma: no cover - best-effort; obs must never break a call
         pass
 
@@ -222,6 +278,6 @@ def close_obs_span(
     if handle is None:
         return
     try:
-        handle._close(error)
+        handle.close(error)
     except Exception:  # pragma: no cover - best-effort
         pass