Domain Reliability: Add sample_rate field to Beacon

Domain Reliability: Add sample_rate field to Beacon

Domain Reliability reports details of URL requests with a certain
probability (sample rate). By including the sample rate in the
beacon, the collector can estimate how many other requests (on
average) were not reported.

BUG=567307
Committed: https://crrev.com/ccc4ffa155c61083ee5f57cbb3e756197494b8e1
Cr-Commit-Position: refs/heads/master@{#364446}

[Deprecated (see juliatuttle), 2015-12-04 18:52:28]

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

[Deprecated (see juliatuttle), 2015-12-04 18:52:29]

PTAL, jkarlin.

[jkarlin, 2015-12-07 14:57:28]

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/beacon.h:69: // was chosen to be reported.
Why inverse? Why not just report the sampling rate?

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/beacon.h:70: int weight;
Why an int? You're losing significant data here.

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/config.h:35: bool DecideIfShouldReportRequest(bool
success, double* sample_rate_out)
I would prefer you added:

double GetSampleRate(bool success) const;

instead of adding the out parameter to DecideIfShouldReportRequest.

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/context.cc:74: bool reported =
config().DecideIfShouldReportRequest(success, &sample_rate);
DecideIfShouldReportRequest needs to be renamed. Its name is general enough that
it sounds like it could decide not to report for various reasons, not just
sample rate. But this function depends on it being for sample rate when
calculating beacon weight below.

Perhaps s/DecideIfShouldReportRequest/ShouldSampleReportRequest/  ?

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/context.cc:83: beacon->weight = 1.0 / sample_rate;
Division by 0 is possible here. config.cc's IsValidSampleRate permits 0.0.
Please fix and add a test.

Another reason to just report sample rate and not 1.0/sample_rate.

Also, isn't the weight potentially wrong in the case of evicted beacons?

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/context_unittest.cc:217:
"\"url\":\"https://localhost/\",\"was_proxied\":false,\"weight\":1}],"
A weight of 1 isn't testing that division is happening.

[jkarlin, 2015-12-07 18:06:53]

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/context.cc:83: beacon->weight = 1.0 / sample_rate;
On 2015/12/07 14:57:27, jkarlin wrote:
> Division by 0 is possible here. config.cc's IsValidSampleRate permits 0.0.
> Please fix and add a test.
> 
> Another reason to just report sample rate and not 1.0/sample_rate.
> 
> Also, isn't the weight potentially wrong in the case of evicted beacons? 

I realized you can't actually get here if it's 0.0 so never mind about that.

[jkarlin, 2015-12-07 20:00:31]

Also, the description needs a bug number.

[Deprecated (see juliatuttle), 2015-12-07 20:17:07]

Description was changed from

==========
Domain Reliability: Add 'weight' (1 / sample_rate) field to Beacon

Domain Reliability reports details of URL requests with a certain
probability (sample rate). By including the inverse of the sample rate
in the beacon (as the "weight"), the collector can tell how many other
requests (on average) were not reported.

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

to

==========
Domain Reliability: Add 'weight' (1 / sample_rate) field to Beacon

Domain Reliability reports details of URL requests with a certain
probability (sample rate). By including the inverse of the sample rate
in the beacon (as the "weight"), the collector can tell how many other
requests (on average) were not reported.

BUG=567307
==========

[Deprecated (see juliatuttle), 2015-12-07 20:17:57]

PTAL, jkarlin.

I switched to reporting sample_rate, so a lot of the comments became obsolete.

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/beacon.h:69: // was chosen to be reported.
On 2015/12/07 14:57:27, jkarlin wrote:
> Why inverse? Why not just report the sampling rate? 

Done.

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/beacon.h:70: int weight;
On 2015/12/07 14:57:27, jkarlin wrote:
> Why an int? You're losing significant data here.

In our own tests, we've only ever used things (other than 100%) like 5% or 10%.

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/config.h:35: bool DecideIfShouldReportRequest(bool
success, double* sample_rate_out)
On 2015/12/07 14:57:27, jkarlin wrote:
> I would prefer you added:
> 
> double GetSampleRate(bool success) const;
> 
> instead of adding the out parameter to DecideIfShouldReportRequest.

Done.

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/context.cc:74: bool reported =
config().DecideIfShouldReportRequest(success, &sample_rate);
On 2015/12/07 14:57:27, jkarlin wrote:
> DecideIfShouldReportRequest needs to be renamed. Its name is general enough
that
> it sounds like it could decide not to report for various reasons, not just
> sample rate. But this function depends on it being for sample rate when
> calculating beacon weight below.
> 
> Perhaps s/DecideIfShouldReportRequest/ShouldSampleReportRequest/  ?

Acknowledged.

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/context.cc:83: beacon->weight = 1.0 / sample_rate;
On 2015/12/07 14:57:27, jkarlin wrote:
> Division by 0 is possible here. config.cc's IsValidSampleRate permits 0.0.
> Please fix and add a test.
> 
> Another reason to just report sample rate and not 1.0/sample_rate.
> 
> Also, isn't the weight potentially wrong in the case of evicted beacons? 

Yes, it's potentially wrong in the case of evicted beacons, but it's too hard to
figure out which one(s) to change when we evict a beacon, so I don't bother.

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/context.cc:83: beacon->weight = 1.0 / sample_rate;
On 2015/12/07 18:06:53, jkarlin wrote:
> On 2015/12/07 14:57:27, jkarlin wrote:
> > Division by 0 is possible here. config.cc's IsValidSampleRate permits 0.0.
> > Please fix and add a test.
> > 
> > Another reason to just report sample rate and not 1.0/sample_rate.
> > 
> > Also, isn't the weight potentially wrong in the case of evicted beacons? 
> 
> I realized you can't actually get here if it's 0.0 so never mind about that.

Acknowledged.

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/context_unittest.cc:217:
"\"url\":\"https://localhost/\",\"was_proxied\":false,\"weight\":1}],"
On 2015/12/07 14:57:27, jkarlin wrote:
> A weight of 1 isn't testing that division is happening.

Acknowledged.

[jkarlin, 2015-12-08 12:31:02]

You haven't uploaded the updated patchset yet. Also, please update the
description with your change.

[jkarlin, 2015-12-08 19:41:36]

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/1497803004/diff/1/components/domain_reliabili...
components/domain_reliability/beacon.h:70: int weight;
On 2015/12/07 20:17:57, ttuttle wrote:
> On 2015/12/07 14:57:27, jkarlin wrote:
> > Why an int? You're losing significant data here.
> 
> In our own tests, we've only ever used things (other than 100%) like 5% or
10%.

But the sample_rate is a double in the range [0,1] right?

[Deprecated (see juliatuttle), 2015-12-08 19:47:22]

Description was changed from

==========
Domain Reliability: Add 'weight' (1 / sample_rate) field to Beacon

Domain Reliability reports details of URL requests with a certain
probability (sample rate). By including the inverse of the sample rate
in the beacon (as the "weight"), the collector can tell how many other
requests (on average) were not reported.

BUG=567307
==========

to

==========
Domain Reliability: Add sample_rate field to Beacon

Domain Reliability reports details of URL requests with a certain
probability (sample rate). By including the sample rate in the
beacon, the collector can estimate how many other requests (on
average) were not reported.

BUG=567307
==========

[Deprecated (see juliatuttle), 2015-12-08 19:47:49]

PTAL, jkarlin.

[jkarlin, 2015-12-09 11:46:22]

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
components/domain_reliability/beacon.h:72: double sample_rate;
This should have been caught by a test. Need moar tests!

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
components/domain_reliability/config.h:35: double GetSampleRate(bool success)
const;
nit: s/success/request_successful/ would help clarify what the parameter is for.

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
components/domain_reliability/context.cc:74: bool report = base::RandDouble() <
sample_rate;
nit: s/report/should_report/

report sounds like an object, not a bool

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
components/domain_reliability/context_unittest.cc:249:
"\"request_elapsed_ms\":250,\"sample_rate\":1.0,"
Needs more tests. Include rates such as 0.0, 1.0, 0.33, and out of bounds checks
like -5 and 50. Also, shouldn't there be tests for success sample rates and
failure sample rates?

[Deprecated (see juliatuttle), 2015-12-10 17:04:23]

PTAL, jkarlin.

This is now rebased on top of the other CL I sent you yesterday.

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
File components/domain_reliability/beacon.h (right):

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
components/domain_reliability/beacon.h:72: double sample_rate;
On 2015/12/09 11:46:22, jkarlin wrote:
> This should have been caught by a test. Need moar tests!

Done.

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
File components/domain_reliability/config.h (right):

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
components/domain_reliability/config.h:35: double GetSampleRate(bool success)
const;
On 2015/12/09 11:46:22, jkarlin wrote:
> nit: s/success/request_successful/ would help clarify what the parameter is
for.

Done.

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
File components/domain_reliability/context.cc (right):

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
components/domain_reliability/context.cc:74: bool report = base::RandDouble() <
sample_rate;
On 2015/12/09 11:46:22, jkarlin wrote:
> nit: s/report/should_report/
> 
> report sounds like an object, not a bool

Done, and same for evicted => should_evict.

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
File components/domain_reliability/context_unittest.cc (right):

https://codereview.chromium.org/1497803004/diff/40001/components/domain_relia...
components/domain_reliability/context_unittest.cc:249:
"\"request_elapsed_ms\":250,\"sample_rate\":1.0,"
On 2015/12/09 11:46:22, jkarlin wrote:
> Needs more tests. Include rates such as 0.0, 1.0, 0.33, and out of bounds
checks
> like -5 and 50. Also, shouldn't there be tests for success sample rates and
> failure sample rates?

I've added tests for 0.0 and 0.5 (1.0 is tested implicitly, as it's the default
in the tests).

