predictors: Collect page load metrics for ResourcePrefetchPredictor.

predictors: Collect page load metrics for ResourcePrefetchPredictor.

This CL adds ResourcePrefetchPredictorPageLoadMetricsObserver that allows to
collect page load metrics only for prefetchable pages.
For now, the page is considered prefetchable if it has resources in the
predictor DB that could be used for prefetching. This is subject to change
depending on triggering selection for the trial.

BUG=680049
Review-Url: https://codereview.chromium.org/2632503005
Cr-Commit-Position: refs/heads/master@{#445723}
Committed: https://chromium.googlesource.com/chromium/src/+/666d5ac67f9172f965581c314de225737ba95d7d

[alexilin, 2017-01-13 16:27:46]

alexilin@chromium.org changed reviewers:
+ lizeb@chromium.org

[alexilin, 2017-01-13 16:27:47]

lizeb: PTAL, especially on IsUrlPrefetchable() method. I need want to know your
opinion before adding more reviewers.

[Benoit L, 2017-01-13 17:31:56]

Code looks fine. I haven't looked at the unittests yet, though.

Thanks!

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

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:32:
return nullptr;
nice: the nullptr_t constructor if unique_ptr is not explicit, the other one is.

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right):

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:99:
resource_prefetch_predictor_observer =
nit: observer is fine as a variable name here :-)

[Benoit L, 2017-01-13 17:34:06]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 17:34:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexilin, 2017-01-13 17:37:36]

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

[alexilin, 2017-01-13 17:37:37]

+csharrison: please review chrome/browser/page_load_metrics, thanks!

[commit-bot: I haz the power, 2017-01-13 18:35:46]

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

[commit-bot: I haz the power, 2017-01-13 18:35:47]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-01-13 18:56:05]

Generally looks good % lifetime question.

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h:57:
predictors::ResourcePrefetchPredictor* predictor_;
Can you document the lifetime of predictor_ and why it will outlive this class?

[Benoit L, 2017-01-16 14:04:53]

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h:57:
predictors::ResourcePrefetchPredictor* predictor_;
On 2017/01/13 18:56:05, Charlie Harrison wrote:
> Can you document the lifetime of predictor_ and why it will outlive this
class?

It is tied to BrowserContext, so this should be fine, right?

[Charlie Harrison, 2017-01-17 14:41:16]

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h:57:
predictors::ResourcePrefetchPredictor* predictor_;
On 2017/01/16 14:04:53, Benoit L wrote:
> On 2017/01/13 18:56:05, Charlie Harrison wrote:
> > Can you document the lifetime of predictor_ and why it will outlive this
> class?
> 
> It is tied to BrowserContext, so this should be fine, right?

Yes sounds good.

[Charlie Harrison, 2017-01-17 14:42:02]

LGTM. You can consider my request for docs optional considering that the
lifetime of the prefetcher is described in its class comments.

[alexilin, 2017-01-23 14:33:13]

lizeb: New PLT histograms are need to be described as suffixes of existing ones
in histograms.xml but in this case we have to introduce
PageLoad.Clients.ResourcePrefetchPredictor.PaintTiming.NavigationToFirstContentfulPaint
histogram as well (without .Prefetchable suffix).
We could just omit .Prefetchable for these histograms, WDYT?

[alexilin, 2017-01-23 17:35:42]

alexilin@chromium.org changed reviewers:
+ isherman@chromium.org

[alexilin, 2017-01-23 17:35:44]

isherman: Please review histograms.xml, thanks!
I would like to ask you the same question as I asked lizeb.
I introduce new PageLoadMetrics histogram adding
Clients.ResourcePrefetchPredictor "suffix" (with ordering="prefix"). At the same
time, I'm adding .Prefetchable suffix to the end of the name.
For now, I'm not sure whether I will introduce other suffixes for this client
suffix, but there is nonzero probability of that. 
I'm not collecting any UMA for the histogram without .Prefetchable suffix. Is
this a problem? I don't have an idea how to manage it properly except omitting
.Prefetchable suffix.

[Ilya Sherman, 2017-01-23 20:08:13]

Metrics LGTM

