feat(traces): span pipeline - #953
turnipdabeets wants to merge 2 commits into
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-15 22:09:38 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.
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.
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.
💡 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