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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const config: PlaywrightTestConfig = {
testMatch: /test.ts/,

use: {
trace: process.env.CI ? 'retain-on-failure' : 'off',
trace: 'retain-on-failure',
},

projects: [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
fetch('http://localhost:123/fake/endpoint/that/will/fail');
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { expect } from '@playwright/test';
import type { Event } from '@sentry/types';

import { sentryTest } from '../../../../utils/fixtures';
import { getMultipleSentryEnvelopeRequests } from '../../../../utils/helpers';

sentryTest('should create errors with stack traces for failing fetch calls', async ({ getLocalTestPath, page }) => {
const url = await getLocalTestPath({ testDir: __dirname });

const envelopes = await getMultipleSentryEnvelopeRequests<Event>(page, 3, { url, timeout: 10000 });
const errorEvent = envelopes.find(event => !event.type)!;
expect(errorEvent?.exception?.values?.[0].stacktrace?.frames?.length).toBeGreaterThan(0);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,8 @@ sentryTest('should create spans for fetch requests', async ({ getLocalTestPath,

// We will wait 500ms for all envelopes to be sent. Generally, in all browsers, the last sent
// envelope contains tracing data.

// If we are on FF or webkit:
// 1st envelope contains CORS error
// 2nd envelope contains the tracing data we want to check here
const envelopes = await getMultipleSentryEnvelopeRequests<Event>(page, 2, { url, timeout: 10000 });
const tracingEvent = envelopes[envelopes.length - 1]; // last envelope contains tracing data on all browsers
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
Comment on lines +20 to +21
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.


const requestSpans = tracingEvent.spans?.filter(({ op }) => op === 'http.client');

Expand Down
22 changes: 21 additions & 1 deletion packages/utils/src/instrument/fetch.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import type { HandlerDataFetch } from '@sentry/types';

import { fill } from '../object';
import { isError } from '../is';
import { addNonEnumerableProperty, fill } from '../object';
import { supportsNativeFetch } from '../supports';
import { timestampInSeconds } from '../time';
import { GLOBAL_OBJ } from '../worldwide';
Expand Down Expand Up @@ -45,6 +46,15 @@ function instrumentFetch(): void {
...handlerData,
});

// We capture the stack right here and not in the Promise error callback because Safari (and probably other
// browsers too) will wipe the stack trace up to this point, only leaving us with this file which is useless.

// NOTE: If you are a Sentry user, and you are seeing this stack frame,
// 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.
const virtualStackTrace = new Error().stack;

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return originalFetch.apply(GLOBAL_OBJ, args).then(
(response: Response) => {
Expand All @@ -65,6 +75,16 @@ function instrumentFetch(): void {
};

triggerHandlers('fetch', erroredHandlerData);

if (isError(error) && error.stack === undefined) {
// NOTE: If you are a Sentry user, and you are seeing this stack frame,
// 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 = virtualStackTrace;
addNonEnumerableProperty(error, 'framesToPop', 1);
}

// NOTE: If you are a Sentry user, and you are seeing this stack frame,
// it means the sentry.javascript SDK caught an error invoking your application code.
// This is expected behavior and NOT indicative of a bug with sentry.javascript.
Expand Down
Loading