Adding ExtraRequest info and exposing request to PLM observers

Adding ExtraRequest info and exposing request to PLM observers

This CL adds a count of requests using DRP and information about
compression. This will allow us to reconsider UMA (e.g., logging UMA for
Mixed content pages (HTTPS pages using DRP from HTTP sub-resources),
logging UMA for <50% of network requests using DRP, >50% of network
requests using DRP, etc.). This will also allow UMA for size of page in
terms of network bytes and in terms of original content length
(precompressed bytes from the origin). This will allow a DRP per-page
data savings histogram as well.


BUG=679893
Review-Url: https://codereview.chromium.org/2619243003
Cr-Commit-Position: refs/heads/master@{#443313}
Committed: https://chromium.googlesource.com/chromium/src/+/d2051030e7d9a6ffdfd5f6bde903ec48862b9f49

[RyanSturm, 2017-01-09 18:37:09]

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

[commit-bot: I haz the power, 2017-01-09 18:37:41]

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-09 18:45:04]

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

[commit-bot: I haz the power, 2017-01-09 18:45:05]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[RyanSturm, 2017-01-09 18:47:38]

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

[RyanSturm, 2017-01-09 18:47:39]

bmcquade, csharrison: This is a draft for the purpose of discussion and I will
clean up minor issues later.

I was thinking of adding this for DRP to get more information for the pingback
and adding more detailed UMA. I realize this is a layering violation in some
sense as long as PLM wants to stay ignorant of DRP. A lot of the UMA I could add
could be deferred until data_use_measurement/ is complete, but some of it could
not (E.g., PLM for <50% of net resources loaded through DRP, HTTPS using HTTP
DRP, etc.).

I can make this more abstract by doing something like ChromeNavigationData, but
I'm not sure what that buys us.

Any suggestions of a more abstract, concise way of doing this would be
appreciated.

WDYT?

[Charlie Harrison, 2017-01-09 21:41:28]

Hm, what if we just surfaced OnLoadedResource to downstream consumers? Then the
DRP observer could just keep track of this stuff itself, rather than pulling it
out of ExtraInfo.

[RyanSturm, 2017-01-09 21:45:02]

On 2017/01/09 21:41:28, Charlie Harrison wrote:
> Hm, what if we just surfaced OnLoadedResource to downstream consumers? Then
the
> DRP observer could just keep track of this stuff itself, rather than pulling
it
> out of ExtraInfo.

That sounds good to me. Would plumbing the two DRP related fields through be
fine?

[Charlie Harrison, 2017-01-09 21:52:32]

It is okay with me. We're "polluting" a single method rather than our weird
global ExtraInfo thing.

It may be a good idea to make a ResourceExtraInfo struct or something similar so
all observers don't need to be updated when we add a field, though. As long as
that struct doesn't do any expensive copies it should be a cleaner interface.

[RyanSturm, 2017-01-09 22:52:28]

On 2017/01/09 21:52:32, Charlie Harrison wrote:
> It is okay with me. We're "polluting" a single method rather than our weird
> global ExtraInfo thing.
> 
> It may be a good idea to make a ResourceExtraInfo struct or something similar
so
> all observers don't need to be updated when we add a field, though. As long as
> that struct doesn't do any expensive copies it should be a cleaner interface.

I'll re-write the CL with that in mind. Thanks.

[RyanSturm, 2017-01-10 22:06:46]

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

[commit-bot: I haz the power, 2017-01-10 22:07:42]

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

[RyanSturm, 2017-01-10 23:02:15]

Description was changed from

==========
Draft of adding DRP request info to PLM

This CL adds a count of requests using DRP and information about
compression. This will allow us to reconsider UMA (e.g., logging UMA for
Mixed content pages (HTTPS pages using DRP from HTTP sub-resources),
logging UMA for <50% of network requests using DRP, >50% of network
requests using DRP, etc.). This will also allow UMA for size of page in
terms of network bytes and in terms of original content length
(precompressed bytes from the origin). This will allow a DRP per-page
data savings histogram as well.

This could be a violation of layering in some respects. To keep PLM
unaware of DRP the alternative would likely be to add an observer
method per-request in the observers, which would require passing a
ChromeNavigationData-like object.

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

to

==========
Adding ExtraRequest info to PLM

