Add Startup.FirstWebContents FCP and LCP histograms [chromium/src : main]
Shunya Shishido (Gerrit)
Shunya Shishido added 5 comments![]( "Open in Gerrit")
Add Startup.FirstWebContents FCP and LCP histogramsCan you clarify what `FirstWebContents` stands for?
// User input finalizes LCP per the spec. Record it now so the histogram
// is available without waiting for the page load to complete.
RecordLargestContentfulPaint();Do we need this? It looks existing observers only implement OnComplete() and FlushMetricsOnAppEnterBackground() to record LCP.
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/observers/core/uma_page_load_metrics_observer.cc;l=878-888;drc=61757561ca2bbfee9b0d179dea1575a786749edc;bpv=1;bpt=1
expires_after="never">Please fix this INFO reported by Metrics: The expiry should only be set to "never" in rare cases. Please double-check that...
The expiry should only be set to "never" in rare cases. Please double-check that this use of "never" is appropriate: https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#Histogram-Expiry.
[Desktop] Time from navigation start to first contentful paint in the firstI wonder if this is recorded only on Desktop.
Related details
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
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. |
Chris Davis (Gerrit)
Chris Davis added 5 comments![]( "Open in Gerrit")
Can you clarify what `FirstWebContents` stands for?
Done
// User input finalizes LCP per the spec. Record it now so the histogram
// is available without waiting for the page load to complete.
RecordLargestContentfulPaint();Do we need this? It looks existing observers only implement OnComplete() and FlushMetricsOnAppEnterBackground() to record LCP.
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/observers/core/uma_page_load_metrics_observer.cc;l=878-888;drc=61757561ca2bbfee9b0d179dea1575a786749edc;bpv=1;bpt=1
According the the spec, input to page should trigger the LCP
Please fix this INFO reported by Metrics: The expiry should only be set to "never" in rare cases. Please double-check that...
The expiry should only be set to "never" in rare cases. Please double-check that this use of "never" is appropriate: https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#Histogram-Expiry.
We do not want this to expire like the other Startup.FirstWebContents.* histograms
[Desktop] Time from navigation start to first contentful paint in the firstI wonder if this is recorded only on Desktop.
In matching with other startup.firstwebcontents.* histograms I'm ensuring these are not fired on Android.
expires_after="never">Chris Davisditto
We do not want this to expire like the other Startup.FirstWebContents.* histograms
Related details
- Charles Harrison
- Francois Pierre Doray
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 2 comments![]( "Open in Gerrit")
// User input finalizes LCP per the spec. Record it now so the histogram
// is available without waiting for the page load to complete.
RecordLargestContentfulPaint();Chris DavisDo we need this? It looks existing observers only implement OnComplete() and FlushMetricsOnAppEnterBackground() to record LCP.
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/observers/core/uma_page_load_metrics_observer.cc;l=878-888;drc=61757561ca2bbfee9b0d179dea1575a786749edc;bpv=1;bpt=1
According the the spec, input to page should trigger the LCP
IIUC we stop "LCP candidate recording" when the browser receives the user input or scroll event. This is triggered in the renderer and managed by LargestContentfulPaintHandler. The actual finalized LCP is recorded in `OnComplete()` and `FlushMetricsOnAppEnterBackground()`, not in `OnFirstInputInPage()`.
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/common/page_load_metrics.mojom;l=102;drc=f9f51ed5ebb14a72e6b28fdd4b39e5def9fdf5b7;bpv=1;bpt=1
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/observers/core/largest_contentful_paint_handler.h;l=138;drc=f9f51ed5ebb14a72e6b28fdd4b39e5def9fdf5b7;bpv=1;bpt=1
expires_after="never">Chris DavisPlease fix this INFO reported by Metrics: The expiry should only be set to "never" in rare cases. Please double-check that...
The expiry should only be set to "never" in rare cases. Please double-check that this use of "never" is appropriate: https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#Histogram-Expiry.
We do not want this to expire like the other Startup.FirstWebContents.* histograms
In rare cases, the expiry can be set to “never”. This is used to denote metrics of critical importance that are, typically, used for other reports. For example, all metrics of the “heartbeat” are set to never expire. All metrics that never expire must have an XML comment describing why so that it can be audited in the future. Setting an expiry to “never” must be reviewed by chromium-met...@google.com.
I'm not sure if there is a legitimate reason not to set the expiry, but please explain why this shouldn't be expired and involve `chromium-met...@google.com` to the review.
Related details
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
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. |
Chris Davis (Gerrit)
Chris Davis added 2 comments![]( "Open in Gerrit")
// User input finalizes LCP per the spec. Record it now so the histogram
// is available without waiting for the page load to complete.
RecordLargestContentfulPaint();Chris DavisDo we need this? It looks existing observers only implement OnComplete() and FlushMetricsOnAppEnterBackground() to record LCP.
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/observers/core/uma_page_load_metrics_observer.cc;l=878-888;drc=61757561ca2bbfee9b0d179dea1575a786749edc;bpv=1;bpt=1
Shunya ShishidoAccording the the spec, input to page should trigger the LCP
IIUC we stop "LCP candidate recording" when the browser receives the user input or scroll event. This is triggered in the renderer and managed by LargestContentfulPaintHandler. The actual finalized LCP is recorded in `OnComplete()` and `FlushMetricsOnAppEnterBackground()`, not in `OnFirstInputInPage()`.
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/common/page_load_metrics.mojom;l=102;drc=f9f51ed5ebb14a72e6b28fdd4b39e5def9fdf5b7;bpv=1;bpt=1
https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/observers/core/largest_contentful_paint_handler.h;l=138;drc=f9f51ed5ebb14a72e6b28fdd4b39e5def9fdf5b7;bpv=1;bpt=1
Gotcha. I'll remove it.
expires_after="never">Chris DavisPlease fix this INFO reported by Metrics: The expiry should only be set to "never" in rare cases. Please double-check that...
The expiry should only be set to "never" in rare cases. Please double-check that this use of "never" is appropriate: https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#Histogram-Expiry.
Shunya ShishidoWe do not want this to expire like the other Startup.FirstWebContents.* histograms
In rare cases, the expiry can be set to “never”. This is used to denote metrics of critical importance that are, typically, used for other reports. For example, all metrics of the “heartbeat” are set to never expire. All metrics that never expire must have an XML comment describing why so that it can be audited in the future. Setting an expiry to “never” must be reviewed by chromium-met...@google.com.
I'm not sure if there is a legitimate reason not to set the expiry, but please explain why this shouldn't be expired and involve `chromium-met...@google.com` to the review.
I'm just going to put the expiration for 1 year for now. When following up with changes to make this a benchmark metric I will work with that alias to change it to never.
Related details
- Charles Harrison
- Francois Pierre Doray
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 2 comments![]( "Open in Gerrit")
class StartupPageLoadMetricsObserverThis observer only implements FCP and LCP for the first page load. Do you have a plan to add further metrics? If so, what kind of metrics are planned? If not, how about implementing the startup metrics on components/page_load_metrics/browser/observers/core/uma_page_load_metrics_observer.cc ? That would be much simpler rather than making a dedicated observer.
// NTP and WebUI) since the first startup page may be any of these.This could include WebUI but not rendered on WebContents e.g., icons. I assume recording them is not intended.
Related details
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
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. |
Chris Davis (Gerrit)
Chris Davis added 2 comments![]( "Open in Gerrit")
This observer only implements FCP and LCP for the first page load. Do you have a plan to add further metrics? If so, what kind of metrics are planned? If not, how about implementing the startup metrics on components/page_load_metrics/browser/observers/core/uma_page_load_metrics_observer.cc ? That would be much simpler rather than making a dedicated observer.
Done
// NTP and WebUI) since the first startup page may be any of these.This could include WebUI but not rendered on WebContents e.g., icons. I assume recording them is not intended.
- Charles Harrison
- Francois Pierre Doray
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Davis (Gerrit)
Message from Chris Davis![]( "Open in Gerrit")
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charles Harrison (Gerrit)
Charles Harrison added 1 comment![]( "Open in Gerrit")
// Only record LCP if FCP was recorded for the same page load.Are you sure this is true? i.e. that a page that reaches FCP is guaranteed to trigger LCP even if the tab is closed, etc.
Related details
- Chris Davis
- Francois Pierre Doray
- Shunya Shishido
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. |
Chris Davis (Gerrit)
Chris Davis voted Commit-Queue+1![]( "Open in Gerrit")
- Francois Pierre Doray
- Gabriel Charette
- Sean Maher
- Shunya Shishido
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. |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
// Only record LCP if FCP was recorded for the same page load.Are you sure this is true? i.e. that a page that reaches FCP is guaranteed to trigger LCP even if the tab is closed, etc.
- Charles Harrison
- Francois Pierre Doray
- Gabriel Charette
- Sean Maher
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 3 comments![]( "Open in Gerrit")
Thank you for patient with review iterations, but let me leave a few suggestions.
should_record_startup_lcp_ =If we could remove `should_record_startup_lcp_`, probably we can simplify something like:
```
if (startup_metric_utils::GetBrowser().ShouldLogStartupHistogram()) {
PAGE_LOAD_HISTOGRAM(StartupFCP);
}
```
// recorded for the same page load.Do we really need this condition? IMHO this makes the condition just complicated. Can't we just send FCP and LCP independently?
As you said FCP is normally recorded before LCP. If so, do we really need this kind of strict check?
Related details
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
- Gabriel Charette
- Sean Maher
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. |
Gabriel Charette (Gerrit)
Gabriel Charette added 1 comment![]( "Open in Gerrit")
.RecordFirstWebContentsFirstContentfulPaint(The "First web contents" is a specific concept, this seems like it just races too the "first" WebContents which hits FCP. That's not working as intended. The metric intends to only record for the "first web contents" or be abandoned (so it doesn't file an unfavorable result if the user re-navigates or hides the initial target).
See the NonEmptyPaint metric for required hooks.
Related details
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
- Sean Maher
Sean Maher (Gerrit)
Sean Maher added 1 comment![]( "Open in Gerrit")
I'll wait for the architecture in page_load_metrics to settle before reviewing startup_metric_utils, but histograms.xml lgtm
Related details
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
Chris Davis (Gerrit)
Chris Davis added 3 comments![]( "Open in Gerrit")
If we could remove `should_record_startup_lcp_`, probably we can simplify something like:
```
if (startup_metric_utils::GetBrowser().ShouldLogStartupHistogram()) {
PAGE_LOAD_HISTOGRAM(StartupFCP);
}
```
Done
The "First web contents" is a specific concept, this seems like it just races too the "first" WebContents which hits FCP. That's not working as intended. The metric intends to only record for the "first web contents" or be abandoned (so it doesn't file an unfavorable result if the user re-navigates or hides the initial target).
See the NonEmptyPaint metric for required hooks.
Ok I did another redesign of this change. Please take a look.
Do we really need this condition? IMHO this makes the condition just complicated. Can't we just send FCP and LCP independently?
As you said FCP is normally recorded before LCP. If so, do we really need this kind of strict check?
- Charles Harrison
- Francois Pierre Doray
- Gabriel Charette
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charles Harrison (Gerrit)
Charles Harrison added 1 comment![]( "Open in Gerrit")
// Only record LCP if FCP was recorded for the same page load.Chris DavisAre you sure this is true? i.e. that a page that reaches FCP is guaranteed to trigger LCP even if the tab is closed, etc.
Yes FCP should be recorded before LCP.
That's not exactly the question I asked. I mean is it ever possible to get into a state where FPC is recorded for a page and LCP is not recorded?
If so, the change is misleading because we could log lcp on the next page load.
Related details
- Chris Davis
- Francois Pierre Doray
- Gabriel Charette
- Shunya Shishido
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. |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
// Only record LCP if FCP was recorded for the same page load.Chris DavisAre you sure this is true? i.e. that a page that reaches FCP is guaranteed to trigger LCP even if the tab is closed, etc.
Charles HarrisonYes FCP should be recorded before LCP.
That's not exactly the question I asked. I mean is it ever possible to get into a state where FPC is recorded for a page and LCP is not recorded?
If so, the change is misleading because we could log lcp on the next page load.
Correct - they were not tied together. Updated change connects them.
Related details
- Charles Harrison
- Francois Pierre Doray
- Gabriel Charette
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Gabriel Charette added 2 comments![]( "Open in Gerrit")
// timing, guarded by the one-shot flag in startup_metric_utils.I don't understand how this magically makes the other code hook to the FirstWebContentsProfiler?
<owner>g...@chromium.org</owner>Mark yourself as owner as well 😊
Related details
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
- Shunya Shishido
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. |
Chris Davis (Gerrit)
Chris Davis added 2 comments![]( "Open in Gerrit")
// timing, guarded by the one-shot flag in startup_metric_utils.I don't understand how this magically makes the other code hook to the FirstWebContentsProfiler?
Looks like this can be completely removed as it is an artifact of a previous implementation. Thanks.
Mark yourself as owner as well 😊
- Charles Harrison
- Francois Pierre Doray
- Gabriel Charette
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido added 2 comments![]( "Open in Gerrit")
// The navigation start of the page that recorded FCP. Used to ensure LCP
// comes from the same page load.
base::TimeTicks startup_fcp_navigation_start_;I'm less confident this is appropriate. Can we use other existing info e.g., `is_first_run_` to only track the first web contents?
base::TimeTicks now) {`now` is misleading. This is the time ticks of FCP completion.
Related details
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
- Gabriel Charette
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. |
Chris Davis (Gerrit)
Chris Davis added 2 comments![]( "Open in Gerrit")
// The navigation start of the page that recorded FCP. Used to ensure LCP
// comes from the same page load.
base::TimeTicks startup_fcp_navigation_start_;I'm less confident this is appropriate. Can we use other existing info e.g., `is_first_run_` to only track the first web contents?
Unfortunately no. is_first_run_ is if this is the first time the browser has ever been launched - not just the first browser instance of this session
`now` is misleading. This is the time ticks of FCP completion.
- Charles Harrison
- Francois Pierre Doray
- Gabriel Charette
- Shunya Shishido
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shunya Shishido (Gerrit)
Shunya Shishido voted and added 2 comments![]( "Open in Gerrit")
Votes added by Shunya Shishido
| Code-Review | +1 |
2 comments
page_load_metrics LGTM w/ nit.
For other changes, please get code review from code owners.
[Desktop] Time from navigation start to first contentful paint in the firstI wonder if this is recorded only on Desktop.
In matching with other startup.firstwebcontents.* histograms I'm ensuring these are not fired on Android.
- Charles Harrison
- Chris Davis
- Francois Pierre Doray
- Gabriel Charette
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. |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
[Desktop] Time from navigation start to first contentful paint in the firstChris DavisI wonder if this is recorded only on Desktop.
Shunya ShishidoIn matching with other startup.firstwebcontents.* histograms I'm ensuring these are not fired on Android.
The latest patch looks not limited to Desktop again?
- Charles Harrison
- Francois Pierre Doray
- Gabriel Charette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Davis (Gerrit)
Chris Davis added 1 comment![]( "Open in Gerrit")
@Sean Maher - can you review the startup_metrics changes?
Related details
- Charles Harrison
- Francois Pierre Doray
- Gabriel Charette
- Sean Maher