✨ [RUMF-992] New CLS implementation (#1026)

* ✨ 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
DataDog · Sep 6, 2021 · 94d9b86 · 94d9b86
1 parent 5d97167
commit 94d9b86
Show file tree

Hide file tree

Showing 3 changed files with 124 additions and 42 deletions.
diff --git 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
@@ -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
@@ -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.
  */