Add ReloadPageLoadMetricsObserver to have a metrics variant for reloads

Add ReloadPageLoadMetricsObserver to have a metrics variant only reloads

To measure reload experiments impact, this change adds
ReloadPageLoadMetricsObserver to report TTFCP variants only for
reload-related page loadings.

BUG=607063
TBR=isherman@chromium.org

Committed: https://crrev.com/d056525caaa71451d25f6113895b376e47177f6e
Cr-Commit-Position: refs/heads/master@{#396395}

[Takashi Toyoshima, 2016-05-25 09:08:25]

Description was changed from

==========
Add ReloadPageLoadMetricsObserver to have a metrics variant only for reloads

To measure reload experiments impact, this change adds
ReloadPageLoadMetricsObserver to report TTFCP variant only for
reload-related page loadings.

BUG=607063
==========

to

==========
Add ReloadPageLoadMetricsObserver to have a metrics variant only reloads

To measure reload experiments impact, this change adds
ReloadPageLoadMetricsObserver to report TTFCP variant only for
reload-related page loadings.

BUG=607063
==========

[Takashi Toyoshima, 2016-05-25 09:12:39]

toyoshim@chromium.org changed reviewers:
+ csharrison@chromium.org, isherman@chromium.org

[Bryan McQuade, 2016-05-25 22:53:28]

bmcquade@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Bryan McQuade, 2016-05-25 22:53:29]

https://codereview.chromium.org/2008233002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2008233002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc:16:
"PageLoad.Reload.NavigationToFirstContentfulPaint";
maybe PageLoad.PaintTiming.NavigationToFirstContentfulPaint.Reload?

should we use 'PageLoad.Clients' here? it's long, but it's consistent.

csharrison, wdyt?

