[PageLoadMetrics] Record bytes usage per page

[PageLoadMetrics] Record bytes usage per page

Records the network, cache, and total bytes loaded via the browser process per page load.

BUG=672956
Review-Url: https://codereview.chromium.org/2560043004
Cr-Commit-Position: refs/heads/master@{#442258}
Committed: https://chromium.googlesource.com/chromium/src/+/74e3405dd042c4e4a3fcded953ec6fc5a87f2c16

[jkarlin, 2016-12-12 19:50:06]

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

[commit-bot: I haz the power, 2016-12-12 19:50:54]

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

[jkarlin, 2016-12-12 19:51:48]

jkarlin@chromium.org changed reviewers:
+ csharrison@chromium.org, ryansturm@chromium.org

[jkarlin, 2016-12-12 19:51:49]

csharrison@ PTAL at everything, thanks!
ryan@ PTAL for an analysis of what this CL might be missing, thanks!

[jkarlin, 2016-12-12 19:56:16]

Description was changed from

==========
[PageLoadMetrics] Record bytes usage per page

BUG=672956
==========

to

==========
[PageLoadMetrics] Record bytes usage per page

Records the network, cache, and total bytes loaded via the browser process per
page load.

BUG=672956
==========

[Charlie Harrison, 2016-12-12 20:17:17]

lgtm

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:173: // Bytes
consumed by this page.
Can you specify that these are just for subresources.

[jkarlin, 2016-12-12 21:40:26]

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:173: // Bytes
consumed by this page.
On 2016/12/12 20:17:17, Charlie Harrison wrote:
> Can you specify that these are just for subresources.

Actually, I want to include the main resource as well. Will think about how to
include that.

[RyanSturm, 2016-12-12 22:29:38]

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831:
url_request->GetRawBodyBytes(),
I'm curious why you are using GetRawBodyBytes instead of GetTotalReceivedBytes.
Does it seem worth tracking GetTotalSentBytes as well? I'm not really sure what
exactly you are comparing and if bytes sent matters or if only bytes received
matter.

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:186: if
(resource_type == content::RESOURCE_TYPE_MAIN_FRAME &&
Does this conditional match the comment?

It looks like according to the comment, it should maybe be an "or" instead of an
"and" because you don't want to track Main frame requests or net errors.

This implementation might be correct right now as the load will commit on the UI
thread before this method runs on the UI thread for the same request. In that
case, main frame requests are being tracked correctly.

You can also check if GlobalRequestID matches on NavigationHandle and
ResourceRequestInfo if you plumb it here (this won't work in a PlzNavigate world
however). However, GlobalRequestID is only available in ReadyToCommitNavigation.

[commit-bot: I haz the power, 2016-12-12 22:57:17]

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

[commit-bot: I haz the power, 2016-12-12 22:57:18]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-12-12 23:17:14]

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:186: if
(resource_type == content::RESOURCE_TYPE_MAIN_FRAME &&
On 2016/12/12 22:29:38, Ryan Sturm wrote:
> Does this conditional match the comment?
> 
> It looks like according to the comment, it should maybe be an "or" instead of
an
> "and" because you don't want to track Main frame requests or net errors.
> 
> This implementation might be correct right now as the load will commit on the
UI
> thread before this method runs on the UI thread for the same request. In that
> case, main frame requests are being tracked correctly.
> 
> You can also check if GlobalRequestID matches on NavigationHandle and
> ResourceRequestInfo if you plumb it here (this won't work in a PlzNavigate
world
> however). However, GlobalRequestID is only available in
ReadyToCommitNavigation.

As of  https://crrev.com/4434137b674aa5703a2f1e3cf10d6ad0bdac9ac2,
GlobalRequestID is available in PlzNavigate.

If you can store a copy of GlobalRequestID in PageLoadTracker and plumb the
Request's GlobalRequestID, you should be able to attribute the main request to
the committed_load_ when they match. I suspect they will match except in the
case of a cancelled navigation.

[jkarlin, 2016-12-14 18:26:29]

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:186: if
(resource_type == content::RESOURCE_TYPE_MAIN_FRAME &&
On 2016/12/12 22:29:38, Ryan Sturm wrote:
> Does this conditional match the comment?
> 
> It looks like according to the comment, it should maybe be an "or" instead of
an
> "and" because you don't want to track Main frame requests or net errors.

You're right, that should be an or. So right now we are in fact tracking main
frame resources.

> 
> This implementation might be correct right now as the load will commit on the
UI
> thread before this method runs on the UI thread for the same request. In that
> case, main frame requests are being tracked correctly.

There is some concern that there is a race. Do we know for sure that
DidFinishNavigation is called on the UI thread before the URLRequest's
NotifyUIThreadOfRequestComplete is run?

Assuming that there is no race, do you believe this will work in a PlzNavigate
world as well?

[jkarlin, 2016-12-14 19:48:29]

PTAL, I've changed it to include the main frame resource (which it was
accidentally doing anyway). I also now filter out resources that were started
before the navigation.

[jkarlin, 2016-12-14 19:52:54]

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

[commit-bot: I haz the power, 2016-12-14 19:53:42]

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

[RyanSturm, 2016-12-14 19:56:07]

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:186: if
(resource_type == content::RESOURCE_TYPE_MAIN_FRAME &&
On 2016/12/14 18:26:29, jkarlin wrote:
> On 2016/12/12 22:29:38, Ryan Sturm wrote:
> > Does this conditional match the comment?
> > 
> > It looks like according to the comment, it should maybe be an "or" instead
of
> an
> > "and" because you don't want to track Main frame requests or net errors.
> 
> You're right, that should be an or. So right now we are in fact tracking main
> frame resources.
> 
> > 
> > This implementation might be correct right now as the load will commit on
the
> UI
> > thread before this method runs on the UI thread for the same request. In
that
> > case, main frame requests are being tracked correctly.
> 
> There is some concern that there is a race. Do we know for sure that
> DidFinishNavigation is called on the UI thread before the URLRequest's
> NotifyUIThreadOfRequestComplete is run?
> 

AFAIK, ChromeResourceDispatcherHostDelegate::RequestComplete() happens on the IO
thread after ResourceLoader::CompleteResponseStarted() happens on the IO thread
(I could be mistaken as there is a lot hpapening between net/ and content/).
This assumes a non-error, non-throttled NavigationHandle. I don't know enough
about NavigationThrottles using DEFER to really answer this question fully. In
the typical case, this should work.

However, I suspect that you are right that this is not a bullet-proof technique,
but there may be a way to match requests and NavigationHandles once both are
complete.


> Assuming that there is no race, do you believe this will work in a PlzNavigate
> world as well?

This should work in PlzNavigate now that GlobalRequestID got added, but I
haven't verified it myself.

[commit-bot: I haz the power, 2016-12-14 21:46:52]

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

[commit-bot: I haz the power, 2016-12-14 21:46:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[RyanSturm, 2016-12-15 20:53:51]

lgtm

I think this CL will get pretty good information for UMA and will be mostly
accurate.

In general, Main frame requests should really be connected to their ultimate
render frame via the GlobalRequestID when the load commits, but getting it to
the right web contents and checking the time of the request start vs navigation
start will probably be fully succesful for main frame requests.

The only issue will come from a situation of racing URLRequests that don't get
cancelled in time, but I think that can be addressed in a later CL. The racing
requests will most likely happen if a user quickly re-navigates when a lot of
URLRequests are still being issued as the old render frame won't stop issuing
and start cancelling those requests until the load commits.

[Bryan McQuade, 2016-12-16 14:28:44]

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

[Bryan McQuade, 2016-12-16 14:28:45]

thanks! a couple suggestions.

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:183: // If the
navigation hasn't committed yet then we'll miss the resource (this
let's maybe add a comment here noting some of the issues ryansturm points out
with races between renderers, and that we think it should be relatively rare,
but we may want to address it in a follow up CL.

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:539:
const page_load_metrics::PageLoadExtraInfo& info) {
On Android, OnComplete doesn't get called about 1/3 of the time that the app
gets backgrounded, due to being killed while in the background. This leads to
data with some bias.

We can't address this perfectly, but a few things we can do to help us improve
our understanding:
1. you could implement the FlushMetricsOnAppEnterBackground callback, and log
bytes received up to that point in a separate histogram. I'm not totally sure
this addresses the issue, though. Is there something more complete we can do to
help us understand potential for bias?

2. We may also want to log bytes received up until the page goes into the
background by adding histograms in the OnHidden callback. That callback gets
invoked reliably for all page loads that are backgrounded.

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:175: int64_t
network_bytes;
i think it's implicit, but just to be extra clear, can you add a comment to
explain that this is HTTP bytes on the wire, excluding HTTP headers? e.g. it's
the gzipped size of a response, rather than the post-ungzip size?

Is the same true for cache bytes? Both network and cache bytes are "wire sizes",
less HTTP headers?

[jkarlin, 2016-12-19 19:25:15]

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

[commit-bot: I haz the power, 2016-12-19 19:25:42]

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

[jkarlin, 2016-12-19 19:26:00]

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

[jkarlin, 2016-12-19 19:26:01]

rkaplow@: PTAL at histograms.xml, thanks!

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831:
url_request->GetRawBodyBytes(),
On 2016/12/12 22:29:38, Ryan Sturm wrote:
> I'm curious why you are using GetRawBodyBytes instead of
GetTotalReceivedBytes.
> Does it seem worth tracking GetTotalSentBytes as well? I'm not really sure
what
> exactly you are comparing and if bytes sent matters or if only bytes received
> matter.

Right now only tracking received bytes. I'm using RawBodyBytes because I'm not
worried about how large the decoded resource is, I want to know how many network
and cache bytes the page load required.

We can always add decoded bytes and sent bytes later.

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:173: // Bytes
consumed by this page.
On 2016/12/12 21:40:26, jkarlin wrote:
> On 2016/12/12 20:17:17, Charlie Harrison wrote:
> > Can you specify that these are just for subresources.
> 
> Actually, I want to include the main resource as well. Will think about how to
> include that.

Done. The main resource is now included.

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:183: // If the
navigation hasn't committed yet then we'll miss the resource (this
On 2016/12/16 14:28:45, Bryan McQuade wrote:
> let's maybe add a comment here noting some of the issues ryansturm points out
> with races between renderers, and that we think it should be relatively rare,
> but we may want to address it in a follow up CL.

Done.

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:539:
const page_load_metrics::PageLoadExtraInfo& info) {
On 2016/12/16 14:28:45, Bryan McQuade wrote:
> On Android, OnComplete doesn't get called about 1/3 of the time that the app
> gets backgrounded, due to being killed while in the background. This leads to
> data with some bias.
> 
> We can't address this perfectly, but a few things we can do to help us improve
> our understanding:
> 1. you could implement the FlushMetricsOnAppEnterBackground callback, and log
> bytes received up to that point in a separate histogram. I'm not totally sure
> this addresses the issue, though. Is there something more complete we can do
to
> help us understand potential for bias?
> 
> 2. We may also want to log bytes received up until the page goes into the
> background by adding histograms in the OnHidden callback. That callback gets
> invoked reliably for all page loads that are backgrounded.

ACK. I've added a comment about the Android bias in the base class OnComplete
docs. Mind if we add additional metrics in a followup however?

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2560043004/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:175: int64_t
network_bytes;
On 2016/12/16 14:28:45, Bryan McQuade wrote:
> i think it's implicit, but just to be extra clear, can you add a comment to
> explain that this is HTTP bytes on the wire, excluding HTTP headers? e.g. it's
> the gzipped size of a response, rather than the post-ungzip size?
> 
> Is the same true for cache bytes? Both network and cache bytes are "wire
sizes",
> less HTTP headers?

Done.

[RyanSturm, 2016-12-19 19:40:05]

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831:
url_request->GetRawBodyBytes(),
On 2016/12/19 19:26:01, jkarlin wrote:
> On 2016/12/12 22:29:38, Ryan Sturm wrote:
> > I'm curious why you are using GetRawBodyBytes instead of
> GetTotalReceivedBytes.
> > Does it seem worth tracking GetTotalSentBytes as well? I'm not really sure
> what
> > exactly you are comparing and if bytes sent matters or if only bytes
received
> > matter.
> 
> Right now only tracking received bytes. I'm using RawBodyBytes because I'm not
> worried about how large the decoded resource is, I want to know how many
network
> and cache bytes the page load required.
> 
> We can always add decoded bytes and sent bytes later.

I thought both of these tracked bytes after decoding SSL, but RawBodyBytes
doesn't include headers, I might be wrong on this though.

[commit-bot: I haz the power, 2016-12-19 21:13:17]

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

[commit-bot: I haz the power, 2016-12-19 21:13:18]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2016-12-20 14:03:58]

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831:
url_request->GetRawBodyBytes(),
On 2016/12/19 19:40:05, Ryan Sturm wrote:
> On 2016/12/19 19:26:01, jkarlin wrote:
> > On 2016/12/12 22:29:38, Ryan Sturm wrote:
> > > I'm curious why you are using GetRawBodyBytes instead of
> > GetTotalReceivedBytes.
> > > Does it seem worth tracking GetTotalSentBytes as well? I'm not really sure
> > what
> > > exactly you are comparing and if bytes sent matters or if only bytes
> received
> > > matter.
> > 
> > Right now only tracking received bytes. I'm using RawBodyBytes because I'm
not
> > worried about how large the decoded resource is, I want to know how many
> network
> > and cache bytes the page load required.
> > 
> > We can always add decoded bytes and sent bytes later.
> 
> I thought both of these tracked bytes after decoding SSL, but RawBodyBytes
> doesn't include headers, I might be wrong on this though.

You're right that they both track bytes before decoding. The difference is that
the GetTotalReceivedBytes only counts bytes over the network while RawBodyBytes
counts body size regardless of if it came from the network or from cache. 

If we want the network and cached UMA values to be comparable, then they should
probably both use the same (body size). I could be persuaded otherwise though,
WDYT?

[RyanSturm, 2016-12-20 14:30:24]

On 2016/12/20 14:03:58, jkarlin wrote:
>
https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
> File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
(right):
> 
>
https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
> chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831:
> url_request->GetRawBodyBytes(),
> On 2016/12/19 19:40:05, Ryan Sturm wrote:
> > On 2016/12/19 19:26:01, jkarlin wrote:
> > > On 2016/12/12 22:29:38, Ryan Sturm wrote:
> > > > I'm curious why you are using GetRawBodyBytes instead of
> > > GetTotalReceivedBytes.
> > > > Does it seem worth tracking GetTotalSentBytes as well? I'm not really
sure
> > > what
> > > > exactly you are comparing and if bytes sent matters or if only bytes
> > received
> > > > matter.
> > > 
> > > Right now only tracking received bytes. I'm using RawBodyBytes because I'm
> not
> > > worried about how large the decoded resource is, I want to know how many
> > network
> > > and cache bytes the page load required.
> > > 
> > > We can always add decoded bytes and sent bytes later.
> > 
> > I thought both of these tracked bytes after decoding SSL, but RawBodyBytes
> > doesn't include headers, I might be wrong on this though.
> 
> You're right that they both track bytes before decoding. The difference is
that
> the GetTotalReceivedBytes only counts bytes over the network while
RawBodyBytes
> counts body size regardless of if it came from the network or from cache. 
> 
> If we want the network and cached UMA values to be comparable, then they
should
> probably both use the same (body size). I could be persuaded otherwise though,
> WDYT?

SGTM. Just make sure the comments reflect that headers aren't included.

[jkarlin, 2016-12-20 14:59:54]

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

[commit-bot: I haz the power, 2016-12-20 15:00:06]

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

[jkarlin, 2016-12-20 15:00:14]

On 2016/12/20 14:30:24, Ryan Sturm wrote:
> On 2016/12/20 14:03:58, jkarlin wrote:
> >
>
https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
> > File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2560043004/diff/60001/chrome/browser/loader/c...
> > chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831:
> > url_request->GetRawBodyBytes(),
> > On 2016/12/19 19:40:05, Ryan Sturm wrote:
> > > On 2016/12/19 19:26:01, jkarlin wrote:
> > > > On 2016/12/12 22:29:38, Ryan Sturm wrote:
> > > > > I'm curious why you are using GetRawBodyBytes instead of
> > > > GetTotalReceivedBytes.
> > > > > Does it seem worth tracking GetTotalSentBytes as well? I'm not really
> sure
> > > > what
> > > > > exactly you are comparing and if bytes sent matters or if only bytes
> > > received
> > > > > matter.
> > > > 
> > > > Right now only tracking received bytes. I'm using RawBodyBytes because
I'm
> > not
> > > > worried about how large the decoded resource is, I want to know how many
> > > network
> > > > and cache bytes the page load required.
> > > > 
> > > > We can always add decoded bytes and sent bytes later.
> > > 
> > > I thought both of these tracked bytes after decoding SSL, but RawBodyBytes
> > > doesn't include headers, I might be wrong on this though.
> > 
> > You're right that they both track bytes before decoding. The difference is
> that
> > the GetTotalReceivedBytes only counts bytes over the network while
> RawBodyBytes
> > counts body size regardless of if it came from the network or from cache. 
> > 
> > If we want the network and cached UMA values to be comparable, then they
> should
> > probably both use the same (body size). I could be persuaded otherwise
though,
> > WDYT?
> 
> SGTM. Just make sure the comments reflect that headers aren't included.

Done.

[jkarlin, 2016-12-20 15:00:33]

bmcquade@ and rkaplow@ PTAL, thanks!

[Bryan McQuade, 2016-12-20 15:56:45]

Looks ok, thanks! I am inclined to segment this into pages that have been in fg
vs bg as we do with other histograms, as I think we have different throttling
policies for these cases and thus the counts may end up being different.

https://codereview.chromium.org/2560043004/diff/160001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2560043004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:207:
"PageLoad.Experimental.Bytes.Total.OnComplete";
Could we omit the 'OnComplete' part of the name and just note in the histogram
description that the histogram tracks bytes over the complete lifetime of the
page load? A histo that tracks something over the entire page load doesn't need
a suffix.

https://codereview.chromium.org/2560043004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:550:
UMA_HISTOGRAM_CUSTOM_COUNTS(internal::kHistogramNetworkBytesOnComplete,
could we add a comment explaining the bias issue due to pages being killed in
the background on Android, just so we don't forget about it?

Do you know if we have different policies for throttling etc that would affect
byte counts for pages while in foreground vs background? If so, it's probably
worth segmenting counts into separate histograms for pages that were loaded
entirely in the foreground vs those that were backgrounded. For other
histograms, we've found that mixing fg and bg into the same histograms makes the
data much less useful.

[commit-bot: I haz the power, 2016-12-20 16:02:01]

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

[commit-bot: I haz the power, 2016-12-20 16:02:02]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2016-12-20 16:12:55]

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

[jkarlin, 2016-12-20 16:13:02]

https://codereview.chromium.org/2560043004/diff/160001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2560043004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:207:
"PageLoad.Experimental.Bytes.Total.OnComplete";
On 2016/12/20 15:56:45, Bryan McQuade wrote:
> Could we omit the 'OnComplete' part of the name and just note in the histogram
> description that the histogram tracks bytes over the complete lifetime of the
> page load? A histo that tracks something over the entire page load doesn't
need
> a suffix.

Done.

https://codereview.chromium.org/2560043004/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:550:
UMA_HISTOGRAM_CUSTOM_COUNTS(internal::kHistogramNetworkBytesOnComplete,
On 2016/12/20 15:56:45, Bryan McQuade wrote:
> could we add a comment explaining the bias issue due to pages being killed in
> the background on Android, just so we don't forget about it?

Done.

> 
> Do you know if we have different policies for throttling etc that would affect
> byte counts for pages while in foreground vs background? If so, it's probably
> worth segmenting counts into separate histograms for pages that were loaded
> entirely in the foreground vs those that were backgrounded. For other
> histograms, we've found that mixing fg and bg into the same histograms makes
the
> data much less useful.

I imagine that a huge proportion of pages will be backgrounded at some point
before they hit OnComplete. In fact, I'd be concerned that foreground pages
would be biased toward those that the user quickly navigates away from. I'm not
aware of backgrounding a page causing a difference in the total number of
resources loaded.

[commit-bot: I haz the power, 2016-12-20 16:13:08]

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

[Bryan McQuade, 2016-12-20 16:20:01]

LGTM.

RE: fg/bg throttling behavior, I think that last I checked, resource scheduler
had some awareness of whether a tab was hidden and reduced parallelism as a
result. Or maybe there was code in place to allow that but it wasn't enabled.

This seems fine - I do want us to try to address some of the bias issues before
we promote out of experimental but this seems great for now. Thanks for adding
these!

[commit-bot: I haz the power, 2016-12-20 16:27:32]

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

[commit-bot: I haz the power, 2016-12-20 16:27:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jkarlin, 2016-12-20 16:42:40]

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

[commit-bot: I haz the power, 2016-12-20 16:42:59]

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-12-20 17:43:29]

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

[commit-bot: I haz the power, 2016-12-20 17:43:30]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2016-12-20 19:49:16]

Thanks! rkaplow@ PTAL

[jkarlin, 2016-12-22 11:58:43]

jkarlin@chromium.org changed reviewers:
+ isheriff@chromium.org
- rkaplow@chromium.org

[jkarlin, 2016-12-22 11:59:08]

isherman@ PTAL at histograms.xml, thanks!

[Charlie Harrison, 2016-12-22 12:50:32]

bmcquade: I think there is no bg/fg difference today when it come s to loading 
policy. The code in ResourceScheduler was an experiment that was deleted about a
year ago.

There may be slight differences when it comes to e.g. process level priority or
task priority in the renderer but I still think the best metric is total bytes.

[Bryan McQuade, 2016-12-28 20:45:33]

On 2016/12/22 at 12:50:32, csharrison wrote:
> bmcquade: I think there is no bg/fg difference today when it come s to loading
 policy. The code in ResourceScheduler was an experiment that was deleted about
a year ago.
> 
> There may be slight differences when it comes to e.g. process level priority
or task priority in the renderer but I still think the best metric is total
bytes.

Ah, sounds good, thanks! Glad to know that code was removed.

I think it may be interesting to track fg vs bg bytes in the future as we
hopefully add smarter network scheduling depending on visibility etc, but I
agree that for now this is not needed.

[Bryan McQuade, 2016-12-28 20:45:45]

bmcquade@chromium.org changed reviewers:
+ isherman@chromium.org
- isheriff@chromium.org

[Bryan McQuade, 2017-01-04 21:18:35]

On 2016/12/28 at 20:45:33, Bryan McQuade wrote:
> On 2016/12/22 at 12:50:32, csharrison wrote:
> > bmcquade: I think there is no bg/fg difference today when it come s to
loading  policy. The code in ResourceScheduler was an experiment that was
deleted about a year ago.
> > 
> > There may be slight differences when it comes to e.g. process level priority
or task priority in the renderer but I still think the best metric is total
bytes.
> 
> Ah, sounds good, thanks! Glad to know that code was removed.
> 
> I think it may be interesting to track fg vs bg bytes in the future as we
hopefully add smarter network scheduling depending on visibility etc, but I
agree that for now this is not needed.

isherman, PTAL, thanks!

[Ilya Sherman, 2017-01-07 00:50:43]

metrics lgtm, thanks

[jkarlin, 2017-01-09 12:43:48]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2017-01-09 12:43:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, ryansturm@chromium.org, bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2560043004/#ps200001
(title: "Fix consts")

[commit-bot: I haz the power, 2017-01-09 12:43:55]

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-09 12:56:48]

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

[commit-bot: I haz the power, 2017-01-09 12:56:50]

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_...)

[jkarlin, 2017-01-09 15:02:41]

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

[commit-bot: I haz the power, 2017-01-09 15:03:00]

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

[jkarlin, 2017-01-09 15:44:25]

The CQ bit was unchecked by jkarlin@chromium.org

[jkarlin, 2017-01-09 15:44:33]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2017-01-09 15:44:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, ryansturm@chromium.org, isherman@chromium.org,
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2560043004/#ps240001
(title: "Fix test")

[commit-bot: I haz the power, 2017-01-09 15:44:57]

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-09 15:51:40]

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

[commit-bot: I haz the power, 2017-01-09 15:51:42]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jkarlin, 2017-01-09 15:53:15]

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

