fix(utils): Handle when requests get aborted in fetch instrumentation#13202

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.

Sign up for GitHub

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
lforst merged 7 commits into from
Aug 5, 2024
Merged

fix(utils): Handle when requests get aborted in fetch instrumentation #13202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7da58d1
fix(utils): Handle when requests get aborted
AbhiPrasad Aug 2, 2024
6cdcd73
test: Add browser integration test for AbortController
chargome Aug 5, 2024
ac407b3
unflake
Aug 5, 2024
a7c8c1b
Merge remote-tracking branch 'origin/develop' into abhi-fetch-abort-c…
Aug 5, 2024
43c6200
Assert on console log
Aug 5, 2024
cd5e89d
this time actually
Aug 5, 2024
62f5fbd
skip non-tracing tests
Aug 5, 2024
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
8 changes: 8 additions & 0 deletions ...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
tracesSampleRate: 1,
});
36 changes: 36 additions & 0 deletions ...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
let controller;

const startFetch = e => {
controller = new AbortController();
const { signal } = controller;

Sentry.startSpan(
{
name: 'with-abort-controller',
forceTransaction: true,
},
async () => {
await fetch('http://localhost:7654/foo', { signal })
.then(response => response.json())
.then(data => {
console.log('Fetch succeeded:', data);
})
.catch(err => {
if (err.name === 'AbortError') {
console.log('Fetch aborted');
} else {
console.error('Fetch error:', err);
}
});
},
);
};

const abortFetch = e => {
if (controller) {
controller.abort();
}
};

document.querySelector('[data-test-id=start-button]').addEventListener('click', startFetch);
document.querySelector('[data-test-id=abort-button]').addEventListener('click', abortFetch);
10 changes: 10 additions & 0 deletions ...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
<!doctype html>
<html>
<head>
<meta charset="utf-8" />
</head>
<body>
<button data-test-id="start-button">Start fetch</button>
<button data-test-id="abort-button">Abort fetch</button>
</body>
</html>
41 changes: 41 additions & 0 deletions ...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';
import type { Event as SentryEvent } from '@sentry/types';
import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';

sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

const url = await getLocalTestPath({ testDir: __dirname });

await page.route('**/foo', async () => {
// never fulfil this route because we abort the request as part of the test
});

const transactionEventPromise = getFirstSentryEnvelopeRequest<SentryEvent>(page);

const hasAbortedFetchPromise = new Promise<void>(resolve => {
page.on('console', msg => {
if (msg.type() === 'log' && msg.text() === 'Fetch aborted') {
resolve();
}
});
});

await page.goto(url);

await page.locator('[data-test-id=start-button]').click();
await page.locator('[data-test-id=abort-button]').click();

const transactionEvent = await transactionEventPromise;

// assert that fetch calls do not return undefined
const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter(
({ category, data }) => category === 'fetch' && data === undefined,
);
expect(fetchBreadcrumbs).toHaveLength(0);

await expect(hasAbortedFetchPromise).resolves.toBeUndefined();
});
14 changes: 12 additions & 2 deletions dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,17 +2,24 @@ import * as Sentry from '@sentry/react';
// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX
import * as React from 'react';