On 2017/01/23 17:35:44, alexilin wrote:
> isherman: Please review histograms.xml, thanks!
> I would like to ask you the same question as I asked lizeb.
> I introduce new PageLoadMetrics histogram adding
> Clients.ResourcePrefetchPredictor "suffix" (with ordering="prefix"). At the
same
> time, I'm adding .Prefetchable suffix to the end of the name.
> For now, I'm not sure whether I will introduce other suffixes for this client
> suffix, but there is nonzero probability of that. 
> I'm not collecting any UMA for the histogram without .Prefetchable suffix. Is
> this a problem? I don't have an idea how to manage it properly except omitting
> .Prefetchable suffix.

This is fine.  As I mentioned inline, you can specify base="true" to explicitly
mark the intermediate names as histograms that are not directly emitted to.

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

https://codereview.chromium.org/2632503005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:115688: +      label="PageLoadMetrics
with enabled ResourcePrefetchPredictor"/>
You can specify base="true" for this suffix to indicate that no data is
generated for these histograms without further suffixes.

[alexilin, 2017-01-24 12:49:21]

lizeb: friendly ping.

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

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:32:
return nullptr;
On 2017/01/13 17:31:56, Benoit L wrote:
> nice: the nullptr_t constructor if unique_ptr is not explicit, the other one
is.

Acknowledged.

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h:57:
predictors::ResourcePrefetchPredictor* predictor_;
On 2017/01/17 14:41:16, Charlie Harrison wrote:
> On 2017/01/16 14:04:53, Benoit L wrote:
> > On 2017/01/13 18:56:05, Charlie Harrison wrote:
> > > Can you document the lifetime of predictor_ and why it will outlive this
> > class?
> > 
> > It is tied to BrowserContext, so this should be fine, right?
> 
> Yes sounds good.

Acknowledged.

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right):

https://codereview.chromium.org/2632503005/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:99:
resource_prefetch_predictor_observer =
On 2017/01/13 17:31:56, Benoit L wrote:
> nit: observer is fine as a variable name here :-)

I'm trying to be more consistent with the names around
(no_state_prefetch_observer, prerender_observer). 
This name isn't scoped so it will be confusing to use it along with other
observers.

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

https://codereview.chromium.org/2632503005/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:115688: +      label="PageLoadMetrics
with enabled ResourcePrefetchPredictor"/>
On 2017/01/23 20:08:12, Ilya Sherman wrote:
> You can specify base="true" for this suffix to indicate that no data is
> generated for these histograms without further suffixes.

Done. Thanks for this tip!

[Benoit L, 2017-01-24 12:51:48]

lgtm, thanks.

Sorry, tried to LGTM that yesterday, was probably holding it wrong.

[alexilin, 2017-01-24 12:57:07]

The CQ bit was checked by alexilin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 12:57:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 13:28:45]

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

[commit-bot: I haz the power, 2017-01-24 13:28:46]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[alexilin, 2017-01-24 14:23:35]

The CQ bit was checked by alexilin@chromium.org

[alexilin, 2017-01-24 14:23:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2632503005/#ps60001
(title: "Mark suffix as base.")

[commit-bot: I haz the power, 2017-01-24 14:23:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 14:39:52]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485267815199480,
"parent_rev": "bf350a623625a7a12a96add5421e59a1faa66fb5", "commit_rev":
"666d5ac67f9172f965581c314de225737ba95d7d"}

[commit-bot: I haz the power, 2017-01-24 14:40:22]

Description was changed from

==========
predictors: Collect page load metrics for ResourcePrefetchPredictor.

This CL adds ResourcePrefetchPredictorPageLoadMetricsObserver that allows to
collect page load metrics only for prefetchable pages.
For now, the page is considered prefetchable if it has resources in the
predictor DB that could be used for prefetching. This is subject to change
depending on triggering selection for the trial.

BUG=680049
==========

to

==========
predictors: Collect page load metrics for ResourcePrefetchPredictor.

This CL adds ResourcePrefetchPredictorPageLoadMetricsObserver that allows to
collect page load metrics only for prefetchable pages.
For now, the page is considered prefetchable if it has resources in the
predictor DB that could be used for prefetching. This is subject to change
depending on triggering selection for the trial.

BUG=680049

Review-Url: https://codereview.chromium.org/2632503005
Cr-Commit-Position: refs/heads/master@{#445723}
Committed:
https://chromium.googlesource.com/chromium/src/+/666d5ac67f9172f965581c314de2...
==========

[commit-bot: I haz the power, 2017-01-24 14:40:24]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/666d5ac67f9172f965581c314de2...