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.

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

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

Merged
merged 4 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
]),
},
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
]),
},
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
]),
},
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
]),
},
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
]),
},
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}),
]),
},
},
],
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions packages/browser-utils/src/instrument/xhr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,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
Expand Down Expand Up @@ -75,6 +82,7 @@ export function instrumentXHR(): void {
endTimestamp: timestampInSeconds() * 1000,
startTimestamp,
xhr: xhrOpenThisArg,
virtualError,
};
triggerHandlers('xhr', handlerData);
}
Expand Down
20 changes: 16 additions & 4 deletions packages/browser/src/integrations/httpclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -89,6 +90,7 @@ function _fetchResponseHandler(
responseHeaders,
requestCookies,
responseCookies,
error,
});

captureEvent(event);
Expand Down Expand Up @@ -127,6 +129,7 @@ function _xhrResponseHandler(
xhr: XMLHttpRequest,
method: string,
headers: Record<string, string>,
error?: unknown,
): void {
if (_shouldCaptureResponse(options, xhr.status, xhr.responseURL)) {
let requestHeaders, responseCookies, responseHeaders;
Expand Down Expand Up @@ -159,6 +162,7 @@ function _xhrResponseHandler(
// Can't access request cookies from XHR
responseHeaders,
responseCookies,
error,
});

captureEvent(event);
Expand Down Expand Up @@ -288,15 +292,15 @@ 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);
});
_fetchResponseHandler(options, requestInfo, response as Response, requestInit, error || virtualError);
}, false);
}

/**
Expand All @@ -312,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];
Expand All @@ -323,7 +329,7 @@ function _wrapXHR(client: Client, options: HttpClientOptions): void {
const { method, request_headers: headers } = sentryXhrData;

try {
_xhrResponseHandler(options, xhr, method, headers);
_xhrResponseHandler(options, xhr, method, headers, error || virtualError);
} catch (e) {
DEBUG_BUILD && logger.warn('Error while extracting response event form XHR response', e);
}
Expand Down Expand Up @@ -358,7 +364,12 @@ function _createEvent(data: {
responseCookies?: Record<string, string>;
requestHeaders?: Record<string, string>;
requestCookies?: Record<string, string>;
error?: unknown;
}): SentryEvent {
const client = getClient();
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 = {
Expand All @@ -368,6 +379,7 @@ function _createEvent(data: {
{
type: 'Error',
value: message,
stacktrace: stack ? { frames: stack } : undefined,
},
],
},
Expand Down
5 changes: 5 additions & 0 deletions packages/core/src/types-hoist/instrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ export interface HandlerDataXhr {
xhr: SentryWrappedXMLHttpRequest;
startTimestamp?: number;
endTimestamp?: number;
error?: unknown;
// This is to be consumed by the HttpClient integration
virtualError?: unknown;
}

interface SentryFetchData {
Expand All @@ -56,6 +59,8 @@ export interface HandlerDataFetch {
headers: WebFetchHeaders;
};
error?: unknown;
// This is to be consumed by the HttpClient integration
virtualError?: unknown;
}

export interface HandlerDataDom {
Expand Down
22 changes: 12 additions & 10 deletions packages/core/src/utils-hoist/instrument/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,15 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat

fill(GLOBAL_OBJ, 'fetch', function (originalFetch: () => void): () => void {
return function (...args: any[]): void {
// 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,
// 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 { method, url } = parseFetchArgs(args);
const handlerData: HandlerDataFetch = {
args,
Expand All @@ -56,6 +65,8 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
url,
},
startTimestamp: timestampInSeconds() * 1000,
// // 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
Expand All @@ -65,15 +76,6 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat
});
}

// 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 virtualStackTrace = new Error().stack;

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
return originalFetch.apply(GLOBAL_OBJ, args).then(
async (response: Response) => {
Expand Down Expand Up @@ -101,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);
}

Expand Down
Loading