Skip to content

feat(tracing): add W3C distributed tracing and incremental handler batching - #225

Open
pradystar wants to merge 6 commits into
mainfrom
feat/w3c-trace-context-migration
Open

feat(tracing): add W3C distributed tracing and incremental handler batching#225
pradystar wants to merge 6 commits into
mainfrom
feat/w3c-trace-context-migration

Conversation

@pradystar

Copy link
Copy Markdown
Collaborator

Summary

This PR completes the SDK’s W3C distributed-tracing migration and satisfies both distributed-tracing requirements:

  • DT.A — Distributed context propagation: SDK-native telemetry, native OpenTelemetry spans, and supported upstream OTel HTTP instrumentation can participate in the same trace across service boundaries.
  • DT.B — Incremental span delivery: Completed native-handler operations enter the existing OpenTelemetry BatchSpanProcessor when their callbacks end without waiting for the entire logical trace to complete.

Both automatic and explicit propagation remain supported.

Detailed requirements mapping, lifecycle behavior, compatibility analysis, deletion rationale, and validation evidence are available in the detailed review context PR_REVIEW_DETAILS.md

What changed

Standard W3C propagation

Distributed context now uses:

  • traceparent
  • tracestate
  • W3C baggage

This replaces the proprietary Splunk-AO-Trace-ID and Splunk-AO-Parent-ID propagation format and allows Splunk AO telemetry to interoperate with upstream OpenTelemetry instrumentation and non-Python services.

Automatic distributed tracing

Applications can configure supported automatic instrumentation with:

from fastapi import FastAPI
from splunk_ao import configure_distributed_tracing

app = FastAPI()
tracer_provider = configure_distributed_tracing(app=app)

This:

  • Creates or accepts a concrete TracerProvider.
  • Registers the Splunk AO span processor once.
  • Enables installed supported HTTP instrumentors.
  • Instruments the supplied FastAPI or Starlette application.
  • Returns the application-owned provider for normal shutdown.
  • Does not replace the process-global tracer provider.

Install the optional dependencies with:

pip install "splunk-ao[distributed-tracing]"

Automatic incoming extraction is supported for FastAPI and Starlette.

Automatic outgoing injection is supported for:

  • Requests
  • HTTPX sync and async clients
  • aiohttp

Applications using other frameworks or custom transports can register the corresponding upstream OTel instrumentor or continue using explicit propagation.

Explicit propagation remains supported

Existing explicit integrations can continue to use:

from splunk_ao import get_tracing_headers

Incoming context can continue to use extract_tracing_context() or TracingMiddleware.

These APIs now use the same W3C context as automatic instrumentation. The explicit approach is not deprecated.

Incremental native-handler completion

LangChain, CrewAI, Google ADK, and OpenAI Agents operations now enter the existing span-processing pipeline when their individual callbacks end.

A completed child can therefore become eligible for scheduled or size-based BSP export while its parent agent or workflow remains active.

This does not mean every callback causes an immediate network request. The existing BatchSpanProcessor still determines network-export timing according to standard BSP configuration.

No per-trace flush() is required, and flush() does not end active work.

Control-plane HTTP suppression

SDK-owned authentication, health-check, routing, CRUD, token-refresh, and related HTTP operations are scoped under standard OTel HTTP suppression.

This prevents SDK control-plane calls from appearing as unrelated application GET or POST traces when automatic HTTP instrumentation is enabled.

Suppression is scoped to the SDK request and does not suppress application HTTP traffic.

Compatibility guarantees

This change preserves:

  • Existing single-service parent-child relationships.
  • Existing local trace structure with and without distributed tracing.
  • Logger, @log, and native handler instrumentation.
  • SDK-native and caller-owned OTel instrumentation.
  • Existing standalone and O11y deployment routing.
  • Caller ownership of caller-created providers.
  • The rule that the internal trace envelope is not exported as a span.
  • The behavior that flush() drains completed work without ending active operations.
  • ingestion_hook whole-tree compatibility.
  • flush_on_chain_end compatibility.

Distributed tracing adds remote ancestry without otherwise reshaping the application’s local telemetry tree.

start_new_trace remains a handler ownership option, not a distributed-tracing switch. Users do not need to change its default value to enable distributed tracing.

Session propagation and privacy

An explicit session is propagated through W3C baggage as:

gen_ai.conversation.id

The implementation does not propagate project, Agent Stream, agent, experiment, application, routing, endpoint, authentication, prompt, response, or embedding data through baggage.

Intentional breaking changes

The migration removes the proprietary propagation surface:

  • trace_id= and span_id= arguments on SplunkAOLogger. Note that all example might not be updated and examples might be stale. The examples will be fixed in a future PR.
  • The logger instance method get_tracing_headers().
  • Splunk-AO-Trace-ID and Splunk-AO-Parent-ID.

Applications should use the module-level W3C helper:

from splunk_ao import get_tracing_headers

or enable automatic propagation through configure_distributed_tracing().

Removed delivery path

The previous distributed-mode REST task handler maintained a second queue, dependency-ordering system, retry lifecycle, and shutdown path.

Normally exported telemetry now follows one processing path:

completed operation
→ SpanSink
→ OpenTelemetry BatchSpanProcessor
→ deployment-aware OTLP exporter

The obsolete task handler and its dedicated tests were removed. This does not remove any logger, decorator, or native framework-handler support.

