Add immediate variants of core page load metric histograms.

Add immediate variants of core page load metric histograms.

BUG=611811
Committed: https://crrev.com/5f481ec582761cec96089b8520e00bb70db14798
Cr-Commit-Position: refs/heads/master@{#394050}

[Bryan McQuade, 2016-05-13 18:21:22]

Description was changed from

==========
Add immediate variants of core page load metric histograms.

BUG=
==========

to

==========
Add immediate variants of core page load metric histograms.

BUG=611811
==========

[Bryan McQuade, 2016-05-13 18:26:42]

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

[Bryan McQuade, 2016-05-13 18:27:15]

PTAL. This adds a bunch of histograms, but I want to make sure we have all this
data (including background histograms to look for mix shift effects) when
analyzing the impact of finch trials.

[Charlie Harrison, 2016-05-13 20:07:55]

Looks ok, but I'm wondering about the histogram naming. AFAIK the plan is to
evaluate these semi-experimental metrics, and eventually replace our current
metrics with these.

If that's the case, then I really think versioning these with the eventual name
that the replacement histograms will have is the way to go. I don't think we
want the default core PLM histograms to all have an "Immediate" suffix.

Can we at least do one of:
1. Replace the core metrics wholesale (I know you don't want to do this)
2. Make our future transition easier wrt naming

I recognize that versioning the core names are a bit harder, because they are
not PLM clients. Maybe these new metrics can have a new "Core" prefix?

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

https://codereview.chromium.org/1979633002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:105:
// Immediate histogram variants.
Can you add a note that these are planned to replace the older metrics, with the
reasoning why you aren't doing the full replacement now?

[Bryan McQuade, 2016-05-16 01:16:53]

On 2016/05/13 at 20:07:55, csharrison wrote:
> Looks ok, but I'm wondering about the histogram naming. AFAIK the plan is to
evaluate these semi-experimental metrics, and eventually replace our current
metrics with these.
> 
> If that's the case, then I really think versioning these with the eventual
name that the replacement histograms will have is the way to go. I don't think
we want the default core PLM histograms to all have an "Immediate" suffix.
> 
> Can we at least do one of:
> 1. Replace the core metrics wholesale (I know you don't want to do this)
> 2. Make our future transition easier wrt naming
> 
> I recognize that versioning the core names are a bit harder, because they are
not PLM clients. Maybe these new metrics can have a new "Core" prefix?
> 
>
https://codereview.chromium.org/1979633002/diff/20001/chrome/browser/page_loa...
> File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):
> 
>
https://codereview.chromium.org/1979633002/diff/20001/chrome/browser/page_loa...
>
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:105:
// Immediate histogram variants.
> Can you add a note that these are planned to replace the older metrics, with
the reasoning why you aren't doing the full replacement now?

I don't think we can replace the core metrics. Some of our metrics are part of
system health council and others are starting to depend on them, so we'll need
to communicate a deprecation plan. My current thinking is that we'll stop
logging the old metrics and deprecate them in M54, which gives us a few releases
of overlap.

I took this opportunity to choose better name categories. We have
PageLoad.AbortTiming already. I've now broken the metrics that were in Timing2
into 3 new categories: PageLoad.DocumentTiming, PageLoad.PaintTiming, and
PageLoad.ParseTiming. Take a look and see what you think.

[Bryan McQuade, 2016-05-16 01:34:10]

On 2016/05/16 at 01:16:53, Bryan McQuade wrote:
> On 2016/05/13 at 20:07:55, csharrison wrote:
> > Looks ok, but I'm wondering about the histogram naming. AFAIK the plan is to
evaluate these semi-experimental metrics, and eventually replace our current
metrics with these.
> > 
> > If that's the case, then I really think versioning these with the eventual
name that the replacement histograms will have is the way to go. I don't think
we want the default core PLM histograms to all have an "Immediate" suffix.
> > 
> > Can we at least do one of:
> > 1. Replace the core metrics wholesale (I know you don't want to do this)
> > 2. Make our future transition easier wrt naming
> > 
> > I recognize that versioning the core names are a bit harder, because they
are not PLM clients. Maybe these new metrics can have a new "Core" prefix?
> > 
> >
https://codereview.chromium.org/1979633002/diff/20001/chrome/browser/page_loa...
> > File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):
> > 
> >
https://codereview.chromium.org/1979633002/diff/20001/chrome/browser/page_loa...
> >
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:105:
// Immediate histogram variants.
> > Can you add a note that these are planned to replace the older metrics, with
the reasoning why you aren't doing the full replacement now?
> 
> I don't think we can replace the core metrics. Some of our metrics are part of
system health council and others are starting to depend on them, so we'll need
to communicate a deprecation plan. My current thinking is that we'll stop
logging the old metrics and deprecate them in M54, which gives us a few releases
of overlap.
> 
> I took this opportunity to choose better name categories. We have
PageLoad.AbortTiming already. I've now broken the metrics that were in Timing2
into 3 new categories: PageLoad.DocumentTiming, PageLoad.PaintTiming, and
PageLoad.ParseTiming. Take a look and see what you think.

One other note: I've left the 'Immediate' suffix on the constant names for these
histograms to make it easy to distinguish them in the codebase. Once we
deprecate and remove the old metrics, we'll also want to rename the constants to
remove the Immediate suffix.

[Charlie Harrison, 2016-05-16 22:18:20]

lgtm good idea.

[Bryan McQuade, 2016-05-17 01:28:08]

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

[Bryan McQuade, 2016-05-17 01:28:19]

rkaplow, PTAL for new metric names.

[rkaplow, 2016-05-17 01:49:51]

lgtm

sweet

[Bryan McQuade, 2016-05-17 03:18:01]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-05-17 03:18:21]

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

[commit-bot: I haz the power, 2016-05-17 04:41:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-17 04:42:13]

Description was changed from

==========
Add immediate variants of core page load metric histograms.

BUG=611811
==========

to

==========
Add immediate variants of core page load metric histograms.

BUG=611811

Committed: https://crrev.com/5f481ec582761cec96089b8520e00bb70db14798
Cr-Commit-Position: refs/heads/master@{#394050}
==========

[commit-bot: I haz the power, 2016-05-17 04:42:14]

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