Skip to content

Commit

Permalink
πŸ› [RUM-5785] fix missing navigation timings on Safari (#2964)
Browse files Browse the repository at this point in the history
* βœ…β™»οΈ move `mockDocumentReadyState` in a common module

And add the possibility to mock onload

* βœ… clone emitted view events

Sometimes when checking for View events emitted in the past, the
properties don't reflect the state at the time because they have been
mutated in place.

* βœ…β™»οΈ factorize the way we mock `window.performance.getEntries*`

* ♻️ make runOnReadyState stoppable

* ♻️ extract the code to polyfill "navigation" performance entry

We'll use this in `trackNavigationTimings` next.

* πŸ› [RUM-5785] fix track navigation timings on Safari

In most browsers, when observing `PerformanceNavigationTiming` via a
`PerformanceObserver` before the page is fully loaded, the same
"navigation" entry is notified twice:

* once with the "buffered" entry, even if it is incomplete

* once when all the timings are defined, after the `load` event is
  complete.

On Safari, the entry is only notified once. The SDK is only aware of the
incomplete entry (which is discarded), so all timings related to the
navigation entry are missing.

To work around this issue and keep things simple, this commit removes
the `PerformanceObserver` usage. Instead, we wait for the `load` event
to be complete, and then just pick up the buffered navigation entry.
This is similar to the strategy we use to generate the "document"
resource, which works reliably.

* βœ… adjust related integration tests

* πŸ‘Œ review feedbacks

* ♻️  NavigationTiming inherits from ResourceTiming

The native PerformanceNavigationTiming is inheriting from
PerformanceResourceTiming, so let's do the same for our interfaces, so
we don't need to copy/paste/document the same properties.

* βœ… fix unit test in IE11
  • Loading branch information
BenoitZugmeyer authored Sep 17, 2024
1 parent 482a025 commit 6e60bba
Show file tree
Hide file tree
Showing 18 changed files with 325 additions and 230 deletions.
9 changes: 5 additions & 4 deletions packages/core/src/browser/runOnReadyState.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
import type { Configuration } from '../domain/configuration'
import { noop } from '../tools/utils/functionUtils'
import { DOM_EVENT, addEventListener } from './addEventListener'

export function runOnReadyState(
configuration: Configuration,
expectedReadyState: 'complete' | 'interactive',
callback: () => void
) {
): { stop: () => void } {
if (document.readyState === expectedReadyState || document.readyState === 'complete') {
callback()
} else {
const eventName = expectedReadyState === 'complete' ? DOM_EVENT.LOAD : DOM_EVENT.DOM_CONTENT_LOADED
addEventListener(configuration, window, eventName, callback, { once: true })
return { stop: noop }
}
const eventName = expectedReadyState === 'complete' ? DOM_EVENT.LOAD : DOM_EVENT.DOM_CONTENT_LOADED
return addEventListener(configuration, window, eventName, callback, { once: true })
}
5 changes: 3 additions & 2 deletions packages/rum-core/src/boot/startRum.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type { RumSessionManagerMock } from '../../test'
import {
createPerformanceEntry,
createRumSessionManagerMock,
mockDocumentReadyState,
mockPageStateHistory,
mockPerformanceObserver,
mockRumConfiguration,
Expand Down Expand Up @@ -295,7 +296,7 @@ describe('rum events url', () => {

it('should keep the same URL when updating an ended view', () => {
clock = mockClock()
const { notifyPerformanceEntries } = mockPerformanceObserver()
const { triggerOnLoad } = mockDocumentReadyState()
setupViewUrlTest()

clock.tick(VIEW_DURATION)
Expand All @@ -304,7 +305,7 @@ describe('rum events url', () => {

serverRumEvents.length = 0

notifyPerformanceEntries([createPerformanceEntry(RumPerformanceEntryType.NAVIGATION)])
triggerOnLoad()
clock.tick(THROTTLE_VIEW_UPDATE_PERIOD)

expect(serverRumEvents.length).toEqual(1)
Expand Down
17 changes: 12 additions & 5 deletions packages/rum-core/src/browser/performanceObservable.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import type { Duration, Subscription } from '@datadog/browser-core'
import type { Clock } from '@datadog/browser-core/test'
import { mockClock } from '@datadog/browser-core/test'
import { createPerformanceEntry, mockPerformanceObserver, mockRumConfiguration } from '../../test'
import type { GlobalPerformanceBufferMock } from '../../test'
import {
createPerformanceEntry,
mockGlobalPerformanceBuffer,
mockPerformanceObserver,
mockRumConfiguration,
} from '../../test'
import { RumPerformanceEntryType, createPerformanceObservable } from './performanceObservable'

describe('performanceObservable', () => {
Expand Down Expand Up @@ -76,11 +82,10 @@ describe('performanceObservable', () => {
})

describe('fallback strategy when type not supported', () => {
let bufferedEntries: PerformanceEntryList
let globalPerformanceBufferMock: GlobalPerformanceBufferMock

beforeEach(() => {
bufferedEntries = []
spyOn(performance, 'getEntriesByType').and.callFake(() => bufferedEntries)
globalPerformanceBufferMock = mockGlobalPerformanceBuffer()
})

it('should notify performance resources when type not supported', () => {
Expand All @@ -97,7 +102,9 @@ describe('performanceObservable', () => {
it('should notify buffered performance resources when type not supported', () => {
mockPerformanceObserver({ typeSupported: false })
// add the performance entry to the buffer
bufferedEntries = [createPerformanceEntry(RumPerformanceEntryType.RESOURCE, { name: allowedUrl })]
globalPerformanceBufferMock.addPerformanceEntry(
createPerformanceEntry(RumPerformanceEntryType.RESOURCE, { name: allowedUrl })
)

const performanceResourceObservable = createPerformanceObservable(configuration, {
type: RumPerformanceEntryType.RESOURCE,
Expand Down
8 changes: 6 additions & 2 deletions packages/rum-core/src/browser/performanceObservable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,17 @@ export interface RumPerformancePaintTiming {
startTime: RelativeTime
}

export interface RumPerformanceNavigationTiming {
export interface RumPerformanceNavigationTiming extends Omit<RumPerformanceResourceTiming, 'entryType'> {
entryType: RumPerformanceEntryType.NAVIGATION
initiatorType: 'navigation'
name: string

domComplete: RelativeTime
domContentLoadedEventEnd: RelativeTime
domInteractive: RelativeTime
loadEventEnd: RelativeTime
responseStart: RelativeTime

toJSON(): Omit<RumPerformanceNavigationTiming, 'toJSON'>
}

export interface RumLargestContentfulPaintTiming {
Expand Down
45 changes: 45 additions & 0 deletions packages/rum-core/src/browser/performanceUtils.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { isIE, type RelativeTime } from '@datadog/browser-core'
import type { RumPerformanceNavigationTiming } from './performanceObservable'
import { RumPerformanceEntryType } from './performanceObservable'
import { getNavigationEntry } from './performanceUtils'

describe('getNavigationEntry', () => {
it('returns the navigation entry', () => {
// Declare the expected value here, so TypeScript can make sure all expected fields are covered,
// even though the actual value contains more fields.
const expectation: jasmine.Expected<RumPerformanceNavigationTiming> = {
entryType: RumPerformanceEntryType.NAVIGATION,
initiatorType: 'navigation',
name: jasmine.any(String),

domComplete: jasmine.any(Number),
domContentLoadedEventEnd: jasmine.any(Number),
domInteractive: jasmine.any(Number),
loadEventEnd: jasmine.any(Number),

startTime: 0 as RelativeTime,
duration: jasmine.any(Number),

fetchStart: jasmine.any(Number),
domainLookupStart: jasmine.any(Number),
domainLookupEnd: jasmine.any(Number),
connectStart: jasmine.any(Number),
...(isIE()
? ({} as unknown as { secureConnectionStart: RelativeTime })
: { secureConnectionStart: jasmine.any(Number) }),
connectEnd: jasmine.any(Number),
requestStart: jasmine.any(Number),
responseStart: jasmine.any(Number),
responseEnd: jasmine.any(Number),
redirectStart: jasmine.any(Number),
redirectEnd: jasmine.any(Number),
decodedBodySize: jasmine.any(Number),
encodedBodySize: jasmine.any(Number),
transferSize: jasmine.any(Number),

toJSON: jasmine.any(Function),
}

expect(getNavigationEntry()).toEqual(jasmine.objectContaining(expectation))
})
})
45 changes: 39 additions & 6 deletions packages/rum-core/src/browser/performanceUtils.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,53 @@
import type { RelativeTime, TimeStamp } from '@datadog/browser-core'
import { getRelativeTime, isNumber } from '@datadog/browser-core'
import { assign, getRelativeTime, isNumber } from '@datadog/browser-core'
import {
RumPerformanceEntryType,
supportPerformanceTimingEvent,
type RumPerformanceNavigationTiming,
} from './performanceObservable'

export type RelativePerformanceTiming = {
export function getNavigationEntry(): RumPerformanceNavigationTiming {
if (supportPerformanceTimingEvent(RumPerformanceEntryType.NAVIGATION)) {
const navigationEntry = performance.getEntriesByType(
RumPerformanceEntryType.NAVIGATION
)[0] as unknown as RumPerformanceNavigationTiming
if (navigationEntry) {
return navigationEntry
}
}

const timings = computeTimingsFromDeprecatedPerformanceTiming()
const entry = assign(
{
entryType: RumPerformanceEntryType.NAVIGATION as const,
initiatorType: 'navigation' as const,
name: window.location.href,
startTime: 0 as RelativeTime,
duration: timings.responseEnd,
decodedBodySize: 0,
encodedBodySize: 0,
transferSize: 0,
toJSON: () => assign({}, entry, { toJSON: undefined }),
},
timings
)
return entry
}

export type TimingsFromDeprecatedPerformanceTiming = {
-readonly [key in keyof Omit<PerformanceTiming, 'toJSON'>]: RelativeTime
}

export function computeRelativePerformanceTiming() {
const result: Partial<RelativePerformanceTiming> = {}
export function computeTimingsFromDeprecatedPerformanceTiming() {
const result: Partial<TimingsFromDeprecatedPerformanceTiming> = {}
const timing = performance.timing

for (const key in timing) {
if (isNumber(timing[key as keyof PerformanceTiming])) {
const numberKey = key as keyof RelativePerformanceTiming
const numberKey = key as keyof TimingsFromDeprecatedPerformanceTiming
const timingElement = timing[numberKey] as TimeStamp
result[numberKey] = timingElement === 0 ? (0 as RelativeTime) : getRelativeTime(timingElement)
}
}
return result as RelativePerformanceTiming
return result as TimingsFromDeprecatedPerformanceTiming
}
Original file line number Diff line number Diff line change
@@ -1,33 +1,36 @@
import type { Duration, RelativeTime } from '@datadog/browser-core'
import { isIE, relativeToClocks } from '@datadog/browser-core'
import { createPerformanceEntry } from '../../../test'
import type { GlobalPerformanceBufferMock } from '../../../test'
import { createPerformanceEntry, mockGlobalPerformanceBuffer } from '../../../test'
import type { RumPerformanceResourceTiming } from '../../browser/performanceObservable'
import { RumPerformanceEntryType } from '../../browser/performanceObservable'
import type { RequestCompleteEvent } from '../requestCollection'

import { matchRequestResourceEntry } from './matchRequestResourceEntry'

describe('matchRequestResourceEntry', () => {
const FAKE_URL = 'https://example.com'
const FAKE_REQUEST: Partial<RequestCompleteEvent> = {
url: FAKE_URL,
startClocks: relativeToClocks(100 as RelativeTime),
duration: 500 as Duration,
}
let entries: RumPerformanceResourceTiming[]
let globalPerformanceObjectMock: GlobalPerformanceBufferMock

beforeEach(() => {
if (isIE()) {
pending('no full rum support')
}
entries = []
spyOn(performance, 'getEntriesByName').and.returnValue(entries)
globalPerformanceObjectMock = mockGlobalPerformanceBuffer()
})

it('should match single entry nested in the request ', () => {
const entry = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 200 as RelativeTime,
duration: 300 as Duration,
})
entries.push(entry)
globalPerformanceObjectMock.addPerformanceEntry(entry)

const matchingEntry = matchRequestResourceEntry(FAKE_REQUEST as RequestCompleteEvent)

Expand All @@ -36,10 +39,11 @@ describe('matchRequestResourceEntry', () => {

it('should match single entry nested in the request with error margin', () => {
const entry = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 99 as RelativeTime,
duration: 502 as Duration,
})
entries.push(entry)
globalPerformanceObjectMock.addPerformanceEntry(entry)

const matchingEntry = matchRequestResourceEntry(FAKE_REQUEST as RequestCompleteEvent)

Expand All @@ -48,10 +52,11 @@ describe('matchRequestResourceEntry', () => {

it('should not match single entry outside the request ', () => {
const entry = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 0 as RelativeTime,
duration: 300 as Duration,
})
entries.push(entry)
globalPerformanceObjectMock.addPerformanceEntry(entry)

const matchingEntry = matchRequestResourceEntry(FAKE_REQUEST as RequestCompleteEvent)

Expand All @@ -60,20 +65,22 @@ describe('matchRequestResourceEntry', () => {

it('should discard already matched entries when multiple identical requests are done conurently', () => {
const entry1 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 200 as RelativeTime,
duration: 300 as Duration,
})
entries.push(entry1)
globalPerformanceObjectMock.addPerformanceEntry(entry1)

const matchingEntry1 = matchRequestResourceEntry(FAKE_REQUEST as RequestCompleteEvent)

expect(matchingEntry1).toEqual(entry1.toJSON() as RumPerformanceResourceTiming)

const entry2 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 99 as RelativeTime,
duration: 502 as Duration,
})
entries.push(entry2)
globalPerformanceObjectMock.addPerformanceEntry(entry2)

const matchingEntry2 = matchRequestResourceEntry(FAKE_REQUEST as RequestCompleteEvent)

Expand All @@ -82,14 +89,17 @@ describe('matchRequestResourceEntry', () => {

it('should not match two not following entries nested in the request ', () => {
const entry1 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 150 as RelativeTime,
duration: 100 as Duration,
})
const entry2 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 200 as RelativeTime,
duration: 100 as Duration,
})
entries.push(entry1, entry2)
globalPerformanceObjectMock.addPerformanceEntry(entry1)
globalPerformanceObjectMock.addPerformanceEntry(entry2)

const matchingEntry = matchRequestResourceEntry(FAKE_REQUEST as RequestCompleteEvent)

Expand All @@ -98,18 +108,23 @@ describe('matchRequestResourceEntry', () => {

it('should not match multiple entries nested in the request', () => {
const entry1 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 100 as RelativeTime,
duration: 50 as Duration,
})
const entry2 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 150 as RelativeTime,
duration: 50 as Duration,
})
const entry3 = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
startTime: 200 as RelativeTime,
duration: 50 as Duration,
})
entries.push(entry1, entry2, entry3)
globalPerformanceObjectMock.addPerformanceEntry(entry1)
globalPerformanceObjectMock.addPerformanceEntry(entry2)
globalPerformanceObjectMock.addPerformanceEntry(entry3)

const matchingEntry = matchRequestResourceEntry(FAKE_REQUEST as RequestCompleteEvent)

Expand All @@ -118,10 +133,10 @@ describe('matchRequestResourceEntry', () => {

it('should not match entry with invalid duration', () => {
const entry = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
duration: -1 as Duration,
})

entries.push(entry)
globalPerformanceObjectMock.addPerformanceEntry(entry)

const matchingEntry = matchRequestResourceEntry(FAKE_REQUEST as RequestCompleteEvent)

Expand All @@ -130,12 +145,12 @@ describe('matchRequestResourceEntry', () => {

it('should not match invalid entry nested in the request ', () => {
const entry = createPerformanceEntry(RumPerformanceEntryType.RESOURCE, {
name: FAKE_URL,
// fetchStart < startTime is invalid
fetchStart: 0 as RelativeTime,
startTime: 200 as RelativeTime,
})

entries.push(entry)
globalPerformanceObjectMock.addPerformanceEntry(entry)

const matchingEntry = matchRequestResourceEntry(FAKE_REQUEST as RequestCompleteEvent)

Expand Down
Loading

0 comments on commit 6e60bba

Please sign in to comment.