🐛 [RUM-4434] fix timing matching for the same resource requested twice at the same time

packages/rum-core/src/browser/polyfills.spec.ts

@@ -1,5 +1,5 @@
 import { appendElement } from '../../test'
-import { cssEscape, elementMatches, getClassList, getParentElement } from './polyfills'
+import { WeakSet, cssEscape, elementMatches, getClassList, getParentElement } from './polyfills'
 
 describe('cssEscape', () => {
   it('should escape a string', () => {
@@ -62,3 +62,31 @@ describe('getClassList', () => {
     expect(classList[1]).toEqual('bar')
   })
 })
+
+describe('WeakSet', () => {
+  it('should support add and has', () => {
+    const set = new WeakSet()
+    const obj = {}
+
+    set.add(obj)
+    expect(set.has(obj)).toEqual(true)
+    expect(set.has({})).toEqual(false)
+  })
+
+  it('should support delete', () => {
+    const set = new WeakSet()
+    const obj = {}
+
+    set.add(obj)
+    expect(set.has(obj)).toEqual(true)
+    set.delete(obj)
+    expect(set.has(obj)).toEqual(false)
+  })
+
+  it('should support initial values', () => {
+    const obj = {}
+    const set = new WeakSet([obj])
+
+    expect(set.has(obj)).toEqual(true)
+  })
+})

packages/rum-core/src/browser/polyfills.ts

@@ -76,3 +76,29 @@ export function getClassList(element: Element): DOMTokenList | string[] {
   const classes = element.getAttribute('class')?.trim()
   return classes ? classes.split(/\s+/) : []
 }
+
+// ie11 supports WeakMap but not WeakSet
+const PLACEHOLDER = 1
+export class WeakSet<T extends object> {
+  private map = new WeakMap<T, typeof PLACEHOLDER>()
+
+  constructor(initialValues?: T[]) {
+    if (initialValues) {
+      initialValues.forEach((value) => this.map.set(value, PLACEHOLDER))
+    }
+  }
+
+  add(value: T) {
+    this.map.set(value, PLACEHOLDER)
+
+    return this
+  }
+
+  delete(value: T) {
+    return this.map.delete(value)
+  }
+
+  has(value: T) {
+    return this.map.has(value)
+  }
+}

packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts

@@ -20,7 +20,7 @@ describe('matchRequestTiming', () => {
       pending('no full rum support')
     }
     entries = []
-    spyOn(performance, 'getEntriesByName').and.returnValues(entries as unknown as PerformanceResourceTiming[])
+    spyOn(performance, 'getEntriesByName').and.returnValue(entries)
   })
 
   it('should match single timing nested in the request ', () => {
@@ -59,6 +59,28 @@ describe('matchRequestTiming', () => {
     expect(matchingTiming).toEqual(undefined)
   })
 
+  it('should discard already matched timings when multiple identical requests are done conurently', () => {
+    const entry1 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
+      startTime: 200 as RelativeTime,
+      duration: 300 as Duration,
+    })
+    entries.push(entry1)
+
+    const matchingTiming1 = matchRequestTiming(FAKE_REQUEST as RequestCompleteEvent)
+
+    expect(matchingTiming1).toEqual(entry1.toJSON() as RumPerformanceResourceTiming)
+
+    const entry2 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
+      startTime: 99 as RelativeTime,
+      duration: 502 as Duration,
+    })
+    entries.push(entry2)
+
+    const matchingTiming2 = matchRequestTiming(FAKE_REQUEST as RequestCompleteEvent)
+
+    expect(matchingTiming2).toEqual(entry2.toJSON() as RumPerformanceResourceTiming)
+  })
+
   it('should not match two not following timings nested in the request ', () => {
     const entry1 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
       startTime: 150 as RelativeTime,

packages/rum-core/src/domain/resource/matchRequestTiming.ts

@@ -2,13 +2,16 @@ import type { Duration, RelativeTime } from '@datadog/browser-core'
 import { addDuration } from '@datadog/browser-core'
 import type { RumPerformanceResourceTiming } from '../../browser/performanceCollection'
 import type { RequestCompleteEvent } from '../requestCollection'
-import { toValidEntry } from './resourceUtils'
+import { WeakSet } from '../../browser/polyfills'
+import { isValidEntry } from './resourceUtils'
 
 interface Timing {
   startTime: RelativeTime
   duration: Duration
 }
 
+const alreadyMatchedEntries = new WeakSet<PerformanceEntry>()
+
 /**
  * Look for corresponding timing in resource timing buffer
  *
@@ -18,22 +21,23 @@ interface Timing {
  *
  * Strategy:
  * - from valid nested entries (with 1 ms error margin)
- * - if a single timing match, return the timing
+ * - filter out timing that were already matched to a request
+ * - then, if a single timing match, return the timing
  * - otherwise we can't decide, return undefined
  */
 export function matchRequestTiming(request: RequestCompleteEvent) {
   if (!performance || !('getEntriesByName' in performance)) {
     return
   }
-  const sameNameEntries = performance.getEntriesByName(request.url, 'resource')
+  const sameNameEntries = performance.getEntriesByName(request.url, 'resource') as RumPerformanceResourceTiming[]
 
   if (!sameNameEntries.length || !('toJSON' in sameNameEntries[0])) {
     return
   }
 
   const candidates = sameNameEntries
-    .map((entry) => entry.toJSON() as RumPerformanceResourceTiming)
-    .filter(toValidEntry)
+    .filter((entry) => !alreadyMatchedEntries.has(entry))
+    .filter((entry) => isValidEntry(entry))
     .filter((entry) =>
       isBetween(
         entry,
@@ -43,7 +47,9 @@ export function matchRequestTiming(request: RequestCompleteEvent) {
     )
 
   if (candidates.length === 1) {
-    return candidates[0]
+    alreadyMatchedEntries.add(candidates[0])
+
+    return candidates[0].toJSON() as RumPerformanceResourceTiming
   }
 
   return

packages/rum-core/src/domain/resource/resourceUtils.ts

@@ -89,9 +89,7 @@ export function computePerformanceResourceDuration(entry: RumPerformanceResource
 export function computePerformanceResourceDetails(
   entry: RumPerformanceResourceTiming
 ): PerformanceResourceDetails | undefined {
-  const validEntry = toValidEntry(entry)
-
-  if (!validEntry) {
+  if (!isValidEntry(entry)) {
     return undefined
   }
   const {
@@ -107,7 +105,7 @@ export function computePerformanceResourceDetails(
     requestStart,
     responseStart,
     responseEnd,
-  } = validEntry
+  } = entry
 
   const details: PerformanceResourceDetails = {
     download: formatTiming(startTime, responseStart, responseEnd),
@@ -137,9 +135,9 @@ export function computePerformanceResourceDetails(
   return details
 }
 
-export function toValidEntry(entry: RumPerformanceResourceTiming) {
+export function isValidEntry(entry: RumPerformanceResourceTiming) {
   if (isExperimentalFeatureEnabled(ExperimentalFeature.TOLERANT_RESOURCE_TIMINGS)) {
-    return entry
+    return true
   }
 
   // Ensure timings are in the right order. On top of filtering out potential invalid
@@ -162,9 +160,7 @@ export function toValidEntry(entry: RumPerformanceResourceTiming) {
     ? areInOrder(entry.startTime, entry.redirectStart, entry.redirectEnd, entry.fetchStart)
     : true
 
-  if (areCommonTimingsInOrder && areRedirectionTimingsInOrder) {
-    return entry
-  }
+  return areCommonTimingsInOrder && areRedirectionTimingsInOrder
 }
 
 function hasRedirection(entry: RumPerformanceResourceTiming) {

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

@@ -217,6 +217,31 @@ describe('rum resources', () => {
       expect(resourceEvent.resource.redirect!.duration).toBeGreaterThan(0)
     })
 
+  createTest('track concurrent fetch to same resource')
+    .withRum()
+    .withSetup(bundleSetup)
+    .run(async ({ intakeRegistry }) => {
+      await browser.executeAsync((done) => {
+        Promise.all([fetch('/ok'), fetch('/ok')])
+          .then(() => done())
+          .catch(() => done())
+      })
+
+      if (!browser.isChromium) {
+        pending('Only Chromium based browsers will emit predictable timings events for concurrent fetches')
+      }
+
+      await flushEvents()
+
+      const resourceEvents = intakeRegistry.rumResourceEvents.filter((event) => event.resource.type === 'fetch')
+
+      expect(resourceEvents[0]).toBeTruthy()
+      expect(resourceEvents[0]?.resource.size).toBeDefined()
+
+      expect(resourceEvents[1]).toBeTruthy()
+      expect(resourceEvents[1]?.resource.size).toBeDefined()
+    })
+
   describe('support XHRs with same XMLHttpRequest instance', () => {
     createTest('track XHRs when calling requests one after another')
       .withRum()