From 94d9b864b396010b26a094d6fdd9760c990d10ac Mon Sep 17 00:00:00 2001 From: Amine Ben hammou Date: Mon, 6 Sep 2021 11:34:07 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20[RUMF-992]=20New=20CLS=20implementa?= =?UTF-8?q?tion=20(#1026)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * ✨ Compute CLS using the new definition * 🎨 Refactor unit tests * ✅ Add unit tests * 🎨 Avoid computing the max twice * 👌 Use function instead of arrow function * 👌 Make arguments more readable * 👌 Update unit test * 👌 Isolate the session window logic * 🎨 Fix linter error * 👌 Clarify that the CLS is the max value --- .../src/browser/performanceCollection.ts | 1 + .../view/trackViewMetrics.spec.ts | 102 ++++++++++++------ .../view/trackViewMetrics.ts | 63 +++++++++-- 3 files changed, 124 insertions(+), 42 deletions(-) diff --git a/packages/rum-core/src/browser/performanceCollection.ts b/packages/rum-core/src/browser/performanceCollection.ts index bd2112712b..f4f6d41c9e 100644 --- a/packages/rum-core/src/browser/performanceCollection.ts +++ b/packages/rum-core/src/browser/performanceCollection.ts @@ -74,6 +74,7 @@ export interface RumFirstInputTiming { export interface RumLayoutShiftTiming { entryType: 'layout-shift' + startTime: RelativeTime value: number hadRecentInput: boolean } diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.spec.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.spec.ts index fe90c46625..07e134cbe6 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.spec.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.spec.ts @@ -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, @@ -281,6 +282,15 @@ describe('rum track view metrics', () => { describe('cumulativeLayoutShift', () => { let isLayoutShiftSupported: boolean + function newLayoutShift(lifeCycle: LifeCycle, { value = 0.1, hadRecentInput = false }) { + 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') @@ -308,22 +318,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, { value: 0.1 }) + clock.tick(100) + newLayoutShift(lifeCycle, { value: 0.2 }) clock.tick(THROTTLE_VIEW_UPDATE_PERIOD) expect(getViewUpdateCount()).toEqual(2) @@ -333,19 +333,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, { value: 1.23456789 }) + clock.tick(100) + newLayoutShift(lifeCycle, { value: 1.11111111111 }) clock.tick(THROTTLE_VIEW_UPDATE_PERIOD) expect(getViewUpdateCount()).toEqual(2) @@ -356,16 +346,62 @@ 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, { value: 0.1, hadRecentInput: true }) 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, { value: 0.1 }) + clock.tick(100) + newLayoutShift(lifeCycle, { value: 0.2 }) + // second session window + clock.tick(1001) + newLayoutShift(lifeCycle, { value: 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 window if the current session window is more than 5 second', () => { + const { lifeCycle, clock } = setupBuilder.withFakeClock().build() + const { getViewUpdate, getViewUpdateCount } = viewTest + newLayoutShift(lifeCycle, { value: 0 }) + for (let i = 0; i < 6; i += 1) { + clock.tick(999) + newLayoutShift(lifeCycle, { value: 0.1 }) + } // window 1: 0.5 | window 2: 0.1 + clock.tick(THROTTLE_VIEW_UPDATE_PERIOD) + expect(getViewUpdateCount()).toEqual(3) + expect(getViewUpdate(2).cumulativeLayoutShift).toBe(0.5) + }) + + it('should get the max value sessions', () => { + const { lifeCycle, clock } = setupBuilder.withFakeClock().build() + const { getViewUpdate, getViewUpdateCount } = viewTest + // first session window + newLayoutShift(lifeCycle, { value: 0.1 }) + newLayoutShift(lifeCycle, { value: 0.2 }) + // second session window + clock.tick(5001) + newLayoutShift(lifeCycle, { value: 0.1 }) + newLayoutShift(lifeCycle, { value: 0.2 }) + newLayoutShift(lifeCycle, { value: 0.2 }) + // third session window + clock.tick(5001) + newLayoutShift(lifeCycle, { value: 0.2 }) + newLayoutShift(lifeCycle, { value: 0.2 }) + + clock.tick(THROTTLE_VIEW_UPDATE_PERIOD) + expect(getViewUpdateCount()).toEqual(3) + expect(getViewUpdate(2).cumulativeLayoutShift).toBe(0.5) + }) }) }) diff --git a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.ts b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.ts index 51446c23e9..eb0853e302 100644 --- a/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.ts +++ b/packages/rum-core/src/domain/rumEventsCollection/view/trackViewMetrics.ts @@ -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' @@ -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, (cumulativeLayoutShift) => { + viewMetrics.cumulativeLayoutShift = cumulativeLayoutShift scheduleViewUpdate() })) } else { @@ -120,18 +129,32 @@ 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 maxClsValue = 0 + const window = slidingSessionWindow() const { unsubscribe: stop } = lifeCycle.subscribe(LifeCycleEventType.PERFORMANCE_ENTRY_COLLECTED, (entry) => { if (entry.entryType === 'layout-shift' && !entry.hadRecentInput) { - callback(entry.value) + window.update(entry) + if (window.value() > maxClsValue) { + maxClsValue = window.value() + callback(round(maxClsValue, 4)) + } } }) @@ -140,6 +163,28 @@ function trackLayoutShift(lifeCycle: LifeCycle, callback: (layoutShift: number) } } +function slidingSessionWindow() { + let value = 0 + let startTime: RelativeTime + let endTime: RelativeTime + return { + update: (entry: RumLayoutShiftTiming) => { + const shouldCreateNewWindow = + startTime === undefined || + entry.startTime - endTime >= ONE_SECOND || + entry.startTime - startTime >= 5 * ONE_SECOND + if (shouldCreateNewWindow) { + startTime = endTime = entry.startTime + value = entry.value + } else { + value += entry.value + endTime = entry.startTime + } + }, + value: () => value, + } +} + /** * Check whether `layout-shift` is supported by the browser. */