Skip to content

feat(browser): Attach virtual stack traces to HttpClient events. #14515

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 4 commits into from
Dec 12, 2024
Merged

feat(browser): Attach virtual stack traces to HttpClient events. #14515

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b776eac
feat(browser): Attach virtual stack traces to `HttpClient` events.
onurtemizkan Nov 27, 2024
648693c
Update integration tests
onurtemizkan Nov 27, 2024
33a5bfa
Pass stacktrace string instead of error.
onurtemizkan Nov 28, 2024
a7b89bc
Switch back to passing error
onurtemizkan Nov 29, 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
Prev Previous commit
Next Next commit
Pass stacktrace string instead of error.
  • Loading branch information
@onurtemizkan
onurtemizkan authored Nov 29, 2024
commit 33a5bfa728ea95cd9ecf0fdf71439cd94da34b32
11 changes: 4 additions & 7 deletions packages/browser-utils/src/instrument/xhr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
}
Expand Down
38 changes: 17 additions & 21 deletions packages/browser/src/integrations/httpclient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -90,7 +90,7 @@ function _fetchResponseHandler(
responseHeaders,
requestCookies,
responseCookies,
stacktrace: error instanceof Error ? error.stack : undefined,
stacktrace: stack,
});

captureEvent(event);
Expand Down Expand Up @@ -129,7 +129,7 @@ function _xhrResponseHandler(
xhr: XMLHttpRequest,
method: string,
headers: Record<string, string>,
error?: unknown,
stack?: string,
): void {
if (_shouldCaptureResponse(options, xhr.status, xhr.responseURL)) {
let requestHeaders, responseCookies, responseHeaders;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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);
});
}

/**
Expand Down
3 changes: 2 additions & 1 deletion packages/core/src/types-hoist/instrument.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export interface HandlerDataXhr {
xhr: SentryWrappedXMLHttpRequest;
startTimestamp?: number;
endTimestamp?: number;
error?: unknown;
stack?: string;
}

interface SentryFetchData {
Expand All @@ -57,6 +57,7 @@ export interface HandlerDataFetch {
headers: WebFetchHeaders;
};
error?: unknown;
stack?: string;
}

export interface HandlerDataDom {
Expand Down
35 changes: 14 additions & 21 deletions packages/core/src/utils-hoist/instrument/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

/**
Expand All @@ -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;
Copy link
Contributor

@lforst lforst Nov 29, 2024

Choose a reason for hiding this comment

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

It is relatively expensive for engines to compute stack traces. I wonder if we could change this a bit so we don't create stack traces every time fetch is called but only when we actually need it.

Copy link
Collaborator Author

@onurtemizkan onurtemizkan Nov 29, 2024

Choose a reason for hiding this comment

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

Ok, passing a boolean whether the fetch/xhr instrumentations are triggered by HttpClient was flaky because of maybeInstrument, so passing the error itself in handler data under virtualError key (not to conflict with other integrations like Breadcrumbs that use error key) and creating the stacks inside HttpClient will hopefully give us a slight performance improvement. a7b89bc


const { method, url } = parseFetchArgs(args);
const handlerData: HandlerDataFetch = {
args,
Expand All @@ -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) => {
Expand All @@ -93,7 +87,6 @@ function instrumentFetch(
...handlerData,
endTimestamp: timestampInSeconds() * 1000,
response,
error: httpClientInstrumented ? virtualError : undefined,
});
}

Expand Down