Modify rappor parameters.

Change the default parameters for ETLD+1 Rappor metrics.
Allow number of cohorts to be set per metric.
Modify metric names to distinguish from reports with old parameters.

BUG=370599

Review URL: https://codereview.chromium.org/264123004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273100 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 67 insertions, 21 deletions

diff --git a/components/rappor/byte_vector_utils.cc b/components/rappor/byte_vector_utils.cc
index 57f2574..69abcd4 100644
--- a/components/rappor/byte_vector_utils.cc
+++ b/components/rappor/byte_vector_utils.cc
@@ -78,9 +78,17 @@ bool HMAC_DRBG_Update(const std::string& provided_data,
 
 }  // namespace
 
+ByteVector* ByteVectorAnd(const ByteVector& lhs, ByteVector* rhs) {
+  DCHECK_EQ(lhs.size(), rhs->size());
+  for (size_t i = 0; i < lhs.size(); ++i) {
+    (*rhs)[i] = lhs[i] & (*rhs)[i];
+  }
+  return rhs;
+}
+
 ByteVector* ByteVectorOr(const ByteVector& lhs, ByteVector* rhs) {
   DCHECK_EQ(lhs.size(), rhs->size());
-  for (size_t i = 0, len = lhs.size(); i < len; ++i) {
+  for (size_t i = 0; i < lhs.size(); ++i) {
     (*rhs)[i] = lhs[i] | (*rhs)[i];
   }
   return rhs;
@@ -90,7 +98,7 @@ ByteVector* ByteVectorMerge(const ByteVector& mask,
                             const ByteVector& lhs,
                             ByteVector* rhs) {
   DCHECK_EQ(lhs.size(), rhs->size());
-  for (size_t i = 0, len = lhs.size(); i < len; ++i) {
+  for (size_t i = 0; i < lhs.size(); ++i) {
     (*rhs)[i] = (lhs[i] & ~mask[i]) | ((*rhs)[i] & mask[i]);
   }
   return rhs;
@@ -127,6 +135,8 @@ ByteVector ByteVectorGenerator::GetWeightedRandomByteVector(
       return *ByteVectorOr(GetRandomByteVector(), &bytes);
     case PROBABILITY_50:
       return bytes;
+    case PROBABILITY_25:
+      return *ByteVectorAnd(GetRandomByteVector(), &bytes);
   }
   NOTREACHED();
   return bytes;
diff --git a/components/rappor/byte_vector_utils.h b/components/rappor/byte_vector_utils.h
index 0e33041..2f2d8c0 100644
--- a/components/rappor/byte_vector_utils.h
+++ b/components/rappor/byte_vector_utils.h
@@ -17,6 +17,10 @@ namespace rappor {
 // A vector of 8-bit integers used to store a set of binary bits.
 typedef std::vector<uint8_t> ByteVector;
 
+// Computes a bitwise AND of byte vectors and stores the result in rhs.
+// Returns rhs for chaining.
+ByteVector* ByteVectorAnd(const ByteVector& lhs, ByteVector* rhs);
+
 // Computes a bitwise OR of byte vectors and stores the result in rhs.
 // Returns rhs for chaining.
 ByteVector* ByteVectorOr(const ByteVector& lhs, ByteVector* rhs);
diff --git a/components/rappor/byte_vector_utils_unittest.cc b/components/rappor/byte_vector_utils_unittest.cc
index 54df9ea..672fa8d 100644
--- a/components/rappor/byte_vector_utils_unittest.cc
+++ b/components/rappor/byte_vector_utils_unittest.cc
@@ -26,6 +26,15 @@ std::string HexToString(const char* hex) {
 
 }  // namespace
 
+TEST(ByteVectorTest, ByteVectorAnd) {
+  ByteVector lhs(2);
+  lhs[1] = 0x12;
+  ByteVector rhs(2);
+  rhs[1] = 0x03;
+
+  EXPECT_EQ(0x02, (*ByteVectorAnd(lhs, &rhs))[1]);
+}
+
 TEST(ByteVectorTest, ByteVectorOr) {
   ByteVector lhs(2);
   lhs[1] = 0x12;
diff --git a/components/rappor/proto/rappor_metric.proto b/components/rappor/proto/rappor_metric.proto
index e5efe21..c1d9cbf 100644
--- a/components/rappor/proto/rappor_metric.proto
+++ b/components/rappor/proto/rappor_metric.proto
@@ -20,7 +20,8 @@ message RapporReports {
   // partioned into cohorts in different ways, to allow better statistics and
   // increased coverage.  In particular, the cohort will serve to choose the
   // hash functions used for Bloom-filter-based reports.  The cohort is
-  // generated randomly by the client and is currently in the range [0,32).
+  // generated randomly by the client and is currently in the range [0,512).
+  // Was in range [0,32) in chrome versions before M37.
   optional int32 cohort = 1;
 
   // Each Report contains the values generated by the RAPPOR process for one
diff --git a/components/rappor/rappor_metric.cc b/components/rappor/rappor_metric.cc
index f8ff162..b4bd950 100644
--- a/components/rappor/rappor_metric.cc
+++ b/components/rappor/rappor_metric.cc
@@ -10,13 +10,15 @@ namespace rappor {
 
 RapporMetric::RapporMetric(const std::string& metric_name,
                            const RapporParameters& parameters,
-                           int32_t cohort)
+                           int32_t cohort_seed)
     : metric_name_(metric_name),
       parameters_(parameters),
       bloom_filter_(parameters.bloom_filter_size_bytes,
                     parameters.bloom_filter_hash_function_count,
-                    cohort * parameters.bloom_filter_hash_function_count) {
-  DCHECK_GE(cohort, 0);
+                    (cohort_seed % parameters.num_cohorts) *
+                        parameters.bloom_filter_hash_function_count) {
+  DCHECK_GE(cohort_seed, 0);
+  DCHECK_LT(cohort_seed, RapporParameters::kMaxCohorts);
 }
 
 RapporMetric::~RapporMetric() {}
diff --git a/components/rappor/rappor_metric_unittest.cc b/components/rappor/rappor_metric_unittest.cc
index 7c50fd4..b5707cf 100644
--- a/components/rappor/rappor_metric_unittest.cc
+++ b/components/rappor/rappor_metric_unittest.cc
@@ -13,6 +13,7 @@
 namespace rappor {
 
 const RapporParameters kTestRapporParameters = {
+    1 /* Num cohorts */,
     16 /* Bloom filter size bytes */,
     4 /* Bloom filter hash count */,
     PROBABILITY_75 /* Fake data probability */,
@@ -21,6 +22,7 @@ const RapporParameters kTestRapporParameters = {
     PROBABILITY_50 /* Zero coin probability */};
 
 const RapporParameters kTestStatsRapporParameters = {
+    1 /* Num cohorts */,
     50 /* Bloom filter size bytes */,
     4 /* Bloom filter hash count */,
     PROBABILITY_75 /* Fake data probability */,
diff --git a/components/rappor/rappor_parameters.cc b/components/rappor/rappor_parameters.cc
index ede7636..ae5b7ba 100644
--- a/components/rappor/rappor_parameters.cc
+++ b/components/rappor/rappor_parameters.cc
@@ -4,12 +4,14 @@
 
 #include "components/rappor/rappor_parameters.h"
 
+#include "base/compiler_specific.h"
 #include "base/strings/stringprintf.h"
 
 namespace rappor {
 
 std::string RapporParameters::ToString() const {
-  return base::StringPrintf("{ %d, %d, %d, %d, %d, %d }",
+  return base::StringPrintf("{ %d, %d, %d, %d, %d, %d, %d }",
+      num_cohorts,
       bloom_filter_size_bytes,
       bloom_filter_hash_function_count,
       fake_prob,
@@ -18,4 +20,6 @@ std::string RapporParameters::ToString() const {
       zero_coin_prob);
 }
 
+const int RapporParameters::kMaxCohorts = 512;
+
 }  // namespace rappor
diff --git a/components/rappor/rappor_parameters.h b/components/rappor/rappor_parameters.h
index 77b1614..ca625ba 100644
--- a/components/rappor/rappor_parameters.h
+++ b/components/rappor/rappor_parameters.h
@@ -12,6 +12,7 @@ namespace rappor {
 enum Probability {
   PROBABILITY_75,    // 75%
   PROBABILITY_50,    // 50%
+  PROBABILITY_25,    // 25%
 };
 
 // An object describing a rappor metric and the parameters used to generate it.
@@ -22,6 +23,12 @@ struct RapporParameters {
   // Get a string representing the parameters, for DCHECK_EQ.
   std::string ToString() const;
 
+  // The maximum number of cohorts we divide clients into.
+  static const int kMaxCohorts;
+
+  // The number of cohorts to divide the reports for this metric into.
+  int num_cohorts;
+
   // The number of bytes stored in the Bloom filter.
   int bloom_filter_size_bytes;
   // The number of hash functions used in the Bloom filter.
diff --git a/components/rappor/rappor_pref_names.cc b/components/rappor/rappor_pref_names.cc
index e62c764..ff444ef 100644
--- a/components/rappor/rappor_pref_names.cc
+++ b/components/rappor/rappor_pref_names.cc
@@ -7,8 +7,11 @@
 namespace rappor {
 namespace prefs {
 
-// A randomly generated number, which determines cohort data is reported for.
-const char kRapporCohort[] = "rappor.cohort";
+// Deprecated: Old cohort selection pref, should be cleared.
+const char kRapporCohortDeprecated[] = "rappor.cohort";
+
+// A randomly generated number, which determines cohorts data is reported for.
+const char kRapporCohortSeed[] = "rappor.cohort_seed";
 
 // A base-64 encoded, randomly generated byte string, which is used as a seed
 // for redacting collected data.
diff --git a/components/rappor/rappor_pref_names.h b/components/rappor/rappor_pref_names.h
index ec42b76..9cb60ca 100644
--- a/components/rappor/rappor_pref_names.h
+++ b/components/rappor/rappor_pref_names.h
@@ -10,7 +10,8 @@ namespace prefs {
 
 // Alphabetical list of preference names specific to the Rappor
 // component. Keep alphabetized, and document each in the .cc file.
-extern const char kRapporCohort[];
+extern const char kRapporCohortDeprecated[];
+extern const char kRapporCohortSeed[];
 extern const char kRapporSecret[];
 
 }  // namespace prefs
diff --git a/components/rappor/rappor_service.cc b/components/rappor/rappor_service.cc
index 01e6ab0..67bc4aa 100644
--- a/components/rappor/rappor_service.cc
+++ b/components/rappor/rappor_service.cc
@@ -22,9 +22,6 @@ namespace rappor {
 
 namespace {
 
-// The number of cohorts we divide clients into.
-const int kNumCohorts = 32;
-
 // Seconds before the initial log is generated.
 const int kInitialLogIntervalSeconds = 15;
 // Interval between ongoing logs.
@@ -46,12 +43,13 @@ GURL GetServerUrl() {
 
 const RapporParameters kRapporParametersForType[NUM_RAPPOR_TYPES] = {
     // ETLD_PLUS_ONE_RAPPOR_TYPE
-    {16 /* Bloom filter size bytes */,
+    {128 /* Num cohorts */,
+     16 /* Bloom filter size bytes */,
      2 /* Bloom filter hash count */,
-     rappor::PROBABILITY_75 /* Fake data probability */,
+     rappor::PROBABILITY_50 /* Fake data probability */,
      rappor::PROBABILITY_50 /* Fake one probability */,
      rappor::PROBABILITY_75 /* One coin probability */,
-     rappor::PROBABILITY_50 /* Zero coin probability */},
+     rappor::PROBABILITY_25 /* Zero coin probability */},
 };
 
 }  // namespace
@@ -96,20 +94,24 @@ void RapporService::OnLogInterval() {
 // static
 void RapporService::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterStringPref(prefs::kRapporSecret, std::string());
-  registry->RegisterIntegerPref(prefs::kRapporCohort, -1);
+  registry->RegisterIntegerPref(prefs::kRapporCohortDeprecated, -1);
+  registry->RegisterIntegerPref(prefs::kRapporCohortSeed, -1);
 }
 
 void RapporService::LoadCohort(PrefService* pref_service) {
   DCHECK(!IsInitialized());
-  cohort_ = pref_service->GetInteger(prefs::kRapporCohort);
+  // Ignore and delete old cohort parameter.
+  pref_service->ClearPref(prefs::kRapporCohortDeprecated);
+
+  cohort_ = pref_service->GetInteger(prefs::kRapporCohortSeed);
   // If the user is already assigned to a valid cohort, we're done.
-  if (cohort_ >= 0 && cohort_ < kNumCohorts)
+  if (cohort_ >= 0 && cohort_ < RapporParameters::kMaxCohorts)
     return;
 
   // This is the first time the client has started the service (or their
   // preferences were corrupted).  Randomly assign them to a cohort.
-  cohort_ = base::RandGenerator(kNumCohorts);
-  pref_service->SetInteger(prefs::kRapporCohort, cohort_);
+  cohort_ = base::RandGenerator(RapporParameters::kMaxCohorts);
+  pref_service->SetInteger(prefs::kRapporCohortSeed, cohort_);
 }
 
 void RapporService::LoadSecret(PrefService* pref_service) {
diff --git a/components/rappor/rappor_service_unittest.cc b/components/rappor/rappor_service_unittest.cc
index a36e960..a91db8c 100644
--- a/components/rappor/rappor_service_unittest.cc
+++ b/components/rappor/rappor_service_unittest.cc
@@ -25,6 +25,7 @@ class TestRapporService : public RapporService {
 
 TEST(RapporServiceTest, RecordAndExportMetrics) {
   const RapporParameters kTestRapporParameters = {
+      1 /* Num cohorts */,
       16 /* Bloom filter size bytes */,
       4 /* Bloom filter hash count */,
       PROBABILITY_75 /* Fake data probability */,