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

Report Web Vital metrics on every chance #949

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Jun 23, 2023

First commit explanation

Note: We've decided to remove the first commit as the second commit already fixes the issue. Here's the changes from the first commit in case we need it again.

func (p *Page) Close(opts goja.Value) error {
	p.logger.Debugf("Page:Close", "sid:%v", p.sessionID())

	// forcing the pagehide event to trigger web vitals metrics.
	v := p.vu.Runtime().ToValue(`() => window.dispatchEvent(new Event('pagehide'))`)
	_ = p.Evaluate(v)
	...
}       

The first commit forces the Web Vital metrics to get triggered on page.Close. The page is closing anyway, so there should be no harm in hiding it before it gets closed. Please see this description and this comment for the details. It turns out that using pagehide is the recommended way here. What they really recommend is to batch the events instead of processing them one by one. Anyway, this can shed light on why this fix works.

Note: see the Page Lifecycle guide for an explanation of why visibilitychange and pagehide are recommended over events like beforeunload and unload.

Second commit explanation

Additionally, CLS should/can be called multiple times.

(CLS) callback is always called when the page's visibility state changes to hidden. As a result, the callback function might be called multiple times during the same page load.

This 2nd commit uses this advice and sets the callbacks to report metrics all the time. See this link for more information.

Results

Before:

browser_web_vital_fcp.......: avg=1.99s    min=1.99s    med=1.99s    max=1.99s    p(90)=1.99s    p(95)=1.99s   
browser_web_vital_ttfb......: avg=1.81s    min=1.81s    med=1.81s    max=1.81s    p(90)=1.81s    p(95)=1.81s 

After:

browser_web_vital_cls.......: avg=0.000057 min=0.000057 med=0.000057 max=0.000057 p(90)=0.000057 p(95)=0.000057
browser_web_vital_fcp.......: avg=1.99s    min=1.99s    med=1.99s    max=1.99s    p(90)=1.99s    p(95)=1.99s   
browser_web_vital_lcp.......: avg=1.99s    min=1.99s    med=1.99s    max=1.99s    p(90)=1.99s    p(95)=1.99s   
browser_web_vital_ttfb......: avg=1.81s    min=1.81s    med=1.81s    max=1.81s    p(90)=1.81s    p(95)=1.81s 

I used the same script from this comment, but without page.reload()! So here it is again.

Test script
import { browser } from 'k6/x/browser';

export const options = {
  scenarios: {
    ui: {
      executor: 'shared-iterations',
      options: {
        browser: {
            type: 'chromium',
        },
      },
    },
  },
};

export default async function () {
  const page = browser.newPage();

  try {
    await page.goto('https://test.k6.io/');
  } finally {
    page.close();
  } 
}

@inancgumus inancgumus changed the base branch from main to fix/914-life-cycle-wait-fix June 23, 2023 10:22
@inancgumus inancgumus changed the title Fix/914 hide page on close Force page unhide to produce more Web Vital metrics Jun 23, 2023
@inancgumus inancgumus requested review from ka3de and ankur22 June 23, 2023 10:25
@inancgumus inancgumus self-assigned this Jun 23, 2023
@inancgumus inancgumus added this to the v0.11.0 milestone Jun 23, 2023
@inancgumus inancgumus added the bug Something isn't working label Jun 23, 2023
@inancgumus inancgumus marked this pull request as ready for review June 23, 2023 10:26
@inancgumus inancgumus linked an issue Jun 23, 2023 that may be closed by this pull request
@inancgumus inancgumus changed the title Force page unhide to produce more Web Vital metrics Force page hide to produce more Web Vital metrics Jun 23, 2023
@inancgumus inancgumus changed the title Force page hide to produce more Web Vital metrics Report Web Vital metrics on every chance Jun 23, 2023
With this change, the Web Vital callbacks will be called for all
changes. instead of waiting for them to complete.

See this comment:
If the `reportAllChanges` configuration option is set to `true`, the
`callback` function will be called as soon as the value is initially
determined as well as any time the value changes throughout the page
lifespan.

https://github.com/GoogleChrome/web-vitals/blob/5973d51be39ab0020268aa2397ead0962eedf4c2/src/onCLS.ts#L43-L46
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

Thanks for @inancgumus. I'm happy to trial this out with #950 to ensure that the measurements are still accurate 👍 LGTM 🙂

Base automatically changed from fix/914-life-cycle-wait-fix to main June 23, 2023 14:32
@inancgumus inancgumus merged commit f25b1d4 into main Jun 23, 2023
8 of 12 checks passed
@inancgumus inancgumus deleted the fix/914-hide-page-on-close branch June 23, 2023 14:32
@inancgumus inancgumus removed this from the v0.11.0 milestone Jun 23, 2023
inancgumus added a commit that referenced this pull request Jun 23, 2023
This brings back the hack we removed from PR #949. I've also added a
test for it. When we remove the hack, the test fails, otherwise passes.
ka3de added a commit that referenced this pull request Jul 11, 2023
ka3de pushed a commit that referenced this pull request Jul 11, 2023
This brings back the hack we removed from PR #949. I've also added a
test for it. When we remove the hack, the test fails, otherwise passes.
ka3de added a commit that referenced this pull request Jul 11, 2023
ka3de pushed a commit that referenced this pull request Jul 11, 2023
This brings back the hack we removed from PR #949. I've also added a
test for it. When we remove the hack, the test fails, otherwise passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web Vitals metrics are not consistently reported
3 participants