Add one-time randomization support for FieldTrial, and the ability to

base/metrics/field_trial.cc

Index: base/metrics/field_trial.cc
diff --git a/base/metrics/field_trial.cc b/base/metrics/field_trial.cc
index c1416dbce2e80a61bc188104c49780c82b9b57c2..44c56b2ec2dfc480577dd9939a1878f3d01619bf 100644
--- a/base/metrics/field_trial.cc
+++ b/base/metrics/field_trial.cc
@@ -1,11 +1,14 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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 "base/metrics/field_trial.h"
 
+#include "base/file_util.h"
 #include "base/logging.h"
+#include "base/pickle.h"
 #include "base/rand_util.h"
+#include "base/sha1.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 
@@ -37,10 +40,11 @@ FieldTrial::FieldTrial(const std::string& name,
   : name_(name),
     divisor_(total_probability),
     default_group_name_(default_group_name),
-    random_(static_cast<Probability>(divisor_ * base::RandDouble())),
+    random_(static_cast<Probability>(divisor_ * RandDouble())),
     accumulated_group_probability_(0),
     next_group_number_(kDefaultGroupNumber+1),
-    group_(kNotFinalized) {
+    group_(kNotFinalized),
+    disable_field_trial_(false) {
   DCHECK_GT(total_probability, 0);
   DCHECK(!default_group_name_.empty());
   FieldTrialList::Register(this);
@@ -51,7 +55,7 @@ FieldTrial::FieldTrial(const std::string& name,
   DCHECK_GT(day_of_month, 0);
   DCHECK_LT(day_of_month, 32);
 
-  base::Time::Exploded exploded;
+  Time::Exploded exploded;
   exploded.year = year;
   exploded.month = month;
   exploded.day_of_week = 0;  // Should be unused.
@@ -61,8 +65,26 @@ FieldTrial::FieldTrial(const std::string& name,
   exploded.second = 0;
   exploded.millisecond = 0;
 
-  base::Time expiration_time = Time::FromLocalExploded(exploded);
-  disable_field_trial_ = (GetBuildTime() > expiration_time) ? true : false;
+  Time expiration_time = Time::FromLocalExploded(exploded);
+  if (GetBuildTime() > expiration_time) {
+    Disable();
+  }

[jar (doing other things), 2011/04/21 01:03:50]

style nit: Don't bother with braces on one line if statements (unless other
clause has braces) in this file.

[Jói, 2011/04/21 19:50:33]

Done.

+}
+
+void FieldTrial::UseOneTimeRandomization(const std::string& machine_id) {
+  DCHECK_EQ(group_, kNotFinalized);
+  if (machine_id.empty()) {
+    NOTREACHED();
+    disable_field_trial_ = true;
+    return;
+  }
+
+  random_ = static_cast<Probability>(
+      divisor_ * MachineIdToUniformDouble(machine_id));

[jar (doing other things), 2011/04/21 01:03:50]

Is your hope to make these repeatable on a given machine?  This doesn't seem
that critical or useful.  In fact, it makes it hard for a caller to get
different values from other users.

If you really want to go this route, you should accept an argument "salt" rather
than machine_id, and that salt should be added to a machine specific string as
the basis of the consistent randomization.  That would at least allow different
experiments to have different selections from their cases.  (i.e., when the
random number comes out near 0.001, we don't always choose the first trial
suggested in all field trials).


It seems much nicer to not depend on the caller for any (explicit) entropy.  If
you do want consistent randomness, you could depend on the field_trial_name_ as
your salt, and combine it with a persistent unique value.  That would simplify
the API, and still get you the proposed perpetual(?) consistency.

With the current interface, what guarantee do you have that the machine_id will
persist?

We generate a random value in the metrics system when we do an UMA upload to tag
the client.  We are very cautious NOT to generate a unique tag unless the user
opts-in to upload, as folks concerned with privacy did not want any tags on
their machine.  The interesting thing is that you're trying to define a tag
(i.e., persistent machine_id), and that may fly in the face of users that DON'T
want to be tagged.

My suggestion is to accelerate the tagging when UMA is enabled, so that you can
call it now and get that id as your major source of entropy.  You should
probably be able to detect that it is not yet set, and force your experiment
into a disabled mode (until the day that the UMA ID is available... and then
you'd start to use it).

[Jói, 2011/04/21 19:50:33]

I'm now using the UMA ID and salting with the trial name.

+}
+
+void FieldTrial::Disable() {
+  disable_field_trial_ = true;
 }
 
 int FieldTrial::AppendGroup(const std::string& name,
@@ -80,7 +102,7 @@ int FieldTrial::AppendGroup(const std::string& name,
     // This is the group that crossed the random line, so we do the assignment.
     group_ = next_group_number_;
     if (name.empty())
-      base::StringAppendF(&group_name_, "%d", group_);
+      StringAppendF(&group_name_, "%d", group_);
     else
       group_name_ = name;
   }
@@ -127,6 +149,28 @@ Time FieldTrial::GetBuildTime() {
   return integral_build_time;
 }
 
+// static
+double FieldTrial::MachineIdToUniformDouble(const std::string& machine_id) {
+  // We use SHA-1 over the unique ID we have for machine/user (we add
+  // the user part by adding the user's home directory).  SHA-1 is designed
+  // to produce a uniformly random spread in its output space, even for
+  // nearly-identical inputs, so it helps make this data look like a uniform
+  // random distribution.
+  Pickle pickle;
+  pickle.WriteString(machine_id);
+  file_util::GetHomeDir().WriteToPickle(&pickle);

[jar (doing other things), 2011/04/21 01:03:50]

Pickling is a waste.  It doesn't add to the entropy, and just takes time and
memory.  

I'd also suggest that you rename this function HashStringToDouble()

it is really a hash function.

The fact that the string is (or might be) a machine id seems inconsequential, so
the argument probably shouldn't be named machine_id.

[Jói, 2011/04/21 19:50:33]

Done.

+
+  unsigned char sha1_hash[SHA1_LENGTH];
+  SHA1HashBytes(reinterpret_cast<const unsigned char*>(pickle.data()),
+                pickle.size(),
+                sha1_hash);
+
+  COMPILE_ASSERT(sizeof(uint64) < sizeof(sha1_hash), need_more_data);
+  uint64* bits = reinterpret_cast<uint64*>(&sha1_hash[0]);
+
+  return BitsToRandDouble(*bits);
+}
+
 //------------------------------------------------------------------------------
 // FieldTrialList methods and members.