Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2498923002:
Add UMA to measure how often pages don't receive any user input (Closed)
|
Created:
4 years, 1 month ago by Kunihiko Sakamoto Modified:
4 years, 1 month ago CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA to measure how often pages don't receive any user input
This adds UMA that records the status of possible first-meaningful-paint
signals (network stable and user input), and breakdown histograms of
current first-meaningful-paint, by the presence or absence of user input.
BUG=632081
Committed: https://crrev.com/9b17cf9455970a7bfa07bc2cc26b9924f8dd3046
Cr-Commit-Position: refs/heads/master@{#433141}
Patch Set 1 #
Total comments: 2
Patch Set 2 : use enum #
Total comments: 6
Patch Set 3 : FMP breakdown by user input #
Total comments: 2
Patch Set 4 : 4-state enum #
Total comments: 2
Patch Set 5 : temporary variable #
Messages
Total messages: 41 (20 generated)
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add HadUserInputAfterFirstPaint UMA BUG= ========== to ========== Add UMA to measure how often pages don't receive any user input This adds a boolean UMA that records whether the user had any interaction on the page after first paint. This is to investigate the feasibility of reporting first meaningful paint upon first user input. BUG=632081 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add UMA to measure how often pages don't receive any user input This adds a boolean UMA that records whether the user had any interaction on the page after first paint. This is to investigate the feasibility of reporting first meaningful paint upon first user input. BUG=632081 ========== to ========== Add UMA to measure how often pages don't receive any user input This adds a boolean UMA that records whether the user had any interaction on the page. This is to investigate the feasibility of reporting first meaningful paint upon first user input. BUG=632081 ==========
ksakamoto@chromium.org changed reviewers: + bmcquade@chromium.org, isherman@chromium.org
LGTM % a nit: https://codereview.chromium.org/2498923002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2498923002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:41230: + enum="Boolean"> nit: Please use a custom enum here, with labels like "No input" and "Had input".
https://codereview.chromium.org/2498923002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2498923002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:41230: + enum="Boolean"> On 2016/11/14 23:00:32, Ilya Sherman wrote: > nit: Please use a custom enum here, with labels like "No input" and "Had input". Done.
https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:576: UMA_HISTOGRAM_ENUMERATION( can we also have 2 separate histograms, one for FMP estimate when there is no input, and another for FMP when there was input? I'd like to get insight into both the absolute count of how many pages don't have input, as well as whether their current FMP calculations have a similar or different distribution. Is it straightforward to add both of these histograms to the existing implementation?
https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:576: UMA_HISTOGRAM_ENUMERATION( You can still use an UMA_HISTOGRAM_BOOLEAN in the C++ code -- my comment was just about the histograms.xml labels.
https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:576: UMA_HISTOGRAM_ENUMERATION( On 2016/11/16 06:30:30, Ilya Sherman wrote: > You can still use an UMA_HISTOGRAM_BOOLEAN in the C++ code -- my comment was > just about the histograms.xml labels. Oh, got it. Changed back to UMA_HISTOGRAM_BOOLEAN. https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:576: UMA_HISTOGRAM_ENUMERATION( On 2016/11/15 13:27:36, Bryan McQuade wrote: > can we also have 2 separate histograms, one for FMP estimate when there is no > input, and another for FMP when there was input? I'd like to get insight into > both the absolute count of how many pages don't have input, as well as whether > their current FMP calculations have a similar or different distribution. Is it > straightforward to add both of these histograms to the existing implementation? Added breakdown histograms of current FMP, by the presence or absence of user input. Note that this is logged only if the page reached network-stable. There are two other groups not covered: - Page didn't reach network stable, but had user input This is the group I want to cover by the user-input based FMP logging. Logging this requires some plumbing from blink side. - Page didn't reach network stable, and no user input No signal that the page had FMP. The boolean histogram this CL adds will help estimating the size of these groups.
Description was changed from ========== Add UMA to measure how often pages don't receive any user input This adds a boolean UMA that records whether the user had any interaction on the page. This is to investigate the feasibility of reporting first meaningful paint upon first user input. BUG=632081 ========== to ========== Add UMA to measure how often pages don't receive any user input This adds UMA that records the status of possible first-meaningful-paint signals (network stable and user input), and breakdown histograms of current first-meaningful-paint, by the presence or absence of user input. BUG=632081 ==========
PTAL https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:576: UMA_HISTOGRAM_ENUMERATION( On 2016/11/16 08:43:46, Kunihiko Sakamoto wrote: > On 2016/11/16 06:30:30, Ilya Sherman wrote: > > You can still use an UMA_HISTOGRAM_BOOLEAN in the C++ code -- my comment was > > just about the histograms.xml labels. > > Oh, got it. Changed back to UMA_HISTOGRAM_BOOLEAN. Instead of a boolean histogram, changed to log an enumeration histogram that logs 4 possible signal state: (had user input or not) x (reached network stable or not)
https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2498923002/diff/20001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:576: UMA_HISTOGRAM_ENUMERATION( On 2016/11/16 10:03:28, Kunihiko Sakamoto wrote: > On 2016/11/16 08:43:46, Kunihiko Sakamoto wrote: > > On 2016/11/16 06:30:30, Ilya Sherman wrote: > > > You can still use an UMA_HISTOGRAM_BOOLEAN in the C++ code -- my comment was > > > just about the histograms.xml labels. > > > > Oh, got it. Changed back to UMA_HISTOGRAM_BOOLEAN. > > Instead of a boolean histogram, changed to log an enumeration histogram that > logs 4 possible signal state: > (had user input or not) x (reached network stable or not) Hmm, I still see a boolean histogram in the code. Did you mean to upload a new patch set, or are you indeed intending to record a boolean histogram? (Just checking because the code doesn't seem to match what you wrote in this comment.)
this looks good - let's just move where we are logging the 2 new histograms. https://codereview.chromium.org/2498923002/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2498923002/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:579: if (timing.first_meaningful_paint) { let's log this in the OnFirstMeaningfulPaint callback. logging here is more susceptible to data loss due to process kills, crashes, etc.
Oops sorry, I forgot to upload the last patch. PTAL. https://codereview.chromium.org/2498923002/diff/40001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2498923002/diff/40001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:579: if (timing.first_meaningful_paint) { On 2016/11/16 23:59:20, Bryan McQuade wrote: > let's log this in the OnFirstMeaningfulPaint callback. logging here is more > susceptible to data loss due to process kills, crashes, etc. I put this here so that it captures how often page load doesn't have any user input. If we moved this to OnFirstMeaningfulPaint, it would count the number of page loads that had user input _before the network stable_.
On 2016/11/17 at 01:42:26, ksakamoto wrote: > Oops sorry, I forgot to upload the last patch. > PTAL. > > https://codereview.chromium.org/2498923002/diff/40001/chrome/browser/page_loa... > File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): > > https://codereview.chromium.org/2498923002/diff/40001/chrome/browser/page_loa... > chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:579: if (timing.first_meaningful_paint) { > On 2016/11/16 23:59:20, Bryan McQuade wrote: > > let's log this in the OnFirstMeaningfulPaint callback. logging here is more > > susceptible to data loss due to process kills, crashes, etc. > > I put this here so that it captures how often page load doesn't have any user input. If we moved this to OnFirstMeaningfulPaint, it would count the number of page loads that had user input _before the network stable_. Ah, makes sense, thanks!
lgtm
The CQ bit was checked by ksakamoto@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
metrics lgtm https://codereview.chromium.org/2498923002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2498923002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:587: FIRST_MEANINGFUL_PAINT_SIGNAL_STATUS_LAST_ENTRY); nit: I find this macro invocation fairly hard to parse, and think it would benefit from moving the metric computation into a temporary variable.
https://codereview.chromium.org/2498923002/diff/60001/chrome/browser/page_loa... File chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2498923002/diff/60001/chrome/browser/page_loa... chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:587: FIRST_MEANINGFUL_PAINT_SIGNAL_STATUS_LAST_ENTRY); On 2016/11/17 22:28:09, Ilya Sherman wrote: > nit: I find this macro invocation fairly hard to parse, and think it would > benefit from moving the metric computation into a temporary variable. Done.
The CQ bit was checked by ksakamoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bmcquade@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2498923002/#ps80001 (title: "temporary variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ksakamoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ksakamoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add UMA to measure how often pages don't receive any user input This adds UMA that records the status of possible first-meaningful-paint signals (network stable and user input), and breakdown histograms of current first-meaningful-paint, by the presence or absence of user input. BUG=632081 ========== to ========== Add UMA to measure how often pages don't receive any user input This adds UMA that records the status of possible first-meaningful-paint signals (network stable and user input), and breakdown histograms of current first-meaningful-paint, by the presence or absence of user input. BUG=632081 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA to measure how often pages don't receive any user input This adds UMA that records the status of possible first-meaningful-paint signals (network stable and user input), and breakdown histograms of current first-meaningful-paint, by the presence or absence of user input. BUG=632081 ========== to ========== Add UMA to measure how often pages don't receive any user input This adds UMA that records the status of possible first-meaningful-paint signals (network stable and user input), and breakdown histograms of current first-meaningful-paint, by the presence or absence of user input. BUG=632081 Committed: https://crrev.com/9b17cf9455970a7bfa07bc2cc26b9924f8dd3046 Cr-Commit-Position: refs/heads/master@{#433141} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9b17cf9455970a7bfa07bc2cc26b9924f8dd3046 Cr-Commit-Position: refs/heads/master@{#433141} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