Out of bounds checks are handled before the Config gets here. Right now,
anything negative behaves as if it's 0.0, and anything above 1.0 behaves as if
it's 1.0. Does that seem reasonable, and does it need explicit testing?

[jkarlin, 2015-12-10 18:58:48]

lgtm!

[Deprecated (see juliatuttle), 2015-12-10 19:21:55]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2015-12-10 19:24:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1497803004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1497803004/80001

[commit-bot: I haz the power, 2015-12-10 20:08:56]

Description was changed from

==========
Domain Reliability: Add sample_rate field to Beacon

Domain Reliability reports details of URL requests with a certain
probability (sample rate). By including the sample rate in the
beacon, the collector can estimate how many other requests (on
average) were not reported.

BUG=567307
==========

to

==========
Domain Reliability: Add sample_rate field to Beacon

Domain Reliability reports details of URL requests with a certain
probability (sample rate). By including the sample rate in the
beacon, the collector can estimate how many other requests (on
average) were not reported.

BUG=567307
==========

[commit-bot: I haz the power, 2015-12-10 20:08:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-10 20:09:40]

Description was changed from

==========
Domain Reliability: Add sample_rate field to Beacon

Domain Reliability reports details of URL requests with a certain
probability (sample rate). By including the sample rate in the
beacon, the collector can estimate how many other requests (on
average) were not reported.

BUG=567307
==========

to

==========
Domain Reliability: Add sample_rate field to Beacon

Domain Reliability reports details of URL requests with a certain
probability (sample rate). By including the sample rate in the
beacon, the collector can estimate how many other requests (on
average) were not reported.

BUG=567307

Committed: https://crrev.com/ccc4ffa155c61083ee5f57cbb3e756197494b8e1
Cr-Commit-Position: refs/heads/master@{#364446}
==========

[commit-bot: I haz the power, 2015-12-10 20:09:41]

Patchset 5 (id:??) landed as
https://crrev.com/ccc4ffa155c61083ee5f57cbb3e756197494b8e1
Cr-Commit-Position: refs/heads/master@{#364446}