Skip to content

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

Merged
merged 8 commits into from
Jun 11, 2024

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Jun 6, 2024

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)
Screenshot 2024-06-11 at 09 56 55

(after)
Screenshot 2024-06-11 at 10 30 11

@lforst lforst requested a review from AbhiPrasad June 6, 2024 14:13
@lforst
Copy link
Contributor Author

lforst commented Jun 6, 2024

@AbhiPrasad is this a very stupid idea?

Copy link
Member

@AbhiPrasad AbhiPrasad left a 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;
Copy link
Member

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).

Copy link
Contributor

github-actions bot commented Jun 6, 2024

size-limit report 📦

Path Size
@sentry/browser 22.04 KB (+0.16% 🔺)
@sentry/browser (incl. Tracing) 33.23 KB (+0.11% 🔺)
@sentry/browser (incl. Tracing, Replay) 68.95 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.27 KB (+0.06% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) 73.02 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) 85.14 KB (+0.05% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 86.98 KB (+0.05% 🔺)
@sentry/browser (incl. metrics) 26.23 KB (+0.14% 🔺)
@sentry/browser (incl. Feedback) 38.21 KB (+0.1% 🔺)
@sentry/browser (incl. sendFeedback) 26.63 KB (+0.16% 🔺)
@sentry/browser (incl. FeedbackAsync) 31.18 KB (+0.12% 🔺)
@sentry/react 24.81 KB (+0.15% 🔺)
@sentry/react (incl. Tracing) 36.27 KB (+0.11% 🔺)
@sentry/vue 26.05 KB (+0.14% 🔺)
@sentry/vue (incl. Tracing) 35.07 KB (+0.11% 🔺)
@sentry/svelte 22.17 KB (+0.16% 🔺)
CDN Bundle 23.39 KB (+0.15% 🔺)
CDN Bundle (incl. Tracing) 34.91 KB (+0.12% 🔺)
CDN Bundle (incl. Tracing, Replay) 69.02 KB (+0.07% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) 74.15 KB (+0.06% 🔺)
CDN Bundle - uncompressed 68.71 KB (+0.12% 🔺)
CDN Bundle (incl. Tracing) - uncompressed 103.3 KB (+0.08% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed 213.75 KB (+0.04% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 226.21 KB (+0.04% 🔺)
@sentry/nextjs (client) 35.63 KB (+0.12% 🔺)
@sentry/sveltekit (client) 33.86 KB (+0.11% 🔺)
@sentry/node 111.93 KB (0%)
@sentry/node - without tracing 89.4 KB (0%)
@sentry/aws-serverless 98.46 KB (0%)

@lforst
Copy link
Contributor Author

lforst commented Jun 6, 2024

We should test this on safari first, I know they have the worst stacktraces for fetch.

Do you know how to produce "failed to fetch" errors?

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Jun 6, 2024

Do you know how to produce "failed to fetch" errors?

Just add fetch("localhost:123/fake/endpoint"); where 123 is an invalid port. Chrome, Safari, and Firefox all throw different errors for this.

image

Copy link
Member

@Lms24 Lms24 left a 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).

@lforst lforst self-assigned this Jun 10, 2024
Comment on lines +20 to +21
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
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, 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.

Copy link
Member

@AbhiPrasad AbhiPrasad left a 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.

@lforst
Copy link
Contributor Author

lforst commented Jun 11, 2024

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 fetch('http://localhost:123/fake/endpoint/that/will/fail') at all. <- Fix number 1

Also, on Safari the fetch call from above will cause a TypeError: Load failed without a stack trace. The change from this PR will add a stack trace. <- Fix number 2

@lforst lforst dismissed AbhiPrasad’s stale review June 11, 2024 08:33

discussed offline: problems addressed

@lforst lforst merged commit 1014da2 into develop Jun 11, 2024
111 checks passed
@lforst lforst deleted the lforst-fix-failed-to-fetch branch June 11, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants