Add PrewarmPrerenderCoverageStatus metric and fix prewarm telemetry [chromium/src : main]
Jiacheng Guo (Gerrit)
Jiacheng Guo has uploaded the change for review![]( "Open in Gerrit")
Commit message
Add PrewarmPrerenderCoverageStatus metric and fix prewarm telemetry
Introduce a two-tiered prewarm state tracking mechanism to resolve
telemetry blind spots during in-flight process reuse, process allocation
misses, and survivorship bias for DSE prewarming and prerendering.
This change records PageLoad.Clients.GoogleSearch.
PrewarmPrerenderCoverageStatus representing 11 coverage outcomes
(including in-flight prewarm reuse, committed prewarm reuse, prerender
host reuse, and process allocation misses). The legacy metrics
PrewarmNavigationStatus2 and HadPriorPrewarmCommitStatus2 are deprecated.
OBSOLETE_HISTOGRAM[PageLoad.Clients.GoogleSearch.Prerender.PrewarmNavigationStatus2{InitiatorOfNavigation}{PrerenderStatus}]=Replaced by PageLoad.Clients.GoogleSearch.PrewarmPrerenderCoverageStatus{InitiatorOfNavigation}{PrerenderStatus}
OBSOLETE_HISTOGRAM[PageLoad.Clients.GoogleSearch.HadPriorPrewarmCommitStatus2{InitiatorOfNavigation}{PrerenderStatus}]=Replaced by PageLoad.Clients.GoogleSearch.PrewarmPrerenderCoverageStatus{InitiatorOfNavigation}{PrerenderStatus}
Change diff
Change information
- M chrome/browser/page_load_metrics/observers/gws_page_load_metrics_observer_unittest.cc
- M chrome/browser/preloading/prerender/prerender_manager.cc
- M chrome/browser/preloading/prerender/search_prewarm_progress_service.cc
- M chrome/browser/preloading/prerender/search_prewarm_progress_service.h
- M components/page_load_metrics/google/browser/BUILD.gn
- M components/page_load_metrics/google/browser/gws_page_load_metrics_observer.cc
- M components/page_load_metrics/google/browser/gws_page_load_metrics_observer.h
- D components/page_load_metrics/google/browser/prerender_prewarm_navigation_data.cc
- D components/page_load_metrics/google/browser/prerender_prewarm_navigation_data.h
- A components/page_load_metrics/google/browser/prerender_prewarm_process_data.cc
- A components/page_load_metrics/google/browser/prerender_prewarm_process_data.h
- A components/page_load_metrics/google/browser/search_prewarm_types.cc
- A components/page_load_metrics/google/browser/search_prewarm_types.h
- M tools/metrics/histograms/metadata/page/enums.xml
- M tools/metrics/histograms/metadata/page/histograms.xml
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jiacheng Guo (Gerrit)
Jiacheng Guo added 11 comments![]( "Open in Gerrit")
Please fix this WARNING reported by Metrics: An obsoletion message has been added to following histograms: PageLoad.Clients.G...
An obsoletion message has been added to following histograms: PageLoad.Clients.GoogleSearch.Prerender.PrewarmNavigationStatus2{InitiatorOfNavigation}{PrerenderStatus}, PageLoad.Clients.GoogleSearch.HadPriorPrewarmCommitStatus2{InitiatorOfNavigation}{PrerenderStatus}, but they are not removed. Please double check if there're typos.
SearchPrewarmProgressServiceFactory::GetForProfile(profile)) {If we are tracking prerender as well in `SearchPrewarmProgressServiceFactory`, consider renaming it.
web_contents(), features::kPrerender2ReuseSearchResultHost.Get());Checking only the feature is not correct, we should check if prewarm host is really reused.
content::RenderProcessHost* rph =The SiteInstance won't be necessarily present for the navigation request when it is initiated by prerendering. Also the bound process may change when we receive the response. We may first tie the `PrerenderPrewarmProcessData` to the NavigationRequest and then set the data to the renderer process in RenderFrameHostManager::GetFrameHostForNavigation. Please also add comments here about this mechanism.
void OnSearchPrewarmStarted(content::PrerenderHostId host_id);Remove this old API.
if (web_contents) {Are there cases where web_contents can be nullptr? If not, we should convert this to a CHECK.
if (web_contents) {Are there cases where web_contents can be nullptr? If not, we should convert this to a CHECK.
page_load_metrics::SearchPrewarmPrerenderCoverageStatus::The reused prerender host doesn't necessarily come from prewarm. We may rename `kPrerenderActivated_WithPrewarmHostReuse` to `kPrerenderActivated_WithPrerenderHostReuse`.
content::SiteInstanceProcessAssignment::REUSED_EXISTING_PROCESS;Reusing a renderer process doesn't necessarily mean it comes from a prewarm or prerender (it can be spare process or kept-alive process). We need to further track the source of the reuse process.
If tracking these source is too difficult, then maybe we can only track the cases where we fail to reuse any process and have one single UMA entry for prerender not activated but process reused.
auto* prewarm_process_data =Overall is checking process level tracking data still required if we can further track down the reuse source of the reused renderer process? Then we can simply log whether we have attempted a prewarm and whether the prewarmed process is reused for the actual navigation.
bool prewarm_attempted() const { return prewarm_attempted_; }Maybe `search_prewarm` and `search_prerender` rather than only `prewarm` and `prerender`.
Related details
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. |
Jiacheng Guo (Gerrit)
Jiacheng Guo added 10 comments![]( "Open in Gerrit")
Please fix this WARNING reported by Metrics: An obsoletion message has been added to following histograms: PageLoad.Clients.G...
An obsoletion message has been added to following histograms: PageLoad.Clients.GoogleSearch.Prerender.PrewarmNavigationStatus2{InitiatorOfNavigation}{PrerenderStatus}, PageLoad.Clients.GoogleSearch.HadPriorPrewarmCommitStatus2{InitiatorOfNavigation}{PrerenderStatus}, but they are not removed. Please double check if there're typos.
Done
Add PrewarmPrerenderCoverageStatus metric and fix prewarm telemetryOverall comment: This has become a very large change. Can we split it a bit. Maybe first a CL for renaming search prewarm service. Then a CL for fixing the prewarm data missing in the renderer process user data before prewarm commit. Then another CL for recording whether prerender reuses the prewarm process. Finally a CL for adding the metrics.
SearchPrewarmProgressServiceFactory::GetForProfile(profile)) {If we are tracking prerender as well in `SearchPrewarmProgressServiceFactory`, consider renaming it.
Done
web_contents(), features::kPrerender2ReuseSearchResultHost.Get());Checking only the feature is not correct, we should check if prewarm host is really reused.
Done
The SiteInstance won't be necessarily present for the navigation request when it is initiated by prerendering. Also the bound process may change when we receive the response. We may first tie the `PrerenderPrewarmProcessData` to the NavigationRequest and then set the data to the renderer process in RenderFrameHostManager::GetFrameHostForNavigation. Please also add comments here about this mechanism.
Done
? navigation_handle.GetStartingSiteInstance()->GetProcess()(Same for AttachSearchPrerenderNavigationHandleUserData): We are assuming the renderer process and the SiteInstance is reused in the navigation for the prerender process. In practice, this is correct since we are always starting from either a blank page or a same-origin reused page. However this assumption is implicit and dangerous. Can we consider moving this logic to PrerenderHost so that the assumption becomes explicit. We need to add comments there and add logic to ensure the process stays the same during navigation.
page_load_metrics::SearchPrewarmPrerenderCoverageStatus::The reused prerender host doesn't necessarily come from prewarm. We may rename `kPrerenderActivated_WithPrewarmHostReuse` to `kPrerenderActivated_WithPrerenderHostReuse`.
Done
content::SiteInstanceProcessAssignment::REUSED_EXISTING_PROCESS;Reusing a renderer process doesn't necessarily mean it comes from a prewarm or prerender (it can be spare process or kept-alive process). We need to further track the source of the reuse process.
If tracking these source is too difficult, then maybe we can only track the cases where we fail to reuse any process and have one single UMA entry for prerender not activated but process reused.
Done
Overall is checking process level tracking data still required if we can further track down the reuse source of the reused renderer process? Then we can simply log whether we have attempted a prewarm and whether the prewarmed process is reused for the actual navigation.
Done
bool prewarm_attempted() const { return prewarm_attempted_; }Maybe `search_prewarm` and `search_prerender` rather than only `prewarm` and `prerender`.
Done
Related details
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. |
Jiacheng Guo (Gerrit)
Jiacheng Guo added 3 comments![]( "Open in Gerrit")
void OnSearchPrewarmStarted(content::PrerenderHostId host_id);Jiacheng GuoRemove this old API.
Done
Are there cases where web_contents can be nullptr? If not, we should convert this to a CHECK.
Done
Are there cases where web_contents can be nullptr? If not, we should convert this to a CHECK.
Done
Related details
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. |