diff --git a/packages/tracing/src/browser/request.ts b/packages/tracing/src/browser/request.ts index 212e15d14bf5..9a71a7387bed 100644 --- a/packages/tracing/src/browser/request.ts +++ b/packages/tracing/src/browser/request.ts @@ -141,8 +141,11 @@ export function fetchCallback( return; } - if (handlerData.endTimestamp && handlerData.fetchData.__span) { - const span = spans[handlerData.fetchData.__span]; + if (handlerData.endTimestamp) { + const spanId = handlerData.fetchData.__span; + if (!spanId) return; + + const span = spans[spanId]; if (span) { if (handlerData.response) { // TODO (kmclb) remove this once types PR goes through @@ -154,7 +157,7 @@ export function fetchCallback( span.finish(); // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete spans[handlerData.fetchData.__span]; + delete spans[spanId]; } return; } @@ -216,14 +219,17 @@ export function xhrCallback( const xhr = handlerData.xhr.__sentry_xhr__; // check first if the request has finished and is tracked by an existing span which should now end - if (handlerData.endTimestamp && handlerData.xhr.__sentry_xhr_span_id__) { - const span = spans[handlerData.xhr.__sentry_xhr_span_id__]; + if (handlerData.endTimestamp) { + const spanId = handlerData.xhr.__sentry_xhr_span_id__; + if (!spanId) return; + + const span = spans[spanId]; if (span) { span.setHttpStatus(xhr.status_code); span.finish(); // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete spans[handlerData.xhr.__sentry_xhr_span_id__]; + delete spans[spanId]; } return; } diff --git a/packages/tracing/test/browser/request.test.ts b/packages/tracing/test/browser/request.test.ts index 1b19b8a1f39c..2ba1c906818a 100644 --- a/packages/tracing/test/browser/request.test.ts +++ b/packages/tracing/test/browser/request.test.ts @@ -48,10 +48,12 @@ describe('callbacks', () => { let transaction: Transaction; const alwaysCreateSpan = () => true; const neverCreateSpan = () => false; + const startTimestamp = 1356996072000; + const endTimestamp = 1356996072000; const fetchHandlerData: FetchData = { args: ['http://dogs.are.great/', {}], fetchData: { url: 'http://dogs.are.great/', method: 'GET' }, - startTimestamp: 1356996072000, + startTimestamp, }; const xhrHandlerData: XHRData = { xhr: { @@ -66,9 +68,8 @@ describe('callbacks', () => { // setRequestHeader: XMLHttpRequest.prototype.setRequestHeader, setRequestHeader, }, - startTimestamp: 1353501072000, + startTimestamp, }; - const endTimestamp = 1356996072000; beforeAll(() => { hub = new Hub(new BrowserClient({ tracesSampleRate: 1 })); @@ -173,6 +174,23 @@ describe('callbacks', () => { expect(newSpan!.status).toBe(spanStatusfromHttpCode(404)); }); + it('ignores response with no associated span', () => { + // the request might be missed somehow. E.g. if it was sent before tracing gets enabled. + + const postRequestFetchHandlerData = { + ...fetchHandlerData, + endTimestamp, + response: { status: 404 } as Response, + }; + + // in that case, the response coming back will be ignored + fetchCallback(postRequestFetchHandlerData, alwaysCreateSpan, {}); + + const newSpan = transaction.spanRecorder?.spans[1]; + + expect(newSpan).toBeUndefined(); + }); + it('adds sentry-trace header to fetch requests', () => { // TODO }); @@ -263,5 +281,26 @@ describe('callbacks', () => { expect(newSpan!.status).toBe(spanStatusfromHttpCode(404)); }); + + it('ignores response with no associated span', () => { + // the request might be missed somehow. E.g. if it was sent before tracing gets enabled. + + const postRequestXHRHandlerData = { + ...{ + xhr: { + __sentry_xhr__: xhrHandlerData.xhr.__sentry_xhr__, + }, + }, + startTimestamp, + endTimestamp, + }; + + // in that case, the response coming back will be ignored + xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, {}); + + const newSpan = transaction.spanRecorder?.spans[1]; + + expect(newSpan).toBeUndefined(); + }); }); }); diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index 3f39bb985ca6..5e4c8a806cac 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -359,7 +359,7 @@ describe('Hub', () => { // TODO the way we dig out the headers to test them doesn't work on Node < 10 testOnlyIfNodeVersionAtLeast(10)( 'should propagate positive sampling decision to child transactions in XHR header', - () => { + async () => { const hub = new Hub( new BrowserClient({ dsn: 'https://1231@dogs.are.great/1121', @@ -375,12 +375,12 @@ describe('Hub', () => { }); const request = new XMLHttpRequest(); - request.open('GET', '/chase-partners'); - - // mock a response having been received successfully (we have to do it in this roundabout way because readyState - // is readonly and changing it doesn't trigger a readystatechange event) - Object.defineProperty(request, 'readyState', { value: 4 }); - request.dispatchEvent(new Event('readystatechange')); + await new Promise(resolve => { + request.timeout = 1; + request.onloadend = request.ontimeout = resolve; + request.open('GET', '/chase-partners'); + request.send(''); + }); // this looks weird, it's true, but it's really just `request.impl.flag.requestHeaders` - it's just that the // `impl` key is a symbol rather than a string, and therefore needs to be referred to by reference rather than @@ -401,7 +401,7 @@ describe('Hub', () => { // TODO the way we dig out the headers to test them doesn't work on Node < 10 testOnlyIfNodeVersionAtLeast(10)( 'should propagate negative sampling decision to child transactions in XHR header', - () => { + async () => { const hub = new Hub( new BrowserClient({ dsn: 'https://1231@dogs.are.great/1121', @@ -417,12 +417,12 @@ describe('Hub', () => { }); const request = new XMLHttpRequest(); - request.open('GET', '/chase-partners'); - - // mock a response having been received successfully (we have to do it in this roundabout way because readyState - // is readonly and changing it doesn't trigger a readystatechange event) - Object.defineProperty(request, 'readyState', { value: 4 }); - request.dispatchEvent(new Event('readystatechange')); + await new Promise(resolve => { + request.timeout = 1; + request.onloadend = request.ontimeout = resolve; + request.open('GET', '/chase-partners'); + request.send(''); + }); // this looks weird, it's true, but it's really just `request.impl.flag.requestHeaders` - it's just that the // `impl` key is a symbol rather than a string, and therefore needs to be referred to by reference rather than