-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(utils): Backfill stack trace on fetch errors if it is missing #12389
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
Conversation
@AbhiPrasad is this a very stupid idea? |
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.
This feels like a really good idea, if we can make this work we solve soooo many issues for users.
We should test this on safari first, I know they have the worst stacktraces for fetch.
// it means the error, that was caused by your fetch call did not | ||
// have a stack trace, so the SDK backfilled the stack trace so | ||
// you can see which fetch call failed. | ||
error.stack = new Error(error.message).stack; |
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.
do we need to trim the frames here? I think we can set framesToPop
to do this (error.framesToPop = 1
).
size-limit report 📦
|
Do you know how to produce "failed to fetch" errors? |
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.
This looks like a good idea that's worth a try! When we release this let's check in the docs project if this changes anything (given we also get some of these stacktrace-less failed to fetch errors in docs).
const envelopes = await getMultipleSentryEnvelopeRequests<Event>(page, 4, { url, timeout: 10000 }); | ||
const tracingEvent = envelopes.find(event => event.type === 'transaction')!; // last envelope contains tracing data on all browsers |
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.
This, admittedly, is a very sketchy test fix. It appears that the failed to fetch errors were previously not reported but now they are. I think this is more correct, so I am fine with it, but if any of @AbhiPrasad or @Lms24 oppose lmk.
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.
could we add an e2e test? Also a screenshot of an example failed to fetch error in the sentry UI to this PR?
I think the test fix seems reasonable to me.
Ok so I went ahead and attempted to add a browser integration test. I quickly found out that unless we add this change we are not recording errors in Chrome for a failing call like Also, on Safari the fetch call from above will cause a |
discussed offline: problems addressed
This is a lowly attempt at fixing stacktrace-less
failed to fetch
errors that have been plagueing users for years.Note that I did not verify whether this actually works, since we haven't figured out yet how to actually reproduce these errors.
Unintentionally fixes a stacktrace-less safari error:
(before)

(after)
