feat(errors): Add support for handling chained exceptions

[neilkakkar]

Problem

Python autocapture now has exception chaining, so would be nice to support it in the UI appropriately as well.

This changes how we send errors (is much closer to sentry's schema). As a result, it also handles showing chained exceptions when captured via sentry :D

Looks something like this:

I've confirmed the old format still works, but would love a sanity check as well :)

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

[github-actions]

Size Change: +272 B (+0.03%)

Total Size: 1.08 MB

ℹ️ View Unchanged

Filename	Size	Change
frontend/dist/toolbar.js	1.08 MB	+272 B (+0.03%)

_{compressed-size-action}

[posthog-bot]

📸 UI snapshots have been updated

10 snapshot changes in total. 0 added, 10 modified, 0 deleted:

• chromium: 0 added, 10 modified, 0 deleted (diff for shard 1, diff for shard 2)
• webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

[marandaneto]

I'd change this to something else (e.g., String|enum) that accepts multiple values, such as handled|unhandled|process_termination. The reason is that Sentry was built for the backend and frontend, but different platforms have different needs.
more context here getsentry/rfcs#10

[neilkakkar]

Ah nice, I think I'd rather just remove it for now, and think about how we want to handle it when its time to add it? It's unused anyway right now

[marandaneto]

I know we are moving closer and closer to the sentry protocol, and that's fine for now. Still, the error monitoring industry is moving closer and closer to the OpenTelemetry protocol, so while this is fine, I think we should have been moving in the other direction (OTel).
eg https://embrace.io/opentelemetry-for-mobile/ and many others including datadog, etc

[marandaneto]

feels like sentry_exception should have been just a $exception_list with a single item, so we don't need to do this (that's how sentry does btw).
https://develop.sentry.dev/sdk/event-payloads/types/

[neilkakkar]

I'm not sure I get this, I think we need to do this because $exception_list is a list of exceptions while sentry has a dict with the key 'values' which then has a list of exceptions.

[marandaneto]

so about the first comment.
sentry does either:

{
  "exception": {}
}

or

{
  "exception": {
    "values": []
  }
}

this is technical debt since chained exceptions weren't a thing before or not implemented at least.
backend|frontend has to check if it's either a single exception or a list of exceptions based on the values being present or not.

Always being:

{
  "exception": {
    "values": [its ok to be a single item here even if its not a chained exception]
  }
}

will reduce those unnecessary checks.

[marandaneto]

in our case, we'd always use $exception_list, even if it is a single item, and forget about sentry_exception

[neilkakkar]

Yeah correct, sounds like we're aligned. I have this here for the case where we're extracting and displaying the exception from sentry, not one captured via posthog (posthog will always send a list)

[daibhin]

We probably still need this for backward compatibility but should we look to update the sentry integration to use $exception_list over $sentry_exception? Happy for that to come in another PR if you prefer

[neilkakkar]

I quite like keeping these two separate, since that ensures we don't have a hard dependency on sentry's schema - the sentry integration can always just get the sentry exception, in sentry's format, and then there's this translation layer that converts it to ours where need be. Doing it here means there's only one place we need to do the conversion (rather than in all SDKs that eventually might support this), and also we don't need to rush to update SDKs when sentry adds/removes things from their schema

[marandaneto]

unless we are using a super old version of the sentry js SDK, the event.exception is always an object that contains a values list, so technically this workaround would not even be needed
https://github.com/getsentry/sentry-javascript/blob/8ce9f7ce9a122bfe70d3c12ebfe50bba5d237037/packages/utils/src/eventbuilder.ts#L150-L154

[neilkakkar]

hmm, the point is if our $exception_list diverges from sentry (like for example, the mechanism is set differently, handled is not a boolean), then we can do that translation from a sentry exception to a posthog exception here. But if we don't know that it came from a sentry exception, this becomes much more annoying, since you then need to test if mechanism is one of acceptable values, and if not, convert it, etc. etc.

This is just a translation layer from sentry exception to posthog exception, not a workaround, if I'm understanding this code correctly, so I'd always want this because of reasons in above comment 😅

[marandaneto]

sentry reverses the exception_list in the server (historical reasons).
https://develop.sentry.dev/sdk/event-payloads/exception/

Multiple values represent chained exceptions and should be sorted oldest to newest. For example, consider this Python code snippet:

I think the SDK has to capture already in the right order, and the parsing/showing has to maintain that order, I am not sure this is correct right now assuming that we don't do the reverse either.

[neilkakkar]

In python we effectively reverse so the first exception that happens is at the top. Maintaining this invariant in the SDKs this should be ok - as long as every SDK of ours does the same?

[marandaneto]

I think each platform has to decide whether should reverse or not (this varies depending on the platform).
So if there's a list of [exp 1, exp 2] in the JSON, it's shown as exp 1 on the top and exp 2 on the bottom.
If Python puts the exp 1 on the top or the bottom, then it depends on how Python itself displays chained exceptions in the console, I'd just follow their format, so the SDK decides the sorting, and the backend|frontend maintains it.

[marandaneto]

we are not using this right now but its definitely important once we figure out grouping and stack trace demangling