Repository navigation
fix: release GenAINestedClientSpanProcessor state with the parent span - #970
syed-ahsan-ishtiaque wants to merge 3 commits into
Conversation
The processor recorded "parent has a same-operation inference child" when an inference CLIENT child ended, and removed the record only when the parent ended locally. Records were never removed when the parent was remote, ended before its child, or was garbage-collected without ending, so each such model call left an entry until shutdown. Keep state only for local parents that are still recording when a child starts, and release it with the parent: when the parent ends, or when it is garbage-collected. Remote and already-ended parents get no state, and a child that ends after its parent does not recreate it. Python keys state by trace and span ID with a weakref.finalize on the parent; the finalizer only queues the key, and every lifecycle call drains the queue, so garbage collection while the lock is held cannot deadlock. Only CLIENT children create state, since span kind is fixed at start. JS keeps state in WeakMaps keyed by the span objects, and does not filter by kind at start because VercelAISpanProcessor promotes spans in onEnd.
…rant Garbage collection can run while the processor holds its lock and end a span on the same thread, for example by closing an abandoned streaming generator that holds a span open. That span's on_end then waited forever on the non-reentrant lock. Build the finalizer, set and keys before taking the lock, and use an RLock so a re-entrant on_end cannot deadlock. Also re-check the parent after inserting its state, so a parent that ended on another thread in between does not keep an entry.
|
Claude finished @syed-ahsan-ishtiaque's task in 2m 41s —— View job Claude PR Review
SummaryNice fix. The redesign cleanly addresses the three leak paths called out in the description (remote parent, parent-ends-first, parent-never-ends), and the re-entrancy reasoning is tight. What stands out as well-handled:
Minor observations (non-blocking, no change requested):
No changes required. LGTM. |
| if span.kind != SpanKind.CLIENT: | ||
| def on_start(self, span: Span, parent_context: Optional[Context] = None) -> None: | ||
| # Only a CLIENT child can demote its parent. Span kind is fixed at start in this SDK. | ||
| if span.kind != SpanKind.CLIENT or span.parent is None or span.parent.is_remote: |
There was a problem hiding this comment.
Kind is assumed fixed at start
on_start skips non-CLIENT children (span.kind != SpanKind.CLIENT, "Span kind is fixed at start in this SDK"). Main checked kind at end, and the JS version (#589) still does, because VercelAISpanProcessor promotes INTERNAL to CLIENT in onEnd.
This is fine today, since no distro code promotes a span to CLIENT. But if a future processor does what the Vercel one does, the outer span will silently stop being demoted, with no error and no failing test.
Suggestion: add a comment at that check explaining the assumption, plus a small guard test that scans the distro source for _kind assignments and compares them with an allowlist. Today there are three:
aws_lambda_span_processor.py→ SERVER- this processor → INTERNAL
- the
opentelemetry_lite_sdk.pyconstructor
Any new _kind assignment would then fail CI and point back here.
There was a problem hiding this comment.
Reviewed current head c7e0ee4 against main and compared it with #971.
The original remote-parent, late-child, and abandoned-parent cases are fixed, and normal two-/three-level demotion works for all four inference operations. One cleanup issue is recorded inline: a parent from another tracer provider can finish without notifying this processor, leaving its state while the ended parent object remains reachable. This also introduces retention outside GenAI traffic: 1,000 completed requests with foreign SERVER parents and ordinary local HTTP CLIENT children retain 1,000 entries here, versus zero on main and #971.
I also verified the reconstructed-local-context trade-off documented in the description: wrapping an active local parent's span context in NonRecordingSpan leaves both spans CLIENT here; main and #971 demote the outer span. This is a comparison trade-off already disclosed by this PR.
Validation: 2,196 core distro tests and 351 subtests passed (25 skipped); Black, isort, and flake8 passed. Added independent baseline/fix lifecycle comparisons, all four inference operations, six-thread GC/end stress (1,600 span pairs and 2,502 kind assertions), and the streaming-generator GC regression. Optional framework suites were not run locally.
| tracked = key in self._parents | ||
| if tracked: | ||
| return | ||
| finalizer = weakref.finalize(parent, self._collected.append, key) |
There was a problem hiding this comment.
[P2] Avoid tracking parents whose end callbacks this processor cannot observe
A recording SDK parent from another TracerProvider passes the checks above, so this registers it in _parents. When that parent ends, it calls its own provider's processors; this processor never receives the parent's on_end. The entry therefore remains while the ended parent object is reachable, since its finalizer observes collection rather than span end.
This also introduces retention for ordinary non-GenAI traffic. I reproduced 1,000 completed requests using SERVER parents from a second provider without this processor and local HTTP CLIENT children; neither span had any GenAI attributes. Retaining the ended parent objects, then running gc.collect(), force_flush(), and 1,000 unrelated completed spans leaves 1,000 entries here, versus zero on main and #971. Releasing the parent objects finally clears the entries.
The same ownership gap also leaves the original GenAI case partially unfixed: completed foreign-provider parents with CLIENT/chat children retain entries, including when the parent ends first. Please restrict tracking to parents whose lifecycle this processor observes, or provide cleanup for foreign-provider parents, and add cross-provider tests for both GenAI and ordinary HTTP children.
Why
GenAINestedClientSpanProcessordemotes the outer span of nested GenAI inference CLIENT spans (a framework span wrapping the model SDK span with the samegen_ai.operation.name) to INTERNAL. To do that it records "this parent has a same-operation inference child" when the child ends, and removes the record when the parent ends.The record was never removed in three cases, so each such model call left one entry until shutdown:
Reproduced on main: 1,000 model calls in each case left 3,000 entries after the calls ended. GC and
force_flushdid not remove them. This affects every application with agent observability enabled today, because that is when the processor is installed.What changes
Trade-off: a child whose context was rebuilt with only the parent's span context (
trace.set_span_in_context(NonRecordingSpan(...))/trace.setSpanContext()) from a local parent can no longer demote that parent, because the parent span object is not reachable from the context. Main handled this case by ID, but it is the same mechanism that leaked. Instrumentations pass the real parent span in the context, so this case is unusual.How it works (Python)
on_start: for a CLIENT child whose parent in the context is a local SDK span that is still recording, add an entry for the parent, keyed by trace and span ID, with aweakref.finalizeon the parent. Only CLIENT children create entries, since span kind is fixed at start.on_end: pop the span's own entry (demote it if a child recorded the same operation), and record this span's operation on its parent's entry if the parent is still tracked. No entry is created for a parent that is not tracked.deque. Every lifecycle call drains the deque, so a collected parent's entry is removed on the next span start or end.RLock, so a re-entranton_endcannot deadlock. An earlier version of this fix built them under a plainLockand deadlocked in a stress test; the regression test below covers it.Testing
force_flush; a finalizer during GC with the lock held does not deadlock;on_endre-entered on the same thread with the lock held does not deadlock; shutdown clears state and detaches finalizers.gc.set_threshold(50)and abandoned streaming generators, which deadlocked the earlier version of this fix and passes now.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.