[jkarlin, 2017-01-09 15:53:16]

droger@chromium.org: Please review changes in chrome/browser/prerender/ Thanks!

[droger, 2017-01-09 16:10:33]

lgtm

[jkarlin, 2017-01-09 16:11:10]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2017-01-09 16:11:35]

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-09 16:16:05]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1483978270278870,
"parent_rev": "d0eda8fc58bd6357bb5a18e4d977b98de988857a", "commit_rev":
"74e3405dd042c4e4a3fcded953ec6fc5a87f2c16"}

[commit-bot: I haz the power, 2017-01-09 16:16:34]

Description was changed from

==========
[PageLoadMetrics] Record bytes usage per page

Records the network, cache, and total bytes loaded via the browser process per
page load.

BUG=672956
==========

to

==========
[PageLoadMetrics] Record bytes usage per page

Records the network, cache, and total bytes loaded via the browser process per
page load.

BUG=672956

Review-Url: https://codereview.chromium.org/2560043004
Cr-Commit-Position: refs/heads/master@{#442258}
Committed:
https://chromium.googlesource.com/chromium/src/+/74e3405dd042c4e4a3fcded953ec...
==========

[commit-bot: I haz the power, 2017-01-09 16:16:36]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/74e3405dd042c4e4a3fcded953ec...