Skip to content

Commit

Permalink
fix(utils): Make xhr instrumentation independent of parallel running …
Browse files Browse the repository at this point in the history
…SDK versions (#7836)
  • Loading branch information
lforst authored Apr 13, 2023
1 parent 7b50bfa commit 95f721f
Show file tree
Hide file tree
Showing 8 changed files with 51 additions and 34 deletions.
7 changes: 5 additions & 2 deletions packages/browser/src/integrations/breadcrumbs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
logger,
parseUrl,
safeJoin,
SENTRY_XHR_DATA_KEY,
severityLevelFromString,
} from '@sentry/utils';

Expand Down Expand Up @@ -225,12 +226,14 @@ function _consoleBreadcrumb(handlerData: HandlerData & { args: unknown[]; level:
function _xhrBreadcrumb(handlerData: HandlerData & HandlerDataXhr): void {
const { startTimestamp, endTimestamp } = handlerData;

const sentryXhrData = handlerData.xhr[SENTRY_XHR_DATA_KEY];

// We only capture complete, non-sentry requests
if (!startTimestamp || !endTimestamp || !handlerData.xhr.__sentry_xhr__) {
if (!startTimestamp || !endTimestamp || !sentryXhrData) {
return;
}

const { method, url, status_code, body } = handlerData.xhr.__sentry_xhr__;
const { method, url, status_code, body } = sentryXhrData;

const data: XhrBreadcrumbData = {
method,
Expand Down
7 changes: 5 additions & 2 deletions packages/integrations/src/httpclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
addInstrumentationHandler,
GLOBAL_OBJ,
logger,
SENTRY_XHR_DATA_KEY,
supportsNativeFetch,
} from '@sentry/utils';

Expand Down Expand Up @@ -322,11 +323,13 @@ export class HttpClient implements Integration {
(handlerData: HandlerDataXhr & { xhr: SentryWrappedXMLHttpRequest & XMLHttpRequest }) => {
const { xhr } = handlerData;

if (!xhr.__sentry_xhr__) {
const sentryXhrData = xhr[SENTRY_XHR_DATA_KEY];

if (!sentryXhrData) {
return;
}

const { method, request_headers: headers } = xhr.__sentry_xhr__;
const { method, request_headers: headers } = sentryXhrData;

if (!method) {
return;
Expand Down
7 changes: 5 additions & 2 deletions packages/replay/src/coreHandlers/handleXhr.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { HandlerDataXhr } from '@sentry/types';
import { SENTRY_XHR_DATA_KEY } from '@sentry/utils';

import type { NetworkRequestData, ReplayContainer, ReplayPerformanceEntry } from '../types';
import { addNetworkBreadcrumb } from './util/addNetworkBreadcrumb';
Expand All @@ -7,12 +8,14 @@ import { addNetworkBreadcrumb } from './util/addNetworkBreadcrumb';
export function handleXhr(handlerData: HandlerDataXhr): ReplayPerformanceEntry<NetworkRequestData> | null {
const { startTimestamp, endTimestamp, xhr } = handlerData;

if (!startTimestamp || !endTimestamp || !xhr.__sentry_xhr__) {
const sentryXhrData = xhr[SENTRY_XHR_DATA_KEY];

if (!startTimestamp || !endTimestamp || !sentryXhrData) {
return null;
}

// This is only used as a fallback, so we know the body sizes are never set here
const { method, url, status_code: statusCode } = xhr.__sentry_xhr__;
const { method, url, status_code: statusCode } = sentryXhrData;

if (url === undefined) {
return null;
Expand Down
7 changes: 4 additions & 3 deletions packages/replay/test/unit/coreHandlers/handleXhr.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import type { HandlerDataXhr, SentryWrappedXMLHttpRequest, SentryXhrData } from '@sentry/types';
import { SENTRY_XHR_DATA_KEY } from '@sentry/utils';

import { handleXhr } from '../../../src/coreHandlers/handleXhr';

const DEFAULT_DATA: HandlerDataXhr = {
args: ['GET', '/api/0/organizations/sentry/'],
xhr: {
__sentry_xhr__: {
[SENTRY_XHR_DATA_KEY]: {
method: 'GET',
url: '/api/0/organizations/sentry/',
status_code: 200,
Expand Down Expand Up @@ -45,8 +46,8 @@ describe('Unit | coreHandlers | handleXhr', () => {
...DEFAULT_DATA,
xhr: {
...DEFAULT_DATA.xhr,
__sentry_xhr__: {
...(DEFAULT_DATA.xhr.__sentry_xhr__ as SentryXhrData),
[SENTRY_XHR_DATA_KEY]: {
...(DEFAULT_DATA.xhr[SENTRY_XHR_DATA_KEY] as SentryXhrData),
request_body_size: 123,
response_body_size: 456,
},
Expand Down
34 changes: 18 additions & 16 deletions packages/tracing-internal/src/browser/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
BAGGAGE_HEADER_NAME,
dynamicSamplingContextToSentryBaggageHeader,
isInstanceOf,
SENTRY_XHR_DATA_KEY,
stringMatchesSomePattern,
} from '@sentry/utils';

Expand Down Expand Up @@ -74,7 +75,7 @@ export interface FetchData {
/** Data returned from XHR request */
export interface XHRData {
xhr?: {
__sentry_xhr__?: {
[SENTRY_XHR_DATA_KEY]?: {
method: string;
url: string;
status_code: number;
Expand Down Expand Up @@ -297,24 +298,25 @@ export function xhrCallback(
shouldAttachHeaders: (url: string) => boolean,
spans: Record<string, Span>,
): void {
const xhr = handlerData.xhr;
const sentryXhrData = xhr && xhr[SENTRY_XHR_DATA_KEY];

if (
!hasTracingEnabled() ||
(handlerData.xhr && handlerData.xhr.__sentry_own_request__) ||
!(handlerData.xhr && handlerData.xhr.__sentry_xhr__ && shouldCreateSpan(handlerData.xhr.__sentry_xhr__.url))
(xhr && xhr.__sentry_own_request__) ||
!(xhr && sentryXhrData && shouldCreateSpan(sentryXhrData.url))
) {
return;
}

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) {
const spanId = handlerData.xhr.__sentry_xhr_span_id__;
const spanId = xhr.__sentry_xhr_span_id__;
if (!spanId) return;

const span = spans[spanId];
if (span) {
span.setHttpStatus(xhr.status_code);
span.setHttpStatus(sentryXhrData.status_code);
span.finish();

// eslint-disable-next-line @typescript-eslint/no-dynamic-delete
Expand All @@ -330,21 +332,21 @@ export function xhrCallback(
if (currentSpan && activeTransaction) {
const span = currentSpan.startChild({
data: {
...xhr.data,
...sentryXhrData.data,
type: 'xhr',
method: xhr.method,
url: xhr.url,
method: sentryXhrData.method,
url: sentryXhrData.url,
},
description: `${xhr.method} ${xhr.url}`,
description: `${sentryXhrData.method} ${sentryXhrData.url}`,
op: 'http.client',
});

handlerData.xhr.__sentry_xhr_span_id__ = span.spanId;
spans[handlerData.xhr.__sentry_xhr_span_id__] = span;
xhr.__sentry_xhr_span_id__ = span.spanId;
spans[xhr.__sentry_xhr_span_id__] = span;

if (handlerData.xhr.setRequestHeader && shouldAttachHeaders(handlerData.xhr.__sentry_xhr__.url)) {
if (xhr.setRequestHeader && shouldAttachHeaders(sentryXhrData.url)) {
try {
handlerData.xhr.setRequestHeader('sentry-trace', span.toTraceparent());
xhr.setRequestHeader('sentry-trace', span.toTraceparent());

const dynamicSamplingContext = activeTransaction.getDynamicSamplingContext();
const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
Expand All @@ -353,7 +355,7 @@ export function xhrCallback(
// From MDN: "If this method is called several times with the same header, the values are merged into one single request header."
// We can therefore simply set a baggage header without checking what was there before
// https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/setRequestHeader
handlerData.xhr.setRequestHeader(BAGGAGE_HEADER_NAME, sentryBaggageHeader);
xhr.setRequestHeader(BAGGAGE_HEADER_NAME, sentryBaggageHeader);
}
} catch (_) {
// Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED.
Expand Down
7 changes: 4 additions & 3 deletions packages/tracing-internal/test/browser/request.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable deprecation/deprecation */
import * as sentryCore from '@sentry/core';
import * as utils from '@sentry/utils';
import { SENTRY_XHR_DATA_KEY } from '@sentry/utils';

import type { Transaction } from '../../../tracing/src';
import { addExtensionMethods, Span, spanStatusfromHttpCode } from '../../../tracing/src';
Expand Down Expand Up @@ -234,7 +235,7 @@ describe('callbacks', () => {
beforeEach(() => {
xhrHandlerData = {
xhr: {
__sentry_xhr__: {
[SENTRY_XHR_DATA_KEY]: {
method: 'GET',
url: 'http://dogs.are.great/',
status_code: 200,
Expand Down Expand Up @@ -328,7 +329,7 @@ describe('callbacks', () => {
...xhrHandlerData,
endTimestamp,
};
postRequestXHRHandlerData.xhr!.__sentry_xhr__!.status_code = 404;
postRequestXHRHandlerData.xhr![SENTRY_XHR_DATA_KEY]!.status_code = 404;

// triggered by response coming back
xhrCallback(postRequestXHRHandlerData, alwaysCreateSpan, alwaysAttachHeaders, spans);
Expand All @@ -342,7 +343,7 @@ describe('callbacks', () => {
const postRequestXHRHandlerData = {
...{
xhr: {
__sentry_xhr__: xhrHandlerData.xhr?.__sentry_xhr__,
[SENTRY_XHR_DATA_KEY]: xhrHandlerData.xhr?.[SENTRY_XHR_DATA_KEY],
},
},
startTimestamp,
Expand Down
3 changes: 2 additions & 1 deletion packages/types/src/instrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@
type XHRSendInput = unknown;

export interface SentryWrappedXMLHttpRequest {
__sentry_xhr__?: SentryXhrData;
__sentry_xhr_v2__?: SentryXhrData;
__sentry_own_request__?: boolean;
}

// WARNING: When the shape of this type is changed bump the version in `SentryWrappedXMLHttpRequest`
export interface SentryXhrData {
method?: string;
url?: string;
Expand Down
13 changes: 8 additions & 5 deletions packages/utils/src/instrument.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import { getGlobalObject } from './worldwide';
// eslint-disable-next-line deprecation/deprecation
const WINDOW = getGlobalObject<Window>();

export const SENTRY_XHR_DATA_KEY = '__sentry_xhr_v2__';

export type InstrumentHandlerType =
| 'console'
| 'dom'
Expand Down Expand Up @@ -244,7 +246,7 @@ function instrumentXHR(): void {
fill(xhrproto, 'open', function (originalOpen: () => void): () => void {
return function (this: XMLHttpRequest & SentryWrappedXMLHttpRequest, ...args: any[]): void {
const url = args[1];
const xhrInfo: SentryXhrData = (this.__sentry_xhr__ = {
const xhrInfo: SentryXhrData = (this[SENTRY_XHR_DATA_KEY] = {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
method: isString(args[0]) ? args[0].toUpperCase() : args[0],
url: args[1],
Expand All @@ -259,7 +261,7 @@ function instrumentXHR(): void {

const onreadystatechangeHandler: () => void = () => {
// For whatever reason, this is not the same instance here as from the outer method
const xhrInfo = this.__sentry_xhr__;
const xhrInfo = this[SENTRY_XHR_DATA_KEY];

if (!xhrInfo) {
return;
Expand Down Expand Up @@ -301,7 +303,7 @@ function instrumentXHR(): void {
return function (this: SentryWrappedXMLHttpRequest, ...setRequestHeaderArgs: unknown[]): void {
const [header, value] = setRequestHeaderArgs as [string, string];

const xhrInfo = this.__sentry_xhr__;
const xhrInfo = this[SENTRY_XHR_DATA_KEY];

if (xhrInfo) {
xhrInfo.request_headers[header] = value;
Expand All @@ -317,8 +319,9 @@ function instrumentXHR(): void {

fill(xhrproto, 'send', function (originalSend: () => void): () => void {
return function (this: XMLHttpRequest & SentryWrappedXMLHttpRequest, ...args: any[]): void {
if (this.__sentry_xhr__ && args[0] !== undefined) {
this.__sentry_xhr__.body = args[0];
const sentryXhrData = this[SENTRY_XHR_DATA_KEY];
if (sentryXhrData && args[0] !== undefined) {
sentryXhrData.body = args[0];
}

triggerHandlers('xhr', {
Expand Down

0 comments on commit 95f721f

Please sign in to comment.