const fetchSSE = async ({ timeout}: { timeout: boolean }) => {
const fetchSSE = async ({ timeout, abort = false }: { timeout: boolean; abort?: boolean }) => {
Sentry.startSpanManual({ name: 'sse stream using fetch' }, async span => {
const controller = new AbortController();

const res = await Sentry.startSpan({ name: 'sse fetch call' }, async () => {
const endpoint = `http://localhost:8080/${timeout ? 'sse-timeout' : 'sse'}`;
return await fetch(endpoint);

const signal = controller.signal;
return await fetch(endpoint, { signal });
});

const stream = res.body;
const reader = stream?.getReader();

const readChunk = async () => {
if (abort) {
controller.abort();
}
const readRes = await reader?.read();
if (readRes?.done) {
return;
Expand DownExpand Up@@ -42,6 +49,9 @@ const SSE = () => {
<button id="fetch-timeout-button" onClick={() => fetchSSE({ timeout: true })}>
Fetch timeout SSE
</button>
<button id="fetch-sse-abort" onClick={() => fetchSSE({ timeout: false, abort: true })}>
Fetch SSE with error
</button>
</>
);
};
Expand Down
37 changes: 37 additions & 0 deletions dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -34,6 +34,43 @@ test('Waits for sse streaming when creating spans', async ({ page }) => {
expect(resolveBodyDuration).toBe(2);
});

test('Waits for sse streaming when sse has been explicitly aborted', async ({ page }) => {
await page.goto('/sse');

const transactionPromise = waitForTransaction('react-router-6', async transactionEvent => {
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
});

const fetchButton = page.locator('id=fetch-sse-abort');
await fetchButton.click();

const rootSpan = await transactionPromise;
console.log(JSON.stringify(rootSpan, null, 2));
const sseFetchCall = rootSpan.spans?.filter(span => span.description === 'sse fetch call')[0] as SpanJSON;
const httpGet = rootSpan.spans?.filter(span => span.description === 'GET http://localhost:8080/sse')[0] as SpanJSON;

expect(sseFetchCall).toBeDefined();
expect(httpGet).toBeDefined();

expect(sseFetchCall?.timestamp).toBeDefined();
expect(sseFetchCall?.start_timestamp).toBeDefined();
expect(httpGet?.timestamp).toBeDefined();
expect(httpGet?.start_timestamp).toBeDefined();

// http headers get sent instantly from the server
const resolveDuration = Math.round((sseFetchCall.timestamp as number) - sseFetchCall.start_timestamp);

// body streams after 0s because it has been aborted
const resolveBodyDuration = Math.round((httpGet.timestamp as number) - httpGet.start_timestamp);

expect(resolveDuration).toBe(0);
expect(resolveBodyDuration).toBe(0);

// validate abort eror was thrown by inspecting console
const consoleBreadcrumb = rootSpan.breadcrumbs?.find(breadcrumb => breadcrumb.category === 'console');
expect(consoleBreadcrumb?.message).toBe('Could not fetch sse AbortError: BodyStreamBuffer was aborted');
});

test('Aborts when stream takes longer than 5s', async ({ page }) => {
await page.goto('/sse');

Expand Down
59 changes: 25 additions & 34 deletions packages/utils/src/instrument/fetch.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -80,47 +80,42 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
if (onFetchResolved) {
onFetchResolved(response);
} else {
const finishedHandlerData: HandlerDataFetch = {
triggerHandlers('fetch', {
...handlerData,
endTimestamp: timestampInSeconds() * 1000,
response,
};
triggerHandlers('fetch', finishedHandlerData);
});
}

return response;
},
(error: Error) => {
if (!onFetchResolved) {
const erroredHandlerData: HandlerDataFetch = {
...handlerData,
endTimestamp: timestampInSeconds() * 1000,
error,
};

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);
}
triggerHandlers('fetch', {
...handlerData,
endTimestamp: timestampInSeconds() * 1000,
error,
});

if (isError(error) && error.stack === undefined) {
// 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.
throw error;
// 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.
throw error;

@amosklamosklAug 25, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@lforst this is a breaking change for apps that do not handle fetch exceptions :( as they did not get the exceptions before and now they do....

@lforstlforstAug 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you elaborate? Ideally in a new issue. I do not think this alters behaviour. This actually fixes a bug where we swallowed errors.

},
);
};
});
}

function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void {
async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise<void> {
if (res && res.body) {
const responseReader = res.body.getReader();

Expand All@@ -146,25 +141,21 @@ function resolveResponse(res: Response | undefined, onFinishedResolving: () => v
}
}

responseReader
return responseReader
.read()
.then(consumeChunks)
.then(() => {
onFinishedResolving();
})
.catch(() => {
// noop
});
.then(onFinishedResolving)
.catch(() => undefined);
}
}

async function streamHandler(response: Response): Promise<void> {
// clone response for awaiting stream
let clonedResponseForResolving: Response | undefined;
let clonedResponseForResolving: Response;
try {
clonedResponseForResolving = response.clone();
} catch (e) {
// noop
} catch {
return;
}

await resolveResponse(clonedResponseForResolving, () => {
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
fix(utils): Handle when requests get aborted in fetch instrumentation#13202

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.

 Sign up for GitHub

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

lforst
merged 7 commits into

from

Aug 5, 2024
 Merged


fix(utils): Handle when requests get aborted in fetch instrumentation
#13202
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
7da58d1

fix(utils): Handle when requests get aborted


AbhiPrasad Aug 2, 2024
6cdcd73

test: Add browser integration test for AbortController


chargome Aug 5, 2024
ac407b3

unflake

Aug 5, 2024
a7c8c1b

Merge remote-tracking branch 'origin/develop' into abhi-fetch-abort-c…

Aug 5, 2024
43c6200

Assert on console log

Aug 5, 2024
cd5e89d

this time actually

Aug 5, 2024
62f5fbd

skip non-tracing tests

Aug 5, 2024

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


8 changes: 8 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
 dsn: 'https://public@dsn.ingest.sentry.io/1337',
 tracesSampleRate: 1,
});

36 changes: 36 additions & 0 deletions
...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
let controller;

const startFetch = e => {
 controller = new AbortController();
 const { signal } = controller;

 Sentry.startSpan(
 {
 name: 'with-abort-controller',
 forceTransaction: true,
 },
 async () => {
 await fetch('http://localhost:7654/foo', { signal })
 .then(response => response.json())
 .then(data => {
 console.log('Fetch succeeded:', data);
 })
 .catch(err => {
 if (err.name === 'AbortError') {
 console.log('Fetch aborted');
 } else {
 console.error('Fetch error:', err);
 }
 });
 },
 );
};

const abortFetch = e => {
 if (controller) {
 controller.abort();
 }
};

document.querySelector('[data-test-id=start-button]').addEventListener('click', startFetch);
document.querySelector('[data-test-id=abort-button]').addEventListener('click', abortFetch);

10 changes: 10 additions & 0 deletions
...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
<!doctype html>
<html>
 <head>
 <meta charset="utf-8" />
 </head>
 <body>
 <button data-test-id="start-button">Start fetch</button>
 <button data-test-id="abort-button">Abort fetch</button>
 </body>
</html>

41 changes: 41 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';
import type { Event as SentryEvent } from '@sentry/types';
import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';

sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => {
 if (shouldSkipTracingTest()) {
 sentryTest.skip();
 }

 const url = await getLocalTestPath({ testDir: __dirname });

 await page.route('**/foo', async () => {
 // never fulfil this route because we abort the request as part of the test
 });

 const transactionEventPromise = getFirstSentryEnvelopeRequest<SentryEvent>(page);

 const hasAbortedFetchPromise = new Promise<void>(resolve => {
 page.on('console', msg => {
 if (msg.type() === 'log' && msg.text() === 'Fetch aborted') {
 resolve();
 }
 });
 });

 await page.goto(url);

 await page.locator('[data-test-id=start-button]').click();
 await page.locator('[data-test-id=abort-button]').click();

 const transactionEvent = await transactionEventPromise;

 // assert that fetch calls do not return undefined
 const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter(
 ({ category, data }) => category === 'fetch' && data === undefined,
 );
 expect(fetchBreadcrumbs).toHaveLength(0);

 await expect(hasAbortedFetchPromise).resolves.toBeUndefined();
});

14 changes: 12 additions & 2 deletions
dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,17 +2,24 @@ import * as Sentry from '@sentry/react';
// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX
import * as React from 'react';

const fetchSSE = async ({ timeout}: { timeout: boolean }) => {
const fetchSSE = async ({ timeout, abort = false }: { timeout: boolean; abort?: boolean }) => {
 Sentry.startSpanManual({ name: 'sse stream using fetch' }, async span => {
 const controller = new AbortController();

 const res = await Sentry.startSpan({ name: 'sse fetch call' }, async () => {
 const endpoint = `http://localhost:8080/${timeout ? 'sse-timeout' : 'sse'}`;
 return await fetch(endpoint);

 const signal = controller.signal;
 return await fetch(endpoint, { signal });
 });

 const stream = res.body;
 const reader = stream?.getReader();

 const readChunk = async () => {
 if (abort) {
 controller.abort();
 }
 const readRes = await reader?.read();
 if (readRes?.done) {
 return;
Expand DownExpand Up@@ -42,6 +49,9 @@ const SSE = () => {
 <button id="fetch-timeout-button" onClick={() => fetchSSE({ timeout: true })}>
 Fetch timeout SSE
 </button>
 <button id="fetch-sse-abort" onClick={() => fetchSSE({ timeout: false, abort: true })}>
 Fetch SSE with error
 </button>
 </>
 );
};
Expand Down

37 changes: 37 additions & 0 deletions
dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -34,6 +34,43 @@ test('Waits for sse streaming when creating spans', async ({ page }) => {
 expect(resolveBodyDuration).toBe(2);
});

test('Waits for sse streaming when sse has been explicitly aborted', async ({ page }) => {
 await page.goto('/sse');

 const transactionPromise = waitForTransaction('react-router-6', async transactionEvent => {
 return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
 });

 const fetchButton = page.locator('id=fetch-sse-abort');
 await fetchButton.click();

 const rootSpan = await transactionPromise;
 console.log(JSON.stringify(rootSpan, null, 2));
 const sseFetchCall = rootSpan.spans?.filter(span => span.description === 'sse fetch call')[0] as SpanJSON;
 const httpGet = rootSpan.spans?.filter(span => span.description === 'GET http://localhost:8080/sse')[0] as SpanJSON;

 expect(sseFetchCall).toBeDefined();
 expect(httpGet).toBeDefined();

 expect(sseFetchCall?.timestamp).toBeDefined();
 expect(sseFetchCall?.start_timestamp).toBeDefined();
 expect(httpGet?.timestamp).toBeDefined();
 expect(httpGet?.start_timestamp).toBeDefined();

 // http headers get sent instantly from the server
 const resolveDuration = Math.round((sseFetchCall.timestamp as number) - sseFetchCall.start_timestamp);

 // body streams after 0s because it has been aborted
 const resolveBodyDuration = Math.round((httpGet.timestamp as number) - httpGet.start_timestamp);

 expect(resolveDuration).toBe(0);
 expect(resolveBodyDuration).toBe(0);

 // validate abort eror was thrown by inspecting console
 const consoleBreadcrumb = rootSpan.breadcrumbs?.find(breadcrumb => breadcrumb.category === 'console');
 expect(consoleBreadcrumb?.message).toBe('Could not fetch sse AbortError: BodyStreamBuffer was aborted');
});

test('Aborts when stream takes longer than 5s', async ({ page }) => {
 await page.goto('/sse');

Expand Down

59 changes: 25 additions & 34 deletions
packages/utils/src/instrument/fetch.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -80,47 +80,42 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
 if (onFetchResolved) {
 onFetchResolved(response);
 } else {
const finishedHandlerData: HandlerDataFetch = {
triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 response,
 };
 triggerHandlers('fetch', finishedHandlerData);
 });
 }

 return response;
 },
 (error: Error) => {
 if (!onFetchResolved) {
 const erroredHandlerData: HandlerDataFetch = {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 };

 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);
 }
 triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 });

 if (isError(error) && error.stack === undefined) {
 // 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.
 throw error;
 // 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.
 throw error;
@amosklamosklAug 25, 2024

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

@lforst this is a breaking change for apps that do not handle fetch exceptions :( as they did not get the exceptions before and now they do....
@lforstlforstAug 26, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you elaborate? Ideally in a new issue. I do not think this alters behaviour. This actually fixes a bug where we swallowed errors.
 },
 );
 };
 });
}

function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void {
async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise<void> {
 if (res && res.body) {
 const responseReader = res.body.getReader();

Expand All@@ -146,25 +141,21 @@ function resolveResponse(res: Response | undefined, onFinishedResolving: () => v
 }
 }

 responseReader
return responseReader
 .read()
 .then(consumeChunks)
 .then(() => {
 onFinishedResolving();
 })
 .catch(() => {
 // noop
 });
 .then(onFinishedResolving)
 .catch(() => undefined);
 }
}

