Skip to content

feat(errors): Add support for handling chained exceptions #24572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
neilkakkar merged 8 commits into from
Aug 28, 2024
Merged

feat(errors): Add support for handling chained exceptions #24572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
903b039
feat(errors): Add support for handling chained exceptions
neilkakkar Aug 26, 2024
3706e87
add test
neilkakkar Aug 26, 2024
17b6bde
Update UI snapshots for `chromium` (1)
github-actions[bot] Aug 26, 2024
0b99859
Update UI snapshots for `chromium` (2)
github-actions[bot] Aug 26, 2024
7262d83
add story
neilkakkar Aug 26, 2024
1132160
add story
neilkakkar Aug 26, 2024
ee215de
Merge branch 'exception-chaining' of github.com:PostHog/posthog into …
neilkakkar Aug 26, 2024
071e836
Update UI snapshots for `chromium` (2)
github-actions[bot] Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified ...s__/components-errors-error-display--anonymous-error-with-stack-trace--dark.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...__/components-errors-error-display--anonymous-error-with-stack-trace--light.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...nd/__snapshots__/components-errors-error-display--chained-error-stack--dark.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...d/__snapshots__/components-errors-error-display--chained-error-stack--light.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified frontend/__snapshots__/components-errors-error-display--importing-module--dark.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...tend/__snapshots__/components-errors-error-display--importing-module--light.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified .../components-errors-error-display--resize-observer-loop-limit-exceeded--dark.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...components-errors-error-display--resize-observer-loop-limit-exceeded--light.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...nd/__snapshots__/components-errors-error-display--safari-script-error--dark.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...d/__snapshots__/components-errors-error-display--safari-script-error--light.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified frontend/__snapshots__/scenes-app-errortracking--group-page--dark.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified frontend/__snapshots__/scenes-app-errortracking--group-page--light.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
113 changes: 113 additions & 0 deletions frontend/src/lib/components/Errors/ErrorDisplay.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,116 @@ export function AnonymousErrorWithStackTrace(): JSX.Element {
/>
)
}

export function ChainedErrorStack(): JSX.Element {
return (
<ErrorDisplay
eventProperties={errorProperties({
$exception_type: 'ZeroDivisionError',
$exception_message: 'division by zero',
$exception_list: [
{
module: null,
type: 'ZeroDivisionError',
value: 'division by zero',
stacktrace: {
frames: [
{
filename: 'example2.py',
abs_path: '/posthog-python/example2.py',
function: 'will_raise',
module: '__main__',
lineno: 33,
pre_context: [
'def more_obfuscation():',
' print(3 / 0)',
'',
'def will_raise():',
' try:',
],
context_line: ' more_obfuscation()',
post_context: [
' except Exception as e:',
' raise CustomException("This is a custom exception") from e',
'',
'will_raise()',
'exit()',
],
},
{
filename: 'example2.py',
abs_path: '/posthog-python/example2.py',
function: 'more_obfuscation',
module: '__main__',
lineno: 29,
pre_context: [
'',
'class CustomException(Exception):',
' pass',
'',
'def more_obfuscation():',
],
context_line: ' print(3 / 0)',
post_context: [
'',
'def will_raise():',
' try:',
' more_obfuscation()',
' except Exception as e:',
],
},
],
},
},
{
module: '__main__',
type: 'CustomException',
value: 'This is a custom exception',
stacktrace: {
frames: [
{
filename: 'example2.py',
abs_path: '/Users/neilkakkar/Project/posthog-python/example2.py',
function: '<module>',
module: '__main__',
lineno: 37,
pre_context: [
' try:',
' more_obfuscation()',
' except Exception as e:',
' raise CustomException("This is a custom exception") from e',
'',
],
context_line: 'will_raise()',
post_context: [
'exit()',
'',
'',
'# print(posthog.get_all_flags("distinct_id_random_22"))',
'# print(',
],
},
{
filename: 'example2.py',
abs_path: '/Users/neilkakkar/Project/posthog-python/example2.py',
function: 'will_raise',
module: '__main__',
lineno: 35,
pre_context: [
'',
'def will_raise():',
' try:',
' more_obfuscation()',
' except Exception as e:',
],
context_line: ' raise CustomException("This is a custom exception") from e',
post_context: ['', 'will_raise()', 'exit()', '', ''],
},
],
},
},
],
})}
/>
)
}
51 changes: 46 additions & 5 deletions frontend/src/lib/components/Errors/ErrorDisplay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@ interface StackFrame {
lineno: number
colno: number
function: string
context_line?: string
}

interface ExceptionTrace {
stacktrace: {
frames: StackFrame[]
}
module: string
type: string
value: string
}

