feat(traces): wire tracing into the client - #957
Conversation
posthog-python Compliance ReportDate: 2026-09-17 03:42:54 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
|
Prompt To Fix All With AI### Issue 1
posthog/client.py:2685-2688
**Exit flush exceeds its deadline**
If thread creation is rejected during interpreter shutdown, this fallback runs the span flush synchronously. The exporter permits its first request even when no budget remains, and that request uses the client's timeout (15 seconds by default). Each client can therefore delay process exit well beyond the shared one-second deadline. Use a worker started before interpreter shutdown for exit flushing rather than falling back to synchronous network I/O, and test the rejected-thread path with a slow request.
### Issue 2
posthog/client.py:2514-2519
**Tracing initialization races with shutdown**
If shutdown runs after the initial state check but before the pipeline assignment, it sees `_traces is None` and completes without closing tracing. Initialization then publishes an open pipeline, and ending the span can start background exports after shutdown has returned. Subsequent shutdown calls skip cleanup because it is already marked complete. Coordinate pipeline initialization and shutdown with shared synchronization so shutdown cannot miss an in-progress initialization.
### Issue 3
posthog/client.py:2278-2281
**Fork cleanup restores inherited spans**
Setting the active ContextVar to `None` does not invalidate tokens held by inherited spans. When a process forks inside nested span blocks, exiting the inherited inner block in the child resets its old token and restores the parent process's outer span. Subsequent child spans then attach to that inherited span, violating fork isolation. Create a fresh active-span ContextVar in the child and rebind the pipeline to it, leaving inherited handles attached to the old variable. Add a nested-span fork test that checks parenting after the inner block exits.
---
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): wire tracing into the clie..." | Re-trigger Greptile |
d7fda59 to
e0fb116
Compare
e0fb116 to
6b3c45c
Compare
be93a2c to
e254ad7
Compare
2597c0e to
402583a
Compare
|
Reviews (2): Last reviewed commit: "fix(traces): keep shutdown and fork isol..." | Re-trigger Greptile |
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: 2 · PR risk: 0/10 |
jzhu13
left a comment
There was a problem hiding this comment.
Reviewed against traces/08-before-span-send, and the rest of the stack from #949 up. Tests pass at the head, ruff, mypy, and the public API snapshot are clean. The lazy init under _traces_lock and the shutdown re-check are sound. Four items I would fix before merge; two of them are inherited from #954 and are best fixed there.
Blocking
posthog/__init__.py:1397setup()runs on every module-level_proxycall and re-appliesposthog.traceswheneverdefault_client._traces is None. Reproduced:posthog.default_client = Posthog(key, traces={"service_name": "svc"})followed byposthog.capture(...)before any span was recorded sets_traces_configtoNone, andposthog.start_span()returnsNoopSpanfor the life of the client. The line mirrors_metrics_config, butmetrics=Nonemeans defaults-on whiletraces=Nonemeans off. It also undoes the init-failure latch atclient.py:2542, so a failing init is retried and re-logged on every call. Suggest syncing only whentraces is not None.posthog/client.py:2890traces.flush(30.0)thentraces.close(): the 30 s budget is never used for a retry becauseSpanExporter._drainstops on the firstretry-laterandflushskips the follow-up pass (#954 item 1). Reproduced: 3 queued spans, one 503 thenok, shutdown completed in under 1 s with 1 send andDiscarding 3 span(s). Events in the same shutdown get lane retries with backoff. Fix in #954, or loop the flush here while queued and before the deadline.posthog/client.py:2639the docstring's "at least one span request is attempted even when the budget is already spent" is false when the timer flush holds_flush_lock(#954 item 2). Reproduced:client.flush(timeout_seconds=0.2)during a slow timer request returned at 0.20 s with 0 requests, 2 spans queued, and no warning. A serverless runtime freezes the process and the spans are lost silently.posthog/client.py:2665span_flush.join()is untimed. Measured:flush(timeout_seconds=0.2)with one queued span and a 3 s stalled request blocked 3.01 s, so the bound istimeout_seconds + client.timeout(15 s default). The exit paths bound the same join via_join_span_flush. Serverless handlers andPosthogCeleryIntegration.shutdown()hit this path. Either bound the join or document the worst case.
Non-blocking, recommended
posthog/__init__.py:15from posthog.tracing.span import Spanin the bare form is private under pyright strict in thispy.typedpackage:from posthog import Spanerrors withreportPrivateImportUsage(reproduced with pyright and basedpyright), while the snapshot advertisesposthog.Span. Adjacent re-exports useX as X.check_strict_types.shnever importsSpan, so CI cannot catch it.posthog/client.py:2537atexit.register(self._atexit_spans)per sync-mode client is never unregistered, including byshutdown(). Reproduced: 20 short-lived sync clients that recorded a span stayed alive aftershutdown()andgc.collect(), sole referrer the bound method. Sync-mode clients were previously collectable.atexit.unregisterin_shutdown_once, or one module-level hook over the existing_client_registryWeakSet.posthog/client.py:2691the inline fallback whenThread.start()raises runs the span flush before the lane flushes in_atexit, so under a container thread limit a slow traces endpoint (exempt first request, up to 15 s) leaves the lanes 0 s of the 1 s exit budget. Run lanes first on that path.- Nits:
_start_span_flushspawns a thread even with no spans queued, since it only checks_traces is None;reinit_after_fork(active_var=None)in_pipeline.py:117has no production caller that omits the argument, and omitting it silently keeps the parent's ContextVar, so make it required;TRACE_ID/SPAN_ID,mock_session, and three copies ofslow_sendintest_client_traces.pyduplicatehelpers.py; a forked child loses span parenting (consistent withposthog.contexts, but worth one line in thestart_spandocstring pointing atparent=span).
Two things I could not verify from the repo: whether the public API shape was agreed on an issue first (CONTRIBUTING), and whether the changeset text is meant to cover AsyncClient, which gets neither the traces option nor the span methods.
Reviewed with Claude Code (Claude Fable 5.1). Behaviors above were reproduced against this branch head.
98856b0 to
f2bba20
Compare
|
Thanks. Fixed in f2bba20:
|
f2bba20 to
c44a14c
Compare
|
Follow-up, c44a14c:
|
Makes tracing usable. Adds the `traces` client option (tracing stays off until it is set), Client.start_span / get_active_span and their posthog module-level counterparts, with the active span scoped per client so two clients never parent to each other's spans. flush() drains spans alongside events within the same budget; shutdown() gives queued spans a final flush of up to 30 s and warns about any it discards; an exit flush bounded by the existing exit deadline covers scripts that never call shutdown(), and a forked child drops the parent's spans. Export failures, limits and the hook are documented on the option. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkZAsCciW4PV8ZdcCHmAbA
Shutdown reads the pipeline under the same lock initialization publishes it with, so an initialization in flight is either closed by shutdown or sees the request and stays off. A forked child gets a fresh active-span variable: an inherited handle exiting in the child resets the old one, which would have restored the parent process's outer span.
…ces config off setup() runs on every module-level call and re-applied posthog.traces whenever no pipeline existed yet, so a default client built with its own traces config lost it on the first capture(), and a failed init was retried and re-logged on every call. The module option now applies only where the client has none, and a failed init latches as False. Shutdown unregisters the sync-mode exit drain so the client is collectable, the flush docstring states its worst case, Span is re-exported for pyright strict and checked in CI, the fork ContextVar is required, and the start_span docstring says what a forked child inherits.
…e span thread when nothing is queued When no thread could start at exit, the span flush ran before the event lanes and could spend the whole exit budget. _start_span_flush now hands back a waiter the caller runs after the lanes, and starts nothing when the span queue is empty. The flush docstring describes the retry-within- budget contract. Tracing test fixtures move to a conftest, and the client tests reuse the shared ids and one slow sender.
c44a14c to
71b482d
Compare
marandaneto
left a comment
There was a problem hiding this comment.
found same issue as here but lgtm if its ok as is
|
Docs for this stack, as drafts to merge after the release: PostHog/posthog.com#20285 (Python library page and tracing install guide), PostHog/posthog#102724 (tracing empty state), PostHog/context-mill#399 (tracing agent skill). |
💡 Motivation and Context
Makes tracing usable. Adds the
tracesclient option (tracing stays off until it is set),Client.start_span/get_active_spanand theirposthogmodule-level counterparts, with the active span scoped per client so two clients never parent to each other's spans.flush()drains spans alongside events within the same budget.shutdown()gives queued spans a final flush of up to 30 s and warns about any it discards.shutdown(), and a forked child drops the parent's spans.Export failures, limits and the hook are documented on the option. Includes the changeset.
Stack (PR 9 of 9, based on
traces/08-before-span-send):traces/01-ids-traceparenttraces/02-otlp-encodingtraces/03-span-handlestraces/04-transporttraces/05-pipelinetraces/06-exporttraces/07-span-limitstraces/08-before-span-sendtraces/09-client-wiring← this PR💚 How did you test it?
Unit tests in
posthog/test/tracing/test_client_traces.pycover the option, the client and module APIs, per-client active spans, flush, shutdown, exit flush and fork. The wholeposthog/test/tracingsuite was run on the rebased stack tip.📝 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