Skip to content

Commit

Permalink
πŸ› [RUMF-876] Improve proxy behavior for xhr reuse (#865)
Browse files Browse the repository at this point in the history
* ♻️ πŸ› Clear event listeners on XMLHttpRequest

* βœ… Add test for multi requests with XMLHttpRequest

* Add a third state interface for open xhr

* ♻️  Move reportXhr out of  XMLHttpRequest.send

* βœ…  Test for XHRs with same XMLHttpRequest instance

* πŸ›  Ensure to call overridden onreadystatechange

* ♻️  Keep same _datadog_xhr ref in sendXhr

* ♻️  Simplify reportXhr
  • Loading branch information
amortemousque authored Jun 10, 2021
1 parent 8f24278 commit 52c475d
Show file tree
Hide file tree
Showing 4 changed files with 237 additions and 108 deletions.
90 changes: 82 additions & 8 deletions packages/core/src/browser/xhrProxy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,21 @@ describe('xhr proxy', () => {
})

it('should track successful request aborted', (done) => {
const onReadyStateChange = jasmine.createSpy()
withXhr({
setup(xhr) {
xhr.onreadystatechange = onReadyStateChange
xhr.addEventListener('load', () => xhr.abort())
xhr.onreadystatechange = () => {
if (xhr.readyState === XMLHttpRequest.DONE) {
xhr.abort()
}
}
spyOn(xhr, 'onreadystatechange').and.callThrough()
xhr.open('GET', '/ok')
xhr.send()
xhr.complete(200, 'ok')
},
onComplete(xhr) {
const request = getRequest(0)
expect(completeSpy.calls.count()).toBe(1)
expect(request.method).toBe('GET')
expect(request.url).toContain('/ok')
expect(request.responseText).toBe('ok')
Expand All @@ -126,7 +130,7 @@ describe('xhr proxy', () => {
expect(request.duration).toEqual(jasmine.any(Number))
expect(request.isAborted).toBe(false)
expect(xhr.status).toBe(0)
expect(onReadyStateChange).toHaveBeenCalled()
expect(xhr.onreadystatechange).toHaveBeenCalledTimes(1)
done()
},
})
Expand Down Expand Up @@ -154,15 +158,15 @@ describe('xhr proxy', () => {
})
})

it('should track request with onreadystatechange overridden', (done) => {
it('should track request with onreadystatechange overridden before open', (done) => {
withXhr({
setup(xhr) {
xhr.onreadystatechange = jasmine.createSpy()
xhr.open('GET', '/ok')
xhr.send()
xhr.onreadystatechange = () => undefined
xhr.complete(200, 'ok')
},
onComplete() {
onComplete(xhr) {
const request = getRequest(0)
expect(request.method).toBe('GET')
expect(request.url).toContain('/ok')
Expand All @@ -171,6 +175,30 @@ describe('xhr proxy', () => {
expect(request.isAborted).toBe(false)
expect(request.startTime).toEqual(jasmine.any(Number))
expect(request.duration).toEqual(jasmine.any(Number))
expect(xhr.onreadystatechange).toHaveBeenCalled()
done()
},
})
})

it('should track request with onreadystatechange overridden after open', (done) => {
withXhr({
setup(xhr) {
xhr.open('GET', '/ok')
xhr.send()
xhr.onreadystatechange = jasmine.createSpy()
xhr.complete(200, 'ok')
},
onComplete(xhr) {
const request = getRequest(0)
expect(request.method).toBe('GET')
expect(request.url).toContain('/ok')
expect(request.responseText).toBe('ok')
expect(request.status).toBe(200)
expect(request.isAborted).toBe(false)
expect(request.startTime).toEqual(jasmine.any(Number))
expect(request.duration).toEqual(jasmine.any(Number))
expect(xhr.onreadystatechange).toHaveBeenCalled()
done()
},
})
Expand All @@ -194,7 +222,7 @@ describe('xhr proxy', () => {
})
})

it('should should not break xhr opened before the instrumentation', (done) => {
it('should not break xhr opened before the instrumentation', (done) => {
resetXhrProxy()
withXhr({
setup(xhr) {
Expand All @@ -209,4 +237,50 @@ describe('xhr proxy', () => {
},
})
})

it('should track multiple requests with the same xhr instance', (done) => {
let listeners: { [k: string]: Array<() => void> }
withXhr({
setup(xhr) {
const secondOnload = () => {
xhr.removeEventListener('load', secondOnload)
}
const onLoad = () => {
xhr.removeEventListener('load', onLoad)
xhr.addEventListener('load', secondOnload)
xhr.open('GET', '/ok?request=2')
xhr.send()
xhr.complete(400, 'ok')
}
xhr.onreadystatechange = jasmine.createSpy()
xhr.addEventListener('load', onLoad)
xhr.open('GET', '/ok?request=1')
xhr.send()
xhr.complete(200, 'ok')
listeners = xhr.listeners
},
onComplete(xhr) {
const firstRequest = getRequest(0)
expect(firstRequest.method).toBe('GET')
expect(firstRequest.url).toContain('/ok?request=1')
expect(firstRequest.status).toBe(200)
expect(firstRequest.isAborted).toBe(false)
expect(firstRequest.startTime).toEqual(jasmine.any(Number))
expect(firstRequest.duration).toEqual(jasmine.any(Number))

const secondRequest = getRequest(1)
expect(secondRequest.method).toBe('GET')
expect(secondRequest.url).toContain('/ok?request=2')
expect(secondRequest.status).toBe(400)
expect(secondRequest.isAborted).toBe(false)
expect(secondRequest.startTime).toEqual(jasmine.any(Number))
expect(secondRequest.duration).toEqual(jasmine.any(Number))

expect(xhr.onreadystatechange).toHaveBeenCalledTimes(2)
expect(listeners.load.length).toBe(0)
expect(listeners.loadend.length).toBe(0)
done()
},
})
})
})
171 changes: 88 additions & 83 deletions packages/core/src/browser/xhrProxy.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { monitor, callMonitored } from '../domain/internalMonitoring'
import { callMonitored, monitor } from '../domain/internalMonitoring'
import { Duration, elapsed, relativeNow, RelativeTime, ClocksState, clocksNow, timeStampNow } from '../tools/timeUtils'
import { normalizeUrl } from '../tools/urlPolyfill'

interface BrowserXHR extends XMLHttpRequest {
_datadog_xhr: XhrStartContext
interface BrowserXHR<T extends XhrOpenContext> extends XMLHttpRequest {
_datadog_xhr?: T
}

export interface XhrProxy<
Expand All @@ -14,12 +14,15 @@ export interface XhrProxy<
onRequestComplete: (callback: (context: CompleteContext) => void) => void
}

export interface XhrStartContext {
export interface XhrOpenContext {
method: string
url: string
}

export interface XhrStartContext extends XhrOpenContext {
startTime: RelativeTime // deprecated
startClocks: ClocksState

isAborted: boolean
/**
* allow clients to enhance the context
*/
Expand All @@ -30,7 +33,6 @@ export interface XhrCompleteContext extends XhrStartContext {
duration: Duration
status: number
responseText: string | undefined
isAborted: boolean
xhr: XMLHttpRequest
}

Expand All @@ -46,7 +48,16 @@ export function startXhrProxy<
CompleteContext extends XhrCompleteContext = XhrCompleteContext
>(): XhrProxy<StartContext, CompleteContext> {
if (!xhrProxySingleton) {
proxyXhr()
// eslint-disable-next-line @typescript-eslint/unbound-method
originalXhrOpen = XMLHttpRequest.prototype.open
// eslint-disable-next-line @typescript-eslint/unbound-method
originalXhrSend = XMLHttpRequest.prototype.send
// eslint-disable-next-line @typescript-eslint/unbound-method
originalXhrAbort = XMLHttpRequest.prototype.abort
XMLHttpRequest.prototype.open = openXhr
XMLHttpRequest.prototype.send = sendXhr
XMLHttpRequest.prototype.abort = abortXhr

xhrProxySingleton = {
beforeSend(callback: (context: XhrStartContext, xhr: XMLHttpRequest) => void) {
beforeSendCallbacks.push(callback)
Expand All @@ -70,86 +81,80 @@ export function resetXhrProxy() {
}
}

function proxyXhr() {
// eslint-disable-next-line @typescript-eslint/unbound-method
originalXhrOpen = XMLHttpRequest.prototype.open
// eslint-disable-next-line @typescript-eslint/unbound-method
originalXhrSend = XMLHttpRequest.prototype.send
// eslint-disable-next-line @typescript-eslint/unbound-method
originalXhrAbort = XMLHttpRequest.prototype.abort

XMLHttpRequest.prototype.open = function (this: BrowserXHR, method: string, url: string) {
callMonitored(() => {
// WARN: since this data structure is tied to the instance, it is shared by both logs and rum
// and can be used by different code versions depending on customer setup
// so it should stay compatible with older versions
;(this._datadog_xhr as Pick<XhrStartContext, 'method' | 'url'>) = {
method,
url: normalizeUrl(url),
}
})
return originalXhrOpen.apply(this, arguments as any)
}
function openXhr(this: BrowserXHR<XhrOpenContext>, method: string, url: string) {
callMonitored(() => {
// WARN: since this data structure is tied to the instance, it is shared by both logs and rum
// and can be used by different code versions depending on customer setup
// so it should stay compatible with older versions
this._datadog_xhr = {
method,
url: normalizeUrl(url),
}
})
return originalXhrOpen.apply(this, arguments as any)
}

XMLHttpRequest.prototype.send = function (this: BrowserXHR) {
callMonitored(() => {
if (this._datadog_xhr) {
const xhrPendingContext = this._datadog_xhr as XhrStartContext &
Pick<XhrCompleteContext, 'startTime' | 'isAborted'>
xhrPendingContext.startTime = relativeNow()
xhrPendingContext.startClocks = clocksNow()
xhrPendingContext.isAborted = false

const originalOnreadystatechange = this.onreadystatechange

this.onreadystatechange = function () {
if (this.readyState === XMLHttpRequest.DONE) {
// Try to report the XHR as soon as possible, because the XHR may be mutated by the
// application during a future event. For example, Angular is calling .abort() on
// completed requests during a onreadystatechange event, so the status becomes '0'
// before the request is collected.
callMonitored(reportXhr)
}

if (originalOnreadystatechange) {
originalOnreadystatechange.apply(this, arguments as any)
}
}

let hasBeenReported = false

const reportXhr = () => {
if (hasBeenReported) {
return
}
hasBeenReported = true

const xhrCompleteContext: XhrCompleteContext = {
...xhrPendingContext,
duration: elapsed(xhrPendingContext.startClocks.timeStamp, timeStampNow()),
responseText: this.response as string | undefined,
status: this.status,
xhr: this,
}

onRequestCompleteCallbacks.forEach((callback) => callback(xhrCompleteContext))
}

this.addEventListener('loadend', monitor(reportXhr))

beforeSendCallbacks.forEach((callback) => callback(xhrPendingContext, this))
function sendXhr(this: BrowserXHR<XhrStartContext>) {
callMonitored(() => {
if (!this._datadog_xhr) {
return
}

this._datadog_xhr.startTime = relativeNow()
this._datadog_xhr.startClocks = clocksNow()
this._datadog_xhr.isAborted = false

let hasBeenReported = false
const originalOnreadystatechange = this.onreadystatechange
const onreadystatechange = function (this: BrowserXHR<XhrStartContext>) {
if (this.readyState === XMLHttpRequest.DONE) {
// Try to report the XHR as soon as possible, because the XHR may be mutated by the
// application during a future event. For example, Angular is calling .abort() on
// completed requests during a onreadystatechange event, so the status becomes '0'
// before the request is collected.
onEnd()
}
})

return originalXhrSend.apply(this, arguments as any)
}
if (originalOnreadystatechange) {
originalOnreadystatechange.apply(this, arguments as any)
}
}

XMLHttpRequest.prototype.abort = function (this: BrowserXHR) {
callMonitored(() => {
if (this._datadog_xhr) {
this._datadog_xhr.isAborted = true
const onEnd = monitor(() => {
this.removeEventListener('loadend', onEnd)
// if the onreadystatechange hasn't been overridden by the user after the send()
if (this.onreadystatechange === onreadystatechange) {
this.onreadystatechange = originalOnreadystatechange
}
if (hasBeenReported) {
return
}
hasBeenReported = true
reportXhr(this as BrowserXHR<XhrCompleteContext>)
})
return originalXhrAbort.apply(this, arguments as any)
}
this.onreadystatechange = onreadystatechange
this.addEventListener('loadend', onEnd)

beforeSendCallbacks.forEach((callback) => callback(this._datadog_xhr!, this))
})

return originalXhrSend.apply(this, arguments as any)
}

function abortXhr(this: BrowserXHR<XhrStartContext>) {
callMonitored(() => {
if (this._datadog_xhr) {
this._datadog_xhr.isAborted = true
}
})
return originalXhrAbort.apply(this, arguments as any)
}

function reportXhr(xhr: BrowserXHR<XhrCompleteContext>) {
xhr._datadog_xhr!.duration = elapsed(xhr._datadog_xhr!.startClocks.timeStamp, timeStampNow())
xhr._datadog_xhr!.responseText = xhr.response as string | undefined
xhr._datadog_xhr!.status = xhr.status
xhr._datadog_xhr!.xhr = xhr

onRequestCompleteCallbacks.forEach((callback) => callback({ ...xhr._datadog_xhr! }))
}
Loading

0 comments on commit 52c475d

Please sign in to comment.