This CL adds a count of requests using DRP and information about
compression. This will allow us to reconsider UMA (e.g., logging UMA for
Mixed content pages (HTTPS pages using DRP from HTTP sub-resources),
logging UMA for <50% of network requests using DRP, >50% of network
requests using DRP, etc.). This will also allow UMA for size of page in
terms of network bytes and in terms of original content length
(precompressed bytes from the origin). This will allow a DRP per-page
data savings histogram as well.


BUG=679893
==========

[RyanSturm, 2017-01-10 23:02:15]

ryansturm@chromium.org changed reviewers:
- bmcquade@chromium.org

[RyanSturm, 2017-01-10 23:03:21]

Description was changed from

==========
Adding ExtraRequest info to PLM

This CL adds a count of requests using DRP and information about
compression. This will allow us to reconsider UMA (e.g., logging UMA for
Mixed content pages (HTTPS pages using DRP from HTTP sub-resources),
logging UMA for <50% of network requests using DRP, >50% of network
requests using DRP, etc.). This will also allow UMA for size of page in
terms of network bytes and in terms of original content length
(precompressed bytes from the origin). This will allow a DRP per-page
data savings histogram as well.


BUG=679893
==========

to

==========
Adding ExtraRequest info and exposing request to PLM observers

This CL adds a count of requests using DRP and information about
compression. This will allow us to reconsider UMA (e.g., logging UMA for
Mixed content pages (HTTPS pages using DRP from HTTP sub-resources),
logging UMA for <50% of network requests using DRP, >50% of network
requests using DRP, etc.). This will also allow UMA for size of page in
terms of network bytes and in terms of original content length
(precompressed bytes from the origin). This will allow a DRP per-page
data savings histogram as well.


BUG=679893
==========

[RyanSturm, 2017-01-10 23:03:27]

ryansturm@chromium.org changed reviewers:
+ tbansal@chromium.org

[RyanSturm, 2017-01-10 23:03:27]

ryansturm@chromium.org changed reviewers:
+ tbansal@chromium.org

[RyanSturm, 2017-01-10 23:03:27]

tbansal: PTAL at data_reduction_proxy
csharrison: PTAL at PLM

[RyanSturm, 2017-01-10 23:03:28]

tbansal: PTAL at data_reduction_proxy
csharrison: PTAL at PLM

[commit-bot: I haz the power, 2017-01-10 23:11:22]

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

[commit-bot: I haz the power, 2017-01-10 23:11:23]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-01-10 23:19:27]

