CLS tracking: change source attribution logic and include `devicePixelRatio` inside `performance.cls` namespace #3377

anediaz · 2025-02-24T18:11:50Z

Motivation

Implementation of small adjustments in Browser SDK for CLS attribution:

Include devicePixelRatio attribute:
- Recent discoveries showed that Layout Instability API does not report viewPort coordinates for prevRect and currentRect.
- We'd need to include this data in the CLS attribution object. See this document for more context
Adapt fix “most shifted“ element calculation:
- The current calculation does not take into account the sources size attached to a CLS entry, it returns always the first one
- It’d be more relevant returning the biggest element (among those having the same shift), since it could be more impactful than a smaller element with the same shift
- Change inspired on this snippet

Changes

Include new attribute devicePixelRatio in the CLS attribution
Adapt cls tracking logic to pick the source having the biggest area among those attributed to the CLS entry

Testing

Local
Staging
Unit
End to end

I have gone over the contributing documentation.

…of maxCLSValue

bits-bot · 2025-02-24T18:11:57Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

codecov-commenter · 2025-02-24T18:13:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.80%. Comparing base (194562f) to head (207ab54).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3377      +/-   ##
==========================================
- Coverage   92.81%   92.80%   -0.01%     
==========================================
  Files         299      299              
  Lines        7860     7864       +4     
  Branches     1790     1792       +2     
==========================================
+ Hits         7295     7298       +3     
- Misses        565      566       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cit-pr-commenter · 2025-02-24T18:22:25Z

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	148.16 KiB	148.55 KiB	406 B	+0.27%	✅
Logs	52.19 KiB	52.19 KiB	0 B	0.00%	✅
Rum Slim	106.82 KiB	107.21 KiB	406 B	+0.37%	✅
Worker	24.50 KiB	24.50 KiB	0 B	0.00%	✅

🚀 CPU Performance

Action Name	Base Average Cpu Time (ms)	Local Average Cpu Time (ms)	𝚫
addglobalcontext	0.002	0.002	0.001
addaction	0.037	0.054	0.017
addtiming	0.001	0.001	0.000
adderror	0.054	0.070	0.017
startstopsessionreplayrecording	0.007	0.013	0.006
startview	0.374	0.521	0.147
logmessage	0.019	0.026	0.007

🧠 Memory Performance

Action Name	Base Consumption Memory (bytes)	Local Consumption Memory (bytes)	𝚫 (bytes)
addglobalcontext	29.77 KiB	30.59 KiB	844 B
addaction	57.09 KiB	53.69 KiB	-3479 B
addtiming	27.82 KiB	27.98 KiB	172 B
adderror	61.37 KiB	62.85 KiB	1.49 KiB
startstopsessionreplayrecording	25.09 KiB	25.96 KiB	884 B
startview	422.81 KiB	421.30 KiB	-1550 B
logmessage	57.70 KiB	57.28 KiB	-430 B

🔗 RealWorld

…rformance.cls namespace

thomas-lebeau · 2025-02-25T06:48:49Z

packages/rum-core/src/domain/view/viewMetrics/trackCumulativeLayoutShift.spec.ts

@@ -306,6 +310,7 @@ describe('trackCumulativeLayoutShift', () => {
    it('should get the target element, time, and rects of the largest layout shift', () => {
      startCLSTracking()
      const divElement = appendElement('<div id="div-element"></div>')
+      const spanElement = appendElement('<span id="span-element"></span>')


💬 suggestion: ‏ what about naming this something like largerElement?

This way, it gives a clue as why this is the element that is attributed to the CLS?

Suggested change

const spanElement = appendElement('<span id="span-element"></span>')

const largerElement = appendElement('<span id="larger-element"></span>')

yes, good idea! change made here

thomas-lebeau · 2025-02-25T06:49:40Z

packages/rum-core/src/domain/view/viewMetrics/trackCumulativeLayoutShift.spec.ts

@@ -306,6 +310,7 @@ describe('trackCumulativeLayoutShift', () => {
    it('should get the target element, time, and rects of the largest layout shift', () => {
      startCLSTracking()
      const divElement = appendElement('<div id="div-element"></div>')
+      const spanElement = appendElement('<span id="span-element"></span>')

      // first session window:  { value: 0.5, time: 1, targetSelector: '#div-element' }


❓ question: ‏ I guess this comment is not correct anymore? Is it?

yeah definitely! I'm removing it here

thomas-lebeau · 2025-02-25T06:54:00Z

packages/rum-core/src/domain/view/viewMetrics/trackCumulativeLayoutShift.ts

@@ -74,10 +75,10 @@ export function trackCumulativeLayoutShift(
        continue
      }

-      const { cumulatedValue, isMaxValue } = window.update(entry)
+      const { cumulatedValue, isMaxValue, devicePixelRatio } = window.update(entry)


❓ question: ‏ Does devicePixelRatio changes over time? Otherwise, it's not needed to get it from the sliding window update (window.update(entry))

the user can change the screen within the same view, and the devicePixelRatio can vary depending on the screen.
This is why I wanted to attach it to the moment where the CLS maxValue is reported.
I'd be happy to hear from your suggestions if you think it is a better way of doing it 😄

🤔 true, but it's not calculated depending on previous values from the sliding window.

I think what you want is to store devicePixelRatio at the time of the max value (i.e. in the if (isMaxValue) condition), the same way it is done for attribution = getBiggestElementAttribution()

I'm adding some suggestion comments here and here, so you see what I mean.

thomas-lebeau · 2025-02-25T07:09:45Z

packages/rum-core/src/domain/view/viewMetrics/trackCumulativeLayoutShift.ts

    (source): source is RumLayoutShiftAttribution & { node: Element } => !!source.node && isElementNode(source.node)
  )
+  if (elementNodeSources.length > 0) {
+    return elementNodeSources.reduce(function (a, b) {
+      return a.node && a.previousRect?.width * a.previousRect?.height > b.previousRect?.width * b.previousRect?.height


💬 suggestion: ‏ I think we don't need to check a.node again as it's seems elementNodeSources is already filtering out object without the node property, no?

Suggested change

return a.node && a.previousRect?.width * a.previousRect?.height > b.previousRect?.width * b.previousRect?.height

return a.previousRect?.width * a.previousRect?.height > b.previousRect?.width * b.previousRect?.height

true! fixed here

thomas-lebeau · 2025-02-25T07:13:05Z

packages/rum-core/src/domain/view/viewMetrics/trackCumulativeLayoutShift.ts

    (source): source is RumLayoutShiftAttribution & { node: Element } => !!source.node && isElementNode(source.node)
  )
+  if (elementNodeSources.length > 0) {


🥜 nitpick: ‏use early returns, it makes it easier to read.

Suggested change

if (elementNodeSources.length > 0) {

if (elementNodeSources.length <= 0) {

return

}

thomas-lebeau · 2025-02-25T07:27:55Z

Thanks @anediaz, Could you please update DataDog/rum-events-format to add the new property?
Here is an example: DataDog/rum-events-format#247. When this is merge, you will need to sync the repos by running rum-events-format:sync in the browser-sdk

anediaz · 2025-02-25T08:05:02Z

Thanks @anediaz, Could you please update DataDog/rum-events-format to add the new property? Here is an example: DataDog/rum-events-format#247. When this is merge, you will need to sync the repos by running rum-events-format:sync in the browser-sdk

Thanks for the explanation!
I've created a PR for this. Here you are: [RUM-8744Add property device_pixel_ratio to CLS](DataDog/rum-events-format#257)

thomas-lebeau · 2025-02-25T08:27:11Z

packages/rum-core/src/domain/view/viewMetrics/trackCumulativeLayoutShift.ts

@@ -74,10 +75,10 @@ export function trackCumulativeLayoutShift(
        continue
      }

-      const { cumulatedValue, isMaxValue } = window.update(entry)
+      const { cumulatedValue, isMaxValue, devicePixelRatio } = window.update(entry)


🤔 true, but it's not calculated depending on previous values from the sliding window.

I think what you want is to store devicePixelRatio at the time of the max value (i.e. in the if (isMaxValue) condition), the same way it is done for attribution = getBiggestElementAttribution()

I'm adding some suggestion comments here and here, so you see what I mean.

thomas-lebeau · 2025-02-25T08:28:04Z

packages/rum-core/src/domain/view/viewMetrics/trackCumulativeLayoutShift.ts

@@ -96,6 +97,7 @@ export function trackCumulativeLayoutShift(
          time: biggestShift?.time,
          previousRect: biggestShift?.previousRect ? asRumRect(biggestShift.previousRect) : undefined,
          currentRect: biggestShift?.currentRect ? asRumRect(biggestShift.currentRect) : undefined,
+          devicePixelRatio,


Suggested change

devicePixelRatio,

devicePixelRatio = biggestShift?.devicePixelRatio,

thomas-lebeau · 2025-02-25T08:29:16Z

packages/rum-core/src/domain/view/viewMetrics/trackCumulativeLayoutShift.ts

        biggestShift = {
          target: attribution?.node ? new WeakRef(attribution.node) : undefined,
          time: elapsed(viewStart, entry.startTime),


Suggested change

biggestShift = {

target: attribution?.node ? new WeakRef(attribution.node) : undefined,

time: elapsed(viewStart, entry.startTime),

biggestShift = {

target: attribution?.node ? new WeakRef(attribution.node) : undefined,

time: elapsed(viewStart, entry.startTime),

previousRect: attribution?.previousRect,

currentRect: attribution?.currentRect,

devicePixelRatio: window.devicePixelRatio,

♻️trackCumulativeLayoutShift: return the biggest attribution element …

a251063

…of maxCLSValue

♻️trackCumulativeLayoutShift: include devicePixelRatio data inside pe…

629d814

…rformance.cls namespace

anediaz changed the title ~~♻️trackCumulativeLayoutShift: return the biggest attribution element …~~ CLS tracking: change source attribution logic and include devicePixelRatio inside performance.cls namespace Feb 24, 2025

anediaz marked this pull request as ready for review February 24, 2025 18:43

anediaz requested a review from a team as a code owner February 24, 2025 18:43

thomas-lebeau self-assigned this Feb 25, 2025

thomas-lebeau reviewed Feb 25, 2025

View reviewed changes

♻️Address PR comments

207ab54

thomas-lebeau reviewed Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLS tracking: change source attribution logic and include `devicePixelRatio` inside `performance.cls` namespace #3377

CLS tracking: change source attribution logic and include `devicePixelRatio` inside `performance.cls` namespace #3377

anediaz commented Feb 24, 2025 •

edited

Loading

bits-bot commented Feb 24, 2025

codecov-commenter commented Feb 24, 2025 •

edited

Loading

cit-pr-commenter bot commented Feb 24, 2025 •

edited

Loading

thomas-lebeau Feb 25, 2025

anediaz Feb 25, 2025

thomas-lebeau Feb 25, 2025

anediaz Feb 25, 2025 •

edited

Loading

thomas-lebeau Feb 25, 2025

anediaz Feb 25, 2025

thomas-lebeau Feb 25, 2025 •

edited

Loading

thomas-lebeau Feb 25, 2025

anediaz Feb 25, 2025 •

edited

Loading

thomas-lebeau Feb 25, 2025

anediaz Feb 25, 2025

thomas-lebeau commented Feb 25, 2025

anediaz commented Feb 25, 2025 •

edited by jira bot

Loading

thomas-lebeau Feb 25, 2025 •

edited

Loading

thomas-lebeau Feb 25, 2025

thomas-lebeau Feb 25, 2025

	const spanElement = appendElement('<span id="span-element"></span>')
	const largerElement = appendElement('<span id="larger-element"></span>')

	return a.node && a.previousRect?.width * a.previousRect?.height > b.previousRect?.width * b.previousRect?.height
	return a.previousRect?.width * a.previousRect?.height > b.previousRect?.width * b.previousRect?.height

	devicePixelRatio,
	devicePixelRatio = biggestShift?.devicePixelRatio,

CLS tracking: change source attribution logic and include devicePixelRatio inside performance.cls namespace #3377

Are you sure you want to change the base?

CLS tracking: change source attribution logic and include devicePixelRatio inside performance.cls namespace #3377

Conversation

anediaz commented Feb 24, 2025 • edited Loading

Motivation

Changes

Testing

bits-bot commented Feb 24, 2025

codecov-commenter commented Feb 24, 2025 • edited Loading

Codecov Report

cit-pr-commenter bot commented Feb 24, 2025 • edited Loading

Bundles Sizes Evolution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anediaz Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-lebeau Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anediaz Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomas-lebeau commented Feb 25, 2025

anediaz commented Feb 25, 2025 • edited by jira bot Loading

thomas-lebeau Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLS tracking: change source attribution logic and include `devicePixelRatio` inside `performance.cls` namespace #3377

CLS tracking: change source attribution logic and include `devicePixelRatio` inside `performance.cls` namespace #3377

anediaz commented Feb 24, 2025 •

edited

Loading

codecov-commenter commented Feb 24, 2025 •

edited

Loading

cit-pr-commenter bot commented Feb 24, 2025 •

edited

Loading

anediaz Feb 25, 2025 •

edited

Loading

thomas-lebeau Feb 25, 2025 •

edited

Loading

anediaz Feb 25, 2025 •

edited

Loading

anediaz commented Feb 25, 2025 •

edited by jira bot

Loading

thomas-lebeau Feb 25, 2025 •

edited

Loading