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.

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

merged 12 commits into from
Sep 6, 2021

Conversation

webNeat
Copy link
Contributor

@webNeat webNeat commented Sep 1, 2021

Motivation

The strategy to compute CLS has changed.

Old strategy: CLS is the sum of all layout shifts

New strategy:

  • Layout shifts are grouped into session windows.
  • A session window is a period that starts when a layout shift happens, and ends when no layout shift happens during 1 second or more. If a layout shift happens after that, it creates a new session window.
  • A session window cannot be longer than 5 seconds. So if adding a layout shift to a session window would make it longer than 5 seconds, then a new session window is created with that layout shift.
  • The value of CLS is max value of session windows

illustration of session windows

Changes

  • Implement the new CLS strategy
  • Add new unit tests

Testing

unit, CI

I have gone over the contributing documentation.

@webNeat webNeat requested a review from a team as a code owner September 1, 2021 10:42
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2021

Codecov Report

Merging #1026 (6dd854d) into main (b72928f) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1026      +/-   ##
==========================================
+ Coverage   89.04%   89.06%   +0.02%     
==========================================
  Files          88       88              
  Lines        4143     4161      +18     
  Branches      952      957       +5     
==========================================
+ Hits         3689     3706      +17     
- Misses        454      455       +1     
Impacted Files Coverage Δ
...ages/rum-core/src/browser/performanceCollection.ts 14.00% <ø> (ø)
...omain/rumEventsCollection/view/trackViewMetrics.ts 100.00% <100.00%> (ø)
packages/core/src/tools/timeUtils.ts 90.90% <0.00%> (-3.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b72928f...6dd854d. Read the comment docs.

Comment on lines 375 to 381
// 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.

Comment on lines 151 to 152
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 { 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() > clsValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (window.value() > clsValue) {
if (window.value() > maxClsValue) {

it could be a bit clearer by expressing that the cls value is the max one

@webNeat webNeat merged commit 94d9b86 into main Sep 6, 2021
@webNeat webNeat deleted the amine/new-cls branch September 6, 2021 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants