Expand FeatureList to support FieldTrial association.

components/variations/proto/study.proto

 // Copyright (c) 2012 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.
 
 syntax = "proto2";
 
 option optimize_for = LITE_RUNTIME;
 
 package variations;
 
@@ skipping 26 matching lines @@
   // Ex: PERMANENT
   optional Consistency consistency = 7 [default = SESSION];
 
   // Name of the experiment that gets the default experience. This experiment
   // must be included in the list below.
   // Ex: "default"
   optional string default_experiment_name = 8;
 
   // An experiment within the study.
   //
-  // Next tag: 11
+  // Next tag: 12
   message Experiment {
     // A named parameter value for this experiment.
     //
     // Next tag: 3
     message Param {
       // The name of the parameter.
       optional string name = 1;
 
       // The value of the parameter.
       optional string value = 2;
@@ skipping 14 matching lines @@
     // Optional id used to allow this experiment to trigger experimental
     // behavior on Google web properties.
     optional uint64 google_web_trigger_experiment_id = 8;
 
     // Optional id used to uniquely identify this experiment for Google Update.
     optional uint64 google_update_experiment_id = 4;
 
     // Optional id used to uniquely identify this experiment for Chrome Sync.
     optional uint64 chrome_sync_experiment_id = 10;
 
+    // Specifies the feature association parameters for this experiment group.
+    //
+    // Next tag: 5
+    message FeatureAssociation {
+      // Optional list of features to enable when this experiment is selected.
+      // Command-line overrides take precedence over this setting. No feature
+      // listed here should exist in the |disable_feature| list.
+      repeated string enable_feature = 1;
+
+      // Optional list of features to disable when this experiment is selected.
+      // Command-line overrides take precedence over this setting. No feature
+      // listed here should exist in the |enable_feature| list.
+      repeated string disable_feature = 2;
+
+      // Similar to |forcing_flag|, this is an optional name of a feature which
+      // will cause this experiment to be forced on, if that feature is enabled
+      // from the command-line. Experiment with this set are not eligible for
+      // selection via a random dice roll.
+      optional string forcing_feature_on = 3;
+
+      // Similar to |forcing_flag|, this is an optional name of a feature which
+      // will cause this experiment to be forced off, if that feature is

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

It's weird to say "will cause this experiment to be forced off", because the
experiment (a.k.a. "group") will be reported -- which I would describe as "on"
-- in the case that the given disable flag is present.

[Alexei Svitkine (slow), 2015/09/23 22:01:27]

Good catch, you're right - the wording is incorrect. I think it was correct in
the original patch set but it looked like a typo since it seemed like it should
mimic the name of the field, i.e. the above should "on" and this one should be
"off". I've reworded it now to be less confusing. :)

+      // disabled from the command-line. Experiment with this set are not
+      // eligible for selection via a random dice roll.
+      optional string forcing_feature_off = 4;

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

Am I understanding correctly that only one of forcing_feature_on and
forcing_feature_off can be set?  If so, it would be nice to just have a single
forcing_feature entry, which is itself a struct containing the feature name and
the feature's state.

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

Am I understanding correctly that for a single experiment, either a
forcing_feature or a set of enable/disable_features should be set, but not both?

[Alexei Svitkine (slow), 2015/09/23 22:01:27]

I considered this approach earlier but didn't like it. Yes, it's true that only
one of these should be set - and we can use a sub-message with a state field.

But this seems a lot of extra boilerplate - especially since at the config level
server-side, I was planning to have the simple two fields (to make the JSON more
concise).

[Alexei Svitkine (slow), 2015/09/23 22:01:27]

Correct. The following are mutually exclusive:

{forcing_flag, forcing_feature_on, forcing_feature_off, a non-zero
probability_weight}

Expanded comments on these fields.

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

Hmm.  Any experiment that has a forcing_feature_on will presumably have a sister
experiment that has a forcing_feature_off.  I'd think that the most concise way
to represent this would be a single top-level entry in the study config.  We
could probably just have users specify the forcing feature name, and we can
autogenerate the group names for them.  WDYT?

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

Again, I think that reduces flexibility. We've seen people use multiple
forcing_flag entries in a study config - for example in QuicStable.json and I
suspect there will be cases where you don't want to do the dice roll experiments
if multiple different things are forced.

Note that this still allows us to have syntax sugar at the config level, if
desired, but the point is it makes the protocol support the extra flexibility,
which is desirable in some cases.

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

Well, the field could be repeated, if the concern is just that people might want
to associate with multiple features.  Was that the concern, or was there an
additional part that I misunderstood?

My main point here is that I'd expect these experiments to always come in pairs:
One for the feature being forced on, and one for being forced off.  Since the
same feature name controls both cases, it seems silly for the protocol to need
to repeat the feature name for the enabled and disabled cases.

[Alexei Svitkine (slow), 2015/09/25 03:56:51]

Groups have other features though, such as associated experiment ids and other
things. So if we move everything top-level to study and make things repeated, we
either lose those functionalities (which people have actually asked for/used -
it used to be that we didn't support them for forcing_flags) or we end up having
to add more fields to support those things, which is not very maintainable and
doesn't allow code sharing. The other option is to have a top-level list of
strings and the protocol to instead specify indexes into that array. But I don't
feel that's worth it - see also my note about gzip below.
I agree that repeating strings is not ideal. However, probably this sort of
thing becomes mostly moot if the data format is gzip-compressed. (I think right
now we don't do this for the server payload, but there's a bug on file to start
doing this.)

[Ilya Sherman, 2015/09/25 04:33:36]

Alright, you've convinced me :)

+    }
+    optional FeatureAssociation feature_association = 11;
+
     // Optional name of a Chrome flag that, when present, causes this experiment
     // to be forced. If the forcing_flag field is set, users will not be
     // assigned to this experiment unless that flag is present in Chrome's
     // command line.
     optional string forcing_flag = 5;
 
     // Parameter values for this experiment.
     repeated Param param = 6;
 
     enum Type {
@@ skipping 161 matching lines @@
     // This is recommended for most studies that include client code.
     ACTIVATION_EXPLICIT  = 0;
     // The study will be automatically activated when it is created. This
     // is recommended for studies that do not have any client logic.
     ACTIVATION_AUTO      = 1;
   }
 
   // Activation type for this study. Defaults to ACTIVATION_EXPLICIT if omitted.
   optional ActivationType activation_type = 12;
 }