Validation summary

Completed validation includes:

  • Full root suite: 2,163 passed, 4 skipped
  • LangChain and CrewAI compatibility: 202 passed
  • Google ADK suite: 252 passed
  • A2A suite: 68 passed
  • Final focused root regression set: 212 passed
  • Automatic instrumentation tests: 20 passed
  • Control-plane suppression and O11y HTTP tests: 41 passed
  • Ruff lint and formatting checks: passed
  • Mypy: 118 source files passed
  • Poetry lock consistency: passed
  • git diff --check: passed

Live standalone validation succeeded for both automatic and explicit cross-service distributed tracing.

@pradystar pradystar changed the title feat(tracing): add automatic W3C distributed tracing and incremental handler batching feat(tracing): add W3C distributed tracing and incremental handler batching Aug 13, 2026

@fercor-cisco fercor-cisco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by the Astra agent (claude-sonnet-5). It may contain mistakes.

Verdict: request_changes — A concrete id-reuse aliasing bug in the new automatic-instrumentation bookkeeping (http_instrumentation.py) can cause silent double-instrumentation, false conflict errors, or unbounded growth of module-level state; there are also stale/incorrect docs left over from the removed proprietary header scheme.

General Comments

  • 🟠 major (bug): http_instrumentation.py tracks instrumentation state using id(app) / id(tracer_provider) as keys in plain module-level dict/set structures (_client_provider_ids, _instrumented_apps) without holding any reference (strong or weak) to the underlying objects. _configured_providers correctly uses a WeakSet to avoid this problem, but the other two do not. Once an app/tracer_provider is garbage-collected, CPython can and does reuse its id() for an unrelated object; a subsequent unrelated app/provider can then collide with a stale entry, causing _instrument_app/_validate_client_ownership to silently skip instrumentation (treating the new object as 'already instrumented') or to raise a false 'already instrumented through Splunk AO with another tracer provider' RuntimeError for objects that never actually conflicted. This is realistic in any process that creates many short-lived FastAPI apps or TracerProviders (tests, multi-tenant setups, hot-reload dev servers) and is also a slow memory leak since entries are never evicted. Recommend keying by id(obj) only while also holding a weakref to the object (or using WeakValueDictionary/an id->weakref map with a weakref.finalize cleanup callback) so aliasing cannot occur and stale entries are pruned automatically, matching the pattern already used for _configured_providers.
  • 🟡 minor (documentation): splunk-ao-migration-tool/README.md (section 6, 'HTTP Tracing Headers') still instructs users to migrate X-Galileo-Trace-ID/X-Galileo-Parent-ID to Splunk-AO-Trace-ID/Splunk-AO-Parent-ID, and says 'The get_tracing_headers() function return value now uses the new header names.' This PR removes Splunk-AO-Trace-ID/Splunk-AO-Parent-ID entirely in favor of W3C traceparent/tracestate, so this guidance is now wrong and will mislead migrating users into propagating headers the SDK no longer understands. Please update this section to point at the W3C headers (or get_tracing_headers()'s new W3C-based output) in the same change, per AGENTS.md's requirement to update docs when propagation/telemetry paths change.

Follow-ups

Suggested follow-up work that could be tracked as Jira tickets:

  • src/splunk_ao/decorator.py:1343-1373: _session_id_context (now defined in session_context.py) is a single global ContextVar shared across all SplunkAOLogger instances within a context, rather than being scoped per-logger. _set_active_session_id/set_session_context therefore make one logger's set_session/clear_session call affect get_effective_session_id() for any other logger sharing the same async/thread context (e.g. two loggers for different agent streams created in the same request). This may be intentional per the 'one explicit session, request-local' design, but is worth a design discussion/doc note since it's a behavior change from the previous per-instance self.session_id semantics.
  • src/splunk_ao/middleware/tracing.py:16-17: The module docstring's usage example calls logger.conclude(output=str(result)) twice in a row. If unintentional this is a confusing copy-paste artifact in documentation; if intentional (concluding the workflow span then the trace) it deserves a comment explaining why, since readers copying the example verbatim may not realize two distinct steps are being concluded.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/splunk_ao/http_instrumentation.py:2441-2443 (line not in diff)

🟠 major (bug): _client_provider_ids and _instrumented_apps key on id(tracer_provider)/id(app) without keeping the objects alive or otherwise validating identity. After the underlying object is garbage collected, Python may reuse the same id for a new, unrelated app/provider, causing silent skip-instrumentation or false ownership-conflict errors for that new object. Track a weakref.ref (or use weakref.finalize to prune the entry when the original object dies) alongside the id, or switch to WeakValueDictionary/WeakSet keyed by the object itself the way _configured_providers already does.

Suggested change
_client_provider_ids: dict[str, tuple[int, "weakref.ReferenceType[Any]"]] = {}
_instrumented_apps: WeakSet[Any] = WeakSet() # store (app, provider, framework) via a wrapper holding weakrefs

🤖 Generated by the Astra agent

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major (bug): Module-level _client_provider_ids: dict[str, int] and _instrumented_apps: set[tuple[int, int, str]] store raw id() values with no reference back to the objects, unlike _configured_providers which is a WeakSet. This is an id-reuse aliasing hazard and an unbounded-growth leak; see PR-level comment for details and a suggested fix (weak references / finalizers keyed alongside the ids).

🤖 Generated by the Astra agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants