Expand FeatureList to support FieldTrial association.

base/feature_list.cc

Index: base/feature_list.cc
diff --git a/base/feature_list.cc b/base/feature_list.cc
index 16eba67e99fd1ecd467cc1b0f20fde6c7f9b9149..fa189154e946030fb0259850b05ba1f1cc325a69 100644
--- a/base/feature_list.cc
+++ b/base/feature_list.cc
@@ -7,6 +7,7 @@
 #include <vector>
 
 #include "base/logging.h"
+#include "base/metrics/field_trial.h"
 #include "base/strings/string_split.h"
 
 namespace base {
@@ -37,13 +38,43 @@ void FeatureList::InitializeFromCommandLine(
   // Process disabled features first, so that disabled ones take precedence over
   // enabled ones (since RegisterOverride() uses insert()).
   for (const auto& feature_name : SplitFeatureListString(disable_features)) {
-    RegisterOverride(feature_name, OVERRIDE_DISABLE_FEATURE);
+    RegisterOverride(feature_name, OVERRIDE_DISABLE_FEATURE, nullptr);
   }
   for (const auto& feature_name : SplitFeatureListString(enable_features)) {
-    RegisterOverride(feature_name, OVERRIDE_ENABLE_FEATURE);
+    RegisterOverride(feature_name, OVERRIDE_ENABLE_FEATURE, nullptr);
   }
 }
 
+void FeatureList::RegisterFieldTrialOverride(const std::string& feature_name,
+                                             OverrideState override_state,
+                                             FieldTrial* field_trial) {
+  RegisterOverride(feature_name, override_state, field_trial);
+}
+
+FieldTrial* FeatureList::AssociateReportingFieldTrial(
+    const std::string& feature_name,
+    OverrideState for_overridden_state,
+    const std::string& field_trial_name,
+    const std::string& group_name) {
+  auto it = overrides_.find(feature_name);
+  // No association if there's no override entry or its state doesn't match
+  // |for_overridden_state|.
+  if (it == overrides_.end() ||
+      it->second.overridden_state != for_overridden_state) {
+    return nullptr;
+  }
+
+  // Only one associated field trial is supported per feature. This is generally
+  // enforced server-side.
+  if (it->second.field_trial) {

[Ilya Sherman, 2015/09/16 00:50:43]

Maybe it's ok if the already set field trial matches the new one?

[Alexei Svitkine (slow), 2015/09/16 20:54:08]

Is there a specific scenario you're imagining? In my mind, I can't think of a
case where we would want that.

i.e.
There shouldn't be two server-side configs trying to affect the same feature
(and this will be validated server-side in the analyzer per the design) for the
same population of users.
And within a single config, association with command-line flags takes precedence
over forcing the state via the experiment. (i.e. VariationsService first calls
all the Associate* functions and does an early return when one succeeds, before
calling Register* functions for that study).

[Ilya Sherman, 2015/09/17 01:02:46]

I think you're right.  I was still trying to wrap my head around the interaction
of AssociateReportingFieldTrial() and RegisterFieldTrialOverride().

+    NOTREACHED();

[Ilya Sherman, 2015/09/16 00:50:43]

nit: Probably helpful to print out both field trial names, or at least the
feature name.

[Alexei Svitkine (slow), 2015/09/22 21:19:59]

Done.

+    return nullptr;
+  }
+  it->second.field_trial =
+      FieldTrialList::CreateFieldTrial(field_trial_name, group_name);

[Ilya Sherman, 2015/09/16 00:50:43]

Hmm, why create the field trial?  What guarantees that it doesn't already exist?
 That is, why doesn't this method accept a FieldTrial* param, same as the other
method?

[Alexei Svitkine (slow), 2015/09/16 20:54:08]

The field trial should only be created through this mechanism if it's been
forced through the command-line. This is similar logic to "forcing_flag" logic
already in VariationsService.

The difference for why the trial is created here vs. for "forcing_flag" where
it's created in VariationsService is because VariationsService can test directly
if a flag was forced, but can't (without adding an API) do so for feature
forcing.

If we wanted the trial to be created in VariationsService, then we'd need to add
an API on FeatureList to tell if a trial was overridden by the command-line
(either one or two functions depending if you want to combine enable/disable
into a more complicated function or have separate one). Then, when you decide to
create the trial, you still need to associate it with the entry - so a separate
Associate() call is still needed.

Per the above, I believe the approach implemented in this CL is actually
simpler, because it encompasses the complexity in the single call, instead of
splitting this logic between here and VariationService and exposing more API
surface.

[Ilya Sherman, 2015/09/17 01:02:46]

I agree that it's simpler in a way, but it also seems to tightly couple the
VariationsService implementation with this one.  I had a lot of trouble
understanding the FeatureList API until I read the VariationsService code.  But,
I'm also not thinking of a great way to simplify this without introducing the
split API like you described.  Maybe the code would be clearer if the field
trial creation were delegated to a callback passed in by the caller, but maybe
that would actually just be even more confusing... :/

[Alexei Svitkine (slow), 2015/09/22 21:19:59]

Yeah, I feel like the callback would be even harder to follow. The good thing
about this API is it's not one that actually users of FeatureList API will need
to use - it's only for variations service, so even if it's hard to follow, not a
lot of people need to follow it.

[Ilya Sherman, 2015/09/24 01:11:32]

After thinking about this more, I think I actually prefer exposing two separate
methods in the API.  It does increase the API surface area slightly, but it also
makes it possible to better separate the VariationsService logic from the
FeatureList logic, and makes the FeatureList logic clearer IMO.  Something like
this:

bool FeatureList::HasOverriddenFeature(const std::string& feature_name,
                                       OverrideState state) {
  auto it = overrides_.find(feature_name);
  return it != overrides_.end() && it->second.overridden_state == state;
}

FieldTrial* FeatureList::AssociateReportingFieldTrial(
    const std::string& feature_name,
    OverrideState for_overridden_state,
    FieldTrial* field_trial) {
  DCHECK(HasOverriddenFeature(feature_name, for_overridden_state));
  
  // Only one associated field trial is supported per feature. This is generally
  // enforced server-side.
  OverrideEntry& entry = overrides_[feature_name];
  if (entry.field_trial) {
    NOTREACHED() << "Feature = " << feature_name
                 << ", has trial = " << entry.field_trial->trial_name()
                 << ", associating trial = " << field_trial_name;
    return;
  }
  
  entry.field_trial = field_trial;
}

[Alexei Svitkine (slow), 2015/09/24 20:49:20]

I'm ok with this approach. It does let some code to be slightly simplified in
variations_seed_processor.cc too.

One thing I'm a little bit iffy on is HasOverriddenFeature() semantics. In your
suggestion, it will return true even if the feature is overridden by something
other than the command-line. And will return different results as more override
methods are called during initialization.

I understand that this helps detect overlap issues in practice (i.e. we don't
want two trials affecting the same feature), by having a DCHECK in the Associate
function, but it still feels like the logic is wrong for VariationsSeedProcessor
using it.

In fact, it doesn't just feel wrong, it is wrong. Because with this approach,
the check flow will be:

if (HasOverriddenFeature()) {
  trial = CreateFieldTrial();
  AssociateFieldTrial(trial);
}

In a debug build, it will just crash. But in prod, it will result in weirdness
if ever it does happen. Specifically, the trial will still be created, although
I guess not associated.

I'd rather avoid that weird behavior. So, we can make the function
IsFeatureOverriddenFromCommandLine() which specifically checks that field_trial
is null on the override entry. This makes the semantics much clearer - i.e.
makes the variations code associate only with command-line overridden things,
instead of all overridden things - which is what it's supposed to be doing,

(It does lose the NOTREACHED() firing in this case, but I think that's OK
because likely in such a case, the dice-rolled group of the experiment might try
to force the feature, which will trigger a NOTREACHED() in
RegisterFieldTrialOverride()).

Anyway, I've implemented the above suggestion % making the new function
IsFeatureOverriddenFromCommandLine() - which required an extra book-keeping
variable on the OverrideEntry to track whether the override is from a trial (or
otherwise from a command-line). Added a test for the new function as well.

[Ilya Sherman, 2015/09/25 03:47:22]

Nice, I like the explicit command line check even better than what I suggested
=)

+  return it->second.field_trial;
+}
+
 // static
 bool FeatureList::IsEnabled(const Feature& feature) {
   return GetInstance()->IsFeatureEnabled(feature);
@@ -81,7 +112,11 @@ bool FeatureList::IsFeatureEnabled(const Feature& feature) {
   auto it = overrides_.find(feature.name);
   if (it != overrides_.end()) {
     const OverrideEntry& entry = it->second;
-    // TODO(asvitkine) Expand this section as more support is added.
+
+    // Activate the corresponding field trial, if necessary.
+    if (entry.field_trial)
+      entry.field_trial->group();
+
     return entry.overridden_state == OVERRIDE_ENABLE_FEATURE;
   }
   // Otherwise, return the default state.
@@ -89,9 +124,11 @@ bool FeatureList::IsFeatureEnabled(const Feature& feature) {
 }
 
 void FeatureList::RegisterOverride(const std::string& feature_name,
-                                   OverrideState overridden_state) {
+                                   OverrideState overridden_state,
+                                   FieldTrial* field_trial) {
   DCHECK(!initialized_);
-  overrides_.insert(make_pair(feature_name, OverrideEntry(overridden_state)));
+  overrides_.insert(
+      make_pair(feature_name, OverrideEntry(overridden_state, field_trial)));

[Ilya Sherman, 2015/09/16 00:50:43]

It's pretty subtle that this does not overwrite in the case that the map key
already exists.  Probably worth documenting in a quick comment.

[Alexei Svitkine (slow), 2015/09/22 21:19:59]

Done.

 }
 
 bool FeatureList::CheckFeatureIdentity(const Feature& feature) {
@@ -107,7 +144,8 @@ bool FeatureList::CheckFeatureIdentity(const Feature& feature) {
   return it->second == &feature;
 }
 
-FeatureList::OverrideEntry::OverrideEntry(OverrideState overridden_state)
-    : overridden_state(overridden_state) {}
+FeatureList::OverrideEntry::OverrideEntry(OverrideState overridden_state,
+                                          FieldTrial* field_trial)
+    : overridden_state(overridden_state), field_trial(field_trial) {}
 
 }  // namespace base