Domain Reliability: Add sample_rate field to Beacon

components/domain_reliability/context.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/domain_reliability/context.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/json/json_writer.h"
@@ skipping 52 matching lines @@
 }
 
 DomainReliabilityContext::~DomainReliabilityContext() {
   ClearBeacons();
 }
 
 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
     // an older beacon. (This histogram is also logged below based on whether
     // an older beacon was actually evicted.)
     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);
   if (!beacon->server_ip.empty()) {
     UMA_HISTOGRAM_SPARSE_SLOWLY(
         "DomainReliability.ReportedBeaconError_HasServerIP",
         -beacon->chrome_error);
   }
   // TODO(ttuttle): Histogram HTTP response code?
 
@@ skipping 152 matching lines @@
   delete beacons_.front();
   beacons_.pop_front();
 
   // If that just removed a beacon counted in uploading_beacons_size_, decrement
   // that.
   if (uploading_beacons_size_ > 0)
     --uploading_beacons_size_;
 }
 
 }  // namespace domain_reliability