[NoStatePrefetch] Add UMA histogram to count prefetch requests

[NoStatePrefetch] Add UMA histogram to count prefetch requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.

BUG=642371
Committed: https://crrev.com/81a292f669a97f97087a18c54558ae65efd094f7
Cr-Commit-Position: refs/heads/master@{#416889}

[droger, 2016-08-29 16:04:38]

Description was changed from

==========
[NoStatePrefetch] Add UMA histogram to count prefetches requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how no-store is prevalent.
==========

to

==========
[NoStatePrefetch] Add UMA histogram to count prefetches requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.
==========

[droger, 2016-08-30 09:01:28]

Patchset #2 (id:20001) has been deleted

[droger, 2016-08-30 09:02:38]

droger@chromium.org changed reviewers:
+ mattcary@chromium.org, pasko@chromium.org

[droger, 2016-08-30 09:02:56]

Description was changed from

==========
[NoStatePrefetch] Add UMA histogram to count prefetches requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.
==========

to

==========
[NoStatePrefetch] Add UMA histogram to count prefetch requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.
==========

[droger, 2016-08-30 09:07:54]

Note: I'm not sure if the histogram should go in prerender_histograms.cc or not.

As I understand it, prerender_histograms.cc has a bunch of logic to build
multiple histograms at once, depending on the origin of the prerender. I don't
think we need this for NoStatePrefetch?

[mattcary, 2016-08-30 09:20:20]

On 2016/08/30 09:07:54, droger wrote:
> Note: I'm not sure if the histogram should go in prerender_histograms.cc or
not.
> 
> As I understand it, prerender_histograms.cc has a bunch of logic to build
> multiple histograms at once, depending on the origin of the prerender. I don't
> think we need this for NoStatePrefetch?

I suspect we're going to want this in with prerender_histograms. Even thought
that does have some deprecated stuff related to experiments, it also has
histograms organized by origin, which I suspect is something we're going to want
to track eventually (if not sooner).

On a similar note, why are you recording the histogram in the throttler and not
in AddPrerender where (as far as I can tell) the existing prerender counting is
done? There may be good reasons to do it in the throttler, but on the other hand
it will be hard if we ever want to compare with existing prerender histograms.

[droger, 2016-08-30 09:24:39]

On 2016/08/30 09:20:20, mattcary wrote:
> On 2016/08/30 09:07:54, droger wrote:
> > Note: I'm not sure if the histogram should go in prerender_histograms.cc or
> not.
> > 
> > As I understand it, prerender_histograms.cc has a bunch of logic to build
> > multiple histograms at once, depending on the origin of the prerender. I
don't
> > think we need this for NoStatePrefetch?
> 
> I suspect we're going to want this in with prerender_histograms. Even thought
> that does have some deprecated stuff related to experiments, it also has
> histograms organized by origin, which I suspect is something we're going to
want
> to track eventually (if not sooner).

Ok, I'll move it there.

> 
> On a similar note, why are you recording the histogram in the throttler and
not
> in AddPrerender where (as far as I can tell) the existing prerender counting
is
> done? There may be good reasons to do it in the throttler, but on the other
hand
> it will be hard if we ever want to compare with existing prerender histograms.

I need to see all the individual requests (including sub resources). I don't
think it is possible to do it in AddPrerender, which is only called on the main
URL.

[pasko, 2016-08-30 09:29:27]

Without looking at the change itself (which is next):

On 2016/08/30 09:07:54, droger wrote:
> Note: I'm not sure if the histogram should go in prerender_histograms.cc or
not.
> 
> As I understand it, prerender_histograms.cc has a bunch of logic to build
> multiple histograms at once, depending on the origin of the prerender. I don't
> think we need this for NoStatePrefetch?

I agree it is better to make separate and more specific histograms for the
prefetch. The prerender_histograms have the power (and the complexity with code
bloat) to record all the various final statuses of prerender (40 of them
afaict), which I suspect we won't need.

However, I think it would still be useful to split by origin because:
* each origin represents a different team of people relying on it, and they'd
probably appreciate our data on their specific use of prerender
* origins have very different efficiency (used/prerendered), splitting would
allow prioritizing our work wrt to origins.

WDYT?

[mattcary, 2016-08-30 09:42:17]

> I need to see all the individual requests (including sub resources). I don't
> think it is possible to do it in AddPrerender, which is only called on the
main
> URL.

Ah, I see. That makes sense, I see that you use a slightly different histogram
name too which is accurate to what you count.

Then I guess my suggestion is to add prefetch to the prerenderer count done in
AddPrerender so we have comparable counts to the current prerenderer as well as
resource counts?

[pasko, 2016-08-30 09:43:33]

https://codereview.chromium.org/2287993003/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2287993003/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:30: enum
NoStatePrefetchResourceType {
nit: enum class is shiny and modern

https://codereview.chromium.org/2287993003/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_resource_throttle.cc:104:
content::IsResourceTypeFrame(info->GetResourceType()),
will main resource redirects be recorded as sub-resources? that's unfortunate
because they may prevent serving the main resource only from cache

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

https://codereview.chromium.org/2287993003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:45250: +<histogram
name="Prerender.NoStatePrefetchResourceCount"
The "Prerender." prefix would be annoying to rename (deprecating histograms,
etc.)

we can occupy "Prefetch.", that would be my preference

[droger, 2016-08-30 11:21:28]

Patchset #3 (id:60001) has been deleted

[droger, 2016-08-30 11:23:18]

Moved to prerender_histograms, and avoiding using the PREFIXED_HISTOGRAM macros
on purpose, so that we can eventually deprecate them.

[droger, 2016-08-30 11:48:26]

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

[commit-bot: I haz the power, 2016-08-30 11:48:42]

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

[droger, 2016-08-30 13:22:06]

On 2016/08/30 09:42:17, mattcary wrote:
> Then I guess my suggestion is to add prefetch to the prerenderer count done in
> AddPrerender so we have comparable counts to the current prerenderer as well
as
> resource counts?

What do you mean by this? The existing code should be updating the histograms
for NoStatePrefetch already, no?

[droger, 2016-08-30 13:25:03]

On 2016/08/30 13:22:06, droger wrote:
> On 2016/08/30 09:42:17, mattcary wrote:
> > Then I guess my suggestion is to add prefetch to the prerenderer count done
in
> > AddPrerender so we have comparable counts to the current prerenderer as well
> as
> > resource counts?
> 
> What do you mean by this? The existing code should be updating the histograms
> for NoStatePrefetch already, no?

Maybe you want to be able to distinguish NoStatePrefetch from other prerendering
modes?
In that case, I thought enabling NoStatePrefetch through an experiment would
give this ability for free.

[pasko, 2016-08-30 13:41:05]

The approach looks good

I was hesitating with naming:
* either "Prerender.NoStatePrefetch.*"
* or straight "Prefetch.*"

the former will need to be updated after deprecation (slight debt), but the
latter risks diverging various is_wash and other minor things that could make
data analysis difficult.

I guess I am more on the former side than I was before.

Also a few questions below

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:27: kMainResourceCacheable = 0,
I think chromium style prefers MACRO_STYLE constants, at least it is so in
chrome/browser/prerender/, let's keep it this way for consistency.

Also, c++11 promotes using 'scoped enums' aka 'enum class Foo {...};'

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:411: // Unrolls
UMA_HISTOGRAM_ENUMERATION, required to support dynamic histogram
Would it be possible to DCHECK_CURRENTLY_ON(BrowserThread::UI) here? AFAIR
FactoryGet is not thread safe.

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:71: void
PrerenderResourceThrottle::WillProcessResponse(bool* defer) {
is it possible to detect redirects on the path to main resource here? If yes,
then we could make more buckets: "MAIN_RESOURCE_REDIRECTS_ALL_CACHEABLE",
"MAIN_RESOURCE_REDIRECTS_SOME_NO_STORE"

TODO for later would SG to me as well

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:72: *defer = false;
I am not familiar with resource throttle API, of course, but ..

other implementations of WillProcessResponse seem to mostly avoid touching the
output argument on early return (such as when ResourceRequestInfo not
available). Seems safer in various edge cases when info is not available during
hypothetical shutdown and the space for |defer| is already gone.

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:190: PrerenderContents*
prerender_contents =
nit: DCHECK_CURRENTLY_ON

[commit-bot: I haz the power, 2016-08-30 13:55:50]

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

[commit-bot: I haz the power, 2016-08-30 13:55:51]

Dry run: This issue passed the CQ dry run.

[droger, 2016-08-30 14:31:42]

Thanks.

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:27: kMainResourceCacheable = 0,
On 2016/08/30 13:41:04, pasko wrote:
> I think chromium style prefers MACRO_STYLE constants, at least it is so in
> chrome/browser/prerender/, let's keep it this way for consistency.
> 
> Also, c++11 promotes using 'scoped enums' aka 'enum class Foo {...};'

changed the naming style, however I need these to be convertible to int, so I
didn't use the enum class.

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:411: // Unrolls
UMA_HISTOGRAM_ENUMERATION, required to support dynamic histogram
On 2016/08/30 13:41:04, pasko wrote:
> Would it be possible to DCHECK_CURRENTLY_ON(BrowserThread::UI) here? AFAIR
> FactoryGet is not thread safe.

Done (using a thread checker).

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:71: void
PrerenderResourceThrottle::WillProcessResponse(bool* defer) {
On 2016/08/30 13:41:04, pasko wrote:
> is it possible to detect redirects on the path to main resource here? If yes,
> then we could make more buckets: "MAIN_RESOURCE_REDIRECTS_ALL_CACHEABLE",
> "MAIN_RESOURCE_REDIRECTS_SOME_NO_STORE"
> 
> TODO for later would SG to me as well

Good idea.

We can detect redirects easily, and reporting each redirect should be easy.

Reporting info regarding the whole redirect chain may be harder but probably
doable too. However, this would probably be in a separate histogram in this
case? I'll do this in a separate CL because it may not be trivial.

https://codereview.chromium.org/2287993003/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:72: *defer = false;
On 2016/08/30 13:41:04, pasko wrote:
> I am not familiar with resource throttle API, of course, but ..
> 
> other implementations of WillProcessResponse seem to mostly avoid touching the
> output argument on early return (such as when ResourceRequestInfo not
> available). Seems safer in various edge cases when info is not available
during
> hypothetical shutdown and the space for |defer| is already gone.

Done.

I wondered about this, and both cases definitely exist in the codebase. I don't
have a strong preference.

[pasko, 2016-08-30 15:11:19]

LGTM with minor suggestions

Suggestion 1: please add BUG=642371 (just created this bug to group all
histogram commits, seems like a non-trivial piece of work)

https://codereview.chromium.org/2287993003/diff/130001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.cc (right):

https://codereview.chromium.org/2287993003/diff/130001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.cc:408: GetHistogramName(origin,
IsOriginWash(), "NoStatePrefetchResourceCount");
nit: I think "Count" is implied in histograms, BlahResourceType might sound
slightly more informative, up to you

https://codereview.chromium.org/2287993003/diff/130001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_histograms.h (right):

https://codereview.chromium.org/2287993003/diff/130001/chrome/browser/prerend...
chrome/browser/prerender/prerender_histograms.h:114: bool is_no_store);
s/;/ const;/

also, while we are here, please make the function above const as well

[droger, 2016-08-30 15:13:45]

Description was changed from

==========
[NoStatePrefetch] Add UMA histogram to count prefetch requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.
==========

to

==========
[NoStatePrefetch] Add UMA histogram to count prefetch requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.

BUG=642371
==========

[mattcary, 2016-09-02 09:59:47]

On 2016/08/30 13:25:03, droger wrote:
> On 2016/08/30 13:22:06, droger wrote:
> > On 2016/08/30 09:42:17, mattcary wrote:
> > > Then I guess my suggestion is to add prefetch to the prerenderer count
done
> in
> > > AddPrerender so we have comparable counts to the current prerenderer as
well
> > as
> > > resource counts?
> > 
> > What do you mean by this? The existing code should be updating the
histograms
> > for NoStatePrefetch already, no?
> 
> Maybe you want to be able to distinguish NoStatePrefetch from other
prerendering
> modes?
> In that case, I thought enabling NoStatePrefetch through an experiment would
> give this ability for free.

The initial experiment would be easy, I'm thinking of the next steps, when we
may have nostate prefetch enabled for everything except certain cases (like
AMP). In that case, I'm not sure we'd be able to determine nostate from
experiment + origin, or, even if we can, it may be practically too hard (lots of
head scratching looking at graphs trying to remember what happened when). So it
might be easier to make a new histogram that makes it explicit.

[droger, 2016-09-02 15:29:34]

droger@chromium.org changed reviewers:
+ mmenke@chromium.org

[droger, 2016-09-02 15:29:34]

+mmenke as owner

[mmenke, 2016-09-02 16:22:36]

On 2016/09/02 15:29:34, droger wrote:
> +mmenke as owner

LGTM, rubberstamp.

[droger, 2016-09-05 11:42:04]

droger@chromium.org changed reviewers:
+ asvitkine@chromium.org

[droger, 2016-09-05 11:42:04]

+asvitkine for histograms.xml

[droger, 2016-09-05 11:42:23]

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

[commit-bot: I haz the power, 2016-09-05 11:42:29]

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, 2016-09-05 12:39:54]

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

[commit-bot: I haz the power, 2016-09-05 12:39:56]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-09-06 19:49:18]

