-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Add onRequestSpanEnd
hook to browser tracing integration
#17884
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
base: develop
Are you sure you want to change the base?
Changes from all commits
545ca89
0757712
eb650ee
2a83753
2faa6cc
69c9baf
c7282c2
c6a1382
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import * as Sentry from '@sentry/browser'; | ||
|
||
window.Sentry = Sentry; | ||
|
||
Sentry.init({ | ||
dsn: 'https://public@dsn.ingest.sentry.io/1337', | ||
integrations: [ | ||
Sentry.browserTracingIntegration({ | ||
idleTimeout: 1000, | ||
onRequestSpanEnd(span, { headers }) { | ||
if (headers) { | ||
span.setAttribute('hook.called.response-type', headers.get('x-response-type')); | ||
} | ||
}, | ||
}), | ||
], | ||
tracesSampleRate: 1, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
fetch('http://sentry-test.io/fetch', { | ||
headers: { | ||
foo: 'fetch', | ||
}, | ||
}); | ||
|
||
const xhr = new XMLHttpRequest(); | ||
|
||
xhr.open('GET', 'http://sentry-test.io/xhr'); | ||
xhr.setRequestHeader('foo', 'xhr'); | ||
xhr.send(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import { expect } from '@playwright/test'; | ||
import type { Event } from '@sentry/core'; | ||
import { sentryTest } from '../../../../utils/fixtures'; | ||
import { getMultipleSentryEnvelopeRequests, shouldSkipTracingTest } from '../../../../utils/helpers'; | ||
|
||
sentryTest('should call onRequestSpanEnd hook', async ({ browserName, getLocalTestUrl, page }) => { | ||
const supportedBrowsers = ['chromium', 'firefox']; | ||
|
||
if (shouldSkipTracingTest() || !supportedBrowsers.includes(browserName)) { | ||
sentryTest.skip(); | ||
} | ||
|
||
await page.route('http://sentry-test.io/fetch', async route => { | ||
await route.fulfill({ | ||
status: 200, | ||
headers: { | ||
'Content-Type': 'application/json', | ||
'X-Response-Type': 'fetch', | ||
'access-control-expose-headers': '*', | ||
}, | ||
body: '', | ||
}); | ||
}); | ||
await page.route('http://sentry-test.io/xhr', async route => { | ||
await route.fulfill({ | ||
status: 200, | ||
headers: { | ||
'Content-Type': 'application/json', | ||
'X-Response-Type': 'xhr', | ||
'access-control-expose-headers': '*', | ||
}, | ||
body: '', | ||
}); | ||
}); | ||
|
||
const url = await getLocalTestUrl({ testDir: __dirname }); | ||
|
||
const envelopes = await getMultipleSentryEnvelopeRequests<Event>(page, 2, { url, timeout: 10000 }); | ||
|
||
const tracingEvent = envelopes[envelopes.length - 1]; // last envelope contains tracing data on all browsers | ||
|
||
expect(tracingEvent.spans).toContainEqual( | ||
expect.objectContaining({ | ||
op: 'http.client', | ||
data: expect.objectContaining({ | ||
type: 'xhr', | ||
'hook.called.response-type': 'xhr', | ||
}), | ||
}), | ||
); | ||
|
||
expect(tracingEvent.spans).toContainEqual( | ||
expect.objectContaining({ | ||
op: 'http.client', | ||
data: expect.objectContaining({ | ||
type: 'fetch', | ||
'hook.called.response-type': 'fetch', | ||
}), | ||
}), | ||
); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,11 @@ | ||
import type { Client, HandlerDataXhr, SentryWrappedXMLHttpRequest, Span, WebFetchHeaders } from '@sentry/core'; | ||
import type { | ||
Client, | ||
HandlerDataXhr, | ||
RequestHookInfo, | ||
ResponseHookInfo, | ||
SentryWrappedXMLHttpRequest, | ||
Span, | ||
} from '@sentry/core'; | ||
import { | ||
addFetchEndInstrumentationHandler, | ||
addFetchInstrumentationHandler, | ||
|
@@ -22,11 +29,12 @@ import type { XhrHint } from '@sentry-internal/browser-utils'; | |
import { | ||
addPerformanceInstrumentationHandler, | ||
addXhrInstrumentationHandler, | ||
parseXhrResponseHeaders, | ||
resourceTimingToSpanAttributes, | ||
SENTRY_XHR_DATA_KEY, | ||
} from '@sentry-internal/browser-utils'; | ||
import type { BrowserClient } from '../client'; | ||
import { WINDOW } from '../helpers'; | ||
import { baggageHeaderHasSentryValues, createHeadersSafely, getFullURL, isPerformanceResourceTiming } from './utils'; | ||
|
||
/** Options for Request Instrumentation */ | ||
export interface RequestInstrumentationOptions { | ||
|
@@ -102,7 +110,12 @@ export interface RequestInstrumentationOptions { | |
/** | ||
* Is called when spans are started for outgoing requests. | ||
*/ | ||
onRequestSpanStart?(span: Span, requestInformation: { headers?: WebFetchHeaders }): void; | ||
onRequestSpanStart?(span: Span, requestInformation: RequestHookInfo): void; | ||
|
||
/** | ||
* Is called when spans end for outgoing requests, providing access to response headers. | ||
*/ | ||
onRequestSpanEnd?(span: Span, responseInformation: ResponseHookInfo): void; | ||
} | ||
|
||
const responseToSpanId = new WeakMap<object, string>(); | ||
|
@@ -125,6 +138,7 @@ export function instrumentOutgoingRequests(client: Client, _options?: Partial<Re | |
enableHTTPTimings, | ||
tracePropagationTargets, | ||
onRequestSpanStart, | ||
onRequestSpanEnd, | ||
} = { | ||
...defaultRequestInstrumentationOptions, | ||
..._options, | ||
|
@@ -171,6 +185,7 @@ export function instrumentOutgoingRequests(client: Client, _options?: Partial<Re | |
addFetchInstrumentationHandler(handlerData => { | ||
const createdSpan = instrumentFetchRequest(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans, { | ||
propagateTraceparent, | ||
onRequestSpanEnd, | ||
}); | ||
|
||
if (handlerData.response && handlerData.fetchData.__span) { | ||
|
@@ -205,34 +220,22 @@ export function instrumentOutgoingRequests(client: Client, _options?: Partial<Re | |
shouldAttachHeadersWithTargets, | ||
spans, | ||
propagateTraceparent, | ||
onRequestSpanEnd, | ||
); | ||
|
||
if (createdSpan) { | ||
if (enableHTTPTimings) { | ||
addHTTPTimings(createdSpan); | ||
} | ||
|
||
let headers; | ||
try { | ||
headers = new Headers(handlerData.xhr.__sentry_xhr_v3__?.request_headers); | ||
} catch { | ||
// noop | ||
} | ||
onRequestSpanStart?.(createdSpan, { headers }); | ||
onRequestSpanStart?.(createdSpan, { | ||
headers: createHeadersSafely(handlerData.xhr.__sentry_xhr_v3__?.request_headers), | ||
}); | ||
} | ||
}); | ||
} | ||
} | ||
|
||
function isPerformanceResourceTiming(entry: PerformanceEntry): entry is PerformanceResourceTiming { | ||
return ( | ||
entry.entryType === 'resource' && | ||
'initiatorType' in entry && | ||
typeof (entry as PerformanceResourceTiming).nextHopProtocol === 'string' && | ||
(entry.initiatorType === 'fetch' || entry.initiatorType === 'xmlhttprequest') | ||
); | ||
} | ||
|
||
/** | ||
* Creates a temporary observer to listen to the next fetch/xhr resourcing timings, | ||
* so that when timings hit their per-browser limit they don't need to be removed. | ||
|
@@ -315,6 +318,7 @@ function xhrCallback( | |
shouldAttachHeaders: (url: string) => boolean, | ||
spans: Record<string, Span>, | ||
propagateTraceparent?: boolean, | ||
onRequestSpanEnd?: RequestInstrumentationOptions['onRequestSpanEnd'], | ||
): Span | undefined { | ||
const xhr = handlerData.xhr; | ||
const sentryXhrData = xhr?.[SENTRY_XHR_DATA_KEY]; | ||
|
@@ -337,6 +341,11 @@ function xhrCallback( | |
setHttpStatus(span, sentryXhrData.status_code); | ||
span.end(); | ||
|
||
onRequestSpanEnd?.(span, { | ||
headers: createHeadersSafely(parseXhrResponseHeaders(xhr as XMLHttpRequest & SentryWrappedXMLHttpRequest)), | ||
error: handlerData.error, | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: XHR Hook Timing and Error Handling IssuesThe |
||
// eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
delete spans[spanId]; | ||
} | ||
|
@@ -438,18 +447,3 @@ function setHeaderOnXhr( | |
// Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED. | ||
} | ||
} | ||
|
||
function baggageHeaderHasSentryValues(baggageHeader: string): boolean { | ||
return baggageHeader.split(',').some(value => value.trim().startsWith('sentry-')); | ||
} | ||
|
||
function getFullURL(url: string): string | undefined { | ||
try { | ||
// By adding a base URL to new URL(), this will also work for relative urls | ||
// If `url` is a full URL, the base URL is ignored anyhow | ||
const parsed = new URL(url, WINDOW.location.origin); | ||
return parsed.href; | ||
} catch { | ||
return undefined; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: XHR Memory Leak and Header Parsing Errors
In
xhrCallback
, XHR spans may leak memory because cleanup and theonRequestSpanEnd
callback are skipped ifsentryXhrData.status_code
is undefined. Additionally,parseXhrResponseHeaders
receives a wrapped XHR object, which could lead to runtime errors when parsing response headers for theonRequestSpanEnd
callback.