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

Fix first visible timestamp and LCPV #37144

Merged
merged 2 commits into from
Dec 7, 2021
Merged

Conversation

jridgewell
Copy link
Contributor

@jridgewell jridgewell commented Dec 7, 2021

In #36273, I switched from Date.now() to performance.now(), thinking it was a higher resolution timestamp time since the unix epoch. But, performance.now is relative to page load time (or performance.timeOrigin), not the epoch.

This matters because our "since visible" metrics use the epoch as their base time, meaning the timestamps are roughly 1.6 trillion milliseconds. But performance.now is tiny, in the range of a few million for a page that was loaded an hour ago. When we calculated the "since visible", we received values that were still in the trillions when we expect millions.

By adding performance.now() and performance.timeOrigin, we get a timestamp relative to the epoch. Now when we calculate the "since visible", we get the correct values.

Fixes #37072.

In ampproject#36273, I switched from `Date.now()` to `performance.now()`, thinking it was a higher resolution timestamp time since the unix epoch. But, `peformance.now` is relative to _page load time_ (or `timeOrigin`), not the epoch.

This matters because our "since visible" metrics use the epoch as their base time, meaning the timestamps are roughly 1.6 trillion milliseconds. But `performance.now` is tiny, in the range of a few million for a page that was loaded an hour ago. When we calculated the "since visible", we received values that were still in the trillions when we expect millions.

By adding `performance.now()` and `performance.timeOrigin`, we get a timestamp relative to the epoch. Now when we calculate the "since visible", we get the correct values.
Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

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

Thank you for the bugfix and clear test updates.

@jridgewell jridgewell merged commit 7645c4e into ampproject:main Dec 7, 2021
ampprojectbot pushed a commit that referenced this pull request Dec 8, 2021
In #36273, I switched from `Date.now()` to `performance.now()`, thinking it was a higher resolution timestamp time since the unix epoch. But, `performance.now` is relative to _page load time_ (or `performance.timeOrigin`), not the epoch.

This matters because our "since visible" metrics use the epoch as their base time, meaning the timestamps are roughly 1.6 trillion milliseconds. But `performance.now` is tiny, in the range of a few million for a page that was loaded an hour ago. When we calculated the "since visible", we received values that were still in the trillions when we expect millions.

By adding `performance.now()` and `performance.timeOrigin`, we get a timestamp relative to the epoch. Now when we calculate the "since visible", we get the correct values.

(cherry picked from commit 7645c4e)
@jridgewell jridgewell deleted the lcpv-fix branch December 10, 2021 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web Vitals: tracking LCP using amp-analytics not working well
3 participants