async function streamHandler(response: Response): Promise<void> {
 // clone response for awaiting stream
 let clonedResponseForResolving: Response | undefined;
 let clonedResponseForResolving: Response;
 try {
 clonedResponseForResolving = response.clone();
 } catch (e) {
// noop
 } catch {
return;
 }

 await resolveResponse(clonedResponseForResolving, () => {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix(utils): Handle when requests get aborted in fetch instrumentation#13202

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.

 Sign up for GitHub

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

lforst
merged 7 commits into

from

Aug 5, 2024
 Merged


fix(utils): Handle when requests get aborted in fetch instrumentation
#13202
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
7da58d1

fix(utils): Handle when requests get aborted


AbhiPrasad Aug 2, 2024
6cdcd73

test: Add browser integration test for AbortController


chargome Aug 5, 2024
ac407b3

unflake

Aug 5, 2024
a7c8c1b

Merge remote-tracking branch 'origin/develop' into abhi-fetch-abort-c…

Aug 5, 2024
43c6200

Assert on console log

Aug 5, 2024
cd5e89d

this time actually

Aug 5, 2024
62f5fbd

skip non-tracing tests

Aug 5, 2024

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


8 changes: 8 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
 dsn: 'https://public@dsn.ingest.sentry.io/1337',
 tracesSampleRate: 1,
});

36 changes: 36 additions & 0 deletions
...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
let controller;

const startFetch = e => {
 controller = new AbortController();
 const { signal } = controller;

 Sentry.startSpan(
 {
 name: 'with-abort-controller',
 forceTransaction: true,
 },
 async () => {
 await fetch('http://localhost:7654/foo', { signal })
 .then(response => response.json())
 .then(data => {
 console.log('Fetch succeeded:', data);
 })
 .catch(err => {
 if (err.name === 'AbortError') {
 console.log('Fetch aborted');
 } else {
 console.error('Fetch error:', err);
 }
 });
 },
 );
};

const abortFetch = e => {
 if (controller) {
 controller.abort();
 }
};

document.querySelector('[data-test-id=start-button]').addEventListener('click', startFetch);
document.querySelector('[data-test-id=abort-button]').addEventListener('click', abortFetch);

10 changes: 10 additions & 0 deletions
...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
<!doctype html>
<html>
 <head>
 <meta charset="utf-8" />
 </head>
 <body>
 <button data-test-id="start-button">Start fetch</button>
 <button data-test-id="abort-button">Abort fetch</button>
 </body>
</html>

41 changes: 41 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';
import type { Event as SentryEvent } from '@sentry/types';
import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';

sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => {
 if (shouldSkipTracingTest()) {
 sentryTest.skip();
 }

 const url = await getLocalTestPath({ testDir: __dirname });

 await page.route('**/foo', async () => {
 // never fulfil this route because we abort the request as part of the test
 });

 const transactionEventPromise = getFirstSentryEnvelopeRequest<SentryEvent>(page);

 const hasAbortedFetchPromise = new Promise<void>(resolve => {
 page.on('console', msg => {
 if (msg.type() === 'log' && msg.text() === 'Fetch aborted') {
 resolve();
 }
 });
 });

 await page.goto(url);

 await page.locator('[data-test-id=start-button]').click();
 await page.locator('[data-test-id=abort-button]').click();

 const transactionEvent = await transactionEventPromise;

 // assert that fetch calls do not return undefined
 const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter(
 ({ category, data }) => category === 'fetch' && data === undefined,
 );
 expect(fetchBreadcrumbs).toHaveLength(0);

 await expect(hasAbortedFetchPromise).resolves.toBeUndefined();
});

14 changes: 12 additions & 2 deletions
dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,17 +2,24 @@ import * as Sentry from '@sentry/react';
// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX
import * as React from 'react';

const fetchSSE = async ({ timeout}: { timeout: boolean }) => {
const fetchSSE = async ({ timeout, abort = false }: { timeout: boolean; abort?: boolean }) => {
 Sentry.startSpanManual({ name: 'sse stream using fetch' }, async span => {
 const controller = new AbortController();

 const res = await Sentry.startSpan({ name: 'sse fetch call' }, async () => {
 const endpoint = `http://localhost:8080/${timeout ? 'sse-timeout' : 'sse'}`;
 return await fetch(endpoint);

 const signal = controller.signal;
 return await fetch(endpoint, { signal });
 });

 const stream = res.body;
 const reader = stream?.getReader();

 const readChunk = async () => {
 if (abort) {
 controller.abort();
 }
 const readRes = await reader?.read();
 if (readRes?.done) {
 return;
Expand DownExpand Up@@ -42,6 +49,9 @@ const SSE = () => {
 <button id="fetch-timeout-button" onClick={() => fetchSSE({ timeout: true })}>
 Fetch timeout SSE
 </button>
 <button id="fetch-sse-abort" onClick={() => fetchSSE({ timeout: false, abort: true })}>
 Fetch SSE with error
 </button>
 </>
 );
};
Expand Down

37 changes: 37 additions & 0 deletions
dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -34,6 +34,43 @@ test('Waits for sse streaming when creating spans', async ({ page }) => {
 expect(resolveBodyDuration).toBe(2);
});

test('Waits for sse streaming when sse has been explicitly aborted', async ({ page }) => {
 await page.goto('/sse');

 const transactionPromise = waitForTransaction('react-router-6', async transactionEvent => {
 return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
 });

 const fetchButton = page.locator('id=fetch-sse-abort');
 await fetchButton.click();

 const rootSpan = await transactionPromise;
 console.log(JSON.stringify(rootSpan, null, 2));
 const sseFetchCall = rootSpan.spans?.filter(span => span.description === 'sse fetch call')[0] as SpanJSON;
 const httpGet = rootSpan.spans?.filter(span => span.description === 'GET http://localhost:8080/sse')[0] as SpanJSON;

 expect(sseFetchCall).toBeDefined();
 expect(httpGet).toBeDefined();

 expect(sseFetchCall?.timestamp).toBeDefined();
 expect(sseFetchCall?.start_timestamp).toBeDefined();
 expect(httpGet?.timestamp).toBeDefined();
 expect(httpGet?.start_timestamp).toBeDefined();

 // http headers get sent instantly from the server
 const resolveDuration = Math.round((sseFetchCall.timestamp as number) - sseFetchCall.start_timestamp);

 // body streams after 0s because it has been aborted
 const resolveBodyDuration = Math.round((httpGet.timestamp as number) - httpGet.start_timestamp);

 expect(resolveDuration).toBe(0);
 expect(resolveBodyDuration).toBe(0);

 // validate abort eror was thrown by inspecting console
 const consoleBreadcrumb = rootSpan.breadcrumbs?.find(breadcrumb => breadcrumb.category === 'console');
 expect(consoleBreadcrumb?.message).toBe('Could not fetch sse AbortError: BodyStreamBuffer was aborted');
});

test('Aborts when stream takes longer than 5s', async ({ page }) => {
 await page.goto('/sse');

Expand Down

59 changes: 25 additions & 34 deletions
packages/utils/src/instrument/fetch.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -80,47 +80,42 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
 if (onFetchResolved) {
 onFetchResolved(response);
 } else {
const finishedHandlerData: HandlerDataFetch = {
triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 response,
 };
 triggerHandlers('fetch', finishedHandlerData);
 });
 }

 return response;
 },
 (error: Error) => {
 if (!onFetchResolved) {
 const erroredHandlerData: HandlerDataFetch = {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 };

 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);
 }
 triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 });

 if (isError(error) && error.stack === undefined) {
 // 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.
 throw error;
 // 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.
 throw error;
@amosklamosklAug 25, 2024

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

@lforst this is a breaking change for apps that do not handle fetch exceptions :( as they did not get the exceptions before and now they do....
@lforstlforstAug 26, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you elaborate? Ideally in a new issue. I do not think this alters behaviour. This actually fixes a bug where we swallowed errors.
 },
 );
 };
 });
}

function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void {
async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise<void> {
 if (res && res.body) {
 const responseReader = res.body.getReader();

Expand All@@ -146,25 +141,21 @@ function resolveResponse(res: Response | undefined, onFinishedResolving: () => v
 }
 }

 responseReader
return responseReader
 .read()
 .then(consumeChunks)
 .then(() => {
 onFinishedResolving();
 })
 .catch(() => {
 // noop
 });
 .then(onFinishedResolving)
 .catch(() => undefined);
 }
}

async function streamHandler(response: Response): Promise<void> {
 // clone response for awaiting stream
 let clonedResponseForResolving: Response | undefined;
 let clonedResponseForResolving: Response;
 try {
 clonedResponseForResolving = response.clone();
 } catch (e) {
// noop
 } catch {
return;
 }

 await resolveResponse(clonedResponseForResolving, () => {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix(utils): Handle when requests get aborted in fetch instrumentation#13202

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.

 Sign up for GitHub

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

lforst
merged 7 commits into

from

Aug 5, 2024
 Merged


fix(utils): Handle when requests get aborted in fetch instrumentation
#13202
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
7da58d1

fix(utils): Handle when requests get aborted


AbhiPrasad Aug 2, 2024
6cdcd73

test: Add browser integration test for AbortController


chargome Aug 5, 2024
ac407b3

unflake

Aug 5, 2024
a7c8c1b

Merge remote-tracking branch 'origin/develop' into abhi-fetch-abort-c…

Aug 5, 2024
43c6200

Assert on console log

Aug 5, 2024
cd5e89d

this time actually

Aug 5, 2024
62f5fbd

skip non-tracing tests

Aug 5, 2024

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


8 changes: 8 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
 dsn: 'https://public@dsn.ingest.sentry.io/1337',
 tracesSampleRate: 1,
});

36 changes: 36 additions & 0 deletions
...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
let controller;

const startFetch = e => {
 controller = new AbortController();
 const { signal } = controller;

 Sentry.startSpan(
 {
 name: 'with-abort-controller',
 forceTransaction: true,
 },
 async () => {
 await fetch('http://localhost:7654/foo', { signal })
 .then(response => response.json())
 .then(data => {
 console.log('Fetch succeeded:', data);
 })
 .catch(err => {
 if (err.name === 'AbortError') {
 console.log('Fetch aborted');
 } else {
 console.error('Fetch error:', err);
 }
 });
 },
 );
};

const abortFetch = e => {
 if (controller) {
 controller.abort();
 }
};

document.querySelector('[data-test-id=start-button]').addEventListener('click', startFetch);
document.querySelector('[data-test-id=abort-button]').addEventListener('click', abortFetch);

10 changes: 10 additions & 0 deletions
...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
<!doctype html>
<html>
 <head>
 <meta charset="utf-8" />
 </head>
 <body>
 <button data-test-id="start-button">Start fetch</button>
 <button data-test-id="abort-button">Abort fetch</button>
 </body>
</html>

41 changes: 41 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';
import type { Event as SentryEvent } from '@sentry/types';
import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';

sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => {
 if (shouldSkipTracingTest()) {
 sentryTest.skip();
 }

 const url = await getLocalTestPath({ testDir: __dirname });

 await page.route('**/foo', async () => {
 // never fulfil this route because we abort the request as part of the test
 });

 const transactionEventPromise = getFirstSentryEnvelopeRequest<SentryEvent>(page);

 const hasAbortedFetchPromise = new Promise<void>(resolve => {
 page.on('console', msg => {
 if (msg.type() === 'log' && msg.text() === 'Fetch aborted') {
 resolve();
 }
 });
 });

 await page.goto(url);

 await page.locator('[data-test-id=start-button]').click();
 await page.locator('[data-test-id=abort-button]').click();

 const transactionEvent = await transactionEventPromise;

 // assert that fetch calls do not return undefined
 const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter(
 ({ category, data }) => category === 'fetch' && data === undefined,
 );
 expect(fetchBreadcrumbs).toHaveLength(0);

 await expect(hasAbortedFetchPromise).resolves.toBeUndefined();
});

14 changes: 12 additions & 2 deletions
dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,17 +2,24 @@ import * as Sentry from '@sentry/react';
// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX
import * as React from 'react';

const fetchSSE = async ({ timeout}: { timeout: boolean }) => {
const fetchSSE = async ({ timeout, abort = false }: { timeout: boolean; abort?: boolean }) => {
 Sentry.startSpanManual({ name: 'sse stream using fetch' }, async span => {
 const controller = new AbortController();

 const res = await Sentry.startSpan({ name: 'sse fetch call' }, async () => {
 const endpoint = `http://localhost:8080/${timeout ? 'sse-timeout' : 'sse'}`;
 return await fetch(endpoint);

 const signal = controller.signal;
 return await fetch(endpoint, { signal });
 });

 const stream = res.body;
 const reader = stream?.getReader();

 const readChunk = async () => {
 if (abort) {
 controller.abort();
 }
 const readRes = await reader?.read();
 if (readRes?.done) {
 return;
Expand DownExpand Up@@ -42,6 +49,9 @@ const SSE = () => {
 <button id="fetch-timeout-button" onClick={() => fetchSSE({ timeout: true })}>
 Fetch timeout SSE
 </button>
 <button id="fetch-sse-abort" onClick={() => fetchSSE({ timeout: false, abort: true })}>
 Fetch SSE with error
 </button>
 </>
 );
};
Expand Down

37 changes: 37 additions & 0 deletions
dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -34,6 +34,43 @@ test('Waits for sse streaming when creating spans', async ({ page }) => {
 expect(resolveBodyDuration).toBe(2);
});

test('Waits for sse streaming when sse has been explicitly aborted', async ({ page }) => {
 await page.goto('/sse');

 const transactionPromise = waitForTransaction('react-router-6', async transactionEvent => {
 return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
 });

 const fetchButton = page.locator('id=fetch-sse-abort');
 await fetchButton.click();

 const rootSpan = await transactionPromise;
 console.log(JSON.stringify(rootSpan, null, 2));
 const sseFetchCall = rootSpan.spans?.filter(span => span.description === 'sse fetch call')[0] as SpanJSON;
 const httpGet = rootSpan.spans?.filter(span => span.description === 'GET http://localhost:8080/sse')[0] as SpanJSON;

 expect(sseFetchCall).toBeDefined();
 expect(httpGet).toBeDefined();

 expect(sseFetchCall?.timestamp).toBeDefined();
 expect(sseFetchCall?.start_timestamp).toBeDefined();
 expect(httpGet?.timestamp).toBeDefined();
 expect(httpGet?.start_timestamp).toBeDefined();

 // http headers get sent instantly from the server
 const resolveDuration = Math.round((sseFetchCall.timestamp as number) - sseFetchCall.start_timestamp);

 // body streams after 0s because it has been aborted
 const resolveBodyDuration = Math.round((httpGet.timestamp as number) - httpGet.start_timestamp);

 expect(resolveDuration).toBe(0);
 expect(resolveBodyDuration).toBe(0);

 // validate abort eror was thrown by inspecting console
 const consoleBreadcrumb = rootSpan.breadcrumbs?.find(breadcrumb => breadcrumb.category === 'console');
 expect(consoleBreadcrumb?.message).toBe('Could not fetch sse AbortError: BodyStreamBuffer was aborted');
});

test('Aborts when stream takes longer than 5s', async ({ page }) => {
 await page.goto('/sse');

Expand Down

59 changes: 25 additions & 34 deletions
packages/utils/src/instrument/fetch.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -80,47 +80,42 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
 if (onFetchResolved) {
 onFetchResolved(response);
 } else {
const finishedHandlerData: HandlerDataFetch = {
triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 response,
 };
 triggerHandlers('fetch', finishedHandlerData);
 });
 }

 return response;
 },
 (error: Error) => {
 if (!onFetchResolved) {
 const erroredHandlerData: HandlerDataFetch = {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 };

 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);
 }
 triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 });

 if (isError(error) && error.stack === undefined) {
 // 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.
 throw error;
 // 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.
 throw error;
@amosklamosklAug 25, 2024

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

@lforst this is a breaking change for apps that do not handle fetch exceptions :( as they did not get the exceptions before and now they do....
@lforstlforstAug 26, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you elaborate? Ideally in a new issue. I do not think this alters behaviour. This actually fixes a bug where we swallowed errors.
 },
 );
 };
 });
}

function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void {
async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise<void> {
 if (res && res.body) {
 const responseReader = res.body.getReader();

Expand All@@ -146,25 +141,21 @@ function resolveResponse(res: Response | undefined, onFinishedResolving: () => v
 }
 }

 responseReader
return responseReader
 .read()
 .then(consumeChunks)
 .then(() => {
 onFinishedResolving();
 })
 .catch(() => {
 // noop
 });
 .then(onFinishedResolving)
 .catch(() => undefined);
 }
}

