Control data saver pingback reporting fraction with protobuf

Control data saver pingback reporting fraction with protobuf

The pingback is modulated by a float that is contained in the client
config protobuf. By default, the pingback is disabled, and the client
config can vary the probability between zero and one.

BUG=616805
Committed: https://crrev.com/31a97850d684c0ea8b891da1b8bebc633a6f1198
Cr-Commit-Position: refs/heads/master@{#400719}

[RyanSturm, 2016-06-17 18:01:16]

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

[RyanSturm, 2016-06-17 18:01:17]

tbansal: PTAL

[tbansal1, 2016-06-17 18:51:13]

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:393:
io_data_->SetPingbackReportingFraction(0.0);
s/0.0/0.0f/
floats should all be suffixed with .f, otherwise compiler will interpret 0.0 as
double, and then typecast.
Here, and everywhere else in this CL.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:478:
io_data_->SetPingbackReportingFraction(reporting_fraction);
DCHECK_LE(0.0f, reporting_fraction);
DCHECK_GE(1.0f, reporting_fraction);

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h:214:
DataReductionProxyIOData* io_data_;
Is |io_data_| guaranteed to outlive |this|? Add a comment why this is
guaranteed.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:96:
if (reporting_fraction >= 0.0 && reporting_fraction <= 1.0) {
Why is this check needed in the test? If you are doing this in test, why not do
it in the main code?

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:193:
CreateConfig("session-key", kConfigRefreshDurationSeconds, 0,
s/session-key/kSuccessSessionKey/

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:209:
ProxyServer_ProxyScheme_HTTP, "origin.net", 0, -1.0);
Just remove this test. We can assume that server will send the right value.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:948:
TEST_F(DataReductionProxyConfigServiceClientTest, ApplySerializedConfig) {
May be add a test ApplySerializedConfigWithPingbackProbability which tests that
the pingback probability is parsed correctly from the persisted config.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:211:
float pingback_reporting_fraction) {
DCHECK(io_task_runner_->BelongsToCurrentThread());

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:121:
return params::IsForcePingbackEnabledViaFlags() ||
I think you can add 2 more tests (better if you can combine in a single test):
(1) Enables ping back flag. Ensures that the ping back is sent
(2) Move static_cast<float>(base::RandDouble()) to a virtual function in this
class. Override that function in the tests. And, write a test that controls what
that overriding function returns (effectively controlling the random number
generator), and makes sure that the random number vs.
pingback_reporting_fraction_ comparison is done properly.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:44:
pingback_reporting_fraction_ = pingback_reporting_fraction;
Add a thread checker, and move the function definition to .cc file.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:44:
pingback_reporting_fraction_ = pingback_reporting_fraction;
DCHECK_LE(0.0f, pingback_reporting_fraction_);
DCHECK_GE(1.0f, pingback_reporting_fraction_);

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:267:
pingback_client_->SetPingbackReportingFraction(pingback_reporting_fraction);
DCHECK(CalledOnValidThread());

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:227:
pingback_reporting_fraction_(0.5) {
Why 0.5?

[RyanSturm, 2016-06-17 20:29:57]

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:393:
io_data_->SetPingbackReportingFraction(0.0);
On 2016/06/17 18:51:13, tbansal1 wrote:
> s/0.0/0.0f/
> floats should all be suffixed with .f, otherwise compiler will interpret 0.0
as
> double, and then typecast.
> Here, and everywhere else in this CL.

Done.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:478:
io_data_->SetPingbackReportingFraction(reporting_fraction);
On 2016/06/17 18:51:12, tbansal1 wrote:
> DCHECK_LE(0.0f, reporting_fraction);
> DCHECK_GE(1.0f, reporting_fraction);

Done.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h:214:
DataReductionProxyIOData* io_data_;
On 2016/06/17 18:51:13, tbansal1 wrote:
> Is |io_data_| guaranteed to outlive |this|? Add a comment why this is
> guaranteed.

I'll copy the similar comments above. In practice, this object is owned by io
data, but I suppose anybody could create this.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:96:
if (reporting_fraction >= 0.0 && reporting_fraction <= 1.0) {
On 2016/06/17 18:51:13, tbansal1 wrote:
> Why is this check needed in the test? If you are doing this in test, why not
do
> it in the main code?

This is not a check on state. I wanted to create a protobuf without a
pageload_metrics_config to verify state. I could have passed in a float and a
bool, or I could overload the float to say "if this isn't in the range, then
leave it empty". Let me know if that is not explicit from the comment, and I
should use a bool and a float.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:193:
CreateConfig("session-key", kConfigRefreshDurationSeconds, 0,
On 2016/06/17 18:51:13, tbansal1 wrote:
> s/session-key/kSuccessSessionKey/

Done.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:209:
ProxyServer_ProxyScheme_HTTP, "origin.net", 0, -1.0);
On 2016/06/17 18:51:13, tbansal1 wrote:
> Just remove this test. We can assume that server will send the right value.

I assume the server might want to optionally send this and default to zero if it
doesn't. It saves a bit of data to leave it empty, and I would prefer to test it
rather than not.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:948:
TEST_F(DataReductionProxyConfigServiceClientTest, ApplySerializedConfig) {
On 2016/06/17 18:51:13, tbansal1 wrote:
> May be add a test ApplySerializedConfigWithPingbackProbability which tests
that
> the pingback probability is parsed correctly from the persisted config.

I added a check in VerifyRemoteSuccess.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:211:
float pingback_reporting_fraction) {
On 2016/06/17 18:51:13, tbansal1 wrote:
> DCHECK(io_task_runner_->BelongsToCurrentThread());

Done.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.cc:121:
return params::IsForcePingbackEnabledViaFlags() ||
On 2016/06/17 18:51:13, tbansal1 wrote:
> I think you can add 2 more tests (better if you can combine in a single test):
> (1) Enables ping back flag. Ensures that the ping back is sent
> (2) Move static_cast<float>(base::RandDouble()) to a virtual function in this
> class. Override that function in the tests. And, write a test that controls
what
> that overriding function returns (effectively controlling the random number
> generator), and makes sure that the random number vs.
> pingback_reporting_fraction_ comparison is done properly.

Done.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:44:
pingback_reporting_fraction_ = pingback_reporting_fraction;
On 2016/06/17 18:51:13, tbansal1 wrote:
> Add a thread checker, and move the function definition to .cc file.

Done.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:44:
pingback_reporting_fraction_ = pingback_reporting_fraction;
On 2016/06/17 18:51:13, tbansal1 wrote:
> DCHECK_LE(0.0f, pingback_reporting_fraction_);
> DCHECK_GE(1.0f, pingback_reporting_fraction_);

Done.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_service.cc:267:
pingback_client_->SetPingbackReportingFraction(pingback_reporting_fraction);
On 2016/06/17 18:51:13, tbansal1 wrote:
> DCHECK(CalledOnValidThread());

Done.

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:227:
pingback_reporting_fraction_(0.5) {
On 2016/06/17 18:51:13, tbansal1 wrote:
> Why 0.5?

So it's not 0 and it's not 1. I wanted to make sure this got set and didn't
default. I could make this negative one instead.

[tbansal1, 2016-06-17 21:31:42]

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
(right):

https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:227:
pingback_reporting_fraction_(0.5) {
On 2016/06/17 20:29:57, RyanSturm wrote:
> On 2016/06/17 18:51:13, tbansal1 wrote:
> > Why 0.5?
> 
> So it's not 0 and it's not 1. I wanted to make sure this got set and didn't
> default. I could make this negative one instead.

May be default to 0.0f? It is slightly confusing because the production code
uses 0.0 as default, and here we are using different value.

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

https://codereview.chromium.org/2076663004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:206:
ClientConfig empty_reporting_fraction_config =
Add a comment that setting it to a negative value is equivalent to server not
setting it.

https://codereview.chromium.org/2076663004/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:431:
// A configuration where the pingback reporting fraction is set to 0.0f.
nit: Add one more config for probability 0.5f.

[RyanSturm, 2016-06-17 21:48:57]

On 2016/06/17 21:31:42, tbansal1 wrote:
>
https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
> (right):
> 
>
https://codereview.chromium.org/2076663004/diff/1/components/data_reduction_p...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:227:
> pingback_reporting_fraction_(0.5) {
> On 2016/06/17 20:29:57, RyanSturm wrote:
> > On 2016/06/17 18:51:13, tbansal1 wrote:
> > > Why 0.5?
> > 
> > So it's not 0 and it's not 1. I wanted to make sure this got set and didn't
> > default. I could make this negative one instead.
> 
> May be default to 0.0f? It is slightly confusing because the production code
> uses 0.0 as default, and here we are using different value.
> 
Done.

>
https://codereview.chromium.org/2076663004/diff/20001/components/data_reducti...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/2076663004/diff/20001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:206:
> ClientConfig empty_reporting_fraction_config =
> Add a comment that setting it to a negative value is equivalent to server not
> setting it.
> 

done.
>
https://codereview.chromium.org/2076663004/diff/20001/components/data_reducti...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:431:
> // A configuration where the pingback reporting fraction is set to 0.0f.
> nit: Add one more config for probability 0.5f.
done.

[tbansal1, 2016-06-17 21:54:18]

lgtm

[RyanSturm, 2016-06-20 17:19:17]

The CQ bit was checked by ryansturm@chromium.org

[commit-bot: I haz the power, 2016-06-20 17:19:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2076663004/60001

[commit-bot: I haz the power, 2016-06-20 17:56:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-20 18:02:24]

Description was changed from

==========
Control data saver pingback reporting fraction with protobuf

The pingback is modulated by a float that is contained in the client
config protobuf. By default, the pingback is disabled, and the client
config can vary the probability between zero and one.

BUG=616805
==========

to

==========
Control data saver pingback reporting fraction with protobuf

The pingback is modulated by a float that is contained in the client
config protobuf. By default, the pingback is disabled, and the client
config can vary the probability between zero and one.

BUG=616805

Committed: https://crrev.com/31a97850d684c0ea8b891da1b8bebc633a6f1198
Cr-Commit-Position: refs/heads/master@{#400719}
==========

[commit-bot: I haz the power, 2016-06-20 18:02:26]

Patchset 4 (id:??) landed as
https://crrev.com/31a97850d684c0ea8b891da1b8bebc633a6f1198
Cr-Commit-Position: refs/heads/master@{#400719}