🐛 [RUMF-876] Improve proxy behavior for xhr reuse

packages/core/src/browser/xhrProxy.spec.ts

@@ -110,9 +110,9 @@ describe('xhr proxy', () => {
     const onReadyStateChange = jasmine.createSpy()
     withXhr({
       setup(xhr) {
-        xhr.onreadystatechange = onReadyStateChange
-        xhr.addEventListener('load', () => xhr.abort())
         xhr.open('GET', '/ok')
+        xhr.addEventListener('readystatechange', onReadyStateChange)
+        xhr.addEventListener('load', () => xhr.abort())
         xhr.send()
         xhr.complete(200, 'ok')
       },
@@ -209,4 +209,26 @@ describe('xhr proxy', () => {
       },
     })
   })
+
+  it('should track only the second request (the first request is canceled)', (done) => {
+    const onLoad = jasmine.createSpy()
+    withXhr({
+      setup(xhr) {
+        xhr.addEventListener('load', onLoad)
+        xhr.open('GET', '/first')
+        xhr.send()
+        xhr.open('GET', '/second')
+        xhr.send()
+        xhr.complete(200, 'ok')
+      },
+      onComplete() {
+        const request = getRequest(0)
+        expect(onLoad).toHaveBeenCalledTimes(1)
+        expect(request.method).toBe('GET')
+        expect(request.url).toContain('/second')
+        expect(request.status).toBe(200)
+        done()
+      },
+    })
+  })
 })

packages/core/src/browser/xhrProxy.ts

@@ -1,9 +1,9 @@
-import { monitor, callMonitored } from '../domain/internalMonitoring'
+import { callMonitored } 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 = XhrStartContext> extends XMLHttpRequest {
+  _datadog_xhr?: T
 }
 
 export interface XhrProxy<
@@ -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
    */
@@ -30,7 +33,6 @@ export interface XhrCompleteContext extends XhrStartContext {
   duration: Duration
   status: number
   response: string | undefined
-  isAborted: boolean
 }
 
 let xhrProxySingleton: XhrProxy | undefined
@@ -45,7 +47,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)
@@ -69,85 +80,75 @@ 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),
+    }
 
-  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()),
-            response: this.response as string | undefined,
-            status: this.status,
-          }
-
-          onRequestCompleteCallbacks.forEach((callback) => callback(xhrCompleteContext))
-        }
-
-        this.addEventListener('loadend', monitor(reportXhr))
-
-        beforeSendCallbacks.forEach((callback) => callback(xhrPendingContext, this))
-      }
-    })
-
-    return originalXhrSend.apply(this, arguments as any)
-  }
+    // To avoid adding listeners each time 'open()' is called,
+    // we take advantage of the automatic listener discarding if multiple identical EventListeners are registered
+    // https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#multiple_identical_event_listeners
+    this.addEventListener('readystatechange', onReadyStateChange)
+    this.addEventListener('loadend', reportXhr)
+  })
+  return originalXhrOpen.apply(this, arguments as any)
+}
 
-  XMLHttpRequest.prototype.abort = function (this: BrowserXHR) {
-    callMonitored(() => {
-      if (this._datadog_xhr) {
-        this._datadog_xhr.isAborted = true
-      }
-    })
-    return originalXhrAbort.apply(this, arguments as any)
+function sendXhr(this: BrowserXHR) {
+  callMonitored(() => {
+    if (!this._datadog_xhr) {
+      return
+    }
+    this._datadog_xhr = {
+      ...this._datadog_xhr,
+      startTime: relativeNow(),
+      startClocks: clocksNow(),
+      isAborted: false,
+    }
+
+    beforeSendCallbacks.forEach((callback) => callback(this._datadog_xhr!, this))
+  })
+
+  return originalXhrSend.apply(this, arguments as any)
+}
+
+function abortXhr(this: BrowserXHR) {
+  callMonitored(() => {
+    if (this._datadog_xhr) {
+      this._datadog_xhr.isAborted = true
+    }
+  })
+  return originalXhrAbort.apply(this, arguments as any)
+}
+
+function onReadyStateChange(this: BrowserXHR) {
+  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.
+    // https://github.com/angular/angular/blob/master/packages/common/http/src/xhr.ts
+    reportXhr.call(this)
   }
 }
+
+function reportXhr(this: BrowserXHR) {
+  callMonitored(() => {
+    const xhrCompleteContext: XhrCompleteContext = {
+      ...this._datadog_xhr!,
+      duration: elapsed(this._datadog_xhr!.startClocks.timeStamp, timeStampNow()),
+      response: this.response as string | undefined,
+      status: this.status,
+    }
+
+    onRequestCompleteCallbacks.forEach((callback) => callback(xhrCompleteContext))
+    // Unsubscribe to avoid being reported twice
+    this.removeEventListener('readystatechange', onReadyStateChange)
+    this.removeEventListener('loadend', reportXhr)
+  })
+}

