Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
May 21, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
04aa81c
🧪 Add test for matching multiple resource at the same time
thomas-lebeau May 8, 2024
423abc3
✅ fix timing matching for the same resource requested twice at the sa…
thomas-lebeau May 8, 2024
768909d
🐛 use WeakMap for ie11 support
thomas-lebeau May 8, 2024
70f5dc4
👌 fix review comments
thomas-lebeau May 14, 2024
ad5e1b2
👌 simplify isValidEntry return statement
thomas-lebeau May 14, 2024
84d8e5e
skip non chromium e2e test
thomas-lebeau May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion packages/rum-core/src/domain/resource/matchRequestTiming.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ', () => {
Expand Down Expand Up @@ -59,6 +59,28 @@ describe('matchRequestTiming', () => {
expect(matchingTiming).toEqual(undefined)
})

it('should match all timings to the same request done at the same time', () => {
thomas-lebeau marked this conversation as resolved.
Show resolved Hide resolved
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,
Expand Down
20 changes: 15 additions & 5 deletions packages/rum-core/src/domain/resource/matchRequestTiming.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@ 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
}

// we use a WeakMap because WeakSet is not supported in ie11
const PLACEHOLDER = 1
thomas-lebeau marked this conversation as resolved.
Show resolved Hide resolved
const matchedResourceTimingEntries = new WeakMap<PerformanceEntry, typeof PLACEHOLDER>()
thomas-lebeau marked this conversation as resolved.
Show resolved Hide resolved

/**
* Look for corresponding timing in resource timing buffer
*
Expand All @@ -32,18 +36,24 @@ export function matchRequestTiming(request: RequestCompleteEvent) {
}

const candidates = sameNameEntries
.map((entry) => entry.toJSON() as RumPerformanceResourceTiming)
.filter(toValidEntry)
.filter((entry) => !matchedResourceTimingEntries.has(entry))
thomas-lebeau marked this conversation as resolved.
Show resolved Hide resolved
.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 })
)
)
thomas-lebeau marked this conversation as resolved.
Show resolved Hide resolved

if (candidates.length === 1) {
return candidates[0]
matchedResourceTimingEntries.set(candidates[0].original, PLACEHOLDER)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ question: any evidence that when two entries are matching the same timing, one entry has already matched another timing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of fetch, having two timing matching the same resource without being already matched, means that both timings are available before the then of the first fetch is called.

This is not the case when doing Promise.all() but I don't exclude the possibility of it happening. In that case no timing will be matched.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth doing more experiments on browser behaviours around that? or measure the effectiveness of this change somehow?

Copy link
Collaborator Author

@thomas-lebeau thomas-lebeau May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding a e2e test, however I've noticed that safari doesn't do like everyone and both PerformanceResourceTiming might be available at the time the first resource is matching it's timings.

This means that this PR reduce the amount of occurrence of missing resource timing on concurrent event (mainly chrome/FF) but does not prevent it totally (safari)

anyone has a better idea?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed during parking lot, this PR won't solve everything but is improving in some cases. So I've skipped the e2e on non chromium browsers as it's not reliable there.


return candidates[0].serialized
}

return
Expand Down
14 changes: 7 additions & 7 deletions packages/rum-core/src/domain/resource/resourceUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -107,7 +105,7 @@ export function computePerformanceResourceDetails(
requestStart,
responseStart,
responseEnd,
} = validEntry
} = entry

const details: PerformanceResourceDetails = {
download: formatTiming(startTime, responseStart, responseEnd),
Expand Down Expand Up @@ -137,9 +135,9 @@ export function computePerformanceResourceDetails(
return details
}

export function toValidEntry(entry: RumPerformanceResourceTiming) {
export function isValidEntry(entry: RumPerformanceResourceTiming) {
thomas-lebeau marked this conversation as resolved.
Show resolved Hide resolved
if (isExperimentalFeatureEnabled(ExperimentalFeature.TOLERANT_RESOURCE_TIMINGS)) {
return entry
return true
}

// Ensure timings are in the right order. On top of filtering out potential invalid
Expand All @@ -163,8 +161,10 @@ export function toValidEntry(entry: RumPerformanceResourceTiming) {
: true

if (areCommonTimingsInOrder && areRedirectionTimingsInOrder) {
return entry
return true
}

return false
thomas-lebeau marked this conversation as resolved.
Show resolved Hide resolved
}

function hasRedirection(entry: RumPerformanceResourceTiming) {
Expand Down