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.

Sign up for GitHub

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

Jump to bottom

Unintented impact of enabling SoftNavigations to existing PerformanceObservers #11

Closed
tunetheweb opened this issue Jan 18, 2023 · 10 comments
Closed

Unintented impact of enabling SoftNavigations to existing PerformanceObservers #11

tunetheweb opened this issue Jan 18, 2023 · 10 comments

Comments

@tunetheweb
Copy link
Contributor

When SoftNavs are enabled with the Chrome flags or the upcoming Origin Trail, we will start to emit LCP and FCP entries for new pages, and also possibly new FID entries.

This might have unintended consequences for existing PerformanceObservers of these events and they may, incorrectly, process these entries as applying to the initial navigation leading to larger FCP and LCP times and incorrect FID measurements. This is especially the case for buffered events which may replay several soft nav's entries.

Should we have an opt-in to the Performance Observers to prevent these being surfaced unexpectedly and to allow the existing FCP, LCP, and FID to continue to be measured as they currently are?
@yoavweiss
Copy link
Collaborator

I got a CL going that adds a PerformanceObserverInit dictionary member named triggeredBySoftNavigation. @tunetheweb tells me this is not a great name :)

What would be a better name? Does an init option make sense as the API shape here?

@tunetheweb
Copy link
Contributor Author

My concern (ignoring the length!) is it's normal entries AND soft nav entries. triggeredBySoftNavigation could be read as "only events triggered by soft navigations and not hard navigation entries".

Brainstorming here:

  • includesSoftNavigation
  • includesSoftNavigationObservations
  • includesSoftNavigationTypes
  • includesSoftNavigationMetrics

Some of them potentially are confusing as to include the soft-navigation entries themselves, rather than "allow additional metrics of this type to be emitted after a soft nav".

Maybe resetOnSoftNavigation is the most accurate?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 8, 2023
@chromium-wpt-export-bot
[soft navigations] Add PerfObserver option for softnav triggered entries
30750ad 
FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 14113691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
Merged
@yoavweiss
Copy link
Collaborator

I like the includes part. Maybe includes{,Post}SoftNavigationMeasurements?

@yoavweiss
Copy link
Collaborator

Maybe resetOnSoftNavigation is the most accurate?

Are you suggesting that to be "true" by default, and have folks that want to softnav data to add resetAfterSoftNavigation: false?

@tunetheweb
Copy link
Contributor Author

No I was suggesting resetOnSoftNavigation is false by default and setting it to true allows the LCP (for example) to be reset, and re-emitted.

But the fact you didn't grok that (and worse - read it the opposite way!) tells me that's not a good name.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 9, 2023
@chromium-wpt-export-bot
[soft navigations] Add PerfObserver option for softnav triggered entries
cd9bad1 
FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 1413691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 9, 2023
@chromium-wpt-export-bot
[soft navigations] Add PerfObserver option for softnav triggered entries
d728348 
FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 1413691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
@yoavweiss
Copy link
Collaborator

OK, changing the CL to includeSoftNavigationObservations. It's a lot to type, but seems descriptive enough..

@tunetheweb
Copy link
Contributor Author

includeSoftNavObs is a bit shorter but maybe more obscure.

@yoavweiss
Copy link
Collaborator

Definitely more obscure..

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 9, 2023
@chromium-wpt-export-bot
[soft navigations] Add PerfObserver option for softnav triggered entries
83ed398 
FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 1413691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
aarongable pushed a commit to chromium/chromium that referenced this issue Feb 9, 2023
[soft navigations] Add PerfObserver option for softnav triggered entries
462ac01 
FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 1413691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4230572
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103256}
@yoavweiss
Copy link
Collaborator

Landed the CL and an explainer PR, so closing, but feel free to reopen if more bikeshedding is required.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 9, 2023
@chromium-wpt-export-bot
[soft navigations] Add PerfObserver option for softnav triggered entries
cf21b67 
FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 1413691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4230572
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103256}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 9, 2023
@chromium-wpt-export-bot
[soft navigations] Add PerfObserver option for softnav triggered entries
98a96d4 
FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 1413691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4230572
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103256}
@tunetheweb
Copy link
Contributor Author

FYI, I've updated the web-vitals soft navs branch in preparation for this landing in a future Chrome

@tunetheweb tunetheweb mentioned this issue Feb 11, 2023
Merged
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 20, 2023
@moz-wptsync-bot
Bug 1815705 [wpt PR 38418] - [soft navigations] Add PerfObserver opti…
36e5196 
…on for softnav triggered entries, a=testonly

Automatic update from web-platform-tests
[soft navigations] Add PerfObserver option for softnav triggered entries

FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 1413691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4230572
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103256}

--

wpt-commits: 98a96d4d57acbbd607d44cbcadc50197cb93035e
wpt-pr: 38418
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Feb 23, 2023
@moz-wptsync-bot
Bug 1815705 [wpt PR 38418] - [soft navigations] Add PerfObserver opti…
b9a1b08 
…on for softnav triggered entries, a=testonly

Automatic update from web-platform-tests
[soft navigations] Add PerfObserver option for softnav triggered entries

FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 1413691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4230572
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103256}

--

wpt-commits: 98a96d4d57acbbd607d44cbcadc50197cb93035e
wpt-pr: 38418
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Mar 28, 2023
@marcoscaceres
[soft navigations] Add PerfObserver option for softnav triggered entries
e434635 
FP, FCP and LCP entries that are triggered by soft navigations could
confuse existing RUM scripts that may not be aware of them.
See WICG/soft-navigations#11

This CL adds and opt-in option to only report these entries when the
observer explicitly asks for them.

Bug: 1413691
Change-Id: I6809973b88ed987f6ca08fbb9546d4a8f654c3fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4230572
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103256}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@yoavweiss @tunetheweb