feat(traces): span pipeline - #953
Conversation
Prompt To Fix All With AI### Issue 1
posthog/tracing/_pipeline.py:148-156
**Shutdown race creates spans**
If `start_span()` passes the `_closed` check just before another thread calls `close()`, it can reserve a new live span after `close()` clears the registry and closes the exporter. Ending that span then calls `enqueue()` on the closed exporter, which defeats shutdown and can lose the span. Re-check `_closed` while holding the lock before reserving the ID.
### Issue 2
posthog/tracing/_pipeline.py:94-95
**Drop logging can escape**
`warn_if_due()` runs in `start_span()`'s `finally` block outside its exception handler. If an application logging handler raises, that exception escapes into the application and can override the span being returned, breaking the method's non-raising contract. Contain failures from `warn_if_due()` within `start_span()`.
### Issue 3
posthog/tracing/_pipeline.py:153-156
**Failed starts leak capacity**
The live-span ID is reserved before context extraction and span construction, but the broad exception path returns an inert span without removing it. If a context mapping raises from `get()`, the orphaned entry consumes capacity until age eviction and can block tracing for the configured maximum age. Remove the reservation whenever a post-reservation operation fails.
### Issue 4
posthog/tracing/_drops.py:46-49
**Fork inherits warning throttle**
Fork reinitialization clears pending drop counts but retains the parent's `_last_warning_at`. If the child drops a span shortly after a parent warning, its warning is suppressed; if no later tracing operation occurs, that drop is never reported. Reset `_last_warning_at` with the other process-local state.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(traces): span pipeline" | Re-trigger Greptile |
posthog-python Compliance ReportDate: 2026-09-17 03:42:58 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
3c1c3d2 to
8101991
Compare
8101991 to
7d8fce9
Compare
7d8fce9 to
8abb393
Compare
8abb393 to
3ad9128
Compare
Prompt To Fix All With AI### Issue 1
posthog/tracing/_drops.py:47-50
**Failed warnings lose counts**
`warn_if_due()` clears the pending count and reasons before calling `log.warning()`. If a logging handler raises, the exception is suppressed but the drop report is permanently lost. Restore the count and reasons when logging fails so a later call can retry while preserving the non-raising contract.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (2): Last reviewed commit: "fix(traces): close the shutdown and fail..." | Re-trigger Greptile |
dustinbyrne
left a comment
There was a problem hiding this comment.
Please preserve explicit service identity when resource attributes collide. At posthog/tracing/_config.py:137–140, the resolver replaces service_name/service_version before the encoder's identity protection runs. A shared resource map can therefore relabel every span despite the explicit service configuration.
For example, service_name="checkout" plus resource_attributes={"service.name": "shared-default"} currently resolves to shared-default; the explicit name should remain checkout. Please reverse that precedence and update the test that currently asserts resource attributes win.
Regression sketch (not executed): resolve conflicting explicit name/version and resource attributes, build the resource envelope, and assert the explicit identity survives while unrelated attributes remain. As a negative control, call the existing encoder directly with the same explicit identity and conflicting map.
Two non-blocking follow-ups:
_pipeline.py:92: the fallback diagnostic can itself escapestart_spanwhen DEBUG is enabled and an application logging handler raises. A malformed inbound parent triggers a diagnostic, then another unguarded diagnostic in the exception handler. Make that fallback log best-effort._pipeline.py:263–275: age eviction removes accounting but leaves the retained handle able to emittraceparentand create recorded children. Consider bounded/weak handle invalidation so it becomes a no-op after eviction.
These concern the private tracing foundation; no shipped Client tracing regression is claimed.
AI-assisted review with independent validation of the findings, source and existing CI; no new tests were run.
3ad9128 to
57542a6
Compare
|
@dustinbyrne Resource attributes winning is on purpose. Node does the same ( On the two notes:
|
dustinbyrne
left a comment
There was a problem hiding this comment.
Thanks for the explanation. I checked Node traces and metrics: resource attributes do take precedence there. I over-weighted the spec wording and should have checked that convention before asking Python to differ. I withdraw the service-identity change request.
I also withdraw the custom-handler logging and age-eviction suggestions: neither warrants extra machinery in this PR. The shared spec wording can be clarified separately rather than changing Python alone. Approving.
AI-assisted follow-up source review, including Node parity and the rebased transport change; no new tests run locally.
jzhu13
left a comment
There was a problem hiding this comment.
Reviewed against traces/04-transport. Tests pass at the head, ruff is clean, parent precedence is correct, no user code runs under _lock, and start_span genuinely never raises. One item I would fix before merge.
Blocking
posthog/tracing/_pipeline.py:223(and_span.py:135for the inert path) an empty or whitespaceparentstring suppresses the active-span default and starts a new root. Reproduced: insidewith start_span("outer"),parent=""yieldsparent_span_id=Nonein a different trace; with the client disabled and a pass-through active,parent=""returnsNoopSpanwhileparent=Nonereturns aPassThroughSpan.request.headers.get("traceparent", "")andrequest.META.get("HTTP_TRACEPARENT", "")are common idioms, and a two-element list in the same position does fall back to the active span, so the two "unusable" paths disagree. Suggest treating a blank string asNoneaftertraceparent_header(parent), in both places, and keeping the non-empty-malformed path as a new root.
Non-blocking, recommended
posthog/tracing/_pipeline.py:267age eviction drops a legitimately long span that its caller does end, and its children export as orphans. Reproduced with a root older thanmax_span_age. The default is one hour; a batch-job wrapper span over an hour is realistic. The leak-recovery goal is served equally well by sweeping only whenlen(_live_spans) >= max_live_spans.posthog/tracing/_config.py:110unknown keys are silently ignored while every known-but-invalid value warns. A mistypedbefore_span_sendkey in #956 means spans export unscrubbed with no signal, which is exactly what #957's fail-closed comment is trying to prevent.log.warning("Ignoring unknown traces option(s): %s", ...).posthog/tracing/_pipeline.py:262join keys are neither stringified nor null-checked the waycapture()does withstringify_id. An int distinct id becomes an OTLPintValueforposthogDistinctId, andif value:drops a distinct id of0.- Nits: a non-Mapping return from
get_contextmakes the span inert because thetrycovers the call but notcontext.get; a child of a refused (at-limit) local parent is markedparent_is_remote=Trueon the wire;FakeTimer/fake_timersship here with no consumer until #954, re-exported through an__all__hack rather thanposthog/test/tracing/conftest.py; duplicate assert attest_pipeline.py:46.
Reviewed with Claude Code (Claude Fable 5.1). Behaviors above were reproduced by probe against this branch head.
57542a6 to
19ea6ef
Compare
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
|
Thanks. Fixed in 19ea6ef:
|
19ea6ef to
78b6318
Compare
|
Follow-up, 78b6318: an unusable explicit parent (two-value list, foreign span object) now starts a new trace like a malformed header does, instead of falling back to the active span. Ready for another look. |
Adds span creation and the end-of-span gates. PostHogTraces resolves a span's parent (an explicit traceparent string or handle, else the active span, else a new trace), attaches the posthogDistinctId and sessionId join keys from the request context, bounds live spans by count and by age so a leak cannot disable tracing, and hands each ended span to an exporter unless the client was disabled. The `traces` option is validated key by key, falling back to the documented default with a warning. Dropped spans are counted per reason and reported at most once per flush interval. The export queue arrives in the next change; this one runs against a stand-in. Not reachable from the client. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkZAsCciW4PV8ZdcCHmAbA
Re-check closed under the lock so a close() that lands mid-start cannot reserve a live span after the registry was cleared. Release the reserved slot when building the span fails, so it does not wait for age eviction. Contain a raising logging handler inside the drop warning, and reset the warning throttle in a forked child with the rest of its state.
…only at the bound
headers.get("traceparent", "") started a new root instead of nesting under
the active span. Aged spans are now swept only when the live-span bound is
reached, so a long span that does end is exported. Unknown traces options
warn instead of vanishing, join keys are stringified and a 0 id is kept,
and a non-mapping context no longer makes the span inert.
A two-value header list or a foreign span object fell back to the active span while a malformed header string started a new root. Both now start a new trace: an explicit parent the caller named must not silently attach the span to a trace they did not.
78b6318 to
6add5dd
Compare
💡 Motivation and Context
Adds span creation and the end-of-span gates.
PostHogTracesresolves a span's parent (an explicittraceparentstring or handle, else the active span, else a new trace), attaches theposthogDistinctIdandsessionIdjoin keys from the request context, bounds live spans by count and by age so a leak cannot disable tracing, and hands each ended span to an exporter unless the client was disabled.The
tracesoption is validated key by key, falling back to the documented default with a warning. Dropped spans are counted per reason and reported at most once per flush interval.The export queue arrives in the next PR; this one runs against a stand-in. Not reachable from the client yet.
Stack (PR 5 of 9, based on
traces/04-transport):traces/01-ids-traceparenttraces/02-otlp-encodingtraces/03-span-handlestraces/04-transporttraces/05-pipeline← this PRtraces/06-exporttraces/07-span-limitstraces/08-before-span-sendtraces/09-client-wiring💚 How did you test it?
Unit tests in
posthog/test/tracing/test_pipeline.pyandtest_config.pycover parent resolution, join keys, live-span bounds, option validation and drop reporting.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented with Claude Code (Claude Opus 5) against the traces spec, one commit per slice so each PR reviews on its own. Rebased onto main and opened as a stacked draft in a later Claude Code session (Claude Fable 5.1).
🤖 Generated with Claude Code
https://claude.ai/code/session_012o7CtHLfcypjmXL7g9ZGRC