lgtm

[droger, 2016-09-07 08:22:02]

The CQ bit was checked by droger@chromium.org

[droger, 2016-09-07 08:22:03]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2287993003/#ps170001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-07 08:22:24]

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, 2016-09-07 09:34:09]

Description was changed from

==========
[NoStatePrefetch] Add UMA histogram to count prefetch requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.

BUG=642371
==========

to

==========
[NoStatePrefetch] Add UMA histogram to count prefetch requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.

BUG=642371
==========

[commit-bot: I haz the power, 2016-09-07 09:34:10]

Committed patchset #8 (id:170001)

[commit-bot: I haz the power, 2016-09-07 09:36:29]

Description was changed from

==========
[NoStatePrefetch] Add UMA histogram to count prefetch requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.

BUG=642371
==========

to

==========
[NoStatePrefetch] Add UMA histogram to count prefetch requests

The prefetches are split into 4 categories:
- main resource, cacheable
- main resource, no-store
- sub-resource, cacheable
- sub-resource, no-store

This will allow to count the number of prefetched pages, count how many
sub resources are discovered on average, and how much no-store is
prevalent.

BUG=642371

Committed: https://crrev.com/81a292f669a97f97087a18c54558ae65efd094f7
Cr-Commit-Position: refs/heads/master@{#416889}
==========

[commit-bot: I haz the power, 2016-09-07 09:36:30]

Patchset 8 (id:??) landed as
https://crrev.com/81a292f669a97f97087a18c54558ae65efd094f7
Cr-Commit-Position: refs/heads/master@{#416889}