feat(tracing): add W3C distributed tracing and incremental handler batching - #225
feat(tracing): add W3C distributed tracing and incremental handler batching#225pradystar wants to merge 6 commits into
Conversation
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 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-leveldict/setstructures (_client_provider_ids,_instrumented_apps) without holding any reference (strong or weak) to the underlying objects._configured_providerscorrectly uses aWeakSetto avoid this problem, but the other two do not. Once anapp/tracer_provideris garbage-collected, CPython can and does reuse itsid()for an unrelated object; a subsequent unrelated app/provider can then collide with a stale entry, causing_instrument_app/_validate_client_ownershipto 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 byid(obj)only while also holding aweakrefto the object (or usingWeakValueDictionary/an id->weakref map with aweakref.finalizecleanup 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 migrateX-Galileo-Trace-ID/X-Galileo-Parent-IDtoSplunk-AO-Trace-ID/Splunk-AO-Parent-ID, and says 'The get_tracing_headers() function return value now uses the new header names.' This PR removesSplunk-AO-Trace-ID/Splunk-AO-Parent-IDentirely in favor of W3Ctraceparent/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 (orget_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_contexttherefore make one logger'sset_session/clear_sessioncall affectget_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-instanceself.session_idsemantics.src/splunk_ao/middleware/tracing.py:16-17: The module docstring's usage example callslogger.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.
There was a problem hiding this comment.
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.
| _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
There was a problem hiding this comment.
🟠 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
Summary
This PR completes the SDK’s W3C distributed-tracing migration and satisfies both distributed-tracing requirements:
BatchSpanProcessorwhen their callbacks end without waiting for the entire logical trace to complete.Both automatic and explicit propagation remain supported.
What changed
Standard W3C propagation
Distributed context now uses:
This replaces the proprietary
Splunk-AO-Trace-IDandSplunk-AO-Parent-IDpropagation 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:
This:
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:
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_headersIncoming context can continue to use
extract_tracing_context()orTracingMiddleware.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, andflush()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:
Distributed tracing adds remote ancestry without otherwise reshaping the application’s local telemetry tree.
start_new_traceremains 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.idThe 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=andspan_id=arguments onSplunkAOLogger. Note that all example might not be updated and examples might be stale. The examples will be fixed in a future PR.get_tracing_headers().Splunk-AO-Trace-IDandSplunk-AO-Parent-ID.Applications should use the module-level W3C helper:
from splunk_ao import get_tracing_headersor 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:
Live standalone validation succeeded for both automatic and explicit cross-service distributed tracing.