https://codereview.chromium.org/2008233002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc:25:
page_transition_was_reload_ = ui::PageTransitionCoreTypeIs(
if it's accurate, it'd be useful to store a variant of this metric for user
gesture reloads by storing the value of navigation_handle->HasUserGesture. That
way we avoid including automatic reloads for pages that reload in a tight loop.

https://codereview.chromium.org/2008233002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2008233002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:36158: +<histogram
name="PageLoad.Reload.NavigationToFirstContentfulPaint" units="ms">
if we switch to PageLoad.Reload.PaintTiming.NavigationToFirstContentfulPaint or
PageLoad.PaintTiming.NavigationToFirstContentfulPaint.Reload, you can use a
histogram suffix instead.

[Ilya Sherman, 2016-05-25 23:13:01]

histograms lgtm % nits:

https://chromiumcodereview.appspot.com/2008233002/diff/1/tools/metrics/histog...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/2008233002/diff/1/tools/metrics/histog...
tools/metrics/histograms/histograms.xml:36162: +    The time from navigation
start to first "contentful" paint only on
nit: "paint only" -> "paint. Only recorded"

https://chromiumcodereview.appspot.com/2008233002/diff/1/tools/metrics/histog...
tools/metrics/histograms/histograms.xml:36163: +    pages that are initiated by
one of reload operations.
nit: "by one of reload operations" -> "by a reload operation" (or just "reload",
or "reload action", I'm not sure what the best term for that is).

[Takashi Toyoshima, 2016-05-26 06:06:50]

Thanks.

I need csharrison's help to decide metric names and to describe them in
histograms.xml with suffix rules.

https://codereview.chromium.org/2008233002/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2008233002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc:16:
"PageLoad.Reload.NavigationToFirstContentfulPaint";
OK, I'll update my CL to use following names tentatively.
 -PageLoad.Clients.Reload.PaintTiming.NavigationToFirstContentfulPaint
 -PageLoad.Clients.Reload.PaintTiming.NavigationToFirstContentfulPaint.ByGesture

csharrison, please let me know if you have any idea.

https://codereview.chromium.org/2008233002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc:25:
page_transition_was_reload_ = ui::PageTransitionCoreTypeIs(
On 2016/05/25 22:53:29, Bryan McQuade wrote:
> if it's accurate, it'd be useful to store a variant of this metric for user
> gesture reloads by storing the value of navigation_handle->HasUserGesture.
That
> way we avoid including automatic reloads for pages that reload in a tight
loop.

Done.

https://codereview.chromium.org/2008233002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2008233002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:36158: +<histogram
name="PageLoad.Reload.NavigationToFirstContentfulPaint" units="ms">
This is my first time to use suffix rules in the histograms.xml, but hum....
format isn't clear enough for me to use it for my cases.

Here are some questions:
- PageLoadMetricsClients histogram_suffixes tag contains mixed lines of
affected-histogram tags and suffix tags in random order. How each instance
affects each other?
- I could not find a document for with-suffix tag, but how can I use it to have
"foo.bar" and "foo.bar.suffix".
- Header document says we can use histogram_suffixes recursively with max depth
limit 5. But, how can I use it recursively? If I can not use with-suffix for the
previous case, I'll use it recursively to add a suffix.
- Is there any way to dump preprocessed xml that expand suffix related tags?
- I can not find most suffixed metric names in the UMA dashboard. Why?

https://codereview.chromium.org/2008233002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2008233002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:94929: +<histogram_suffixes
name="PageLoadMetricsClients.Reload" separator="."
I created a separate histogram_suffixes tag to avoid being affected by unrelated
affected-histogram tags.

https://codereview.chromium.org/2008233002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:94934: +     
name="PageLoad.PaintTiming.NavigationToFirstContentfulPaint"/>
I also should have -.ByGesture here. But I could not find the best practice
here. I guess with-suffix tag or recursive histogram_suffixes is a solution, but
usage was unclear for me.

[Bryan McQuade, 2016-05-26 10:43:34]

LGTM, thanks for this addition! This looks good to me (with one small rename
request), but I defer to Charles for final call.

https://codereview.chromium.org/2008233002/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2008233002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc:19:
"ByGesture";
I like these names. One small request: let's use '.UserGesture' rather than
'.ByGesture'.

https://codereview.chromium.org/2008233002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2008233002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:94934: +     
name="PageLoad.PaintTiming.NavigationToFirstContentfulPaint"/>
On 2016/05/26 at 06:06:50, Takashi Toyoshima wrote:
> I also should have -.ByGesture here. But I could not find the best practice
here. I guess with-suffix tag or recursive histogram_suffixes is a solution, but
usage was unclear for me.

You can create a second suffix for it, with affected-histogram
"PageLoad.Clients.Reload.PaintTiming.NavigationToFirstContentfulPaint"

[Charlie Harrison, 2016-05-26 21:41:09]

lgtm

https://codereview.chromium.org/2008233002/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2008233002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc:19:
"ByGesture";
On 2016/05/26 10:43:34, Bryan McQuade wrote:
> I like these names. One small request: let's use '.UserGesture' rather than
> '.ByGesture'.

Agreed.

[Takashi Toyoshima, 2016-05-27 03:07:22]

Description was changed from

==========
Add ReloadPageLoadMetricsObserver to have a metrics variant only reloads

To measure reload experiments impact, this change adds
ReloadPageLoadMetricsObserver to report TTFCP variant only for
reload-related page loadings.

BUG=607063
==========

to

==========
Add ReloadPageLoadMetricsObserver to have a metrics variant only reloads

To measure reload experiments impact, this change adds
ReloadPageLoadMetricsObserver to report TTFCP variants only for
reload-related page loadings.

BUG=607063
TBR=isherman@chromium.org
==========

[Takashi Toyoshima, 2016-05-27 03:09:48]

Thanks.

Let me move Ilya to TBR since code was changed after his LGTM but he is OOO for
a while.

https://codereview.chromium.org/2008233002/diff/20001/chrome/browser/page_loa...
File
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2008233002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/reload_page_load_metrics_observer.cc:19:
"ByGesture";
On 2016/05/26 10:43:34, Bryan McQuade wrote:
> I like these names. One small request: let's use '.UserGesture' rather than
> '.ByGesture'.

Done.

https://codereview.chromium.org/2008233002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2008233002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:94934: +     
name="PageLoad.PaintTiming.NavigationToFirstContentfulPaint"/>
Thanks. Let me try it.

[Takashi Toyoshima, 2016-05-27 03:10:02]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2016-05-27 03:10:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, bmcquade@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2008233002/#ps40001
(title: "review #7, #8")

[commit-bot: I haz the power, 2016-05-27 03:10:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2008233002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2008233002/40001

[commit-bot: I haz the power, 2016-05-27 03:56:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 03:56:28]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Takashi Toyoshima, 2016-05-27 04:22:19]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2016-05-27 04:22:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2008233002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2008233002/40001

[commit-bot: I haz the power, 2016-05-27 04:57:41]

Description was changed from

==========
Add ReloadPageLoadMetricsObserver to have a metrics variant only reloads

To measure reload experiments impact, this change adds
ReloadPageLoadMetricsObserver to report TTFCP variants only for
reload-related page loadings.

BUG=607063
TBR=isherman@chromium.org
==========

to

==========
Add ReloadPageLoadMetricsObserver to have a metrics variant only reloads

To measure reload experiments impact, this change adds
ReloadPageLoadMetricsObserver to report TTFCP variants only for
reload-related page loadings.

BUG=607063
TBR=isherman@chromium.org
==========

[commit-bot: I haz the power, 2016-05-27 04:57:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-27 04:58:44]

Description was changed from

==========
Add ReloadPageLoadMetricsObserver to have a metrics variant only reloads

To measure reload experiments impact, this change adds
ReloadPageLoadMetricsObserver to report TTFCP variants only for
reload-related page loadings.

BUG=607063
TBR=isherman@chromium.org
==========

to

==========
Add ReloadPageLoadMetricsObserver to have a metrics variant only reloads

To measure reload experiments impact, this change adds
ReloadPageLoadMetricsObserver to report TTFCP variants only for
reload-related page loadings.

BUG=607063
TBR=isherman@chromium.org

Committed: https://crrev.com/d056525caaa71451d25f6113895b376e47177f6e
Cr-Commit-Position: refs/heads/master@{#396395}
==========

[commit-bot: I haz the power, 2016-05-27 04:58:45]

Patchset 3 (id:??) landed as
https://crrev.com/d056525caaa71451d25f6113895b376e47177f6e
Cr-Commit-Position: refs/heads/master@{#396395}