From 04aa81c87c413760b9ea50593406b38f86d77ef7 Mon Sep 17 00:00:00 2001 From: Thomas Lebeau Date: Wed, 8 May 2024 10:05:18 +0200 Subject: [PATCH 1/6] =?UTF-8?q?=F0=9F=A7=AA=20Add=20test=20for=20matching?= =?UTF-8?q?=20multiple=20resource=20at=20the=20same=20time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../resource/matchRequestTiming.spec.ts | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts b/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts index bfc6bf9370..57e6b3c784 100644 --- a/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts +++ b/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 match all timings to the same request done at the same time', () => { + 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, From 423abc3ee9ec9556d5e941dc261c91ae906adaf8 Mon Sep 17 00:00:00 2001 From: Thomas Lebeau Date: Wed, 8 May 2024 10:14:41 +0200 Subject: [PATCH 2/6] =?UTF-8?q?=E2=9C=85=20fix=20timing=20matching=20for?= =?UTF-8?q?=20the=20same=20resource=20requested=20twice=20at=20the=20same?= =?UTF-8?q?=20time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/domain/resource/matchRequestTiming.ts | 18 +++++++++++++----- .../src/domain/resource/resourceUtils.ts | 14 +++++++------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/packages/rum-core/src/domain/resource/matchRequestTiming.ts b/packages/rum-core/src/domain/resource/matchRequestTiming.ts index 763faedfc2..6f393792a7 100644 --- a/packages/rum-core/src/domain/resource/matchRequestTiming.ts +++ b/packages/rum-core/src/domain/resource/matchRequestTiming.ts @@ -2,13 +2,15 @@ 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 { isValidEntry } from './resourceUtils' interface Timing { startTime: RelativeTime duration: Duration } +const matchedResourceTimingEntries = new WeakSet() + /** * Look for corresponding timing in resource timing buffer * @@ -32,18 +34,24 @@ export function matchRequestTiming(request: RequestCompleteEvent) { } const candidates = sameNameEntries - .map((entry) => entry.toJSON() as RumPerformanceResourceTiming) - .filter(toValidEntry) + .filter((entry) => !matchedResourceTimingEntries.has(entry)) + .map((entry) => ({ + original: entry, + serialized: entry.toJSON() as RumPerformanceResourceTiming, + })) + .filter((entry) => isValidEntry(entry.serialized)) .filter((entry) => isBetween( - entry, + entry.serialized, request.startClocks.relative, endTime({ startTime: request.startClocks.relative, duration: request.duration }) ) ) if (candidates.length === 1) { - return candidates[0] + matchedResourceTimingEntries.add(candidates[0].original) + + return candidates[0].serialized } return diff --git a/packages/rum-core/src/domain/resource/resourceUtils.ts b/packages/rum-core/src/domain/resource/resourceUtils.ts index 792902d184..341a989d6f 100644 --- a/packages/rum-core/src/domain/resource/resourceUtils.ts +++ b/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 @@ -163,8 +161,10 @@ export function toValidEntry(entry: RumPerformanceResourceTiming) { : true if (areCommonTimingsInOrder && areRedirectionTimingsInOrder) { - return entry + return true } + + return false } function hasRedirection(entry: RumPerformanceResourceTiming) { From 768909dcbaa043d865bce658a2653c718d5d9a4c Mon Sep 17 00:00:00 2001 From: Thomas Lebeau Date: Wed, 8 May 2024 11:42:08 +0200 Subject: [PATCH 3/6] =?UTF-8?q?=F0=9F=90=9B=20use=20WeakMap=20for=20ie11?= =?UTF-8?q?=20support?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/rum-core/src/domain/resource/matchRequestTiming.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/rum-core/src/domain/resource/matchRequestTiming.ts b/packages/rum-core/src/domain/resource/matchRequestTiming.ts index 6f393792a7..30d2ae20d4 100644 --- a/packages/rum-core/src/domain/resource/matchRequestTiming.ts +++ b/packages/rum-core/src/domain/resource/matchRequestTiming.ts @@ -9,7 +9,9 @@ interface Timing { duration: Duration } -const matchedResourceTimingEntries = new WeakSet() +// we use a WeakMap because WeakSet is not supported in ie11 +const PLACEHOLDER = 1 +const matchedResourceTimingEntries = new WeakMap() /** * Look for corresponding timing in resource timing buffer @@ -49,7 +51,7 @@ export function matchRequestTiming(request: RequestCompleteEvent) { ) if (candidates.length === 1) { - matchedResourceTimingEntries.add(candidates[0].original) + matchedResourceTimingEntries.set(candidates[0].original, PLACEHOLDER) return candidates[0].serialized } From 70f5dc472e6c17ece17cf25778aeb09666fc8b86 Mon Sep 17 00:00:00 2001 From: Thomas Lebeau Date: Tue, 14 May 2024 16:32:39 +0200 Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=91=8C=20fix=20review=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../rum-core/src/browser/polyfills.spec.ts | 30 ++++++++++++++++++- packages/rum-core/src/browser/polyfills.ts | 26 ++++++++++++++++ .../resource/matchRequestTiming.spec.ts | 2 +- .../src/domain/resource/matchRequestTiming.ts | 24 +++++++-------- test/e2e/scenario/rum/resources.scenario.ts | 21 +++++++++++++ 5 files changed, 87 insertions(+), 16 deletions(-) diff --git a/packages/rum-core/src/browser/polyfills.spec.ts b/packages/rum-core/src/browser/polyfills.spec.ts index 6452412778..96f406d22f 100644 --- a/packages/rum-core/src/browser/polyfills.spec.ts +++ b/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) + }) +}) diff --git a/packages/rum-core/src/browser/polyfills.ts b/packages/rum-core/src/browser/polyfills.ts index 6b9a7f79ee..9e68431008 100644 --- a/packages/rum-core/src/browser/polyfills.ts +++ b/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 { + private map = new WeakMap() + + 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) + } +} diff --git a/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts b/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts index 57e6b3c784..54fc69245e 100644 --- a/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts +++ b/packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts @@ -59,7 +59,7 @@ describe('matchRequestTiming', () => { expect(matchingTiming).toEqual(undefined) }) - it('should match all timings to the same request done at the same time', () => { + 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, diff --git a/packages/rum-core/src/domain/resource/matchRequestTiming.ts b/packages/rum-core/src/domain/resource/matchRequestTiming.ts index 30d2ae20d4..8ea747cf65 100644 --- a/packages/rum-core/src/domain/resource/matchRequestTiming.ts +++ b/packages/rum-core/src/domain/resource/matchRequestTiming.ts @@ -2,6 +2,7 @@ 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 { WeakSet } from '../../browser/polyfills' import { isValidEntry } from './resourceUtils' interface Timing { @@ -9,9 +10,7 @@ interface Timing { duration: Duration } -// we use a WeakMap because WeakSet is not supported in ie11 -const PLACEHOLDER = 1 -const matchedResourceTimingEntries = new WeakMap() +const alreadyMatchedEntries = new WeakSet() /** * Look for corresponding timing in resource timing buffer @@ -22,38 +21,35 @@ const matchedResourceTimingEntries = new WeakMap !matchedResourceTimingEntries.has(entry)) - .map((entry) => ({ - original: entry, - serialized: entry.toJSON() as RumPerformanceResourceTiming, - })) - .filter((entry) => isValidEntry(entry.serialized)) + .filter((entry) => !alreadyMatchedEntries.has(entry)) + .filter((entry) => isValidEntry(entry)) .filter((entry) => isBetween( - entry.serialized, + entry, request.startClocks.relative, endTime({ startTime: request.startClocks.relative, duration: request.duration }) ) ) if (candidates.length === 1) { - matchedResourceTimingEntries.set(candidates[0].original, PLACEHOLDER) + alreadyMatchedEntries.add(candidates[0]) - return candidates[0].serialized + return candidates[0].toJSON() as RumPerformanceResourceTiming } return diff --git a/test/e2e/scenario/rum/resources.scenario.ts b/test/e2e/scenario/rum/resources.scenario.ts index 17c62f10eb..0ed84c6c69 100644 --- a/test/e2e/scenario/rum/resources.scenario.ts +++ b/test/e2e/scenario/rum/resources.scenario.ts @@ -217,6 +217,27 @@ describe('rum resources', () => { expect(resourceEvent.resource.redirect!.duration).toBeGreaterThan(0) }) + createTest('track concurent fetch to same resource') + .withRum() + .withSetup(bundleSetup) + .run(async ({ intakeRegistry }) => { + await browser.executeAsync((done) => { + Promise.all([fetch('/ok'), fetch('/ok')]) + .then(() => done()) + .catch(() => done()) + }) + + 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() From ad5e1b2ce23d009e35c517d15b34763f780a8bdd Mon Sep 17 00:00:00 2001 From: Thomas Lebeau Date: Tue, 14 May 2024 18:08:04 +0200 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=91=8C=20simplify=20isValidEntry=20re?= =?UTF-8?q?turn=20statement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/rum-core/src/domain/resource/resourceUtils.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/rum-core/src/domain/resource/resourceUtils.ts b/packages/rum-core/src/domain/resource/resourceUtils.ts index 341a989d6f..66e4dc5599 100644 --- a/packages/rum-core/src/domain/resource/resourceUtils.ts +++ b/packages/rum-core/src/domain/resource/resourceUtils.ts @@ -160,11 +160,7 @@ export function isValidEntry(entry: RumPerformanceResourceTiming) { ? areInOrder(entry.startTime, entry.redirectStart, entry.redirectEnd, entry.fetchStart) : true - if (areCommonTimingsInOrder && areRedirectionTimingsInOrder) { - return true - } - - return false + return areCommonTimingsInOrder && areRedirectionTimingsInOrder } function hasRedirection(entry: RumPerformanceResourceTiming) { From 84d8e5e4695497df402f920fd5c812e674567804 Mon Sep 17 00:00:00 2001 From: Thomas Lebeau Date: Thu, 16 May 2024 13:26:44 +0200 Subject: [PATCH 6/6] skip non chromium e2e test --- test/e2e/scenario/rum/resources.scenario.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/e2e/scenario/rum/resources.scenario.ts b/test/e2e/scenario/rum/resources.scenario.ts index 0ed84c6c69..a7081344c5 100644 --- a/test/e2e/scenario/rum/resources.scenario.ts +++ b/test/e2e/scenario/rum/resources.scenario.ts @@ -217,7 +217,7 @@ describe('rum resources', () => { expect(resourceEvent.resource.redirect!.duration).toBeGreaterThan(0) }) - createTest('track concurent fetch to same resource') + createTest('track concurrent fetch to same resource') .withRum() .withSetup(bundleSetup) .run(async ({ intakeRegistry }) => { @@ -227,6 +227,10 @@ describe('rum resources', () => { .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')