From 7da58d15d0fbaf7795fece7c9b34df3709833198 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 2 Aug 2024 12:51:47 -0400 Subject: [PATCH 1/6] fix(utils): Handle when requests get aborted --- .../react-router-6/src/pages/SSE.tsx | 14 ++++- .../react-router-6/tests/sse.test.ts | 37 ++++++++++++ packages/utils/src/instrument/fetch.ts | 59 ++++++++----------- 3 files changed, 74 insertions(+), 36 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx b/dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx index 49e53b09cfa2..64a9f5717114 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx @@ -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; @@ -42,6 +49,9 @@ const SSE = () => { + ); }; diff --git a/dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts b/dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts index 5d4533726e36..92c06543c0b8 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts @@ -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'); diff --git a/packages/utils/src/instrument/fetch.ts b/packages/utils/src/instrument/fetch.ts index 0ea1a4ec8d9f..afa209c01929 100644 --- a/packages/utils/src/instrument/fetch.ts +++ b/packages/utils/src/instrument/fetch.ts @@ -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; }, ); }; }); } -function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void { +async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise { if (res && res.body) { const responseReader = res.body.getReader(); @@ -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 { // clone response for awaiting stream - let clonedResponseForResolving: Response | undefined; + let clonedResponseForResolving: Response; try { clonedResponseForResolving = response.clone(); - } catch (e) { - // noop + } catch { + return; } await resolveResponse(clonedResponseForResolving, () => { From 6cdcd73fa81f0d9b97b796fe62693c631ac6d33f Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Mon, 5 Aug 2024 11:08:48 +0200 Subject: [PATCH 2/6] test: Add browser integration test for AbortController --- .../fetch/withAbortController/init.js | 9 +++++ .../fetch/withAbortController/subject.js | 28 ++++++++++++++ .../fetch/withAbortController/template.html | 10 +++++ .../fetch/withAbortController/test.ts | 37 +++++++++++++++++++ 4 files changed, 84 insertions(+) create mode 100644 dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js create mode 100644 dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js create mode 100644 dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html create mode 100644 dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js new file mode 100644 index 000000000000..83076460599f --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js @@ -0,0 +1,9 @@ +import * as Sentry from '@sentry/browser'; + +window.Sentry = Sentry; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + integrations: [Sentry.browserTracingIntegration()], + tracesSampleRate: 1, +}); diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js new file mode 100644 index 000000000000..1b6e4e567937 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js @@ -0,0 +1,28 @@ +let controller; + +const startFetch = e => { + controller = new AbortController(); + const { signal } = controller; + + 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); diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html new file mode 100644 index 000000000000..18cd917fe30f --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts new file mode 100644 index 000000000000..e8f7e4dddb28 --- /dev/null +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts @@ -0,0 +1,37 @@ +import { expect } from '@playwright/test'; +import type { Event as SentryEvent } from '@sentry/types'; +import { sentryTest } from '../../../../../utils/fixtures'; +import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers'; + +sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => { + const url = await getLocalTestPath({ testDir: __dirname }); + + await page.route('**/foo', async route => { + setTimeout(() => { + route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ message: 'This is the server response' }), + }); + }, 2000); + }); + + await page.goto(url); + + await page.locator('[data-test-id=start-button]').click(); + await page.waitForTimeout(500); + await page.locator('[data-test-id=abort-button]').click(); + const eventData = await getFirstSentryEnvelopeRequest(page); + + // assert that fetch calls do not return undefined + const fetchBreadcrumbs = eventData.breadcrumbs?.filter( + ({ category, data }) => category === 'fetch' && data === undefined, + ); + expect(fetchBreadcrumbs).toHaveLength(0); + + // assert that fetch call has been aborted + const abortedBreadcrumb = eventData.breadcrumbs?.filter( + ({ category, message }) => category === 'console' && message === 'Fetch aborted', + ); + expect(abortedBreadcrumb).toHaveLength(1); +}); From ac407b3d081695a01fb98989faaaef6a71a543a7 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 5 Aug 2024 09:30:20 +0000 Subject: [PATCH 3/6] unflake --- .../fetch/withAbortController/init.js | 1 - .../fetch/withAbortController/subject.js | 32 ++++++++++++------- .../fetch/withAbortController/test.ts | 20 +++++------- 3 files changed, 28 insertions(+), 25 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js index 83076460599f..7c200c542c56 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js @@ -4,6 +4,5 @@ window.Sentry = Sentry; Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', - integrations: [Sentry.browserTracingIntegration()], tracesSampleRate: 1, }); diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js index 1b6e4e567937..78028b473ad7 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js @@ -4,18 +4,26 @@ const startFetch = e => { controller = new AbortController(); const { signal } = controller; - 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); - } - }); + 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 => { diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts index e8f7e4dddb28..faedec914e76 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts @@ -6,31 +6,27 @@ import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers'; sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => { const url = await getLocalTestPath({ testDir: __dirname }); - await page.route('**/foo', async route => { - setTimeout(() => { - route.fulfill({ - status: 200, - contentType: 'application/json', - body: JSON.stringify({ message: 'This is the server response' }), - }); - }, 2000); + await page.route('**/foo', async () => { + // never fulfil this route because we abort the request as part of the test }); + const transactionEventPromise = getFirstSentryEnvelopeRequest(page); + await page.goto(url); await page.locator('[data-test-id=start-button]').click(); - await page.waitForTimeout(500); await page.locator('[data-test-id=abort-button]').click(); - const eventData = await getFirstSentryEnvelopeRequest(page); + + const transactionEvent = await transactionEventPromise; // assert that fetch calls do not return undefined - const fetchBreadcrumbs = eventData.breadcrumbs?.filter( + const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter( ({ category, data }) => category === 'fetch' && data === undefined, ); expect(fetchBreadcrumbs).toHaveLength(0); // assert that fetch call has been aborted - const abortedBreadcrumb = eventData.breadcrumbs?.filter( + const abortedBreadcrumb = transactionEvent.breadcrumbs?.filter( ({ category, message }) => category === 'console' && message === 'Fetch aborted', ); expect(abortedBreadcrumb).toHaveLength(1); From 43c6200b8eb56a09dc67d0b06897a2aaa224014c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 5 Aug 2024 09:54:45 +0000 Subject: [PATCH 4/6] Assert on console log --- .../httpclient/fetch/withAbortController/test.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts index faedec914e76..22b36280bf23 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts @@ -12,6 +12,13 @@ sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page const transactionEventPromise = getFirstSentryEnvelopeRequest(page); + let hasPrintedFetchAborted = false; + page.on('console', msg => { + if (msg.type() === 'log' && msg.text() === 'Fetch aborted') { + hasPrintedFetchAborted = true; + } + }); + await page.goto(url); await page.locator('[data-test-id=start-button]').click(); @@ -24,10 +31,5 @@ sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page ({ category, data }) => category === 'fetch' && data === undefined, ); expect(fetchBreadcrumbs).toHaveLength(0); - - // assert that fetch call has been aborted - const abortedBreadcrumb = transactionEvent.breadcrumbs?.filter( - ({ category, message }) => category === 'console' && message === 'Fetch aborted', - ); - expect(abortedBreadcrumb).toHaveLength(1); + expect(hasPrintedFetchAborted).toBe(true); }); From cd5e89d66438f24d22b1c0ce61773cafaa9356fb Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 5 Aug 2024 10:00:33 +0000 Subject: [PATCH 5/6] this time actually --- .../httpclient/fetch/withAbortController/test.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts index 22b36280bf23..3716962350ed 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts @@ -12,11 +12,12 @@ sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page const transactionEventPromise = getFirstSentryEnvelopeRequest(page); - let hasPrintedFetchAborted = false; - page.on('console', msg => { - if (msg.type() === 'log' && msg.text() === 'Fetch aborted') { - hasPrintedFetchAborted = true; - } + const hasAbortedFetchPromise = new Promise(resolve => { + page.on('console', msg => { + if (msg.type() === 'log' && msg.text() === 'Fetch aborted') { + resolve(); + } + }); }); await page.goto(url); @@ -31,5 +32,6 @@ sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page ({ category, data }) => category === 'fetch' && data === undefined, ); expect(fetchBreadcrumbs).toHaveLength(0); - expect(hasPrintedFetchAborted).toBe(true); + + await expect(hasAbortedFetchPromise).resolves.toBeUndefined(); }); From 62f5fbdb823c8760f7c41e08a1b1509a71353981 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 5 Aug 2024 10:49:29 +0000 Subject: [PATCH 6/6] skip non-tracing tests --- .../httpclient/fetch/withAbortController/test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts index 3716962350ed..6cc3a0cd32a9 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts @@ -1,9 +1,13 @@ import { expect } from '@playwright/test'; import type { Event as SentryEvent } from '@sentry/types'; import { sentryTest } from '../../../../../utils/fixtures'; -import { getFirstSentryEnvelopeRequest } from '../../../../../utils/helpers'; +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 () => {