Skip to content
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.

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

feat: add stack to stacktraceless "exceptions" #1472

Merged
merged 4 commits into from
Oct 16, 2024

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Oct 15, 2024

Changes

Why miss out on a stack trace if you call captureException with a string?

This changes things to throw an error to capture the current stack trace and strips the final frame (e.g. inside our captureException function)

Copy link

vercel bot commented Oct 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Oct 16, 2024 8:37am

@@ -51,7 +51,6 @@ export interface StackFrame {
}

const WEBPACK_ERROR_REGEXP = /\(error: (.*)\)/
const STRIP_FRAME_REGEXP = /captureException/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a superior approach but I couldn't get it to work. The function name I was seeing was t.value in the stack trace. Perhaps there's some Javascript function construction magic needed to ensure the function name gets caught in the stack. Open to any thoughts if people have them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably because array.js is minified? Or the loaded extensions are if we're using the npm version of main package..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably true - very meta given our current work 😂

Copy link

github-actions bot commented Oct 15, 2024

Size Change: +2.05 kB (+0.07%)

Total Size: 2.83 MB

Filename Size Change
dist/all-external-dependencies.js 192 kB +244 B (+0.13%)
dist/array.full.js 355 kB +309 B (+0.09%)
dist/array.full.no-external.js 354 kB +309 B (+0.09%)
dist/array.js 169 kB +65 B (+0.04%)
dist/array.no-external.js 168 kB +65 B (+0.04%)
dist/exception-autocapture.js 11.4 kB +247 B (+2.22%)
dist/main.js 170 kB +65 B (+0.04%)
dist/module.full.js 355 kB +309 B (+0.09%)
dist/module.full.no-external.js 354 kB +309 B (+0.09%)
dist/module.js 169 kB +65 B (+0.04%)
dist/module.no-external.js 168 kB +65 B (+0.04%)
ℹ️ View Unchanged
Filename Size
dist/external-scripts-loader.js 2.35 kB
dist/recorder-v2.js 111 kB
dist/recorder.js 111 kB
dist/surveys-preview.js 59.8 kB
dist/surveys.js 66 kB
dist/tracing-headers.js 8.36 kB
dist/web-vitals.js 10.3 kB

compressed-size-action

@daibhin daibhin requested a review from a team October 15, 2024 15:11
@daibhin daibhin changed the title use synthetic stack trace if not originally captured feat: add stacktrace to non error exceptions Oct 15, 2024
@daibhin daibhin changed the title feat: add stacktrace to non error exceptions feat: add stack to stacktraceless exceptions Oct 15, 2024
@@ -1826,14 +1826,15 @@ export class PostHog {

/** Capture a caught exception manually */
captureException(error: Error, additionalProperties?: Properties): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to update the type here as well now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was torn... technically I'd still like people to send us errors but it is a little misleading to ourselves to think that the input will always be an error. What I really want is to type it as unknown but for a user to be shown Error as the type

Copy link
Collaborator

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

nice

@neilkakkar
Copy link
Collaborator

oh wait, this doesn't work well for the case where currently we're not seeing stack traces, it's a different captureException function that's called: https://github.com/PostHog/posthog-js/blob/master/src/extensions/exception-autocapture/index.ts#L76

@daibhin daibhin changed the title feat: add stack to stacktraceless exceptions feat: add stack to stacktraceless "exceptions" Oct 16, 2024
@daibhin
Copy link
Contributor Author

daibhin commented Oct 16, 2024

@neilkakkar you're right, this actually only fixes the issue where the stack trace is missing after a user manually called captureException with a string / object that does not have a stack

The global onerror and unhandledrejection you linked should always have an error and thus not need a synthetic exception The captureException you linked is actually the callback used in the wrapped functions (here and here) to parse that error.

The case where the stack trace is missing from a genuine error is probably more likely related to this issue: https://sentry.io/answers/script-error/ (I'm going to try recreating this now, I would suspect that the script just needs to be loaded anonymously)

@daibhin daibhin added the bump patch Bump patch version when this PR gets merged label Oct 16, 2024
@daibhin daibhin merged commit ffad724 into main Oct 16, 2024
15 checks passed
@daibhin daibhin deleted the dn-feat/stackless-stack-traces branch October 16, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants