Extract RandInt() from IsSampledIn() for deterministic testing.

Postmortem Bug: https://monorail-staging.appspot.com/p/chromium/issues/detail?id=899293

Bug: 931710
Change-Id: I94c3e321d77890c59987894ce00f16a3d7ec2128
Reviewed-on: https://chromium-review.googlesource.com/c/1479910
Commit-Queue: Brian White <bcwhite@chromium.org>
Auto-Submit: Brian White <bcwhite@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635627}

components/ukm/ukm_recorder_impl.cc

@@ -665,11 +665,15 @@ void UkmRecorderImpl::LoadExperimentSamplingInfo() {
     default_sampling_rate_ = 1;
 }
 
+int UkmRecorderImpl::RandInt(int begin, int end) {
+  return base::RandInt(begin, end);
+}
+
 bool UkmRecorderImpl::IsSampledIn(int sampling_rate) {
   // A sampling rate of 0 is "never"; everything else is 1-in-N but skip
   // the RandInt() call if N==1.
   return sampling_rate > 0 &&
-         (sampling_rate == 1 || base::RandInt(1, sampling_rate) == 1);
+         (sampling_rate == 1 || RandInt(1, sampling_rate) == 1);
 }
 
 void UkmRecorderImpl::StoreWhitelistedEntries() {

components/ukm/ukm_recorder_impl.h

@@ -68,9 +68,11 @@ class UkmRecorderImpl : public UkmRecorder {
       const IsWebstoreExtensionCallback& callback);
 
  protected:
-  // Calculates sampled in/out based on a given |rate|. This is virtual so
-  // it can be overriden by tests.
-  virtual bool IsSampledIn(int sampling_rate);
+  // Generates a random number. This is virtual so it can be overriden by tests.
+  virtual int RandInt(int begin, int end);
+
+  // Calculates sampled in/out based on a given |rate|.
+  bool IsSampledIn(int sampling_rate);
 
   // Cache the list of whitelisted entries from the field trial parameter.
   void StoreWhitelistedEntries();

components/ukm/ukm_recorder_impl_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/metrics/ukm_source_id.h"
+#include "base/test/scoped_feature_list.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "services/metrics/public/cpp/ukm_entry_builder.h"
 #include "services/metrics/public/cpp/ukm_source.h"
@@ -21,22 +22,27 @@ class TestUkmRecorderImpl : public UkmRecorderImpl {
   TestUkmRecorderImpl() {}
   ~TestUkmRecorderImpl() override {}
 
-  void set_sampled_in(bool sampled_in) { sampled_in_ = sampled_in; }
+  // Set the value returned by the "random number generator" used to control
+  // whether something gets sampled "in" (1) or "out" (anything else).
+  void set_rand_int(int rand_int) { rand_int_ = rand_int; }
 
-  size_t sampled_callback_count() { return sampled_callback_count_; }
+  // Used to verify that random sampling is taking place.
+  size_t rand_callback_count() { return rand_callback_count_; }
 
  protected:
   // UkmRecorderImpl:
   bool ShouldRestrictToWhitelistedSourceIds() const override { return false; }
   bool ShouldRestrictToWhitelistedEntries() const override { return false; }
-  bool IsSampledIn(int sampling_rate) override {
-    ++sampled_callback_count_;
-    return sampled_in_;
+
+  // Override the "random" number returned and count number of calls.
+  int RandInt(int begin, int end) override {
+    ++rand_callback_count_;
+    return rand_int_;
   }
 
  private:
-  bool sampled_in_ = true;
-  size_t sampled_callback_count_ = 0;
+  int rand_int_ = 0;
+  size_t rand_callback_count_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(TestUkmRecorderImpl);
 };
@@ -52,9 +58,9 @@ class UkmRecorderImplTest : public testing::Test {
 
   UkmRecorderImpl& impl() { return impl_; }
 
-  void set_sampled_in(bool sampled_in) { impl_.set_sampled_in(sampled_in); }
+  void set_rand_int(int rand_int) { impl_.set_rand_int(rand_int); }
 
-  size_t sampled_callback_count() { return impl_.sampled_callback_count(); }
+  size_t rand_callback_count() { return impl_.rand_callback_count(); }
 
   size_t source_sampling_count() { return impl_.source_event_sampling_.size(); }
 
@@ -76,26 +82,29 @@ class UkmRecorderImplTest : public testing::Test {
 };
 
 TEST_F(UkmRecorderImplTest, PageSamplingCondition) {
-  EXPECT_FALSE(impl().UkmRecorderImpl::IsSampledIn(0));
-  EXPECT_TRUE(impl().UkmRecorderImpl::IsSampledIn(1));
-
-  // Actual sampling is "random" so there's no single operation to test.
-  // Instead, sample many times and expect that some, but less than 1/2,
-  // will be sampled-in.  This is sufficient to ensure that the condition
-  // is not "dead" (always false) and has not been mistakenly reversed...
-  // without the test being flaky.
-  const int kRandomCheckCount = 1000;
-  int sampled_in_count = 0;
-  for (int i = 0; i < kRandomCheckCount; ++i) {
-    // Sample 1-in-10...
-    if (impl().UkmRecorderImpl::IsSampledIn(10))
-      sampled_in_count += 1;
+  EXPECT_FALSE(impl().IsSampledIn(0));
+  EXPECT_TRUE(impl().IsSampledIn(1));
+
+  const int kSamplingRate = 10;
+  for (int i = 1; i <= kSamplingRate; ++i) {
+    set_rand_int(i);
+    EXPECT_EQ(i == 1, impl().IsSampledIn(kSamplingRate));
   }
-  EXPECT_LT(0, sampled_in_count);
-  EXPECT_GT(kRandomCheckCount / 2, sampled_in_count);
 }
 
 TEST_F(UkmRecorderImplTest, PageSampling) {
+  const base::Feature kUkmSamplingRateFeature{
+      "UkmSamplingRate", base::FEATURE_DISABLED_BY_DEFAULT};
+  std::map<std::string, std::string> sampling_params;
+
+  // A default sampling of "2" will ensure that RandInt() gets called for
+  // every "sampled in" check.
+  sampling_params["_default_sampling"] = "2";
+
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeatureWithParameters(
+      kUkmSamplingRateFeature, sampling_params);
+
   SourceId page1_source =
       ConvertToSourceId(101, base::UkmSourceId::Type::NAVIGATION_ID);
   UkmSource::NavigationData page1_nav;
@@ -106,19 +115,19 @@ TEST_F(UkmRecorderImplTest, PageSampling) {
   AddEntry(builders::PageLoad(page1_source)
                .SetDocumentTiming_NavigationToLoadEventFired(1000)
                .TakeEntry());
-  EXPECT_EQ(1U, sampled_callback_count());
+  EXPECT_EQ(1U, rand_callback_count());
 
   // Second event will use what was already determined.
   AddEntry(builders::PageLoad(page1_source)
                .SetDocumentTiming_NavigationToLoadEventFired(2000)
                .TakeEntry());
-  EXPECT_EQ(1U, sampled_callback_count());
+  EXPECT_EQ(1U, rand_callback_count());
 
   // Different event will again do the callback.
   AddEntry(builders::Memory_Experimental(page1_source)
                .SetCommandBuffer(3000)
                .TakeEntry());
-  EXPECT_EQ(2U, sampled_callback_count());
+  EXPECT_EQ(2U, rand_callback_count());
 
   SourceId page2_source =
       ConvertToSourceId(102, base::UkmSourceId::Type::NAVIGATION_ID);
@@ -130,7 +139,7 @@ TEST_F(UkmRecorderImplTest, PageSampling) {
   AddEntry(builders::PageLoad(page2_source)
                .SetDocumentTiming_NavigationToLoadEventFired(1000)
                .TakeEntry());
-  EXPECT_EQ(3U, sampled_callback_count());
+  EXPECT_EQ(3U, rand_callback_count());
 
   // First report won't clear this information.
   EXPECT_EQ(2U, source_sampling_count());