Domain Reliability: Add sample_rate field to Beacon

components/domain_reliability/context.cc

Index: components/domain_reliability/context.cc
diff --git a/components/domain_reliability/context.cc b/components/domain_reliability/context.cc
index f0ce6c54c624cb15f1273495d72d5b9c84cc1a83..1852296df617598043e25426a982415b37481f1e 100644
--- a/components/domain_reliability/context.cc
+++ b/components/domain_reliability/context.cc
@@ -70,7 +70,8 @@ void DomainReliabilityContext::OnBeacon(
     scoped_ptr<DomainReliabilityBeacon> beacon) {
   bool success = (beacon->status == "ok");
 
-  bool reported = config().DecideIfShouldReportRequest(success);
+  double sample_rate;
+  bool reported = config().DecideIfShouldReportRequest(success, &sample_rate);

[jkarlin, 2015/12/07 14:57:27]

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/  ?

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

Acknowledged.

   UMA_HISTOGRAM_BOOLEAN("DomainReliability.BeaconReported", reported);
   if (!reported) {
     // If the beacon isn't queued to be reported, it definitely cannot evict
@@ -79,6 +80,7 @@ void DomainReliabilityContext::OnBeacon(
     LogOnBeaconDidEvictHistogram(false);
     return;
   }
+  beacon->weight = 1.0 / sample_rate;

[jkarlin, 2015/12/07 14:57:27]

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? 

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

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

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

Acknowledged.

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

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.

 
   UMA_HISTOGRAM_SPARSE_SLOWLY("DomainReliability.ReportedBeaconError",
                               -beacon->chrome_error);