lgtm % nits components/d_r_p

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:37:
namespace data_reduction_proxy {
why was this moved up?

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:115:
// usage for |request|. Passing in -1 for |original_content_length| indicates
Fix stale comment.

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc
(right):

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:175: if
(request.status().error() == net::OK)
include net/base/net_errors.h

[Charlie Harrison, 2017-01-11 14:37:18]

c/b/l and page_load_metrics looks good too, but just one request for a bit more
refactoring.

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

https://codereview.chromium.org/2619243003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:199:
committed_load_->OnLoadedResource(was_cached, raw_body_bytes,
Can was_cached and raw_body_bytes go in the ExtraRequestInfo? That simplifies
the tracker so it doesn't have to worry about byte aggregation.

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

https://codereview.chromium.org/2619243003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:183: struct
ExtraRequestInfo {
Can you add a sentence or two of top level comment for this struct?

[Bryan McQuade, 2017-01-11 14:46:49]

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

[Bryan McQuade, 2017-01-11 14:46:50]

looks good, thanks! just one question - glad to see this land.

https://codereview.chromium.org/2619243003/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2619243003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:531: cache_bytes_ +=
raw_body_bytes;
how much work would it be to get rid of cache_bytes_ and network_bytes_ and move
all consumers to implementing the OnLoadedResource callback instead? I think I
prefer that, though we don't necessarily have to do it in this change.

[Bryan McQuade, 2017-01-11 14:47:11]

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

[RyanSturm, 2017-01-11 22:57:28]

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

[commit-bot: I haz the power, 2017-01-11 22:58:34]

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

[RyanSturm, 2017-01-11 23:03:53]

ryansturm@chromium.org changed reviewers:
+ bmcquade@google.com

[RyanSturm, 2017-01-11 23:03:54]

Adding bmcquade back since I addressed his comment and a little more.

csharrison/bmcquade: PTAL @ plm again, thanks
tbansal: PTAL @ drp

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

https://codereview.chromium.org/2619243003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:199:
committed_load_->OnLoadedResource(was_cached, raw_body_bytes,
On 2017/01/11 14:37:18, Charlie Harrison wrote:
> Can was_cached and raw_body_bytes go in the ExtraRequestInfo? That simplifies
> the tracker so it doesn't have to worry about byte aggregation.

Done.

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

https://codereview.chromium.org/2619243003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:183: struct
ExtraRequestInfo {
On 2017/01/11 14:37:18, Charlie Harrison wrote:
> Can you add a sentence or two of top level comment for this struct?

Done.

https://codereview.chromium.org/2619243003/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2619243003/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:531: cache_bytes_ +=
raw_body_bytes;
On 2017/01/11 14:46:50, Bryan McQuade wrote:
> how much work would it be to get rid of cache_bytes_ and network_bytes_ and
move
> all consumers to implementing the OnLoadedResource callback instead? I think I
> prefer that, though we don't necessarily have to do it in this change.

Done. I also moved the count of requests into the core_plm_observer class. the
DRP observer will end up tracking it as well, but I agree that the observer
should record what it needs for aggregate request level information.

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:37:
namespace data_reduction_proxy {
On 2017/01/10 23:19:27, tbansal1 wrote:
> why was this moved up?

I wanted util::CalculateEffectiveOCL(request) instead of
data_reduction_proxy::util::CalculateEffectiveOCL(request)

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:115:
// usage for |request|. Passing in -1 for |original_content_length| indicates
On 2017/01/10 23:19:27, tbansal1 wrote:
> Fix stale comment.

Done.

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc
(right):

https://codereview.chromium.org/2619243003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_util.cc:175: if
(request.status().error() == net::OK)
On 2017/01/10 23:19:27, tbansal1 wrote:
> include net/base/net_errors.h

Done.

[RyanSturm, 2017-01-11 23:04:09]

ryansturm@chromium.org changed reviewers:
+ bmcquade@chromium.org
- bmcquade@google.com

[RyanSturm, 2017-01-11 23:05:58]

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

[RyanSturm, 2017-01-11 23:05:59]

droger: PTAL @ prerender_browsertest.cc. Thanks!

[Bryan McQuade, 2017-01-11 23:40:25]

LGTM, thanks!

[commit-bot: I haz the power, 2017-01-12 00:24:50]

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

[commit-bot: I haz the power, 2017-01-12 00:24:51]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-01-12 03:16:22]

page_load_metrics and chrome/browser/loader lgtm, thanks!

[droger, 2017-01-12 13:20:23]

lgtm

[RyanSturm, 2017-01-12 19:00:21]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2017-01-12 19:00:21]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2619243003/#ps60001
(title: "comments")

[commit-bot: I haz the power, 2017-01-12 19:01:11]

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-12 19:07:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484247621209810,
"parent_rev": "c013415abe383dc6bddfa06791c109917b822c4f", "commit_rev":
"d2051030e7d9a6ffdfd5f6bde903ec48862b9f49"}

[commit-bot: I haz the power, 2017-01-12 19:07:56]

Description was changed from

==========
Adding ExtraRequest info and exposing request to PLM observers

This CL adds a count of requests using DRP and information about
compression. This will allow us to reconsider UMA (e.g., logging UMA for
Mixed content pages (HTTPS pages using DRP from HTTP sub-resources),
logging UMA for <50% of network requests using DRP, >50% of network
requests using DRP, etc.). This will also allow UMA for size of page in
terms of network bytes and in terms of original content length
(precompressed bytes from the origin). This will allow a DRP per-page
data savings histogram as well.


BUG=679893
==========

to

==========
Adding ExtraRequest info and exposing request to PLM observers

This CL adds a count of requests using DRP and information about
compression. This will allow us to reconsider UMA (e.g., logging UMA for
Mixed content pages (HTTPS pages using DRP from HTTP sub-resources),
logging UMA for <50% of network requests using DRP, >50% of network
requests using DRP, etc.). This will also allow UMA for size of page in
terms of network bytes and in terms of original content length
(precompressed bytes from the origin). This will allow a DRP per-page
data savings histogram as well.


BUG=679893

Review-Url: https://codereview.chromium.org/2619243003
Cr-Commit-Position: refs/heads/master@{#443313}
Committed:
https://chromium.googlesource.com/chromium/src/+/d2051030e7d9a6ffdfd5f6bde903...
==========

[commit-bot: I haz the power, 2017-01-12 19:07:57]

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