disable field trials if the build is 30 days or older

[raman, 2011-01-09 04:18:14]

Hi Jim,
  This is a first cut at disabling the field trials. I still have read only
check out. Would appreciate your comments.

thanks very much,
raman

[jar (doing other things), 2011-01-09 07:00:04]

I was a little concerned about the user that had a funky clock.  I'm not sure if
it is worth it to worry about it.  

I also think you might remove the "GMT", so that a local compile-run-test would
all use the same timezone.  Without this, you could be as far as about 23.5
hours off (due to time zone), and then the test would have to be run within 30
minutes of compilation.  WDYT?

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.cc#new...
base/metrics/field_trial.cc:92: const char* kDateTime = __DATE__ " " __TIME__ "
GMT";
caveat: This is OK.... but we should be sure our tests use GMT as well, or we'll
probably fail when we run in certain time zones.  I messed up some related
code... and so I'm now extra wary of his issue.

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:119: void
SetDurationAfterWhichDisableFieldTrials(base::TimeDelta duration) {
nit: for classes as arguments, pass in:
const CLASSNAME&

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:119: void
SetDurationAfterWhichDisableFieldTrials(base::TimeDelta duration) {
nit: Since this is an instance of FieldTrial, we only control this one single
field trial with this setting, so the name of the method should not end in an
"s" (we don't impact TrialS).

In fact, since this is a FieldTrial method, we probably don't need the
FieldTrial posfix.

That would get the name down to something like:

SetDurationAfterWhichDisabled

Which might even be written:

SetDisableDuration

The slot describing the corresponding member (disable_duration_??) would have a
comment that only the default value (all remaining probabilities) can be
selected after the duration, which is based on the build time of the module.

WDYT?

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial_unitte...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial_unitte...
base/metrics/field_trial_unittest.cc:171: trial->AppendGroup(loser, 99999);  //
99.999% chance of being chosen.
This seems pretty good.... we may as well make it even rarer.  I'd suggest all
the way to 1 in 100,000,000.  If you'd prefer, you could do two tests, and check
that at least one did what you wanted to show.

[rtenneti, 2011-01-09 23:16:42]

Hi Jim,
  Made all the changes you had suggested (I am not 100% sure if I got the
comments right or not).

thanks,
raman

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.cc#new...
base/metrics/field_trial.cc:92: const char* kDateTime = __DATE__ " " __TIME__ "
GMT";
On 2011/01/09 07:00:04, jar wrote:
> caveat: This is OK.... but we should be sure our tests use GMT as well, or
we'll
> probably fail when we run in certain time zones.  I messed up some related
> code... and so I'm now extra wary of his issue.

Done.

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:119: void
SetDurationAfterWhichDisableFieldTrials(base::TimeDelta duration) {
On 2011/01/09 07:00:04, jar wrote:
> nit: for classes as arguments, pass in:
> const CLASSNAME&

Done.

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:119: void
SetDurationAfterWhichDisableFieldTrials(base::TimeDelta duration) {
On 2011/01/09 07:00:04, jar wrote:
> nit: Since this is an instance of FieldTrial, we only control this one single
> field trial with this setting, so the name of the method should not end in an
> "s" (we don't impact TrialS).
> 
> In fact, since this is a FieldTrial method, we probably don't need the
> FieldTrial posfix.
> 
> That would get the name down to something like:
> 
> SetDurationAfterWhichDisabled
> 
> Which might even be written:
> 
> SetDisableDuration
> 
> The slot describing the corresponding member (disable_duration_??) would have
a
> comment that only the default value (all remaining probabilities) can be
> selected after the duration, which is based on the build time of the module.
> 
> WDYT?

Done.

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial_unitte...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6213001/diff/1/base/metrics/field_trial_unitte...
base/metrics/field_trial_unittest.cc:171: trial->AppendGroup(loser, 99999);  //
99.999% chance of being chosen.
On 2011/01/09 07:00:04, jar wrote:
> This seems pretty good.... we may as well make it even rarer.  I'd suggest all
> the way to 1 in 100,000,000.  If you'd prefer, you could do two tests, and
check
> that at least one did what you wanted to show.

Done.

[rtenneti, 2011-01-10 05:38:37]

Hi Jim,
 Added a routine to cache if we need to disable field trial or not. We call this
routine either during construction or when the disable_duration is set. Hope you
don't mind this small optimization.

thanks much,
raman

[rtenneti, 2011-01-10 05:51:14]

Hi Jim,
  Made a small change to the comment. Sorry for the extra uploads.

thanks,
raman

[rtenneti, 2011-01-10 19:35:16]

Hi Jim,
  Fixed the unit tests and tested the unit tests.

thanks,
raman

[jar (doing other things), 2011-01-10 22:01:21]

http://codereview.chromium.org/6213001/diff/17001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6213001/diff/17001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:99: // disable_field_trail_ to true, if the
difference between build time of the
nit: trail-->trial

http://codereview.chromium.org/6213001/diff/17001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:125: base::TimeDelta disableDuration() const {
nit: Reading this name, it looks like the duration for which this will be
disabled, rather than the (much loo long name) duration until it will be
impossible to enable.

We need a better name forthe setter and slot.  Possibilities include:

InhibitDate
UsageTimeout
EnablementDeadline

WDYT?

[jar (doing other things), 2011-01-10 22:03:18]

http://codereview.chromium.org/6213001/diff/23001/base/metrics/field_trial_un...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6213001/diff/23001/base/metrics/field_trial_un...
base/metrics/field_trial_unittest.cc:177: EXPECT_EQ(winner,
trial->group_name());
Why is this line repeated?  What was intended?

[rtenneti, 2011-01-10 22:48:03]

Hi Jim,
  Made all the changes you had suggested. Used "UsageTimeout" and that seems to
read well.

thanks,
raman

http://codereview.chromium.org/6213001/diff/17001/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6213001/diff/17001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:99: // disable_field_trail_ to true, if the
difference between build time of the
On 2011/01/10 22:01:21, jar wrote:
> nit: trail-->trial

Done.

http://codereview.chromium.org/6213001/diff/17001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:125: base::TimeDelta disableDuration() const {
On 2011/01/10 22:01:21, jar wrote:
> nit: Reading this name, it looks like the duration for which this will be
> disabled, rather than the (much loo long name) duration until it will be
> impossible to enable.
> 
> We need a better name forthe setter and slot.  Possibilities include:
> 
> InhibitDate
> UsageTimeout
> EnablementDeadline
> 
> WDYT?
> 

Hi Jim,
  UsageTimeout seems to read better. Going with it. Hope that is ok.

thanks,
raman

http://codereview.chromium.org/6213001/diff/17001/base/metrics/field_trial.h#...
base/metrics/field_trial.h:125: base::TimeDelta disableDuration() const {
On 2011/01/10 22:01:21, jar wrote:
> nit: Reading this name, it looks like the duration for which this will be
> disabled, rather than the (much loo long name) duration until it will be
> impossible to enable.
> 
> We need a better name forthe setter and slot.  Possibilities include:
> 
> InhibitDate
> UsageTimeout
> EnablementDeadline
> 
> WDYT?
> 

Done.

http://codereview.chromium.org/6213001/diff/23001/base/metrics/field_trial_un...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6213001/diff/23001/base/metrics/field_trial_un...
base/metrics/field_trial_unittest.cc:177: EXPECT_EQ(winner,
trial->group_name());
On 2011/01/10 22:03:18, jar wrote:
> Why is this line repeated?  What was intended?

Done.

[rvargas (doing something else), 2011-01-12 19:25:14]

What problem are we actually trying to solve?

It seems that we never have a build for 30 days without being superseded by
another one, in which case we either are looking at the new build or, if we look
at the old build, we care about old users sending reports.

http://codereview.chromium.org/6213001/diff/45001/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/6213001/diff/45001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:186: base::FieldTrial::kAllRemainingProbability);
 // 90 % control.
This is not the same. The experiment has a control group, otherwise it is not
possible to really make any assertion about what's going on with the profiles
that are part of the experiment.

Why the need to have an extra group with all remaining users? It seems that it
only adds overhead (+ noise) everywhere.

[jar (doing other things), 2011-01-13 00:04:54]

http://codereview.chromium.org/6213001/diff/45001/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/6213001/diff/45001/net/disk_cache/backend_impl...
net/disk_cache/backend_impl.cc:186: base::FieldTrial::kAllRemainingProbability);
 // 90 % control.
As per our discussion....

We need some way to establish what a "reasonable" default would be.  The current
hack is to use the "all remaining probability" group to signal that.  Raman may
end up formalizing this in the constructor (as a new argument set), and that may
allow us to effectively reduce or turn-off trials under a variety of conditions
(old trials? reduce trial sets in Stable and beta?).  

To start any of this, we need a clear understanding of a "default" thing to do
when we effectively disable a trial.

On 2011/01/12 19:25:14, rvargas wrote:
> This is not the same. The experiment has a control group, otherwise it is not
> possible to really make any assertion about what's going on with the profiles
> that are part of the experiment.
> 
> Why the need to have an extra group with all remaining users? It seems that it
> only adds overhead (+ noise) everywhere.