async function streamHandler(response: Response): Promise<void> {
 // clone response for awaiting stream
 let clonedResponseForResolving: Response | undefined;
 let clonedResponseForResolving: Response;
 try {
 clonedResponseForResolving = response.clone();
 } catch (e) {
// noop
 } catch {
return;
 }

 await resolveResponse(clonedResponseForResolving, () => {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
fix(utils): Handle when requests get aborted in fetch instrumentation#13202

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.

 Sign up for GitHub

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

lforst
merged 7 commits into

from

Aug 5, 2024
 Merged


fix(utils): Handle when requests get aborted in fetch instrumentation
#13202
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
7da58d1

fix(utils): Handle when requests get aborted


AbhiPrasad Aug 2, 2024
6cdcd73

test: Add browser integration test for AbortController


chargome Aug 5, 2024
ac407b3

unflake

Aug 5, 2024
a7c8c1b

Merge remote-tracking branch 'origin/develop' into abhi-fetch-abort-c…

Aug 5, 2024
43c6200

Assert on console log

Aug 5, 2024
cd5e89d

this time actually

Aug 5, 2024
62f5fbd

skip non-tracing tests

Aug 5, 2024

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


8 changes: 8 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
 dsn: 'https://public@dsn.ingest.sentry.io/1337',
 tracesSampleRate: 1,
});

36 changes: 36 additions & 0 deletions
...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
let controller;

const startFetch = e => {
 controller = new AbortController();
 const { signal } = controller;

 Sentry.startSpan(
 {
 name: 'with-abort-controller',
 forceTransaction: true,
 },
 async () => {
 await fetch('http://localhost:7654/foo', { signal })
 .then(response => response.json())
 .then(data => {
 console.log('Fetch succeeded:', data);
 })
 .catch(err => {
 if (err.name === 'AbortError') {
 console.log('Fetch aborted');
 } else {
 console.error('Fetch error:', err);
 }
 });
 },
 );
};

const abortFetch = e => {
 if (controller) {
 controller.abort();
 }
};

document.querySelector('[data-test-id=start-button]').addEventListener('click', startFetch);
document.querySelector('[data-test-id=abort-button]').addEventListener('click', abortFetch);

10 changes: 10 additions & 0 deletions
...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
<!doctype html>
<html>
 <head>
 <meta charset="utf-8" />
 </head>
 <body>
 <button data-test-id="start-button">Start fetch</button>
 <button data-test-id="abort-button">Abort fetch</button>
 </body>
</html>

41 changes: 41 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';
import type { Event as SentryEvent } from '@sentry/types';
import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';

sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => {
 if (shouldSkipTracingTest()) {
 sentryTest.skip();
 }

 const url = await getLocalTestPath({ testDir: __dirname });

 await page.route('**/foo', async () => {
 // never fulfil this route because we abort the request as part of the test
 });

 const transactionEventPromise = getFirstSentryEnvelopeRequest<SentryEvent>(page);

 const hasAbortedFetchPromise = new Promise<void>(resolve => {
 page.on('console', msg => {
 if (msg.type() === 'log' && msg.text() === 'Fetch aborted') {
 resolve();
 }
 });
 });

 await page.goto(url);

 await page.locator('[data-test-id=start-button]').click();
 await page.locator('[data-test-id=abort-button]').click();

 const transactionEvent = await transactionEventPromise;

 // assert that fetch calls do not return undefined
 const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter(
 ({ category, data }) => category === 'fetch' && data === undefined,
 );
 expect(fetchBreadcrumbs).toHaveLength(0);

 await expect(hasAbortedFetchPromise).resolves.toBeUndefined();
});

14 changes: 12 additions & 2 deletions
dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,17 +2,24 @@ import * as Sentry from '@sentry/react';
// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX
import * as React from 'react';

const fetchSSE = async ({ timeout}: { timeout: boolean }) => {
const fetchSSE = async ({ timeout, abort = false }: { timeout: boolean; abort?: boolean }) => {
 Sentry.startSpanManual({ name: 'sse stream using fetch' }, async span => {
 const controller = new AbortController();

 const res = await Sentry.startSpan({ name: 'sse fetch call' }, async () => {
 const endpoint = `http://localhost:8080/${timeout ? 'sse-timeout' : 'sse'}`;
 return await fetch(endpoint);

 const signal = controller.signal;
 return await fetch(endpoint, { signal });
 });

 const stream = res.body;
 const reader = stream?.getReader();

 const readChunk = async () => {
 if (abort) {
 controller.abort();
 }
 const readRes = await reader?.read();
 if (readRes?.done) {
 return;
Expand DownExpand Up@@ -42,6 +49,9 @@ const SSE = () => {
 <button id="fetch-timeout-button" onClick={() => fetchSSE({ timeout: true })}>
 Fetch timeout SSE
 </button>
 <button id="fetch-sse-abort" onClick={() => fetchSSE({ timeout: false, abort: true })}>
 Fetch SSE with error
 </button>
 </>
 );
};
Expand Down

37 changes: 37 additions & 0 deletions
dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -34,6 +34,43 @@ test('Waits for sse streaming when creating spans', async ({ page }) => {
 expect(resolveBodyDuration).toBe(2);
});

test('Waits for sse streaming when sse has been explicitly aborted', async ({ page }) => {
 await page.goto('/sse');

 const transactionPromise = waitForTransaction('react-router-6', async transactionEvent => {
 return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
 });

 const fetchButton = page.locator('id=fetch-sse-abort');
 await fetchButton.click();

 const rootSpan = await transactionPromise;
 console.log(JSON.stringify(rootSpan, null, 2));
 const sseFetchCall = rootSpan.spans?.filter(span => span.description === 'sse fetch call')[0] as SpanJSON;
 const httpGet = rootSpan.spans?.filter(span => span.description === 'GET http://localhost:8080/sse')[0] as SpanJSON;

 expect(sseFetchCall).toBeDefined();
 expect(httpGet).toBeDefined();

 expect(sseFetchCall?.timestamp).toBeDefined();
 expect(sseFetchCall?.start_timestamp).toBeDefined();
 expect(httpGet?.timestamp).toBeDefined();
 expect(httpGet?.start_timestamp).toBeDefined();

 // http headers get sent instantly from the server
 const resolveDuration = Math.round((sseFetchCall.timestamp as number) - sseFetchCall.start_timestamp);

 // body streams after 0s because it has been aborted
 const resolveBodyDuration = Math.round((httpGet.timestamp as number) - httpGet.start_timestamp);

 expect(resolveDuration).toBe(0);
 expect(resolveBodyDuration).toBe(0);

 // validate abort eror was thrown by inspecting console
 const consoleBreadcrumb = rootSpan.breadcrumbs?.find(breadcrumb => breadcrumb.category === 'console');
 expect(consoleBreadcrumb?.message).toBe('Could not fetch sse AbortError: BodyStreamBuffer was aborted');
});

test('Aborts when stream takes longer than 5s', async ({ page }) => {
 await page.goto('/sse');

Expand Down

59 changes: 25 additions & 34 deletions
packages/utils/src/instrument/fetch.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -80,47 +80,42 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
 if (onFetchResolved) {
 onFetchResolved(response);
 } else {
const finishedHandlerData: HandlerDataFetch = {
triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 response,
 };
 triggerHandlers('fetch', finishedHandlerData);
 });
 }

 return response;
 },
 (error: Error) => {
 if (!onFetchResolved) {
 const erroredHandlerData: HandlerDataFetch = {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 };

 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);
 }
 triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 });

 if (isError(error) && error.stack === undefined) {
 // 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.
 throw error;
 // 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.
 throw error;
@amosklamosklAug 25, 2024

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

@lforst this is a breaking change for apps that do not handle fetch exceptions :( as they did not get the exceptions before and now they do....
@lforstlforstAug 26, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you elaborate? Ideally in a new issue. I do not think this alters behaviour. This actually fixes a bug where we swallowed errors.
 },
 );
 };
 });
}

function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void {
async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise<void> {
 if (res && res.body) {
 const responseReader = res.body.getReader();

Expand All@@ -146,25 +141,21 @@ function resolveResponse(res: Response | undefined, onFinishedResolving: () => v
 }
 }

 responseReader
return responseReader
 .read()
 .then(consumeChunks)
 .then(() => {
 onFinishedResolving();
 })
 .catch(() => {
 // noop
 });
 .then(onFinishedResolving)
 .catch(() => undefined);
 }
}

