Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Jump to bottom

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

Merged
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
lforst Aug 5, 2024
a7c8c1b
Merge remote-tracking branch 'origin/develop' into abhi-fetch-abort-c…
lforst Aug 5, 2024
43c6200
Assert on console log
lforst Aug 5, 2024
cd5e89d
this time actually
lforst Aug 5, 2024
62f5fbd
skip non-tracing tests
lforst 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 number Diff line number Diff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/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 number Diff line number Diff 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 number Diff line number Diff 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 number Diff line number Diff 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 number Diff line number Diff 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 Down Expand 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 number Diff line number Diff 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 number Diff line number Diff 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;
Copy link

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

Copy link
Member

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
Loading