Fix SimpleCache field trial creation.

Fix SimpleCache field trial creation.

By not putting opt out users in the "No" group, our dashboard in Finch
was showing only the explicit opt in users. Not ideal for A/B
comparisons!

R=stevet, pasko

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194987

[gavinp, 2013-04-17 14:11:00]

SteveT, PLAL.

pasko, FYI.

[SteveT, 2013-04-17 15:41:02]

See my comment inline.

cc+Alexei to confirm the pattern that I am endorsing.

https://codereview.chromium.org/14048014/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/14048014/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:212: if
(LowerCaseEqualsASCII(opt_value, "off")) {
Couple things here:

(1) Appending a new group with the same name as the default name is something
that we avoid doing. If you want to force your experiment to be disabled, we
instead recommend appending Yes with weight 0 (which makes the default group 
No" get chosen with 100% probability).

(2) However, it looks like you're dealing with the case where the user is aware
of SimpleCacheBackend, which means that if they opt in or out explicitly, we
don't want them in an experiment that returns data about the feature.

You should instead use a pattern where:

If the user opts in explicitly, tell the feature to be turned on and DON'T make
a FieldTrial.

If the user opts out explicitly, tell the feature to be turned off and DON'T
make a Field Trial.

If the user does not opt in or out, create the field trial with the experimental
weight that you want (kSimpleCacheProbability = 1 on ANDROID, 100 otherwise).

Does this make sense/work for you?

[pasko-google - do not use, 2013-04-17 15:42:04]

thank you, Gavin

[Alexei Svitkine (slow), 2013-04-17 15:46:52]

https://codereview.chromium.org/14048014/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/14048014/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:212: if
(LowerCaseEqualsASCII(opt_value, "off")) {
On 2013/04/17 15:41:02, SteveT wrote:
> Couple things here:
> 
> (1) Appending a new group with the same name as the default name is something
> that we avoid doing. If you want to force your experiment to be disabled, we
> instead recommend appending Yes with weight 0 (which makes the default group 
> No" get chosen with 100% probability).
> 
> (2) However, it looks like you're dealing with the case where the user is
aware
> of SimpleCacheBackend, which means that if they opt in or out explicitly, we
> don't want them in an experiment that returns data about the feature.
> 
> You should instead use a pattern where:
> 
> If the user opts in explicitly, tell the feature to be turned on and DON'T
make
> a FieldTrial.
> 
> If the user opts out explicitly, tell the feature to be turned off and DON'T
> make a Field Trial.
> 
> If the user does not opt in or out, create the field trial with the
experimental
> weight that you want (kSimpleCacheProbability = 1 on ANDROID, 100 otherwise).
> 
> Does this make sense/work for you?

Agree with Steve on all points.

Also, I'd like to expand on one aspect - I'm not sure if you're ever planning to
do a server-side config for this trial, but if you are, you should also make
sure to not query the state of the field trial (i.e. not use
FieldTrialList::FindFullName()) if the user explicitly opted in or opted out of
the trial.

[gavinp, 2013-04-17 16:19:41]

PTAL.

https://codereview.chromium.org/14048014/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/14048014/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:212: if
(LowerCaseEqualsASCII(opt_value, "off")) {
On 2013/04/17 15:41:02, SteveT wrote:
> Couple things here:
> 
> (1) Appending a new group with the same name as the default name is something
> that we avoid doing. If you want to force your experiment to be disabled, we
> instead recommend appending Yes with weight 0 (which makes the default group 
> No" get chosen with 100% probability).
> 
> (2) However, it looks like you're dealing with the case where the user is
aware
> of SimpleCacheBackend, which means that if they opt in or out explicitly, we
> don't want them in an experiment that returns data about the feature.
> 
> You should instead use a pattern where:
> 
> If the user opts in explicitly, tell the feature to be turned on and DON'T
make
> a FieldTrial.
> 
> If the user opts out explicitly, tell the feature to be turned off and DON'T
> make a Field Trial.
> 
> If the user does not opt in or out, create the field trial with the
experimental
> weight that you want (kSimpleCacheProbability = 1 on ANDROID, 100 otherwise).
> 
> Does this make sense/work for you?

There's two considerations that cause me to want everyone in the field trial

(1) We use the field trial name, with FindFullName() to control the feature in
net::. Currently net:: doesn't take command line options or global options, so
this has been the pattern. But, for now, this is what we are doing. This came up
in the initial review of this code.

(2) I'd really really like data on explicit opt in vs opt out, especially since
this feature has no field trial running (no experiments approval yet (hint hint
odean)), and it was absolutely amazing using the field trial to see the results
for the dev channel opt ins already.

So I made an upload that calls the groups "ExplicitFoo" and "ExperimentFoo". Is
this better?

https://codereview.chromium.org/14048014/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:212: if
(LowerCaseEqualsASCII(opt_value, "off")) {
On 2013/04/17 15:46:52, Alexei Svitkine wrote:
> On 2013/04/17 15:41:02, SteveT wrote:
> > Couple things here:
> > 
> > (1) Appending a new group with the same name as the default name is
something
> > that we avoid doing. If you want to force your experiment to be disabled, we
> > instead recommend appending Yes with weight 0 (which makes the default group

> > No" get chosen with 100% probability).
> > 
> > (2) However, it looks like you're dealing with the case where the user is
> aware
> > of SimpleCacheBackend, which means that if they opt in or out explicitly, we
> > don't want them in an experiment that returns data about the feature.
> > 
> > You should instead use a pattern where:
> > 
> > If the user opts in explicitly, tell the feature to be turned on and DON'T
> make
> > a FieldTrial.
> > 
> > If the user opts out explicitly, tell the feature to be turned off and DON'T
> > make a Field Trial.
> > 
> > If the user does not opt in or out, create the field trial with the
> experimental
> > weight that you want (kSimpleCacheProbability = 1 on ANDROID, 100
otherwise).
> > 
> > Does this make sense/work for you?
> 
> Agree with Steve on all points.
> 
> Also, I'd like to expand on one aspect - I'm not sure if you're ever planning
to
> do a server-side config for this trial, but if you are, you should also make
> sure to not query the state of the field trial (i.e. not use
> FieldTrialList::FindFullName()) if the user explicitly opted in or opted out
of
> the trial.

Alexei,

Oh no! Right now FindFullName() is exactly how we turn the cache on and off. Is
there an alternative?

[Alexei Svitkine (slow), 2013-04-17 16:49:04]

On 2013/04/17 16:19:41, gavinp wrote:
> Alexei,
> 
> Oh no! Right now FindFullName() is exactly how we turn the cache on and off.
Is
> there an alternative?

So my suggestion was premised on the fact that per Steve's suggestion you were
not going to have a field trial if the feature was enabled/disabled explicitly.

Not calling FindFullName() will make sure the trial will not be reported through
metrics - the idea being that you want non-biased results in metrics (i.e. only
users who were placed in the trial by the field trial randomization code, and
not via forcing). This would have ensured the percentages you'd have seen on the
Finch dashboard would have corresponded to what the trial was set to (i.e. since
explicitly forced used wouldn't be counted).

Other than that, FindFullName() should be fine to use from the technical
perspective, just be aware the effects it will have on your reporting.

[Alexei Svitkine (slow), 2013-04-17 16:50:25]

https://codereview.chromium.org/14048014/diff/3002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/14048014/diff/3002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_field_trials.cc:214:
trial->AppendGroup("ExplicitNo", 100);
Use kDivisor?

https://codereview.chromium.org/14048014/diff/3002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_field_trials.cc:218:
trial->AppendGroup("ExplicitYes", 100);
Use kDivisor?

[SteveT, 2013-04-17 18:16:11]

lgtm given you've addressed asvitkine's comments.

[commit-bot: I haz the power, 2013-04-18 06:23:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14048014/16001

[commit-bot: I haz the power, 2013-04-18 06:55:23]

Retried try job too often on mac_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-18 07:44:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14048014/16001

[commit-bot: I haz the power, 2013-04-18 08:10:56]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-18 09:54:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14048014/34001

[commit-bot: I haz the power, 2013-04-18 10:22:13]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-04-18 11:20:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14048014/34001

[commit-bot: I haz the power, 2013-04-18 11:45:33]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-04-18 11:54:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14048014/34001

[commit-bot: I haz the power, 2013-04-18 12:21:25]

Retried try job too often on mac_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-18 12:28:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14048014/34001

[commit-bot: I haz the power, 2013-04-18 13:01:33]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-04-18 13:09:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14048014/34001

[commit-bot: I haz the power, 2013-04-18 13:39:09]

Retried try job too often on mac_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-18 15:15:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14048014/34001

[commit-bot: I haz the power, 2013-04-18 15:43:38]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-04-18 15:48:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/14048014/69001

[commit-bot: I haz the power, 2013-04-18 19:50:34]

Change committed as 194987