async function streamHandler(response: Response): Promise<void> {
 // clone response for awaiting stream
 let clonedResponseForResolving: Response | undefined;
 let clonedResponseForResolving: Response;
 try {
 clonedResponseForResolving = response.clone();
 } catch (e) {
// noop
 } catch {
return;
 }

 await resolveResponse(clonedResponseForResolving, () => {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix(utils): Handle when requests get aborted in fetch instrumentation#13202

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.

 Sign up for GitHub

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

lforst
merged 7 commits into

from

Aug 5, 2024
 Merged


fix(utils): Handle when requests get aborted in fetch instrumentation
#13202
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
7da58d1

fix(utils): Handle when requests get aborted


AbhiPrasad Aug 2, 2024
6cdcd73

test: Add browser integration test for AbortController


chargome Aug 5, 2024
ac407b3

unflake

Aug 5, 2024
a7c8c1b

Merge remote-tracking branch 'origin/develop' into abhi-fetch-abort-c…

Aug 5, 2024
43c6200

Assert on console log

Aug 5, 2024
cd5e89d

this time actually

Aug 5, 2024
62f5fbd

skip non-tracing tests

Aug 5, 2024

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


8 changes: 8 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
 dsn: 'https://public@dsn.ingest.sentry.io/1337',
 tracesSampleRate: 1,
});

36 changes: 36 additions & 0 deletions
...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
let controller;

const startFetch = e => {
 controller = new AbortController();
 const { signal } = controller;

 Sentry.startSpan(
 {
 name: 'with-abort-controller',
 forceTransaction: true,
 },
 async () => {
 await fetch('http://localhost:7654/foo', { signal })
 .then(response => response.json())
 .then(data => {
 console.log('Fetch succeeded:', data);
 })
 .catch(err => {
 if (err.name === 'AbortError') {
 console.log('Fetch aborted');
 } else {
 console.error('Fetch error:', err);
 }
 });
 },
 );
};

const abortFetch = e => {
 if (controller) {
 controller.abort();
 }
};

document.querySelector('[data-test-id=start-button]').addEventListener('click', startFetch);
document.querySelector('[data-test-id=abort-button]').addEventListener('click', abortFetch);

10 changes: 10 additions & 0 deletions
...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
<!doctype html>
<html>
 <head>
 <meta charset="utf-8" />
 </head>
 <body>
 <button data-test-id="start-button">Start fetch</button>
 <button data-test-id="abort-button">Abort fetch</button>
 </body>
</html>

41 changes: 41 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';
import type { Event as SentryEvent } from '@sentry/types';
import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';

sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => {
 if (shouldSkipTracingTest()) {
 sentryTest.skip();
 }

 const url = await getLocalTestPath({ testDir: __dirname });

 await page.route('**/foo', async () => {
 // never fulfil this route because we abort the request as part of the test
 });

 const transactionEventPromise = getFirstSentryEnvelopeRequest<SentryEvent>(page);

 const hasAbortedFetchPromise = new Promise<void>(resolve => {
 page.on('console', msg => {
 if (msg.type() === 'log' && msg.text() === 'Fetch aborted') {
 resolve();
 }
 });
 });

 await page.goto(url);

 await page.locator('[data-test-id=start-button]').click();
 await page.locator('[data-test-id=abort-button]').click();

 const transactionEvent = await transactionEventPromise;

 // assert that fetch calls do not return undefined
 const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter(
 ({ category, data }) => category === 'fetch' && data === undefined,
 );
 expect(fetchBreadcrumbs).toHaveLength(0);

 await expect(hasAbortedFetchPromise).resolves.toBeUndefined();
});

14 changes: 12 additions & 2 deletions
dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,17 +2,24 @@ import * as Sentry from '@sentry/react';
// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX
import * as React from 'react';

const fetchSSE = async ({ timeout}: { timeout: boolean }) => {
const fetchSSE = async ({ timeout, abort = false }: { timeout: boolean; abort?: boolean }) => {
 Sentry.startSpanManual({ name: 'sse stream using fetch' }, async span => {
 const controller = new AbortController();

 const res = await Sentry.startSpan({ name: 'sse fetch call' }, async () => {
 const endpoint = `http://localhost:8080/${timeout ? 'sse-timeout' : 'sse'}`;
 return await fetch(endpoint);

 const signal = controller.signal;
 return await fetch(endpoint, { signal });
 });

 const stream = res.body;
 const reader = stream?.getReader();

 const readChunk = async () => {
 if (abort) {
 controller.abort();
 }
 const readRes = await reader?.read();
 if (readRes?.done) {
 return;
Expand DownExpand Up@@ -42,6 +49,9 @@ const SSE = () => {
 <button id="fetch-timeout-button" onClick={() => fetchSSE({ timeout: true })}>
 Fetch timeout SSE
 </button>
 <button id="fetch-sse-abort" onClick={() => fetchSSE({ timeout: false, abort: true })}>
 Fetch SSE with error
 </button>
 </>
 );
};
Expand Down

37 changes: 37 additions & 0 deletions
dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -34,6 +34,43 @@ test('Waits for sse streaming when creating spans', async ({ page }) => {
 expect(resolveBodyDuration).toBe(2);
});

test('Waits for sse streaming when sse has been explicitly aborted', async ({ page }) => {
 await page.goto('/sse');

 const transactionPromise = waitForTransaction('react-router-6', async transactionEvent => {
 return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
 });

 const fetchButton = page.locator('id=fetch-sse-abort');
 await fetchButton.click();

 const rootSpan = await transactionPromise;
 console.log(JSON.stringify(rootSpan, null, 2));
 const sseFetchCall = rootSpan.spans?.filter(span => span.description === 'sse fetch call')[0] as SpanJSON;
 const httpGet = rootSpan.spans?.filter(span => span.description === 'GET http://localhost:8080/sse')[0] as SpanJSON;

 expect(sseFetchCall).toBeDefined();
 expect(httpGet).toBeDefined();

 expect(sseFetchCall?.timestamp).toBeDefined();
 expect(sseFetchCall?.start_timestamp).toBeDefined();
 expect(httpGet?.timestamp).toBeDefined();
 expect(httpGet?.start_timestamp).toBeDefined();

 // http headers get sent instantly from the server
 const resolveDuration = Math.round((sseFetchCall.timestamp as number) - sseFetchCall.start_timestamp);

 // body streams after 0s because it has been aborted
 const resolveBodyDuration = Math.round((httpGet.timestamp as number) - httpGet.start_timestamp);

 expect(resolveDuration).toBe(0);
 expect(resolveBodyDuration).toBe(0);

 // validate abort eror was thrown by inspecting console
 const consoleBreadcrumb = rootSpan.breadcrumbs?.find(breadcrumb => breadcrumb.category === 'console');
 expect(consoleBreadcrumb?.message).toBe('Could not fetch sse AbortError: BodyStreamBuffer was aborted');
});

test('Aborts when stream takes longer than 5s', async ({ page }) => {
 await page.goto('/sse');

Expand Down

59 changes: 25 additions & 34 deletions
packages/utils/src/instrument/fetch.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -80,47 +80,42 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
 if (onFetchResolved) {
 onFetchResolved(response);
 } else {
const finishedHandlerData: HandlerDataFetch = {
triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 response,
 };
 triggerHandlers('fetch', finishedHandlerData);
 });
 }

 return response;
 },
 (error: Error) => {
 if (!onFetchResolved) {
 const erroredHandlerData: HandlerDataFetch = {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 };

 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);
 }
 triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 });

 if (isError(error) && error.stack === undefined) {
 // 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.
 throw error;
 // 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.
 throw error;
@amosklamosklAug 25, 2024

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

@lforst this is a breaking change for apps that do not handle fetch exceptions :( as they did not get the exceptions before and now they do....
@lforstlforstAug 26, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you elaborate? Ideally in a new issue. I do not think this alters behaviour. This actually fixes a bug where we swallowed errors.
 },
 );
 };
 });
}

function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void {
async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise<void> {
 if (res && res.body) {
 const responseReader = res.body.getReader();

Expand All@@ -146,25 +141,21 @@ function resolveResponse(res: Response | undefined, onFinishedResolving: () => v
 }
 }

 responseReader
return responseReader
 .read()
 .then(consumeChunks)
 .then(() => {
 onFinishedResolving();
 })
 .catch(() => {
 // noop
 });
 .then(onFinishedResolving)
 .catch(() => undefined);
 }
}

