From b776eac149f737ae99928d46d864765f8f25f667 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 27 Nov 2024 17:11:50 +0000 Subject: [PATCH 1/4] feat(browser): Attach virtual stack traces to `HttpClient` events. --- packages/browser-utils/src/instrument/xhr.ts | 17 +++++-- .../browser/src/integrations/httpclient.ts | 46 +++++++++++++------ packages/core/src/types-hoist/instrument.ts | 1 + .../core/src/utils-hoist/instrument/fetch.ts | 18 ++++++-- 4 files changed, 61 insertions(+), 21 deletions(-) diff --git a/packages/browser-utils/src/instrument/xhr.ts b/packages/browser-utils/src/instrument/xhr.ts index df67d1c1f1be..e9933ed53a4a 100644 --- a/packages/browser-utils/src/instrument/xhr.ts +++ b/packages/browser-utils/src/instrument/xhr.ts @@ -15,14 +15,17 @@ type WindowWithXhr = Window & { XMLHttpRequest?: typeof XMLHttpRequest }; * Use at your own risk, this might break without changelog notice, only used internally. * @hidden */ -export function addXhrInstrumentationHandler(handler: (data: HandlerDataXhr) => void): void { +export function addXhrInstrumentationHandler( + handler: (data: HandlerDataXhr) => void, + httpClientInstrumented?: boolean, +): void { const type = 'xhr'; addHandler(type, handler); - maybeInstrument(type, instrumentXHR); + maybeInstrument(type, () => instrumentXHR(httpClientInstrumented)); } /** Exported only for tests. */ -export function instrumentXHR(): void { +export function instrumentXHR(httpClientInstrumented: boolean = false): void { if (!(WINDOW as WindowWithXhr).XMLHttpRequest) { return; } @@ -32,6 +35,13 @@ export function instrumentXHR(): void { // eslint-disable-next-line @typescript-eslint/unbound-method xhrproto.open = new Proxy(xhrproto.open, { apply(originalOpen, xhrOpenThisArg: XMLHttpRequest & SentryWrappedXMLHttpRequest, xhrOpenArgArray) { + // NOTE: If you are a Sentry user, and you are seeing this stack frame, + // it means the error, that was caused by your XHR call did not + // have a stack trace. If you are using HttpClient integration, + // this is the expected behavior, as we are using this virtual error to capture + // the location of your XHR call, and group your HttpClient events accordingly. + const virtualError = new Error(); + const startTimestamp = timestampInSeconds() * 1000; // open() should always be called with two or more arguments @@ -75,6 +85,7 @@ export function instrumentXHR(): void { endTimestamp: timestampInSeconds() * 1000, startTimestamp, xhr: xhrOpenThisArg, + error: httpClientInstrumented ? virtualError : undefined, }; triggerHandlers('xhr', handlerData); } diff --git a/packages/browser/src/integrations/httpclient.ts b/packages/browser/src/integrations/httpclient.ts index cf1dc6ab6896..43d514561453 100644 --- a/packages/browser/src/integrations/httpclient.ts +++ b/packages/browser/src/integrations/httpclient.ts @@ -46,7 +46,7 @@ const _httpClientIntegration = ((options: Partial = {}) => { return { name: INTEGRATION_NAME, - setup(client): void { + setup(client: Client): void { _wrapFetch(client, _options); _wrapXHR(client, _options); }, @@ -70,6 +70,7 @@ function _fetchResponseHandler( requestInfo: RequestInfo, response: Response, requestInit?: RequestInit, + error?: unknown, ): void { if (_shouldCaptureResponse(options, response.status, response.url)) { const request = _getRequest(requestInfo, requestInit); @@ -89,9 +90,13 @@ function _fetchResponseHandler( responseHeaders, requestCookies, responseCookies, + stacktrace: error instanceof Error ? error.stack : undefined, }); + // withScope(scope => { + // scope.setFingerprint([request.url, request.method, response.status.toString()]); captureEvent(event); + // }); } } @@ -127,6 +132,7 @@ function _xhrResponseHandler( xhr: XMLHttpRequest, method: string, headers: Record, + error?: unknown, ): void { if (_shouldCaptureResponse(options, xhr.status, xhr.responseURL)) { let requestHeaders, responseCookies, responseHeaders; @@ -159,6 +165,7 @@ function _xhrResponseHandler( // Can't access request cookies from XHR responseHeaders, responseCookies, + stacktrace: error instanceof Error ? error.stack : undefined, }); captureEvent(event); @@ -283,20 +290,24 @@ function _wrapFetch(client: Client, options: HttpClientOptions): void { return; } - addFetchInstrumentationHandler(handlerData => { - if (getClient() !== client) { - return; - } + addFetchInstrumentationHandler( + handlerData => { + if (getClient() !== client) { + return; + } - const { response, args } = handlerData; - const [requestInfo, requestInit] = args as [RequestInfo, RequestInit | undefined]; + const { response, args } = handlerData; + const [requestInfo, requestInit] = args as [RequestInfo, RequestInit | undefined]; - if (!response) { - return; - } + if (!response) { + return; + } - _fetchResponseHandler(options, requestInfo, response as Response, requestInit); - }); + _fetchResponseHandler(options, requestInfo, response as Response, requestInit, handlerData.error); + }, + false, + true, + ); } /** @@ -323,11 +334,11 @@ function _wrapXHR(client: Client, options: HttpClientOptions): void { const { method, request_headers: headers } = sentryXhrData; try { - _xhrResponseHandler(options, xhr, method, headers); + _xhrResponseHandler(options, xhr, method, headers, handlerData.error); } catch (e) { DEBUG_BUILD && logger.warn('Error while extracting response event form XHR response', e); } - }); + }, true); } /** @@ -358,7 +369,13 @@ function _createEvent(data: { responseCookies?: Record; requestHeaders?: Record; requestCookies?: Record; + stacktrace?: string; }): SentryEvent { + const client = getClient(); + const virtualStackTrace = client && data.stacktrace ? data.stacktrace : undefined; + // Remove the first frame from the stack as it's the HttpClient call + const stack = virtualStackTrace && client ? client.getOptions().stackParser(virtualStackTrace, 0, 1) : undefined; + const message = `HTTP Client Error with status code: ${data.status}`; const event: SentryEvent = { @@ -368,6 +385,7 @@ function _createEvent(data: { { type: 'Error', value: message, + stacktrace: stack ? { frames: stack } : undefined, }, ], }, diff --git a/packages/core/src/types-hoist/instrument.ts b/packages/core/src/types-hoist/instrument.ts index f0b239e86b14..f472a663c5d5 100644 --- a/packages/core/src/types-hoist/instrument.ts +++ b/packages/core/src/types-hoist/instrument.ts @@ -32,6 +32,7 @@ export interface HandlerDataXhr { xhr: SentryWrappedXMLHttpRequest; startTimestamp?: number; endTimestamp?: number; + error?: unknown; } interface SentryFetchData { diff --git a/packages/core/src/utils-hoist/instrument/fetch.ts b/packages/core/src/utils-hoist/instrument/fetch.ts index 39c8862ba618..a7b1a4972252 100644 --- a/packages/core/src/utils-hoist/instrument/fetch.ts +++ b/packages/core/src/utils-hoist/instrument/fetch.ts @@ -21,10 +21,11 @@ type FetchResource = string | { toString(): string } | { url: string }; export function addFetchInstrumentationHandler( handler: (data: HandlerDataFetch) => void, skipNativeFetchCheck?: boolean, + httpClientInstrumented?: boolean, ): void { const type = 'fetch'; addHandler(type, handler); - maybeInstrument(type, () => instrumentFetch(undefined, skipNativeFetchCheck)); + maybeInstrument(type, () => instrumentFetch(undefined, skipNativeFetchCheck, httpClientInstrumented)); } /** @@ -41,7 +42,11 @@ export function addFetchEndInstrumentationHandler(handler: (data: HandlerDataFet maybeInstrument(type, () => instrumentFetch(streamHandler)); } -function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { +function instrumentFetch( + onFetchResolved?: (response: Response) => void, + skipNativeFetchCheck: boolean = false, + httpClientInstrumented: boolean = false, +): void { if (skipNativeFetchCheck && !supportsNativeFetch()) { return; } @@ -59,7 +64,9 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat }; // if there is no callback, fetch is instrumented directly - if (!onFetchResolved) { + // if httpClientInstrumented is true, we are in the HttpClient instrumentation + // and we may need to capture the stacktrace even when the fetch promise is resolved + if (!onFetchResolved && !httpClientInstrumented) { triggerHandlers('fetch', { ...handlerData, }); @@ -72,7 +79,8 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat // it means the error, that was caused by your fetch call did not // have a stack trace, so the SDK backfilled the stack trace so // you can see which fetch call failed. - const virtualStackTrace = new Error().stack; + const virtualError = new Error(); + const virtualStackTrace = virtualError.stack; // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access return originalFetch.apply(GLOBAL_OBJ, args).then( @@ -80,10 +88,12 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat if (onFetchResolved) { onFetchResolved(response); } else { + // Adding the stacktrace to be able to fingerprint the failed fetch event in HttpClient instrumentation triggerHandlers('fetch', { ...handlerData, endTimestamp: timestampInSeconds() * 1000, response, + error: httpClientInstrumented ? virtualError : undefined, }); } From 648693c9feb8f2bec5c145b9973d04d29d9d8002 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Wed, 27 Nov 2024 19:34:33 +0000 Subject: [PATCH 2/4] Update integration tests --- .../suites/integrations/httpclient/axios/test.ts | 9 +++++++++ .../suites/integrations/httpclient/fetch/simple/test.ts | 9 +++++++++ .../integrations/httpclient/fetch/withRequest/test.ts | 9 +++++++++ .../fetch/withRequestAndBodyAndOptions/test.ts | 9 +++++++++ .../httpclient/fetch/withRequestAndOptions/test.ts | 9 +++++++++ .../suites/integrations/httpclient/xhr/test.ts | 9 +++++++++ .../browser-integration-tests/utils/generatePlugin.ts | 1 - packages/browser/src/integrations/httpclient.ts | 5 +---- 8 files changed, 55 insertions(+), 5 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts index d4e7ba222b30..19161be50ad3 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/axios/test.ts @@ -40,6 +40,15 @@ sentryTest( type: 'http.client', handled: false, }, + stacktrace: { + frames: expect.arrayContaining([ + expect.objectContaining({ + filename: 'http://sentry-test.io/subject.bundle.js', + function: '?', + in_app: true, + }), + ]), + }, }, ], }, diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/simple/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/simple/test.ts index 09390fffd573..c92419aae72f 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/simple/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/simple/test.ts @@ -42,6 +42,15 @@ sentryTest( type: 'http.client', handled: false, }, + stacktrace: { + frames: expect.arrayContaining([ + expect.objectContaining({ + filename: 'http://sentry-test.io/subject.bundle.js', + function: '?', + in_app: true, + }), + ]), + }, }, ], }, diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequest/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequest/test.ts index f1e23bbd080b..4ceec6d51c38 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequest/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequest/test.ts @@ -38,6 +38,15 @@ sentryTest('works with a Request passed in', async ({ getLocalTestUrl, page }) = type: 'http.client', handled: false, }, + stacktrace: { + frames: expect.arrayContaining([ + expect.objectContaining({ + filename: 'http://sentry-test.io/subject.bundle.js', + function: '?', + in_app: true, + }), + ]), + }, }, ], }, diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndBodyAndOptions/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndBodyAndOptions/test.ts index ba34c4fdfa0f..9c408c8ade50 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndBodyAndOptions/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndBodyAndOptions/test.ts @@ -40,6 +40,15 @@ sentryTest( type: 'http.client', handled: false, }, + stacktrace: { + frames: expect.arrayContaining([ + expect.objectContaining({ + filename: 'http://sentry-test.io/subject.bundle.js', + function: '?', + in_app: true, + }), + ]), + }, }, ], }, diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndOptions/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndOptions/test.ts index 0ea8fc7bc0cb..92e31819673f 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndOptions/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/fetch/withRequestAndOptions/test.ts @@ -38,6 +38,15 @@ sentryTest('works with a Request (without body) & options passed in', async ({ g type: 'http.client', handled: false, }, + stacktrace: { + frames: expect.arrayContaining([ + expect.objectContaining({ + filename: 'http://sentry-test.io/subject.bundle.js', + function: '?', + in_app: true, + }), + ]), + }, }, ], }, diff --git a/dev-packages/browser-integration-tests/suites/integrations/httpclient/xhr/test.ts b/dev-packages/browser-integration-tests/suites/integrations/httpclient/xhr/test.ts index d342e9abf748..d323c73725d6 100644 --- a/dev-packages/browser-integration-tests/suites/integrations/httpclient/xhr/test.ts +++ b/dev-packages/browser-integration-tests/suites/integrations/httpclient/xhr/test.ts @@ -40,6 +40,15 @@ sentryTest( type: 'http.client', handled: false, }, + stacktrace: { + frames: expect.arrayContaining([ + expect.objectContaining({ + filename: 'http://sentry-test.io/subject.bundle.js', + function: '?', + in_app: true, + }), + ]), + }, }, ], }, diff --git a/dev-packages/browser-integration-tests/utils/generatePlugin.ts b/dev-packages/browser-integration-tests/utils/generatePlugin.ts index c3b515ada32d..b9b4dcb4c1f3 100644 --- a/dev-packages/browser-integration-tests/utils/generatePlugin.ts +++ b/dev-packages/browser-integration-tests/utils/generatePlugin.ts @@ -176,7 +176,6 @@ class SentryScenarioGenerationPlugin { } : {}; - // Checking if the current scenario has imported `@sentry/integrations`. compiler.hooks.normalModuleFactory.tap(this._name, factory => { factory.hooks.parser.for('javascript/auto').tap(this._name, parser => { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access diff --git a/packages/browser/src/integrations/httpclient.ts b/packages/browser/src/integrations/httpclient.ts index 43d514561453..3690c62679ab 100644 --- a/packages/browser/src/integrations/httpclient.ts +++ b/packages/browser/src/integrations/httpclient.ts @@ -46,7 +46,7 @@ const _httpClientIntegration = ((options: Partial = {}) => { return { name: INTEGRATION_NAME, - setup(client: Client): void { + setup(client): void { _wrapFetch(client, _options); _wrapXHR(client, _options); }, @@ -93,10 +93,7 @@ function _fetchResponseHandler( stacktrace: error instanceof Error ? error.stack : undefined, }); - // withScope(scope => { - // scope.setFingerprint([request.url, request.method, response.status.toString()]); captureEvent(event); - // }); } } From 33a5bfa728ea95cd9ecf0fdf71439cd94da34b32 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 28 Nov 2024 11:32:31 +0000 Subject: [PATCH 3/4] Pass stacktrace string instead of error. --- packages/browser-utils/src/instrument/xhr.ts | 11 ++---- .../browser/src/integrations/httpclient.ts | 38 +++++++++---------- packages/core/src/types-hoist/instrument.ts | 3 +- .../core/src/utils-hoist/instrument/fetch.ts | 35 +++++++---------- 4 files changed, 37 insertions(+), 50 deletions(-) diff --git a/packages/browser-utils/src/instrument/xhr.ts b/packages/browser-utils/src/instrument/xhr.ts index e9933ed53a4a..f46ecd67b0b6 100644 --- a/packages/browser-utils/src/instrument/xhr.ts +++ b/packages/browser-utils/src/instrument/xhr.ts @@ -15,17 +15,14 @@ type WindowWithXhr = Window & { XMLHttpRequest?: typeof XMLHttpRequest }; * Use at your own risk, this might break without changelog notice, only used internally. * @hidden */ -export function addXhrInstrumentationHandler( - handler: (data: HandlerDataXhr) => void, - httpClientInstrumented?: boolean, -): void { +export function addXhrInstrumentationHandler(handler: (data: HandlerDataXhr) => void): void { const type = 'xhr'; addHandler(type, handler); - maybeInstrument(type, () => instrumentXHR(httpClientInstrumented)); + maybeInstrument(type, instrumentXHR); } /** Exported only for tests. */ -export function instrumentXHR(httpClientInstrumented: boolean = false): void { +export function instrumentXHR(): void { if (!(WINDOW as WindowWithXhr).XMLHttpRequest) { return; } @@ -85,7 +82,7 @@ export function instrumentXHR(httpClientInstrumented: boolean = false): void { endTimestamp: timestampInSeconds() * 1000, startTimestamp, xhr: xhrOpenThisArg, - error: httpClientInstrumented ? virtualError : undefined, + stack: virtualError.stack, }; triggerHandlers('xhr', handlerData); } diff --git a/packages/browser/src/integrations/httpclient.ts b/packages/browser/src/integrations/httpclient.ts index 3690c62679ab..15c8b3412963 100644 --- a/packages/browser/src/integrations/httpclient.ts +++ b/packages/browser/src/integrations/httpclient.ts @@ -70,7 +70,7 @@ function _fetchResponseHandler( requestInfo: RequestInfo, response: Response, requestInit?: RequestInit, - error?: unknown, + stack?: string, ): void { if (_shouldCaptureResponse(options, response.status, response.url)) { const request = _getRequest(requestInfo, requestInit); @@ -90,7 +90,7 @@ function _fetchResponseHandler( responseHeaders, requestCookies, responseCookies, - stacktrace: error instanceof Error ? error.stack : undefined, + stacktrace: stack, }); captureEvent(event); @@ -129,7 +129,7 @@ function _xhrResponseHandler( xhr: XMLHttpRequest, method: string, headers: Record, - error?: unknown, + stack?: string, ): void { if (_shouldCaptureResponse(options, xhr.status, xhr.responseURL)) { let requestHeaders, responseCookies, responseHeaders; @@ -162,7 +162,7 @@ function _xhrResponseHandler( // Can't access request cookies from XHR responseHeaders, responseCookies, - stacktrace: error instanceof Error ? error.stack : undefined, + stacktrace: stack, }); captureEvent(event); @@ -287,24 +287,20 @@ function _wrapFetch(client: Client, options: HttpClientOptions): void { return; } - addFetchInstrumentationHandler( - handlerData => { - if (getClient() !== client) { - return; - } + addFetchInstrumentationHandler(handlerData => { + if (getClient() !== client) { + return; + } - const { response, args } = handlerData; - const [requestInfo, requestInit] = args as [RequestInfo, RequestInit | undefined]; + const { response, args } = handlerData; + const [requestInfo, requestInit] = args as [RequestInfo, RequestInit | undefined]; - if (!response) { - return; - } + if (!response) { + return; + } - _fetchResponseHandler(options, requestInfo, response as Response, requestInit, handlerData.error); - }, - false, - true, - ); + _fetchResponseHandler(options, requestInfo, response as Response, requestInit, handlerData.stack); + }, false); } /** @@ -331,11 +327,11 @@ function _wrapXHR(client: Client, options: HttpClientOptions): void { const { method, request_headers: headers } = sentryXhrData; try { - _xhrResponseHandler(options, xhr, method, headers, handlerData.error); + _xhrResponseHandler(options, xhr, method, headers, handlerData.stack); } catch (e) { DEBUG_BUILD && logger.warn('Error while extracting response event form XHR response', e); } - }, true); + }); } /** diff --git a/packages/core/src/types-hoist/instrument.ts b/packages/core/src/types-hoist/instrument.ts index f472a663c5d5..f779b2096597 100644 --- a/packages/core/src/types-hoist/instrument.ts +++ b/packages/core/src/types-hoist/instrument.ts @@ -32,7 +32,7 @@ export interface HandlerDataXhr { xhr: SentryWrappedXMLHttpRequest; startTimestamp?: number; endTimestamp?: number; - error?: unknown; + stack?: string; } interface SentryFetchData { @@ -57,6 +57,7 @@ export interface HandlerDataFetch { headers: WebFetchHeaders; }; error?: unknown; + stack?: string; } export interface HandlerDataDom { diff --git a/packages/core/src/utils-hoist/instrument/fetch.ts b/packages/core/src/utils-hoist/instrument/fetch.ts index a7b1a4972252..c822066a7c2d 100644 --- a/packages/core/src/utils-hoist/instrument/fetch.ts +++ b/packages/core/src/utils-hoist/instrument/fetch.ts @@ -21,11 +21,10 @@ type FetchResource = string | { toString(): string } | { url: string }; export function addFetchInstrumentationHandler( handler: (data: HandlerDataFetch) => void, skipNativeFetchCheck?: boolean, - httpClientInstrumented?: boolean, ): void { const type = 'fetch'; addHandler(type, handler); - maybeInstrument(type, () => instrumentFetch(undefined, skipNativeFetchCheck, httpClientInstrumented)); + maybeInstrument(type, () => instrumentFetch(undefined, skipNativeFetchCheck)); } /** @@ -42,17 +41,23 @@ export function addFetchEndInstrumentationHandler(handler: (data: HandlerDataFet maybeInstrument(type, () => instrumentFetch(streamHandler)); } -function instrumentFetch( - onFetchResolved?: (response: Response) => void, - skipNativeFetchCheck: boolean = false, - httpClientInstrumented: boolean = false, -): void { +function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { if (skipNativeFetchCheck && !supportsNativeFetch()) { return; } fill(GLOBAL_OBJ, 'fetch', function (originalFetch: () => void): () => void { return function (...args: any[]): void { + // We capture the stack right here and not in the Promise error callback because Safari (and probably other + // browsers too) will wipe the stack trace up to this point, only leaving us with this file which is useless. + + // NOTE: If you are a Sentry user, and you are seeing this stack frame, + // it means the error, that was caused by your fetch call did not + // have a stack trace, so the SDK backfilled the stack trace so + // you can see which fetch call failed. + const virtualError = new Error(); + const virtualStackTrace = virtualError.stack; + const { method, url } = parseFetchArgs(args); const handlerData: HandlerDataFetch = { args, @@ -61,27 +66,16 @@ function instrumentFetch( url, }, startTimestamp: timestampInSeconds() * 1000, + stack: virtualStackTrace, }; // if there is no callback, fetch is instrumented directly - // if httpClientInstrumented is true, we are in the HttpClient instrumentation - // and we may need to capture the stacktrace even when the fetch promise is resolved - if (!onFetchResolved && !httpClientInstrumented) { + if (!onFetchResolved) { triggerHandlers('fetch', { ...handlerData, }); } - // We capture the stack right here and not in the Promise error callback because Safari (and probably other - // browsers too) will wipe the stack trace up to this point, only leaving us with this file which is useless. - - // NOTE: If you are a Sentry user, and you are seeing this stack frame, - // it means the error, that was caused by your fetch call did not - // have a stack trace, so the SDK backfilled the stack trace so - // you can see which fetch call failed. - const virtualError = new Error(); - const virtualStackTrace = virtualError.stack; - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access return originalFetch.apply(GLOBAL_OBJ, args).then( async (response: Response) => { @@ -93,7 +87,6 @@ function instrumentFetch( ...handlerData, endTimestamp: timestampInSeconds() * 1000, response, - error: httpClientInstrumented ? virtualError : undefined, }); } From a7b89bce01397d3babbcaceec7ee9a8f051420a2 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Fri, 29 Nov 2024 15:23:18 +0000 Subject: [PATCH 4/4] Switch back to passing error --- packages/browser-utils/src/instrument/xhr.ts | 2 +- .../browser/src/integrations/httpclient.ts | 21 ++++++++++--------- packages/core/src/types-hoist/instrument.ts | 7 +++++-- .../core/src/utils-hoist/instrument/fetch.ts | 9 ++++---- 4 files changed, 21 insertions(+), 18 deletions(-) diff --git a/packages/browser-utils/src/instrument/xhr.ts b/packages/browser-utils/src/instrument/xhr.ts index f46ecd67b0b6..468155ef3b2a 100644 --- a/packages/browser-utils/src/instrument/xhr.ts +++ b/packages/browser-utils/src/instrument/xhr.ts @@ -82,7 +82,7 @@ export function instrumentXHR(): void { endTimestamp: timestampInSeconds() * 1000, startTimestamp, xhr: xhrOpenThisArg, - stack: virtualError.stack, + virtualError, }; triggerHandlers('xhr', handlerData); } diff --git a/packages/browser/src/integrations/httpclient.ts b/packages/browser/src/integrations/httpclient.ts index 15c8b3412963..3ff5218f1ab8 100644 --- a/packages/browser/src/integrations/httpclient.ts +++ b/packages/browser/src/integrations/httpclient.ts @@ -70,7 +70,7 @@ function _fetchResponseHandler( requestInfo: RequestInfo, response: Response, requestInit?: RequestInit, - stack?: string, + error?: unknown, ): void { if (_shouldCaptureResponse(options, response.status, response.url)) { const request = _getRequest(requestInfo, requestInit); @@ -90,7 +90,7 @@ function _fetchResponseHandler( responseHeaders, requestCookies, responseCookies, - stacktrace: stack, + error, }); captureEvent(event); @@ -129,7 +129,7 @@ function _xhrResponseHandler( xhr: XMLHttpRequest, method: string, headers: Record, - stack?: string, + error?: unknown, ): void { if (_shouldCaptureResponse(options, xhr.status, xhr.responseURL)) { let requestHeaders, responseCookies, responseHeaders; @@ -162,7 +162,7 @@ function _xhrResponseHandler( // Can't access request cookies from XHR responseHeaders, responseCookies, - stacktrace: stack, + error, }); captureEvent(event); @@ -292,14 +292,14 @@ function _wrapFetch(client: Client, options: HttpClientOptions): void { return; } - const { response, args } = handlerData; + const { response, args, error, virtualError } = handlerData; const [requestInfo, requestInit] = args as [RequestInfo, RequestInit | undefined]; if (!response) { return; } - _fetchResponseHandler(options, requestInfo, response as Response, requestInit, handlerData.stack); + _fetchResponseHandler(options, requestInfo, response as Response, requestInit, error || virtualError); }, false); } @@ -316,6 +316,8 @@ function _wrapXHR(client: Client, options: HttpClientOptions): void { return; } + const { error, virtualError } = handlerData; + const xhr = handlerData.xhr as SentryWrappedXMLHttpRequest & XMLHttpRequest; const sentryXhrData = xhr[SENTRY_XHR_DATA_KEY]; @@ -327,7 +329,7 @@ function _wrapXHR(client: Client, options: HttpClientOptions): void { const { method, request_headers: headers } = sentryXhrData; try { - _xhrResponseHandler(options, xhr, method, headers, handlerData.stack); + _xhrResponseHandler(options, xhr, method, headers, error || virtualError); } catch (e) { DEBUG_BUILD && logger.warn('Error while extracting response event form XHR response', e); } @@ -362,13 +364,12 @@ function _createEvent(data: { responseCookies?: Record; requestHeaders?: Record; requestCookies?: Record; - stacktrace?: string; + error?: unknown; }): SentryEvent { const client = getClient(); - const virtualStackTrace = client && data.stacktrace ? data.stacktrace : undefined; + const virtualStackTrace = client && data.error && data.error instanceof Error ? data.error.stack : undefined; // Remove the first frame from the stack as it's the HttpClient call const stack = virtualStackTrace && client ? client.getOptions().stackParser(virtualStackTrace, 0, 1) : undefined; - const message = `HTTP Client Error with status code: ${data.status}`; const event: SentryEvent = { diff --git a/packages/core/src/types-hoist/instrument.ts b/packages/core/src/types-hoist/instrument.ts index f779b2096597..420482579dd9 100644 --- a/packages/core/src/types-hoist/instrument.ts +++ b/packages/core/src/types-hoist/instrument.ts @@ -32,7 +32,9 @@ export interface HandlerDataXhr { xhr: SentryWrappedXMLHttpRequest; startTimestamp?: number; endTimestamp?: number; - stack?: string; + error?: unknown; + // This is to be consumed by the HttpClient integration + virtualError?: unknown; } interface SentryFetchData { @@ -57,7 +59,8 @@ export interface HandlerDataFetch { headers: WebFetchHeaders; }; error?: unknown; - stack?: string; + // This is to be consumed by the HttpClient integration + virtualError?: unknown; } export interface HandlerDataDom { diff --git a/packages/core/src/utils-hoist/instrument/fetch.ts b/packages/core/src/utils-hoist/instrument/fetch.ts index c822066a7c2d..954ab50a7536 100644 --- a/packages/core/src/utils-hoist/instrument/fetch.ts +++ b/packages/core/src/utils-hoist/instrument/fetch.ts @@ -48,7 +48,7 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat fill(GLOBAL_OBJ, 'fetch', function (originalFetch: () => void): () => void { return function (...args: any[]): void { - // We capture the stack right here and not in the Promise error callback because Safari (and probably other + // We capture the error right here and not in the Promise error callback because Safari (and probably other // browsers too) will wipe the stack trace up to this point, only leaving us with this file which is useless. // NOTE: If you are a Sentry user, and you are seeing this stack frame, @@ -56,7 +56,6 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat // have a stack trace, so the SDK backfilled the stack trace so // you can see which fetch call failed. const virtualError = new Error(); - const virtualStackTrace = virtualError.stack; const { method, url } = parseFetchArgs(args); const handlerData: HandlerDataFetch = { @@ -66,7 +65,8 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat url, }, startTimestamp: timestampInSeconds() * 1000, - stack: virtualStackTrace, + // // Adding the error to be able to fingerprint the failed fetch event in HttpClient instrumentation + virtualError, }; // if there is no callback, fetch is instrumented directly @@ -82,7 +82,6 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat if (onFetchResolved) { onFetchResolved(response); } else { - // Adding the stacktrace to be able to fingerprint the failed fetch event in HttpClient instrumentation triggerHandlers('fetch', { ...handlerData, endTimestamp: timestampInSeconds() * 1000, @@ -104,7 +103,7 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat // 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; + error.stack = virtualError.stack; addNonEnumerableProperty(error, 'framesToPop', 1); }