function parseToFrames(rawTrace: string): StackFrame[] {
Expand All @@ -25,7 +35,7 @@ function StackTrace({ rawTrace }: { rawTrace: string }): JSX.Element | null {
<>
{frames.length ? (
frames.map((frame, index) => {
const { filename, lineno, colno, function: functionName } = frame
const { filename, lineno, colno, function: functionName, context_line } = frame

return (
<TitledSnack
Expand All @@ -34,6 +44,7 @@ function StackTrace({ rawTrace }: { rawTrace: string }): JSX.Element | null {
value={
<>
{filename}:{lineno}:{colno}
{context_line ? `:${context_line}` : ''}
</>
}
/>
Expand All @@ -51,6 +62,23 @@ function StackTrace({ rawTrace }: { rawTrace: string }): JSX.Element | null {
}
}

function ChainedStackTraces({ exceptionList }: { exceptionList: ExceptionTrace[] }): JSX.Element {
return (
<>
<LemonDivider dashed={true} />
<h2 className="mb-0">Stack Trace</h2>
{exceptionList.map(({ stacktrace, value }, index) => {
return (
<div key={index} className="flex flex-col gap-1 mt-6">
<h3 className="mb-0">{value}</h3>
<StackTrace rawTrace={JSON.stringify(stacktrace.frames)} />
</div>
)
})}
</>
)
}

function ActiveFlags({ flags }: { flags: string[] }): JSX.Element {
return (
<>
Expand Down Expand Up @@ -91,6 +119,7 @@ export function getExceptionPropertiesFrom(eventProperties: Record<string, any>)
} = eventProperties

let $exception_stack_trace_raw = eventProperties.$exception_stack_trace_raw
let $exception_list = eventProperties.$exception_list
// exception autocapture sets $exception_stack_trace_raw as a string
// if it isn't present then this is probably a sentry exception.
// try and grab the frames from that
Expand All @@ -102,6 +131,14 @@ export function getExceptionPropertiesFrom(eventProperties: Record<string, any>)
}
}
}
// exception autocapture sets $exception_list for chained exceptions.
// If it's not present, get this list from the sentry_exception
Comment on lines +134 to +135
Copy link
Copy Markdown
Member

@marandaneto marandaneto Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

Copy link
Copy Markdown
Contributor Author

@neilkakkar neilkakkar Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@marandaneto marandaneto Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@marandaneto marandaneto Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@neilkakkar neilkakkar Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

@daibhin daibhin Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@neilkakkar neilkakkar Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@marandaneto marandaneto Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@neilkakkar neilkakkar Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

if (!$exception_list?.length && $sentry_exception) {
if (Array.isArray($sentry_exception.values)) {
$exception_list = $sentry_exception.values
}
}

return {
$exception_type,
$exception_message,
Expand All @@ -115,6 +152,7 @@ export function getExceptionPropertiesFrom(eventProperties: Record<string, any>)
$active_feature_flags,
$sentry_url,
$exception_stack_trace_raw,
$exception_list,
$level,
}
}
Expand All @@ -133,6 +171,7 @@ export function ErrorDisplay({ eventProperties }: { eventProperties: EventType['
$active_feature_flags,
$sentry_url,
$exception_stack_trace_raw,
$exception_list,
$level,
} = getExceptionPropertiesFrom(eventProperties)

Expand Down Expand Up @@ -162,18 +201,20 @@ export function ErrorDisplay({ eventProperties }: { eventProperties: EventType['
/>
<TitledSnack title="synthetic" value={$exception_synthetic ? 'true' : 'false'} />
<TitledSnack title="library" value={`${$lib} ${$lib_version}`} />
<TitledSnack title="browser" value={`${$browser} ${$browser_version}`} />
<TitledSnack title="os" value={`${$os} ${$os_version}`} />
<TitledSnack title="browser" value={$browser ? `${$browser} ${$browser_version}` : 'unknown'} />
<TitledSnack title="os" value={$os ? `${$os} ${$os_version}` : 'unknown'} />
</div>
{!!$exception_stack_trace_raw?.length && (
{$exception_list?.length ? (
<ChainedStackTraces exceptionList={$exception_list} />
Copy link
Copy Markdown
Member

@marandaneto marandaneto Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@neilkakkar neilkakkar Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@marandaneto marandaneto Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) : $exception_stack_trace_raw?.length ? (
<>
<LemonDivider dashed={true} />
<div className="flex flex-col gap-1 mt-6">
<h2 className="mb-0">Stack Trace</h2>
<StackTrace rawTrace={$exception_stack_trace_raw} />
</div>
</>
)}
) : null}
<LemonDivider dashed={true} />
<div className="flex flex-col gap-1 mt-6">
<h2 className="mb-0">Active Feature Flags</h2>
Expand Down
21 changes: 21 additions & 0 deletions frontend/src/lib/components/Errors/error-display.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,27 @@ describe('Error Display', () => {
$exception_message: 'There was an error creating the support ticket with zendesk.',
$exception_stack_trace_raw:
'[{"colno":220,"filename":"https://app-static-prod.posthog.com/static/chunk-UFQKIDIH.js","function":"submitZendeskTicket","in_app":true,"lineno":25}]',
$exception_list: [
{
mechanism: {
handled: true,
type: 'generic',
},
stacktrace: {
frames: [
{
colno: 220,
filename: 'https://app-static-prod.posthog.com/static/chunk-UFQKIDIH.js',
function: 'submitZendeskTicket',
in_app: true,
Copy link
Copy Markdown
Member

@marandaneto marandaneto Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lineno: 25,
},
],
},
type: 'Error',
value: 'There was an error creating the support ticket with zendesk.',
},
],
$exception_synthetic: undefined,
$exception_type: 'Error',
$lib: 'posthog-js',
Expand Down