async function streamHandler(response: Response): Promise<void> {
 // clone response for awaiting stream
 let clonedResponseForResolving: Response | undefined;
 let clonedResponseForResolving: Response;
 try {
 clonedResponseForResolving = response.clone();
 } catch (e) {
// noop
 } catch {
return;
 }

 await resolveResponse(clonedResponseForResolving, () => {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
fix(utils): Handle when requests get aborted in fetch instrumentation#13202

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.

 Sign up for GitHub

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

lforst
merged 7 commits into

from

Aug 5, 2024
 Merged


fix(utils): Handle when requests get aborted in fetch instrumentation
#13202
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
7da58d1

fix(utils): Handle when requests get aborted


AbhiPrasad Aug 2, 2024
6cdcd73

test: Add browser integration test for AbortController


chargome Aug 5, 2024
ac407b3

unflake

Aug 5, 2024
a7c8c1b

Merge remote-tracking branch 'origin/develop' into abhi-fetch-abort-c…

Aug 5, 2024
43c6200

Assert on console log

Aug 5, 2024
cd5e89d

this time actually

Aug 5, 2024
62f5fbd

skip non-tracing tests

Aug 5, 2024

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


8 changes: 8 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
 dsn: 'https://public@dsn.ingest.sentry.io/1337',
 tracesSampleRate: 1,
});

36 changes: 36 additions & 0 deletions
...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
let controller;

const startFetch = e => {
 controller = new AbortController();
 const { signal } = controller;

 Sentry.startSpan(
 {
 name: 'with-abort-controller',
 forceTransaction: true,
 },
 async () => {
 await fetch('http://localhost:7654/foo', { signal })
 .then(response => response.json())
 .then(data => {
 console.log('Fetch succeeded:', data);
 })
 .catch(err => {
 if (err.name === 'AbortError') {
 console.log('Fetch aborted');
 } else {
 console.error('Fetch error:', err);
 }
 });
 },
 );
};

const abortFetch = e => {
 if (controller) {
 controller.abort();
 }
};

document.querySelector('[data-test-id=start-button]').addEventListener('click', startFetch);
document.querySelector('[data-test-id=abort-button]').addEventListener('click', abortFetch);

10 changes: 10 additions & 0 deletions
...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
<!doctype html>
<html>
 <head>
 <meta charset="utf-8" />
 </head>
 <body>
 <button data-test-id="start-button">Start fetch</button>
 <button data-test-id="abort-button">Abort fetch</button>
 </body>
</html>

41 changes: 41 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';
import type { Event as SentryEvent } from '@sentry/types';
import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';

sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => {
 if (shouldSkipTracingTest()) {
 sentryTest.skip();
 }

 const url = await getLocalTestPath({ testDir: __dirname });

 await page.route('**/foo', async () => {
 // never fulfil this route because we abort the request as part of the test
 });

 const transactionEventPromise = getFirstSentryEnvelopeRequest<SentryEvent>(page);

 const hasAbortedFetchPromise = new Promise<void>(resolve => {
 page.on('console', msg => {
 if (msg.type() === 'log' && msg.text() === 'Fetch aborted') {
 resolve();
 }
 });
 });

 await page.goto(url);

 await page.locator('[data-test-id=start-button]').click();
 await page.locator('[data-test-id=abort-button]').click();

 const transactionEvent = await transactionEventPromise;

 // assert that fetch calls do not return undefined
 const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter(
 ({ category, data }) => category === 'fetch' && data === undefined,
 );
 expect(fetchBreadcrumbs).toHaveLength(0);

 await expect(hasAbortedFetchPromise).resolves.toBeUndefined();
});

14 changes: 12 additions & 2 deletions
dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,17 +2,24 @@ import * as Sentry from '@sentry/react';
// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX
import * as React from 'react';

const fetchSSE = async ({ timeout}: { timeout: boolean }) => {
const fetchSSE = async ({ timeout, abort = false }: { timeout: boolean; abort?: boolean }) => {
 Sentry.startSpanManual({ name: 'sse stream using fetch' }, async span => {
 const controller = new AbortController();

 const res = await Sentry.startSpan({ name: 'sse fetch call' }, async () => {
 const endpoint = `http://localhost:8080/${timeout ? 'sse-timeout' : 'sse'}`;
 return await fetch(endpoint);

 const signal = controller.signal;
 return await fetch(endpoint, { signal });
 });

 const stream = res.body;
 const reader = stream?.getReader();

 const readChunk = async () => {
 if (abort) {
 controller.abort();
 }
 const readRes = await reader?.read();
 if (readRes?.done) {
 return;
Expand DownExpand Up@@ -42,6 +49,9 @@ const SSE = () => {
 <button id="fetch-timeout-button" onClick={() => fetchSSE({ timeout: true })}>
 Fetch timeout SSE
 </button>
 <button id="fetch-sse-abort" onClick={() => fetchSSE({ timeout: false, abort: true })}>
 Fetch SSE with error
 </button>
 </>
 );
};
Expand Down

37 changes: 37 additions & 0 deletions
dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -34,6 +34,43 @@ test('Waits for sse streaming when creating spans', async ({ page }) => {
 expect(resolveBodyDuration).toBe(2);
});

test('Waits for sse streaming when sse has been explicitly aborted', async ({ page }) => {
 await page.goto('/sse');

 const transactionPromise = waitForTransaction('react-router-6', async transactionEvent => {
 return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
 });

 const fetchButton = page.locator('id=fetch-sse-abort');
 await fetchButton.click();

 const rootSpan = await transactionPromise;
 console.log(JSON.stringify(rootSpan, null, 2));
 const sseFetchCall = rootSpan.spans?.filter(span => span.description === 'sse fetch call')[0] as SpanJSON;
 const httpGet = rootSpan.spans?.filter(span => span.description === 'GET http://localhost:8080/sse')[0] as SpanJSON;

 expect(sseFetchCall).toBeDefined();
 expect(httpGet).toBeDefined();

 expect(sseFetchCall?.timestamp).toBeDefined();
 expect(sseFetchCall?.start_timestamp).toBeDefined();
 expect(httpGet?.timestamp).toBeDefined();
 expect(httpGet?.start_timestamp).toBeDefined();

 // http headers get sent instantly from the server
 const resolveDuration = Math.round((sseFetchCall.timestamp as number) - sseFetchCall.start_timestamp);

 // body streams after 0s because it has been aborted
 const resolveBodyDuration = Math.round((httpGet.timestamp as number) - httpGet.start_timestamp);

 expect(resolveDuration).toBe(0);
 expect(resolveBodyDuration).toBe(0);

 // validate abort eror was thrown by inspecting console
 const consoleBreadcrumb = rootSpan.breadcrumbs?.find(breadcrumb => breadcrumb.category === 'console');
 expect(consoleBreadcrumb?.message).toBe('Could not fetch sse AbortError: BodyStreamBuffer was aborted');
});

test('Aborts when stream takes longer than 5s', async ({ page }) => {
 await page.goto('/sse');

Expand Down

59 changes: 25 additions & 34 deletions
packages/utils/src/instrument/fetch.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -80,47 +80,42 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
 if (onFetchResolved) {
 onFetchResolved(response);
 } else {
const finishedHandlerData: HandlerDataFetch = {
triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 response,
 };
 triggerHandlers('fetch', finishedHandlerData);
 });
 }

 return response;
 },
 (error: Error) => {
 if (!onFetchResolved) {
 const erroredHandlerData: HandlerDataFetch = {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 };

 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);
 }
 triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 });

 if (isError(error) && error.stack === undefined) {
 // 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.
 throw error;
 // 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.
 throw error;
@amosklamosklAug 25, 2024

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

@lforst this is a breaking change for apps that do not handle fetch exceptions :( as they did not get the exceptions before and now they do....
@lforstlforstAug 26, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you elaborate? Ideally in a new issue. I do not think this alters behaviour. This actually fixes a bug where we swallowed errors.
 },
 );
 };
 });
}

function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void {
async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise<void> {
 if (res && res.body) {
 const responseReader = res.body.getReader();

Expand All@@ -146,25 +141,21 @@ function resolveResponse(res: Response | undefined, onFinishedResolving: () => v
 }
 }

 responseReader
return responseReader
 .read()
 .then(consumeChunks)
 .then(() => {
 onFinishedResolving();
 })
 .catch(() => {
 // noop
 });
 .then(onFinishedResolving)
 .catch(() => undefined);
 }
}