packages/core/test/specHelper.ts

@@ -135,11 +135,7 @@ class StubXhr {
   public onreadystatechange: () => void = noop
 
   private hasEnded = false
-  private fakeEventTarget: HTMLDivElement
-
-  constructor() {
-    this.fakeEventTarget = document.createElement('div')
-  }
+  private listeners: { [k: string]: Array<() => void> } = {}
 
   /* eslint-disable @typescript-eslint/no-empty-function,@typescript-eslint/no-unused-vars */
   open(method: string, url: string) {}
@@ -154,7 +150,7 @@ class StubXhr {
     }
     this.hasEnded = true
     this.readyState = XMLHttpRequest.DONE
-    this.onreadystatechange()
+    this.dispatchEvent('readystatechange')
     this.dispatchEvent('abort')
     this.dispatchEvent('loadend')
   }
@@ -167,7 +163,8 @@ class StubXhr {
     this.response = response
     this.status = status
     this.readyState = XMLHttpRequest.DONE
-    this.onreadystatechange()
+    this.dispatchEvent('readystatechange')
+
     if (status >= 200 && status < 500) {
       this.dispatchEvent('load')
     }
@@ -178,11 +175,26 @@ class StubXhr {
   }
 
   addEventListener(name: string, callback: () => void) {
-    this.fakeEventTarget.addEventListener(name, callback)
+    if (!this.listeners[name]) {
+      this.listeners[name] = []
+    }
+
+    this.listeners[name].push(callback)
+  }
+
+  removeEventListener(name: string, callback: () => void) {
+    if (!this.listeners[name]) {
+      throw new Error(`Can't remove a listener. Event "${name}" doesn't exits.`)
+    }
+
+    this.listeners[name] = this.listeners[name].filter((listener) => listener !== callback)
   }
 
   private dispatchEvent(name: string) {
-    this.fakeEventTarget.dispatchEvent(createNewEvent(name))
+    if (!this.listeners[name]) {
+      return
+    }
+    this.listeners[name].forEach((listener) => listener.call(this))
   }
 }
 

test/e2e/scenario/rum/resources.scenario.ts

@@ -201,6 +201,56 @@ describe('rum resources', () => {
         expect(resourceEvent?.resource.status_code).toBe(0)
       })
   })
+
+  describe('support XHRs with same XMLHttpRequest instance', () => {
+    createTest('track only second XHR (first is canceled by the browser)')
+      .withRum()
+      .withSetup(bundleSetup)
+      .run(async ({ events }) => {
+        await browserExecuteAsync((done) => {
+          const xhr = new XMLHttpRequest()
+          xhr.open('GET', '/ok?duration=200&call=1')
+          xhr.send()
+          xhr.open('GET', '/ok?duration=100&call=2')
+          xhr.send()
+          setTimeout(() => {
+            done(undefined)
+          }, 200)
+        })
+
+        await flushEvents()
+
+        const resourceEvents = events.rumResources.filter((event) => event.resource.type === 'xhr')
+        expect(resourceEvents.length).toEqual(1)
+        expect(events.rumErrors.length).toBe(0)
+        expect(resourceEvents[0]?.resource.status_code).toBe(200)
+      })
+
+    createTest('track both XHRs when the second wait for the respond of the first')
+      .withRum()
+      .withSetup(bundleSetup)
+      .run(async ({ events }) => {
+        await browserExecuteAsync((done) => {
+          const xhr = new XMLHttpRequest()
+          const triggerSecondCall = () => {
+            xhr.removeEventListener('loadend', triggerSecondCall)
+            xhr.addEventListener('loadend', () => done(undefined))
+            xhr.open('GET', '/ok?duration=100&call=2')
+            xhr.send()
+          }
+          xhr.addEventListener('loadend', triggerSecondCall)
+          xhr.open('GET', '/ok?duration=100&call=1')
+          xhr.send()
+        })
+
+        await flushEvents()
+
+        const resourceEvents = events.rumResources.filter((event) => event.resource.type === 'xhr')
+        expect(resourceEvents.length).toEqual(2)
+        expect(events.rumErrors.length).toBe(0)
+        expect(resourceEvents.map((resourceEvent) => resourceEvent.resource.status_code)).toEqual([200, 200])
+      })
+  })
 })
 
 function expectToHaveValidTimings(resourceEvent: RumResourceEvent) {