Repository navigation
fix(errors): order and mark the stack of a never-raised exception - #1181
luantaraschi wants to merge 4 commits into
Conversation
An exception passed to capture_exception without being raised gets a stack built by the SDK. That stack came out innermost first, started with the SDK's own capture frames, and was sent with mechanism.synthetic false. Build it outermost first like a raised traceback, leave out the SDK frames, and mark it synthetic.
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review.
Store the id of the synthesized traceback instead of the traceback, which cannot be pickled.
| continue | ||
| frames.append(frame) | ||
|
|
||
| frames.reverse() |
There was a problem hiding this comment.
#266 did reverse it on purpose. Going by the screenshots there, the UI drew frames in the order they arrived, and the reversal put capture_exception at the top. The current spec rules that reason out: frames[0] has to be the outermost frame and "SDKs never reorder frames for display reasons". Raised exceptions already arrive outermost first, so on main the two kinds come in opposite orders.
In #898 I only found the synthetic default, it doesn't touch how the traceback is built. Is there something else in it I should look at? I haven't checked how the UI renders these today. If you'd rather keep the reversal I'll take that part out and leave the SDK frames and synthetic.
There was a problem hiding this comment.
yeah we just need to confirm with @PostHog/team-error-tracking before we merge and break but i think its fine
SDK performance: posthog-pythonBenchmarks completed. No changes flagged for review. Flags require at least a 5% change in the median and all six pairs agreeing on direction. This is a noise filter, not proof of a performance change or a CI gate. View all results
Values are medians of paired measurements. Not flagged does not mean unchanged. Unavailable means a measurement could not be provided, not zero. Estimated allocation measurements retain their instrumentation uncertainty. RSS measures the whole process, not just SDK allocations; observed peaks are sampled checkpoints. Paired ranges in the diagnostics describe variability, not confidence intervals. Each shard uses its own runner; compare Before PR against With PR within a row, not across shards. Compared 102 metric rows; 102 rows shown in all results. Omitted or shortened data remain in |
|
Checked the changed behavior against PostHog/sdk-specs at ff629e2, using PR head
Validation: 34 tests passed, covering Dropping SDK capture frames is compatible with the spec, but is not explicitly required. Keeping captured exceptions pickleable is a Python compatibility requirement rather than an explicit SDK-spec requirement. No spec divergence found within this PR’s changed behavior; this is not a claim of full SDK conformance. |
ioannisj
left a comment
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
Nothing blocking from me, two edge cases in the synthesized stack handling. The SDK frame filter doesn't match the posthoganalytics mirror, and the id() marker can outlive its traceback and mark a raised stack as synthetic. Both reproduce locally and neither touches the common path.
| exception_value["mechanism"].setdefault( | ||
| "synthetic", | ||
| tb is not None | ||
| and getattr(exc_value, _SYNTHESIZED_TRACEBACK_ID_ATTR, None) == id(tb), |
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
[suggestion] id(tb) is only unique while that traceback is alive, so I think this can report synthetic: true for a real raised stack.
The marker sits in the instance dict, so pickle and copy.copy carry it to a copy that has no traceback. Once the synthesized traceback is freed and the copy gets raised, the new traceback often lands on the same address. Capture, pickle.loads(pickle.dumps(e)), then raise gave me synthetic: true in 198 of 200 runs on Python 3.14, and about half the time with copy.copy.
Would a small holder object work instead? It keeps a reference to the traceback, gets compared with is, and reduces to an empty holder so pickling still drops it.
There was a problem hiding this comment.
Reproduced. In a loop that frees the traceback, a pickle round trip came back synthetic: true in 199 of 200 on 3.14 and deepcopy in 100 of 200 (copy.copy only 1 of 200 for me). The rate depends on allocation: that loop gave 0 on 3.12, while the pytest version fails on 3.12 and passes on 3.14.
Moved to the holder in 20de5fc, compared with is. One difference: it reduces to None instead of an empty holder, so the class stays out of the pickle and the exception still loads where the SDK isn't installed.
| def _is_sdk_frame(frame): | ||
| # type: (FrameType) -> bool | ||
| module = frame.f_globals.get("__name__") or "" | ||
| return module == "posthog" or module.startswith("posthog.") |
There was a problem hiding this comment.
🤖 An AI agent wrote this on behalf of @ioannisj.
[suggestion] I think _is_sdk_frame misses the posthoganalytics mirror. It matches the literal posthog, but the mirror build in the Makefile only rewrites from posthog imports, so there every SDK module is posthoganalytics.* and nothing gets skipped.
I built the mirror with those sed rules and the same never-raised capture still ends at exc_info_from_error and construct_artificial_traceback.
Would deriving the root from the module work, something like __name__.partition(".")[0]? A test with the package renamed would be cheap too, as it stands a regression on the mirror name would still pass.
There was a problem hiding this comment.
Same result here with the Makefile's sed rules. In 20de5fc the package comes from __name__.partition(".")[0], and a new test loads the module as posthoganalytics.exception_utils and checks the stack ends at the caller.
| continue | ||
| frames.append(frame) | ||
|
|
||
| frames.reverse() |
…ackage An id can be reused once the traceback is freed, so a pickled or copied exception could report a raised stack as synthetic. Keep the traceback in a small holder compared by identity, which reduces to None when the exception is pickled or deep copied. The SDK frame check only matched the posthog package, so the posthoganalytics mirror kept its capture frames. Take the package name from the module.
💡 Motivation and Context
When
capture_exceptiongets an exception that was never raised,construct_artificial_tracebackbuilds a traceback from the current stack. That traceback has three problems:tb_nextof the next one, so the head of the chain is the innermost frame.frames[0]ends up being the innermost frame and the last one is<module>. A raised exception gives the opposite order, and the capture-exception spec in PostHog/sdk-specs ("Stack frame ordering") asks for outermost first.sys._getframe(0), so the innermost frames areconstruct_artificial_traceback,exc_info_from_errorandcapture_exception. The event then points at SDK code as the crash site instead of the application line that called the SDK. posthog-js drops its own frame from a synthetic stack (skipFirstLines).mechanism.synthetictofalse, with a comment saying the SDK never replaces a stack with a generated one. The exception-event-metadata spec says an SDK-generated current stack issynthetic: true.For example, with
report()callingcapture_exception(ValueError("never raised"))fromhandler()andentry_point(), main sends:With this change it sends
<module>, entry_point, handler, reportand"synthetic": true.The fix links the traceback from the innermost frame outwards, skips the innermost frames that belong to the SDK's own package (
posthog, orposthoganalyticsin the mirror), and keeps the traceback it built in a small holder on the exception. The holder reduces toNonewhen the exception is pickled or deep copied, because a traceback cannot be pickled. The builder setssynthetictotrueonly when the traceback it serializes is the one in the holder. A raised exception and an explicitsyntheticin the mechanism behave as before.The reversal goes back to #266, which built the full stack this way on purpose. I think the spec and the order used for raised exceptions now point the other way, but I'm happy to adjust if there is a reason to keep it.
💚 How did you test it?
test_exception_capture.py. They call the SDK from functions compiled into a module namedapp, so the frames look like application code. Four of them (order, SDK frames,synthetic, and the same checks throughClient.capture_exception) fail on main and pass with the fix. A control checks that a raised exception passed by object stayssynthetic: false, and passes on both. Two more cover the first review: the exception still pickles after capture, and raising it after capture reportssynthetic: false. Three more cover the second one: raising a pickled or copied exception reportssynthetic: false, a pickled exception loads without the SDK installed, and the stack ends at the caller with the module loaded under the mirror's name.python:3.12-slimcontainer: 6106 passed, 63 skipped, no failures.test_exception_capture.pyalso passes with the lock on 3.10, 3.11 and 3.13. The lock didn't install onpython:3.14-slim(zstandard0.23), so on 3.14 I ran that file without it: onlytest_excepthookfailed, with and without this change.ruff checkandruff format --check(0.12.2), the mypy baseline andpython -W error -c "import posthog"are clean.📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code with Claude Opus 5.5 found this while checking posthog-python against the error tracking specs in PostHog/sdk-specs, reproduced it, and wrote the fix and the tests. Luan directed the work.