async function streamHandler(response: Response): Promise<void> {
 // clone response for awaiting stream
 let clonedResponseForResolving: Response | undefined;
 let clonedResponseForResolving: Response;
 try {
 clonedResponseForResolving = response.clone();
 } catch (e) {
// noop
 } catch {
return;
 }

 await resolveResponse(clonedResponseForResolving, () => {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
fix(utils): Handle when requests get aborted in fetch instrumentation#13202

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.

 Sign up for GitHub

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

lforst
merged 7 commits into

from

Aug 5, 2024
 Merged


fix(utils): Handle when requests get aborted in fetch instrumentation
#13202
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


7 commits


Select commit
Hold shift + click to select a range
7da58d1

fix(utils): Handle when requests get aborted


AbhiPrasad Aug 2, 2024
6cdcd73

test: Add browser integration test for AbortController


chargome Aug 5, 2024
ac407b3

unflake

Aug 5, 2024
a7c8c1b

Merge remote-tracking branch 'origin/develop' into abhi-fetch-abort-c…

Aug 5, 2024
43c6200

Assert on console log

Aug 5, 2024
cd5e89d

this time actually

Aug 5, 2024
62f5fbd

skip non-tracing tests

Aug 5, 2024

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


8 changes: 8 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
 dsn: 'https://public@dsn.ingest.sentry.io/1337',
 tracesSampleRate: 1,
});

36 changes: 36 additions & 0 deletions
...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
let controller;

const startFetch = e => {
 controller = new AbortController();
 const { signal } = controller;

 Sentry.startSpan(
 {
 name: 'with-abort-controller',
 forceTransaction: true,
 },
 async () => {
 await fetch('http://localhost:7654/foo', { signal })
 .then(response => response.json())
 .then(data => {
 console.log('Fetch succeeded:', data);
 })
 .catch(err => {
 if (err.name === 'AbortError') {
 console.log('Fetch aborted');
 } else {
 console.error('Fetch error:', err);
 }
 });
 },
 );
};

const abortFetch = e => {
 if (controller) {
 controller.abort();
 }
};

document.querySelector('[data-test-id=start-button]').addEventListener('click', startFetch);
document.querySelector('[data-test-id=abort-button]').addEventListener('click', abortFetch);

10 changes: 10 additions & 0 deletions
...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
<!doctype html>
<html>
 <head>
 <meta charset="utf-8" />
 </head>
 <body>
 <button data-test-id="start-button">Start fetch</button>
 <button data-test-id="abort-button">Abort fetch</button>
 </body>
</html>

41 changes: 41 additions & 0 deletions
...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
import { expect } from '@playwright/test';
import type { Event as SentryEvent } from '@sentry/types';
import { sentryTest } from '../../../../../utils/fixtures';
import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';

sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => {
 if (shouldSkipTracingTest()) {
 sentryTest.skip();
 }

 const url = await getLocalTestPath({ testDir: __dirname });

 await page.route('**/foo', async () => {
 // never fulfil this route because we abort the request as part of the test
 });

 const transactionEventPromise = getFirstSentryEnvelopeRequest<SentryEvent>(page);

 const hasAbortedFetchPromise = new Promise<void>(resolve => {
 page.on('console', msg => {
 if (msg.type() === 'log' && msg.text() === 'Fetch aborted') {
 resolve();
 }
 });
 });

 await page.goto(url);

 await page.locator('[data-test-id=start-button]').click();
 await page.locator('[data-test-id=abort-button]').click();

 const transactionEvent = await transactionEventPromise;

 // assert that fetch calls do not return undefined
 const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter(
 ({ category, data }) => category === 'fetch' && data === undefined,
 );
 expect(fetchBreadcrumbs).toHaveLength(0);

 await expect(hasAbortedFetchPromise).resolves.toBeUndefined();
});

14 changes: 12 additions & 2 deletions
dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -2,17 +2,24 @@ import * as Sentry from '@sentry/react';
// biome-ignore lint/nursery/noUnusedImports: Need React import for JSX
import * as React from 'react';

const fetchSSE = async ({ timeout}: { timeout: boolean }) => {
const fetchSSE = async ({ timeout, abort = false }: { timeout: boolean; abort?: boolean }) => {
 Sentry.startSpanManual({ name: 'sse stream using fetch' }, async span => {
 const controller = new AbortController();

 const res = await Sentry.startSpan({ name: 'sse fetch call' }, async () => {
 const endpoint = `http://localhost:8080/${timeout ? 'sse-timeout' : 'sse'}`;
 return await fetch(endpoint);

 const signal = controller.signal;
 return await fetch(endpoint, { signal });
 });

 const stream = res.body;
 const reader = stream?.getReader();

 const readChunk = async () => {
 if (abort) {
 controller.abort();
 }
 const readRes = await reader?.read();
 if (readRes?.done) {
 return;
Expand DownExpand Up@@ -42,6 +49,9 @@ const SSE = () => {
 <button id="fetch-timeout-button" onClick={() => fetchSSE({ timeout: true })}>
 Fetch timeout SSE
 </button>
 <button id="fetch-sse-abort" onClick={() => fetchSSE({ timeout: false, abort: true })}>
 Fetch SSE with error
 </button>
 </>
 );
};
Expand Down

37 changes: 37 additions & 0 deletions
dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -34,6 +34,43 @@ test('Waits for sse streaming when creating spans', async ({ page }) => {
 expect(resolveBodyDuration).toBe(2);
});

test('Waits for sse streaming when sse has been explicitly aborted', async ({ page }) => {
 await page.goto('/sse');

 const transactionPromise = waitForTransaction('react-router-6', async transactionEvent => {
 return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload';
 });

 const fetchButton = page.locator('id=fetch-sse-abort');
 await fetchButton.click();

 const rootSpan = await transactionPromise;
 console.log(JSON.stringify(rootSpan, null, 2));
 const sseFetchCall = rootSpan.spans?.filter(span => span.description === 'sse fetch call')[0] as SpanJSON;
 const httpGet = rootSpan.spans?.filter(span => span.description === 'GET http://localhost:8080/sse')[0] as SpanJSON;

 expect(sseFetchCall).toBeDefined();
 expect(httpGet).toBeDefined();

 expect(sseFetchCall?.timestamp).toBeDefined();
 expect(sseFetchCall?.start_timestamp).toBeDefined();
 expect(httpGet?.timestamp).toBeDefined();
 expect(httpGet?.start_timestamp).toBeDefined();

 // http headers get sent instantly from the server
 const resolveDuration = Math.round((sseFetchCall.timestamp as number) - sseFetchCall.start_timestamp);

 // body streams after 0s because it has been aborted
 const resolveBodyDuration = Math.round((httpGet.timestamp as number) - httpGet.start_timestamp);

 expect(resolveDuration).toBe(0);
 expect(resolveBodyDuration).toBe(0);

 // validate abort eror was thrown by inspecting console
 const consoleBreadcrumb = rootSpan.breadcrumbs?.find(breadcrumb => breadcrumb.category === 'console');
 expect(consoleBreadcrumb?.message).toBe('Could not fetch sse AbortError: BodyStreamBuffer was aborted');
});

test('Aborts when stream takes longer than 5s', async ({ page }) => {
 await page.goto('/sse');

Expand Down

59 changes: 25 additions & 34 deletions
packages/utils/src/instrument/fetch.ts

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -80,47 +80,42 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
 if (onFetchResolved) {
 onFetchResolved(response);
 } else {
const finishedHandlerData: HandlerDataFetch = {
triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 response,
 };
 triggerHandlers('fetch', finishedHandlerData);
 });
 }

 return response;
 },
 (error: Error) => {
 if (!onFetchResolved) {
 const erroredHandlerData: HandlerDataFetch = {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 };

 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);
 }
 triggerHandlers('fetch', {
 ...handlerData,
 endTimestamp: timestampInSeconds() * 1000,
 error,
 });

 if (isError(error) && error.stack === undefined) {
 // 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.
 throw error;
 // 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.
 throw error;
@amosklamosklAug 25, 2024

Copy link


Copy Markdown

Choose a reason for hiding this comment

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

@lforst this is a breaking change for apps that do not handle fetch exceptions :( as they did not get the exceptions before and now they do....
@lforstlforstAug 26, 2024

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you elaborate? Ideally in a new issue. I do not think this alters behaviour. This actually fixes a bug where we swallowed errors.
 },
 );
 };
 });
}

function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void {
async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise<void> {
 if (res && res.body) {
 const responseReader = res.body.getReader();

Expand All@@ -146,25 +141,21 @@ function resolveResponse(res: Response | undefined, onFinishedResolving: () => v
 }
 }

 responseReader
return responseReader
 .read()
 .then(consumeChunks)
 .then(() => {
 onFinishedResolving();
 })
 .catch(() => {
 // noop
 });
 .then(onFinishedResolving)
 .catch(() => undefined);
 }
}

async function streamHandler(response: Response): Promise<void> {
 // clone response for awaiting stream
 let clonedResponseForResolving: Response | undefined;
 let clonedResponseForResolving: Response;
 try {
 clonedResponseForResolving = response.clone();
 } catch (e) {
// noop
 } catch {
return;
 }

 await resolveResponse(clonedResponseForResolving, () => {
Expand Down