PageLoadMetrics: Prevent PageLoadMetricsObserver timing event emissions during BFCache [chromium/src : main]
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
Related details
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
Hi Ken!
I'd like to understand this change and it's implications.
I think what it's aiming to address is a real issue (my understanding may be incomplete): The information about entering and getting restored from the back-forward cache arrives more quickly at the PageLoadTracker since it's not buffered, whereas timing data comes from page_timing_metrics_sender.cc, which buffers updates. And therefore, PageLoadMetricsObserver subclasses have to be conscious of this race if they are interested in the intersection of both types of events. There is also a potential source of incorrectness, since the timing data that may be included in the back-forward-cache restore event may be delayed (it comes from the page load metrics update dispatcher via UpdateTiming). In some of the observers we have logic that deals with the problem, specific to their needs - including dropping some events that arrive too late.
In this change, I think the dropping strategy will apply to all PageLoadMetricsObservers. I can see why that's nice, it simplifies their assumptions.
What I'm worried about:
1) Some observers may not care about the intersection of timing data and bfcache - e.g., they may just care about the timing data. I think an example may be uma_page_load_metrics_observer.cc - it just wants to log FCP and several other metrics in some histograms, so if the FCP arrives after the "enter bfcache notification" it may not care, it's still a good measurement. The worry would be that filtering will reduce the volume of measurements and potentially bias, but this UMA stuff is possibly important (like, it may mess up core web vitals / guardrails in the worst case).
2) Some observers have come up with their own handling, or have expectations that some message is eventually received. E.g., a soft navigation that's detected in the renderer could be sent at paint time, and we'd expect a message about the presentation to follow after that. It's possible that it paints but never gets presented (an unfortunate edge case maybe), but, dropping the message because of the race between sending timing data delayed and knowing about bfcache more quickly is a different condition (possibly more common). So, it would seem that a blanket measure like the one implemented in this change reduces the possibility of an observer implementation to detect and log the condition - dropping the message due to the race masks its opportunity to quantify the painted but never presented situation, for example. This example is just something that I am thinking of right now since I'm working on soft navigations - but I imagine similar stuff may apply to the relatively large number of existing observers.
So, maybe the question here is - have you thought about how to mitigate possible fallout from this changelist, or how to limit it; e.g., if it's best to apply this to all observers, make sure we understand the changes really well (e.g. with Finch), or if we're not so sure, let the observer specify whether it desires this behavior?
More broadly: Are there alternatives to dropping the timing notifications? E.g., what if we'd delay the bfcache notifications until the corresponding timing data had a chance to arrive? It sounds slightly crazy - but I think the page load metrics update dispatcher is already delaying some stuff because it needs a time window to merge data from other frames (e.g. a layout shift or LCP inside of an iframe), so it can count toward the whole page aggregated metric.
Thanks for considering and please let me know what you think!
Related details
- Ken Okada
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 1 comment![]( "Open in Gerrit")
Hi Ken!
I'd like to understand this change and it's implications.
I think what it's aiming to address is a real issue (my understanding may be incomplete): The information about entering and getting restored from the back-forward cache arrives more quickly at the PageLoadTracker since it's not buffered, whereas timing data comes from page_timing_metrics_sender.cc, which buffers updates. And therefore, PageLoadMetricsObserver subclasses have to be conscious of this race if they are interested in the intersection of both types of events. There is also a potential source of incorrectness, since the timing data that may be included in the back-forward-cache restore event may be delayed (it comes from the page load metrics update dispatcher via UpdateTiming). In some of the observers we have logic that deals with the problem, specific to their needs - including dropping some events that arrive too late.
In this change, I think the dropping strategy will apply to all PageLoadMetricsObservers. I can see why that's nice, it simplifies their assumptions.
What I'm worried about:
1) Some observers may not care about the intersection of timing data and bfcache - e.g., they may just care about the timing data. I think an example may be uma_page_load_metrics_observer.cc - it just wants to log FCP and several other metrics in some histograms, so if the FCP arrives after the "enter bfcache notification" it may not care, it's still a good measurement. The worry would be that filtering will reduce the volume of measurements and potentially bias, but this UMA stuff is possibly important (like, it may mess up core web vitals / guardrails in the worst case).
2) Some observers have come up with their own handling, or have expectations that some message is eventually received. E.g., a soft navigation that's detected in the renderer could be sent at paint time, and we'd expect a message about the presentation to follow after that. It's possible that it paints but never gets presented (an unfortunate edge case maybe), but, dropping the message because of the race between sending timing data delayed and knowing about bfcache more quickly is a different condition (possibly more common). So, it would seem that a blanket measure like the one implemented in this change reduces the possibility of an observer implementation to detect and log the condition - dropping the message due to the race masks its opportunity to quantify the painted but never presented situation, for example. This example is just something that I am thinking of right now since I'm working on soft navigations - but I imagine similar stuff may apply to the relatively large number of existing observers.
So, maybe the question here is - have you thought about how to mitigate possible fallout from this changelist, or how to limit it; e.g., if it's best to apply this to all observers, make sure we understand the changes really well (e.g. with Finch), or if we're not so sure, let the observer specify whether it desires this behavior?
More broadly: Are there alternatives to dropping the timing notifications? E.g., what if we'd delay the bfcache notifications until the corresponding timing data had a chance to arrive? It sounds slightly crazy - but I think the page load metrics update dispatcher is already delaying some stuff because it needs a time window to merge data from other frames (e.g. a layout shift or LCP inside of an iframe), so it can count toward the whole page aggregated metric.
Thanks for considering and please let me know what you think!
Thank you for your comments!
---
More broadly: Are there alternatives to dropping the timing notifications? E.g., what if we'd delay the bfcache notifications until the corresponding timing data had a chance to arrive?
This is generally impossible. For example, IIUC, FCP and LCP are not ensured to be emitted, especially before entering BFCache. (The last point is in the meaning of `TimeDelta`, not the method notification order.)
One possible mitigation for this is waiting 1sec for each event. But it increases complexity. (I'm not a big fan of this idea. PLT/PLMO is already complex...)
---
I know there are some types of PLMOs. E.g.
1. Interested in full events of **page**.
- Soft navigations, WaaP
2. Interested in PLM of non-prerender/non-bfcache **navigation**. (Including FCP/LCP, which are reported **after** commit of navigation.)
3. Plus, either/both prerender/bfcache.
I believe that "omit FCP-like events" is suitable for 2 and 3. Rationale:
- The best practice (for this type) is finalizing/recording PLM at Oncomplete/FlushMetricsOnAppEnterBackground/OnEnterBackForardCache.
- Typical PLMOs are already filtering events/metrics: E.g. `WasStartedInForegroundOptionalEventInForeground()`.
- I expect that FCP after entering BFCache is rare in wild. Typically, I guess, it needs click before FCP.
Actually, as I noted in the commit message, all existing PLMOs look to expect that FCP is not reported in BFCache.
---
So, maybe the question here is - have you thought about how to mitigate possible fallout from this changelist, or how to limit it;
That said, this idea is great! My philosophy here is "Simple thing should be writen in simple." Most of PLMOs handle complex thing in 0-or-1 direction, e.g. MPArch(prerender/bfcache), child frames, soft navigations. So, it would be nice a PLMO to declare a type and restrict itself.
Ideally, we might have something like this:
```
enum PageLoadMetricsObserverRestrictionType {
kUnrestricted,
// Records PLM for navigation. (Including prerender, BFCache.) Not so interested to page.
kObserveNavigation,
};
```
But I feel that currently the following is enough:
```
bool PageLoadMetricsObserverInterface::ShouldIgnoreEventsInBFCache();
```
WDYT?
Related details
- Johannes Henkel
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
Well, I have a similar concern with what Johannes said.
What could be the main goal of this change? AssertPLMO has been used to verify the event expectations, but now the tracker masked events and the AssertPLMO checks it. This is good to ensure the added mask behaviors, but another concern is this masks renderer side behaviors, and allows some ambiguities there.
Related details
- Johannes Henkel
- Ken Okada
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |