Sending a page load pingback to data saver for holdback users

Sending a page load pingback to data saver for holdback users

When a user is in the holdback experiment, data saver's proxy server is
not actually used, so a pingback was not actually sent to the server in
that case. However, for sake of comparing similar page loads, a pingback
should now be sent for the holdback user when it is likely they would
have used data reduction proxy. The pingback also needs to indicate that
the specific page load is a holdback experiment page load.

BUG=731180
Review-Url: https://codereview.chromium.org/2934543002
Cr-Commit-Position: refs/heads/master@{#478738}
Committed: https://chromium.googlesource.com/chromium/src/+/698863fd7bbf3a52bfe40625b0eb8044701430c8

[RyanSturm, 2017-06-09 17:14:21]

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

[commit-bot: I haz the power, 2017-06-09 17:14:50]

Dry run: CQ is trying da patch.

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

[RyanSturm, 2017-06-09 17:15:25]

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

[RyanSturm, 2017-06-09 17:15:26]

tbansal, buettner: PTAL

[buettner, 2017-06-09 17:17:52]

https://codereview.chromium.org/2934543002/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/proto/pageload_metrics.proto (right):

https://codereview.chromium.org/2934543002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/proto/pageload_metrics.proto:107: optional bool
holdback_experiment = 19;
Just, "holdback"?

[buettner, 2017-06-09 17:20:36]

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

https://codereview.chromium.org/2934543002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:281:
if (data_reduction_proxy::params::IsIncludedInTamperDetectionExperiment()) {
We turned down tamper detection on the server long ago. Regardless, I'm not sure
why we'd gate PLM reports based on the tamper detection experiment.

[tbansal1, 2017-06-09 17:32:29]

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

https://codereview.chromium.org/2934543002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:281:
if (data_reduction_proxy::params::IsIncludedInTamperDetectionExperiment()) {
On 2017/06/09 17:20:35, buettner wrote:
> We turned down tamper detection on the server long ago. Regardless, I'm not
sure
> why we'd gate PLM reports based on the tamper detection experiment.

https://bugs.chromium.org/p/chromium/issues/detail?id=675761 is for cleaning up
the code.

https://codereview.chromium.org/2934543002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:284:
// Only consider timing events that happened before the first background
unrelated but can we add a DCHECK that url() is http, not https.

[commit-bot: I haz the power, 2017-06-09 18:24:09]

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

[commit-bot: I haz the power, 2017-06-09 18:24:09]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-06-09 18:46:07]

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

[commit-bot: I haz the power, 2017-06-09 18:46:40]

Dry run: CQ is trying da patch.

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

[RyanSturm, 2017-06-09 19:40:20]

I updated some of network delegate to send URL, ECT, and session key for the
holdback.

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

https://codereview.chromium.org/2934543002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:281:
if (data_reduction_proxy::params::IsIncludedInTamperDetectionExperiment()) {
On 2017/06/09 17:32:29, tbansal1 wrote:
> On 2017/06/09 17:20:35, buettner wrote:
> > We turned down tamper detection on the server long ago. Regardless, I'm not
> sure
> > why we'd gate PLM reports based on the tamper detection experiment.
> 
> https://bugs.chromium.org/p/chromium/issues/detail?id=675761 is for cleaning
up
> the code.

Done.

https://codereview.chromium.org/2934543002/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:284:
// Only consider timing events that happened before the first background
On 2017/06/09 17:32:29, tbansal1 wrote:
> unrelated but can we add a DCHECK that url() is http, not https.

Added one above for checking something related.

[RyanSturm, 2017-06-09 19:40:21]

I updated some of network delegate to send URL, ECT, and session key for the
holdback.

[commit-bot: I haz the power, 2017-06-09 20:00:34]

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

[commit-bot: I haz the power, 2017-06-09 20:00:35]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-09 22:32:55]

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

https://codereview.chromium.org/2934543002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:150:
if (!data || !data->used_data_reduction_proxy())
keep observing if holdback is enabled?

https://codereview.chromium.org/2934543002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:435:
->used_data_reduction_proxy()) {
check for holdback?

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

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:309:
if (params::IsIncludedInHoldbackFieldTrial()) {
nit: combine these into single statement:
bool X = condition_1 && condition_2;

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:324:
if (data) {
Why would data be null here?

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:325:
data->set_used_data_reduction_proxy(true);
Can you update comment for |used_data_reduction_proxy_| to say that it may be
true if holdback was set. or rename it to |eligible_data_reduction_proxy_|?

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:374:
if (!page_id) {
do we need page_id for holdback?

[RyanSturm, 2017-06-09 22:43:29]

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

[commit-bot: I haz the power, 2017-06-09 22:43:55]

Dry run: CQ is trying da patch.

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

[RyanSturm, 2017-06-09 22:46:02]

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

https://codereview.chromium.org/2934543002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:150:
if (!data || !data->used_data_reduction_proxy())
On 2017/06/09 22:32:55, tbansal1 wrote:
> keep observing if holdback is enabled?

?

This checks if DRP would have been used for the holdback case.

https://codereview.chromium.org/2934543002/diff/20001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:435:
->used_data_reduction_proxy()) {
On 2017/06/09 22:32:55, tbansal1 wrote:
> check for holdback?

This will track the number of requests that would have used DRP as is.

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

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:309:
if (params::IsIncludedInHoldbackFieldTrial()) {
On 2017/06/09 22:32:55, tbansal1 wrote:
> nit: combine these into single statement:
> bool X = condition_1 && condition_2;

Done.

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:324:
if (data) {
On 2017/06/09 22:32:55, tbansal1 wrote:
> Why would data be null here?

Done.

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:325:
data->set_used_data_reduction_proxy(true);
On 2017/06/09 22:32:55, tbansal1 wrote:
> Can you update comment for |used_data_reduction_proxy_| to say that it may be
> true if holdback was set. or rename it to |eligible_data_reduction_proxy_|?

Done.

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:374:
if (!page_id) {
On 2017/06/09 22:32:55, tbansal1 wrote:
> do we need page_id for holdback?

Talked to server team, and decided against it.

[commit-bot: I haz the power, 2017-06-09 23:48:35]

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

[commit-bot: I haz the power, 2017-06-09 23:48:37]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-12 18:28:17]

lgtm % comment.

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

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:374:
if (!page_id) {
On 2017/06/09 22:46:02, RyanSturm wrote:
> On 2017/06/09 22:32:55, tbansal1 wrote:
> > do we need page_id for holdback?
> 
> Talked to server team, and decided against it.

Add a comment on the reasoning.

[buettner, 2017-06-12 18:29:33]

On 2017/06/12 18:28:17, tbansal1 wrote:
> lgtm % comment.
> 
>
https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
> (right):
> 
>
https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:374:
> if (!page_id) {
> On 2017/06/09 22:46:02, RyanSturm wrote:
> > On 2017/06/09 22:32:55, tbansal1 wrote:
> > > do we need page_id for holdback?
> > 
> > Talked to server team, and decided against it.
> 
> Add a comment on the reasoning.

lgtm for proto changes.

[RyanSturm, 2017-06-12 18:37:49]

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

[RyanSturm, 2017-06-12 18:38:28]

tbansal: made a change to reporting/generating page_id for consistency (since it
was computational cheap as well).

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

https://codereview.chromium.org/2934543002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:374:
if (!page_id) {
On 2017/06/12 18:28:17, tbansal1 wrote:
> On 2017/06/09 22:46:02, RyanSturm wrote:
> > On 2017/06/09 22:32:55, tbansal1 wrote:
> > > do we need page_id for holdback?
> > 
> > Talked to server team, and decided against it.
> 
> Add a comment on the reasoning.

Done.

[commit-bot: I haz the power, 2017-06-12 18:38:30]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-12 19:47:53]

lgtm

[RyanSturm, 2017-06-12 19:48:11]

The CQ bit was unchecked by ryansturm@chromium.org

[RyanSturm, 2017-06-12 19:48:12]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2017-06-12 19:48:12]

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

[commit-bot: I haz the power, 2017-06-12 19:48:25]

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-06-12 20:14:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497296892014970,
"parent_rev": "b761304982323b99736f85ecffd2af79db224bb7", "commit_rev":
"698863fd7bbf3a52bfe40625b0eb8044701430c8"}

[commit-bot: I haz the power, 2017-06-12 20:14:39]

Description was changed from

==========
Sending a page load pingback to data saver for holdback users

When a user is in the holdback experiment, data saver's proxy server is
not actually used, so a pingback was not actually sent to the server in
that case. However, for sake of comparing similar page loads, a pingback
should now be sent for the holdback user when it is likely they would
have used data reduction proxy. The pingback also needs to indicate that
the specific page load is a holdback experiment page load.

BUG=731180
==========

to

==========
Sending a page load pingback to data saver for holdback users

When a user is in the holdback experiment, data saver's proxy server is
not actually used, so a pingback was not actually sent to the server in
that case. However, for sake of comparing similar page loads, a pingback
should now be sent for the holdback user when it is likely they would
have used data reduction proxy. The pingback also needs to indicate that
the specific page load is a holdback experiment page load.

BUG=731180

Review-Url: https://codereview.chromium.org/2934543002
Cr-Commit-Position: refs/heads/master@{#478738}
Committed:
https://chromium.googlesource.com/chromium/src/+/698863fd7bbf3a52bfe40625b0eb...
==========

[commit-bot: I haz the power, 2017-06-12 20:14:44]

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