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

✨ [RUMF-992] New CLS implementation #1026

Merged
merged 12 commits into from
Sep 6, 2021
Merged

✨ [RUMF-992] New CLS implementation #1026

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3db6e52
✨ Compute CLS using the new definition
webNeat Aug 31, 2021
cc9027b
🎨 Refactor unit tests
webNeat Sep 1, 2021
733f765
✅ Add unit tests
webNeat Sep 1, 2021
b3306ea
🎨 Avoid computing the max twice
webNeat Sep 1, 2021
596901a
👌 Use function instead of arrow function
webNeat Sep 3, 2021
a53bc82
👌 Make arguments more readable
webNeat Sep 3, 2021
82252e3
👌 Update unit test
webNeat Sep 3, 2021
3c0e37f
👌 Isolate the session window logic
webNeat Sep 3, 2021
047fc5f
🎨 Fix linter error
webNeat Sep 3, 2021
0268939
Merge branch 'main' into amine/new-cls
webNeat Sep 6, 2021
6dd854d
👌 Clarify that the CLS is the max value
webNeat Sep 6, 2021
9bf1bf1
Merge branch 'main' into amine/new-cls
webNeat Sep 6, 2021
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
1 change: 1 addition & 0 deletions packages/rum-core/src/browser/performanceCollection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export interface RumFirstInputTiming {

export interface RumLayoutShiftTiming {
entryType: 'layout-shift'
startTime: RelativeTime
value: number
hadRecentInput: boolean
}
Expand Down
104 changes: 71 additions & 33 deletions packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { Context, RelativeTime, Duration } from '@datadog/browser-core'
import { Context, RelativeTime, Duration, relativeNow } from '@datadog/browser-core'
import { LifeCycleEventType, RumEvent } from '@datadog/browser-rum-core'
import { TestSetupBuilder, setup, setupViewTest, ViewTest } from '../../../../test/specHelper'
import { RumPerformanceNavigationTiming } from '../../../browser/performanceCollection'
import { RumEventType } from '../../../rawRumEvent.types'
import { LifeCycle } from '../../lifeCycle'
import {
PAGE_ACTIVITY_END_DELAY,
PAGE_ACTIVITY_MAX_DURATION,
Expand Down Expand Up @@ -281,6 +282,14 @@ describe('rum track view metrics', () => {

describe('cumulativeLayoutShift', () => {
let isLayoutShiftSupported: boolean
const newLayoutShift = (lifeCycle: LifeCycle, value: number, hadRecentInput = false) => {
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, {
entryType: 'layout-shift',
startTime: relativeNow(),
hadRecentInput,
value,
})
}
beforeEach(() => {
if (!('PerformanceObserver' in window) || !('supportedEntryTypes' in PerformanceObserver)) {
pending('No PerformanceObserver support')
Expand Down Expand Up @@ -308,22 +317,12 @@ describe('rum track view metrics', () => {
expect(getViewUpdate(0).cumulativeLayoutShift).toBe(undefined)
})

it('should accumulate layout shift values', () => {
it('should accumulate layout shift values for the first session window', () => {
const { lifeCycle, clock } = setupBuilder.withFakeClock().build()
const { getViewUpdate, getViewUpdateCount } = viewTest

lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, {
entryType: 'layout-shift',
hadRecentInput: false,
value: 0.1,
})

lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, {
entryType: 'layout-shift',
hadRecentInput: false,
value: 0.2,
})

newLayoutShift(lifeCycle, 0.1)
clock.tick(100)
newLayoutShift(lifeCycle, 0.2)
clock.tick(THROTTLE_VIEW_UPDATE_PERIOD)

expect(getViewUpdateCount()).toEqual(2)
Expand All @@ -333,19 +332,9 @@ describe('rum track view metrics', () => {
it('should round the cumulative layout shift value to 4 decimals', () => {
const { lifeCycle, clock } = setupBuilder.withFakeClock().build()
const { getViewUpdate, getViewUpdateCount } = viewTest

lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, {
entryType: 'layout-shift',
hadRecentInput: false,
value: 1.23456789,
})

lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, {
entryType: 'layout-shift',
hadRecentInput: false,
value: 1.11111111111,
})

newLayoutShift(lifeCycle, 1.23456789)
clock.tick(100)
newLayoutShift(lifeCycle, 1.11111111111)
clock.tick(THROTTLE_VIEW_UPDATE_PERIOD)

expect(getViewUpdateCount()).toEqual(2)
Expand All @@ -356,16 +345,65 @@ describe('rum track view metrics', () => {
const { lifeCycle, clock } = setupBuilder.withFakeClock().build()
const { getViewUpdate, getViewUpdateCount } = viewTest

lifeCycle.notify(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, {
entryType: 'layout-shift',
hadRecentInput: true,
value: 0.1,
})
newLayoutShift(lifeCycle, 0.1, true)
bcaudan marked this conversation as resolved.
Show resolved Hide resolved

clock.tick(THROTTLE_VIEW_UPDATE_PERIOD)

expect(getViewUpdateCount()).toEqual(1)
expect(getViewUpdate(0).cumulativeLayoutShift).toBe(0)
})

it('should create a new session window if the gap is more than 1 second', () => {
const { lifeCycle, clock } = setupBuilder.withFakeClock().build()
const { getViewUpdate, getViewUpdateCount } = viewTest
// first session window
newLayoutShift(lifeCycle, 0.1)
clock.tick(100)
newLayoutShift(lifeCycle, 0.2)
// second session window
clock.tick(1001)
newLayoutShift(lifeCycle, 0.1)

clock.tick(THROTTLE_VIEW_UPDATE_PERIOD)
expect(getViewUpdateCount()).toEqual(2)
expect(getViewUpdate(1).cumulativeLayoutShift).toBe(0.3)
})

it('should create a new session if the current session is more than 5 second', () => {
const { lifeCycle, clock } = setupBuilder.withFakeClock().build()
const { getViewUpdate, getViewUpdateCount } = viewTest
// first session window
newLayoutShift(lifeCycle, 0.1)
clock.tick(4500)
newLayoutShift(lifeCycle, 0.2)
// second session window
clock.tick(501)
newLayoutShift(lifeCycle, 0.1)
Copy link
Member

Choose a reason for hiding this comment

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

FMU, this is not what is expected to be tested: the first shift creates a session, then the session expires (after 1 second), then the second shift creates the second session, and the third shift gets added to the second session. So this test is similar to the previous one.

You should do something like:

for (let i = 0; i < 6; i += 1) {
  newLayoutShift(lifeCycle, 0.1)
  clock.tick(999)
}

(the first five are in the same session, the sixth creates a new session)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I will update this test.


clock.tick(THROTTLE_VIEW_UPDATE_PERIOD)
expect(getViewUpdateCount()).toEqual(3)
expect(getViewUpdate(2).cumulativeLayoutShift).toBe(0.3)
})

it('should get the max value sessions', () => {
const { lifeCycle, clock } = setupBuilder.withFakeClock().build()
const { getViewUpdate, getViewUpdateCount } = viewTest
// first session window
newLayoutShift(lifeCycle, 0.1)
newLayoutShift(lifeCycle, 0.2)
// second session window
clock.tick(5001)
newLayoutShift(lifeCycle, 0.1)
newLayoutShift(lifeCycle, 0.2)
newLayoutShift(lifeCycle, 0.2)
// third session window
clock.tick(5001)
newLayoutShift(lifeCycle, 0.2)
newLayoutShift(lifeCycle, 0.2)

clock.tick(THROTTLE_VIEW_UPDATE_PERIOD)
expect(getViewUpdateCount()).toEqual(3)
expect(getViewUpdate(2).cumulativeLayoutShift).toBe(0.5)
})
})
})
56 changes: 47 additions & 9 deletions packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
import { Duration, noop, elapsed, round, timeStampNow, Configuration } from '@datadog/browser-core'
import { supportPerformanceTimingEvent } from '../../../browser/performanceCollection'
import {
Duration,
noop,
elapsed,
round,
timeStampNow,
Configuration,
RelativeTime,
ONE_SECOND,
} from '@datadog/browser-core'
import { RumLayoutShiftTiming, supportPerformanceTimingEvent } from '../../../browser/performanceCollection'
import { ViewLoadingType } from '../../../rawRumEvent.types'
import { LifeCycle, LifeCycleEventType } from '../../lifeCycle'
import { EventCounts, trackEventCounts } from '../../trackEventCounts'
Expand Down Expand Up @@ -47,8 +56,8 @@ export function trackViewMetrics(
let stopCLSTracking: () => void
if (isLayoutShiftSupported()) {
viewMetrics.cumulativeLayoutShift = 0
;({ stop: stopCLSTracking } = trackLayoutShift(lifeCycle, (layoutShift) => {
viewMetrics.cumulativeLayoutShift = round(viewMetrics.cumulativeLayoutShift! + layoutShift, 4)
;({ stop: stopCLSTracking } = trackCumulativeLayoutShift(lifeCycle, (layoutShift) => {
viewMetrics.cumulativeLayoutShift = layoutShift
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
scheduleViewUpdate()
}))
} else {
Expand Down Expand Up @@ -120,18 +129,47 @@ function trackActivityLoadingTime(
}

/**
* Track layout shifts (LS) occurring during the Views. This yields multiple values that can be
* added up to compute the cumulated layout shift (CLS).
* Track the cumulative layout shifts (CLS).
* Layout shifts are grouped into session windows.
* The minimum gap between session windows is 1 second.
* The maximum duration of a session window is 5 second.
* The session window layout shift value is the sum of layout shifts inside it.
* The CLS value is the max of session windows values.
*
* This yields a new value whenever the CLS value is updated (a higher session window value is computed).
*
* See isLayoutShiftSupported to check for browser support.
*
* Documentation: https://web.dev/cls/
* Documentation:
* https://web.dev/cls/
* https://web.dev/evolving-cls/
* Reference implementation: https://github.com/GoogleChrome/web-vitals/blob/master/src/getCLS.ts
*/
function trackLayoutShift(lifeCycle: LifeCycle, callback: (layoutShift: number) => void) {
function trackCumulativeLayoutShift(lifeCycle: LifeCycle, callback: (layoutShift: number) => void) {
let clsValue = 0
let windowValue = 0
let windowStartedAt = 0 as RelativeTime
let windowLastEntryAt = 0 as RelativeTime
Copy link
Member

Choose a reason for hiding this comment

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

We might want to start with an "undefined" window so it is created at the first shift instead of at the begining of the navigation.


const updateSessionWindow = (entry: RumLayoutShiftTiming) => {
const durationSinceStart = entry.startTime - windowStartedAt
const durationSinceLastEntry = entry.startTime - windowLastEntryAt
if (durationSinceLastEntry < 1 * ONE_SECOND && durationSinceStart < 5 * ONE_SECOND) {
bcaudan marked this conversation as resolved.
Show resolved Hide resolved
windowValue += entry.value
} else {
windowValue = entry.value
windowStartedAt = entry.startTime
}
windowLastEntryAt = entry.startTime
}

const { unsubscribe: stop } = lifeCycle.subscribe(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, (entry) => {
if (entry.entryType === 'layout-shift' && !entry.hadRecentInput) {
callback(entry.value)
updateSessionWindow(entry)
if (windowValue > clsValue) {
clsValue = windowValue
callback(round(clsValue, 4))
}
}
})
bcaudan marked this conversation as resolved.
Show resolved Hide resolved

Expand Down