Implement A/B experiment for anti-DDoS.

base/metrics/field_trial.cc

 // 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/logging.h"
 #include "base/rand_util.h"
 #include "base/sha1.h"
 #include "base/string_util.h"
@@ skipping 351 matching lines @@
 // static
 size_t FieldTrialList::GetFieldTrialCount() {
   if (!global_)
     return 0;
   AutoLock auto_lock(global_->lock_);
   return global_->registered_.size();
 }
 
 // static
 bool FieldTrialList::IsOneTimeRandomizationEnabled() {
-  DCHECK(global_);

[jar (doing other things), 2011/06/16 18:50:12]

NIT: Ok... based on phone chat, I'm fine with removing this DCHECK(), and
putting in a TODO() to get it back in RSN.

I thought it would have been easy ot leave it in now, but I'm convinced it has
broader ramifications (wider unit test changes need to be made since you're
putting the field trial into code that gets tickled/tested).

+  // This can occur in valid cases, e.g. when certain unit tests initialize
+  // and tickle code that wants to set up a trial only if one-time
+  // randomization is supported, so we do not DCHECK.

[jar (doing other things), 2011/06/12 06:49:12]

Why is it valuable to test in this mode, and not initialize the global?

I can believe that there are (now) tests that don't initialize it, but the class
was written with the expectation that it is always initialized, and it doesn't
seem critical to test without it.  In fact, this global is always present *IF*
we are capable of recording stats (i.e., we have a client_id that can be used in
a stats report).  I don't see the value of enabling field trials if we are not
reporting results.

Bottom line: I *think* I'd prefer the DCHECK() to continue to exist, and I
*think* I'd prefer to see test code fully initialize this module to test it.

[Jói, 2011/06/13 17:27:14]

That's just it, this is the method my field trial setup code calls to check if
the trial should be enabled.  I could leave the DCHECK here and in the field
trial setup code first call FieldTrialList::GetFieldTrialCount to see if it's
zero, but it seems simpler to remove the DCHECK since I don't think it will help
us catch any usage that is "wrong".

[jar (doing other things), 2011/06/14 02:28:46]

The common(?) error case is for someone to place a field-trial into the source
code, but have the setting happen soooooo early, that we haven't enabled the
field trials (oops).  They then go along happily until they realize that they
aren't getting results (sigh), and then complain to me... and we debug... and
then they fix it.  Better seems to be to have a DCHECK fire as the developer
lands their test ;-).

Am I missing something?  Am I confused about the scenarios?

On 2011/06/13 17:27:14, Jói wrote:

   if (!global_)
     return false;
 
   return !global_->client_id_.empty();
 }
 
 // static
 const std::string& FieldTrialList::client_id() {
   DCHECK(global_);
   if (!global_)
     return EmptyString();
 
   return global_->client_id_;
 }
 
 FieldTrial* FieldTrialList::PreLockedFind(const std::string& name) {
   RegistrationList::iterator it = registered_.find(name);
   if (registered_.end() == it)
     return NULL;
   return it->second;
 }
 
 }  // namespace base