feat(errors): Add support for handling chained exceptions #24572
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,20 @@ interface StackFrame { | |
| lineno: number | ||
| colno: number | ||
| function: string | ||
| context_line?: string | ||
| } | ||
|
|
||
| interface ExceptionTrace { | ||
| stacktrace: { | ||
| frames: StackFrame[] | ||
| } | ||
| mechanism: { | ||
| handled: boolean | ||
| type: string | ||
| } | ||
| module: string | ||
| type: string | ||
| value: string | ||
| } | ||
|
|
||
| function parseToFrames(rawTrace: string): StackFrame[] { | ||
|
|
@@ -25,7 +39,7 @@ function StackTrace({ rawTrace }: { rawTrace: string }): JSX.Element | null { | |
| <> | ||
| {frames.length ? ( | ||
| frames.map((frame, index) => { | ||
| const { filename, lineno, colno, function: functionName, context_line } = frame | ||
|
|
||
| return ( | ||
| <TitledSnack | ||
|
|
@@ -34,6 +48,7 @@ function StackTrace({ rawTrace }: { rawTrace: string }): JSX.Element | null { | |
| value={ | ||
| <> | ||
| {filename}:{lineno}:{colno} | ||
| {context_line ? `:${context_line}` : ''} | ||
| </> | ||
| } | ||
| /> | ||
|
|
@@ -51,6 +66,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 ( | ||
| <> | ||
|
|
@@ -91,6 +123,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 | ||
|
|
@@ -102,6 +135,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
There was a problem hiding this comment. feels like There was a problem hiding this comment. I'm not sure I get this, I think we need to do this because There was a problem hiding this comment. so about the first comment. {
"exception": {}
}or {
"exception": {
"values": []
}
}this is technical debt since chained exceptions weren't a thing before or not implemented at least. Always being: {
"exception": {
"values": [its ok to be a single item here even if its not a chained exception]
}
}will reduce those unnecessary checks. There was a problem hiding this comment. in our case, we'd always use There was a problem hiding this comment. 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) There was a problem hiding this comment. We probably still need this for backward compatibility but should we look to update the sentry integration to use There was a problem hiding this comment. 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 There was a problem hiding this comment. unless we are using a super old version of the sentry js SDK, the There was a problem hiding this comment. hmm, the point is if our 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 && $sentry_exception) { | ||
| if (Array.isArray($sentry_exception.values)) { | ||
| $exception_list = $sentry_exception.values | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| $exception_type, | ||
| $exception_message, | ||
|
|
@@ -115,6 +156,7 @@ export function getExceptionPropertiesFrom(eventProperties: Record<string, any>) | |
| $active_feature_flags, | ||
| $sentry_url, | ||
| $exception_stack_trace_raw, | ||
| $exception_list, | ||
| $level, | ||
| } | ||
| } | ||
|
|
@@ -133,6 +175,7 @@ export function ErrorDisplay({ eventProperties }: { eventProperties: EventType[' | |
| $active_feature_flags, | ||
| $sentry_url, | ||
| $exception_stack_trace_raw, | ||
| $exception_list, | ||
| $level, | ||
| } = getExceptionPropertiesFrom(eventProperties) | ||
|
|
||
|
|
@@ -162,18 +205,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} ${$browser_version}` : 'unknown'} /> | ||
| <TitledSnack title="os" value={$os ? `${$os} ${$os_version}` : 'unknown'} /> | ||
| </div> | ||
| {$exception_list?.length ? ( | ||
| <ChainedStackTraces exceptionList={$exception_list} /> | ||
|
There was a problem hiding this comment. sentry reverses the
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. There was a problem hiding this comment. 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? There was a problem hiding this comment. I think each platform has to decide whether should reverse or not (this varies depending on the platform). |
||
| ) : $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> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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