Implement A/B experiment for anti-DDoS.

Implement A/B experiment for anti-DDoS.

BUG=85258
TEST=manual testing done by developer

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=89376

[Jói, 2011-06-07 23:05:05]

Reviewers:
- yzshen as main reviewer.
- wtc for chrome/browser/net/OWNERS rubber stamp.

Please note that I already added the metrics needed for the experiment, and the
per-trial-group config for those metrics, in http://crrev.com/87056.

Also, I realize that users who have already changed the throttling setting using
about:net-internals might possibly see it change to something different.  I
can't see any way to fix this, and I believe the group is tiny.

Cheers,
Jói

[yzshen1, 2011-06-08 00:56:51]

http://codereview.chromium.org/7027040/diff/1/chrome/browser/trials/throttlin...
File chrome/browser/trials/throttling_trial.cc (right):

http://codereview.chromium.org/7027040/diff/1/chrome/browser/trials/throttlin...
chrome/browser/trials/throttling_trial.cc:32: "ThrottlingEnabled",
kTotalProbability, "Default", 2011, 6, 30));
[out of curiosity]
June 30, 2011 means this will only stay in Dev and Canary builds, right? How
many users are expected to participate in this experiment?

http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
chrome/browser/ui/webui/net_internals_ui.cc:737: void
NetInternalsMessageHandler::OnDisableThrottlingExperiments(
The logic within this method can be put in the method OnEnableHttpThrottling, so
that you don't need to modify *.js files, and it is more clear.

http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
chrome/browser/ui/webui/net_internals_ui.cc:748: trial->Disable();
It may be better to move the part of disabling the trial to
NetPrefObserver::ApplySettings().

[Jói, 2011-06-08 16:44:47]

+jar@ for base/metrics/OWNERS rubber stamp (removed a DCHECK that can occur in
valid cases).

Yuzhu, I've uploaded a new version that addresses your review comments and also
fixes some unit tests by removing the dcheck mentioned above.  Please take
another look.

Cheers,
Jói

http://codereview.chromium.org/7027040/diff/1/chrome/browser/trials/throttlin...
File chrome/browser/trials/throttling_trial.cc (right):

http://codereview.chromium.org/7027040/diff/1/chrome/browser/trials/throttlin...
chrome/browser/trials/throttling_trial.cc:32: "ThrottlingEnabled",
kTotalProbability, "Default", 2011, 6, 30));
On 2011/06/08 00:56:51, yzshen1 wrote:
> [out of curiosity]
> June 30, 2011 means this will only stay in Dev and Canary builds, right? How
> many users are expected to participate in this experiment?

Correct, this is just 5% of the dev channel for now.  Depending on the results,
I may tweak things and extend the experiment, or extend it to a larger audience
or just turn the feature on by default.

http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
chrome/browser/ui/webui/net_internals_ui.cc:737: void
NetInternalsMessageHandler::OnDisableThrottlingExperiments(
On 2011/06/08 00:56:51, yzshen1 wrote:
> The logic within this method can be put in the method OnEnableHttpThrottling,
so
> that you don't need to modify *.js files, and it is more clear.

You're right. I thought this method was also used at startup  (without user
action) but was confusing it with NetPrefObserver::ApplySettings().

I've removed the additional message and handler function.

http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
chrome/browser/ui/webui/net_internals_ui.cc:748: trial->Disable();
On 2011/06/08 00:56:51, yzshen1 wrote:
> It may be better to move the part of disabling the trial to
> NetPrefObserver::ApplySettings().

I can't do that because that method gets called not only on user action but also
just at the startup of Chrome - so it's not a signal that the user changed the
setting.

Strictly speaking, I _could_ do it there when pref_name is not NULL, but it
feels a bit brittle to do it this way.  The user gesture is much more apparent
in the NetInternalsMessageHandler::OnEnableHttpThrottling which is only used as
a handler for explicit user action.

[yzshen1, 2011-06-08 16:55:54]

LGTM

Thanks!

http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
chrome/browser/ui/webui/net_internals_ui.cc:748: trial->Disable();
Makes sense. Thanks for explaining.
On 2011/06/08 16:44:47, Jói wrote:
> On 2011/06/08 00:56:51, yzshen1 wrote:
> > It may be better to move the part of disabling the trial to
> > NetPrefObserver::ApplySettings().
> 
> I can't do that because that method gets called not only on user action but
also
> just at the startup of Chrome - so it's not a signal that the user changed the
> setting.
> 
> Strictly speaking, I _could_ do it there when pref_name is not NULL, but it
> feels a bit brittle to do it this way.  The user gesture is much more apparent
> in the NetInternalsMessageHandler::OnEnableHttpThrottling which is only used
as
> a handler for explicit user action.

[Jói, 2011-06-08 17:05:59]

Thanks Yuzhu.

Wan-Teh and Jim, I await your owners approvals for these files:

jar: base/metrics/field_trial.cc
wtc: chrome/browser/net/net_pref_observer.cc

Cheers,
Jói


On Wed, Jun 8, 2011 at 12:55 PM, <yzshen@chromium.org> wrote:
>
> LGTM
>
> Thanks!
>
>
>
http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
> File chrome/browser/ui/webui/net_internals_ui.cc (right):
>
>
http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
> chrome/browser/ui/webui/net_internals_ui.cc:748: trial->Disable();
> Makes sense. Thanks for explaining.
> On 2011/06/08 16:44:47, Jói wrote:
>>
>> On 2011/06/08 00:56:51, yzshen1 wrote:
>> > It may be better to move the part of disabling the trial to
>> > NetPrefObserver::ApplySettings().
>
>> I can't do that because that method gets called not only on user
>
> action but also
>>
>> just at the startup of Chrome - so it's not a signal that the user
>
> changed the
>>
>> setting.
>
>> Strictly speaking, I _could_ do it there when pref_name is not NULL,
>
> but it
>>
>> feels a bit brittle to do it this way.  The user gesture is much more
>
> apparent
>>
>> in the NetInternalsMessageHandler::OnEnableHttpThrottling which is
>
> only used as
>>
>> a handler for explicit user action.
>
> http://codereview.chromium.org/7027040/

[Jói, 2011-06-09 13:57:03]

wtc/jar: ping

> Wan-Teh and Jim, I await your owners approvals for these files:
>
> jar: base/metrics/field_trial.cc
> wtc: chrome/browser/net/net_pref_observer.cc


On Wed, Jun 8, 2011 at 1:05 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Thanks Yuzhu.
>
> Wan-Teh and Jim, I await your owners approvals for these files:
>
> jar: base/metrics/field_trial.cc
> wtc: chrome/browser/net/net_pref_observer.cc
>
> Cheers,
> Jói
>
>
> On Wed, Jun 8, 2011 at 12:55 PM, <yzshen@chromium.org> wrote:
>>
>> LGTM
>>
>> Thanks!
>>
>>
>>
http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
>> File chrome/browser/ui/webui/net_internals_ui.cc (right):
>>
>>
http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
>> chrome/browser/ui/webui/net_internals_ui.cc:748: trial->Disable();
>> Makes sense. Thanks for explaining.
>> On 2011/06/08 16:44:47, Jói wrote:
>>>
>>> On 2011/06/08 00:56:51, yzshen1 wrote:
>>> > It may be better to move the part of disabling the trial to
>>> > NetPrefObserver::ApplySettings().
>>
>>> I can't do that because that method gets called not only on user
>>
>> action but also
>>>
>>> just at the startup of Chrome - so it's not a signal that the user
>>
>> changed the
>>>
>>> setting.
>>
>>> Strictly speaking, I _could_ do it there when pref_name is not NULL,
>>
>> but it
>>>
>>> feels a bit brittle to do it this way.  The user gesture is much more
>>
>> apparent
>>>
>>> in the NetInternalsMessageHandler::OnEnableHttpThrottling which is
>>
>> only used as
>>>
>>> a handler for explicit user action.
>>
>> http://codereview.chromium.org/7027040/
>

[Jói, 2011-06-10 16:05:46]

wtc & jar: ping for OWNERS approval

jar: base/metrics/field_trial.cc
wtc: chrome/browser/net/net_pref_observer.cc

Cheers,
Jói


On Thu, Jun 9, 2011 at 9:56 AM, Jói Sigurðsson <joi@chromium.org> wrote:
> wtc/jar: ping
>
>> Wan-Teh and Jim, I await your owners approvals for these files:
>>
>> jar: base/metrics/field_trial.cc
>> wtc: chrome/browser/net/net_pref_observer.cc
>
>
> On Wed, Jun 8, 2011 at 1:05 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> Thanks Yuzhu.
>>
>> Wan-Teh and Jim, I await your owners approvals for these files:
>>
>> jar: base/metrics/field_trial.cc
>> wtc: chrome/browser/net/net_pref_observer.cc
>>
>> Cheers,
>> Jói
>>
>>
>> On Wed, Jun 8, 2011 at 12:55 PM, <yzshen@chromium.org> wrote:
>>>
>>> LGTM
>>>
>>> Thanks!
>>>
>>>
>>>
http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
>>> File chrome/browser/ui/webui/net_internals_ui.cc (right):
>>>
>>>
http://codereview.chromium.org/7027040/diff/1/chrome/browser/ui/webui/net_int...
>>> chrome/browser/ui/webui/net_internals_ui.cc:748: trial->Disable();
>>> Makes sense. Thanks for explaining.
>>> On 2011/06/08 16:44:47, Jói wrote:
>>>>
>>>> On 2011/06/08 00:56:51, yzshen1 wrote:
>>>> > It may be better to move the part of disabling the trial to
>>>> > NetPrefObserver::ApplySettings().
>>>
>>>> I can't do that because that method gets called not only on user
>>>
>>> action but also
>>>>
>>>> just at the startup of Chrome - so it's not a signal that the user
>>>
>>> changed the
>>>>
>>>> setting.
>>>
>>>> Strictly speaking, I _could_ do it there when pref_name is not NULL,
>>>
>>> but it
>>>>
>>>> feels a bit brittle to do it this way.  The user gesture is much more
>>>
>>> apparent
>>>>
>>>> in the NetInternalsMessageHandler::OnEnableHttpThrottling which is
>>>
>>> only used as
>>>>
>>>> a handler for explicit user action.
>>>
>>> http://codereview.chromium.org/7027040/
>>
>

[wtc, 2011-06-10 22:31:00]

LGTM.

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
File chrome/browser/trials/throttling_trial.cc (right):

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
chrome/browser/trials/throttling_trial.cc:32: "ThrottlingEnabled",
kTotalProbability, "Default", 2011, 6, 30));
Nit: same here.  Not sure if the string "ThrottlingEnabled" is
only used in a context where it obviously refers to HTTP throttling.  If not,
then naming the string "HttpThrottlingEnabled" would be more clear.

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
File chrome/browser/trials/throttling_trial.h (right):

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
chrome/browser/trials/throttling_trial.h:9: #include <string>
Nit: no need to include <string> because this header doesn't
use std::string, etc.

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
chrome/browser/trials/throttling_trial.h:13: namespace throttling_trial {
Nit: it seems better to name the namespace http_throttling
because throttling is vague.

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

I was only going to look at the metrics code... but accidentally drove into one
other file, and had a comment.  Please at least resepond to both, and I'll have
another look.

Thanks,

Jim

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

http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc#...
base/metrics/field_trial.cc:374: // randomization is supported, so we do not
DCHECK.
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.

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
File chrome/browser/trials/throttling_trial.h (right):

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
chrome/browser/trials/throttling_trial.h:16: void CreateTrial(PrefService*
prefs);
This seems strange.  You're declaring a namespace (it would seem) to declare
only one item in that namespace.  Namespaces are traditionally defined to reduce
global namespace polution... but it is costing you one element of polution to
preclude one elements :-/.

I'd suggest that you should just name the function well.

I think it is for instance more common to define a namespace that corresponds to
a directory... and include a LOT of names... but that does not seem to be
happening.

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

PTAL, I've responded to all of your review comments.

Jim, let me know if you still would like me to bring back the DCHECK after
reading my response below.

Cheers,
Jói

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

http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc#...
base/metrics/field_trial.cc:374: // randomization is supported, so we do not
DCHECK.
On 2011/06/12 06:49:12, jar wrote:
> [...] I don't see the value of enabling field trials if we are not
> reporting results.

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".

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
File chrome/browser/trials/throttling_trial.cc (right):

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
chrome/browser/trials/throttling_trial.cc:32: "ThrottlingEnabled",
kTotalProbability, "Default", 2011, 6, 30));
On 2011/06/10 22:31:00, wtc wrote:
> Nit: same here.  Not sure if the string "ThrottlingEnabled" is
> only used in a context where it obviously refers to HTTP throttling.  If not,
> then naming the string "HttpThrottlingEnabled" would be more clear.

Switched to "HttpThrottlingEnabled" everywhere.

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
File chrome/browser/trials/throttling_trial.h (right):

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
chrome/browser/trials/throttling_trial.h:9: #include <string>
On 2011/06/10 22:31:00, wtc wrote:
> Nit: no need to include <string> because this header doesn't
> use std::string, etc.

Done.

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
chrome/browser/trials/throttling_trial.h:13: namespace throttling_trial {
On 2011/06/10 22:31:00, wtc wrote:
> Nit: it seems better to name the namespace http_throttling
> because throttling is vague.

I went with Jim's suggestion to remove the namespace instead. The function is
now called CreateHttpThrottlingTrial.

http://codereview.chromium.org/7027040/diff/5001/chrome/browser/trials/thrott...
chrome/browser/trials/throttling_trial.h:16: void CreateTrial(PrefService*
prefs);
On 2011/06/12 06:49:12, jar wrote:
> This seems strange.  You're declaring a namespace (it would seem) to declare
> only one item in that namespace.  Namespaces are traditionally defined to
reduce
> global namespace polution... but it is costing you one element of polution to
> preclude one elements :-/.
> 
> I'd suggest that you should just name the function well.
> 
> I think it is for instance more common to define a namespace that corresponds
to
> a directory... and include a LOT of names... but that does not seem to be
> happening.

You're absolutely right, thanks for pointing this out.  What happened was I
initially had a few functions in the namespace and then as implementation
progressed it simplified to just one.  I've removed the namespace and named the
function CreateHttpThrottlingTrial.

[jar (doing other things), 2011-06-14 02:28:45]

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

http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc#...
base/metrics/field_trial.cc:374: // randomization is supported, so we do not
DCHECK.
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:
> On 2011/06/12 06:49:12, jar wrote:
> > [...] I don't see the value of enabling field trials if we are not
> > reporting results.
> 
> 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".

[Jói, 2011-06-14 03:04:11]

Hi Jim,

I get that, but if I keep the DHCECK in then instead I would be
calling FieldTrialList::GetFieldTrialCount to see if there are zero
field trials before I call
FieldTrialList::IsOneTimeRandomizationEnabled, and I would have
exactly the same failure mode you described.  Not sure why there is no
DCHECK on that function, maybe there should be?  If we add it, then we
definitely need to always initialize as there will be no way to
conditionally initialize a trial.

Hmm, thinking about it some more, perhaps you are suggesting I always
create the field trial but only add groups other than the default
group to it if one-time randomization is enabled.  That would work, is
that what you meant?

Cheers,
Jói



On Mon, Jun 13, 2011 at 10:28 PM,  <jar@chromium.org> wrote:
>
> http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc
> File base/metrics/field_trial.cc (right):
>
>
http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc#...
> base/metrics/field_trial.cc:374: // randomization is supported, so we do
> not DCHECK.
> 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:
>>
>> On 2011/06/12 06:49:12, jar wrote:
>> > [...] I don't see the value of enabling field trials if we are not
>> > reporting results.
>
>> 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".
>
> http://codereview.chromium.org/7027040/
>

[Jói, 2011-06-14 18:13:42]

Sorry, no that doesn't work either.  The fix required to be able to
remove the DCHECK would be to have a FieldTrialList instance in every
unit test EXE that doesn't already have one.  I could look into that
separately as a TODO, would that work?

Cheers,
Jói


On Mon, Jun 13, 2011 at 11:03 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Hi Jim,
>
> I get that, but if I keep the DHCECK in then instead I would be
> calling FieldTrialList::GetFieldTrialCount to see if there are zero
> field trials before I call
> FieldTrialList::IsOneTimeRandomizationEnabled, and I would have
> exactly the same failure mode you described.  Not sure why there is no
> DCHECK on that function, maybe there should be?  If we add it, then we
> definitely need to always initialize as there will be no way to
> conditionally initialize a trial.
>
> Hmm, thinking about it some more, perhaps you are suggesting I always
> create the field trial but only add groups other than the default
> group to it if one-time randomization is enabled.  That would work, is
> that what you meant?
>
> Cheers,
> Jói
>
>
>
> On Mon, Jun 13, 2011 at 10:28 PM,  <jar@chromium.org> wrote:
>>
>> http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc
>> File base/metrics/field_trial.cc (right):
>>
>>
http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc#...
>> base/metrics/field_trial.cc:374: // randomization is supported, so we do
>> not DCHECK.
>> 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:
>>>
>>> On 2011/06/12 06:49:12, jar wrote:
>>> > [...] I don't see the value of enabling field trials if we are not
>>> > reporting results.
>>
>>> 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".
>>
>> http://codereview.chromium.org/7027040/
>>
>

[Jói, 2011-06-14 20:47:40]

jar: ping

On Tue, Jun 14, 2011 at 2:13 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Sorry, no that doesn't work either.  The fix required to be able to
> remove the DCHECK would be to have a FieldTrialList instance in every
> unit test EXE that doesn't already have one.  I could look into that
> separately as a TODO, would that work?
>
> Cheers,
> Jói
>
>
> On Mon, Jun 13, 2011 at 11:03 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> Hi Jim,
>>
>> I get that, but if I keep the DHCECK in then instead I would be
>> calling FieldTrialList::GetFieldTrialCount to see if there are zero
>> field trials before I call
>> FieldTrialList::IsOneTimeRandomizationEnabled, and I would have
>> exactly the same failure mode you described.  Not sure why there is no
>> DCHECK on that function, maybe there should be?  If we add it, then we
>> definitely need to always initialize as there will be no way to
>> conditionally initialize a trial.
>>
>> Hmm, thinking about it some more, perhaps you are suggesting I always
>> create the field trial but only add groups other than the default
>> group to it if one-time randomization is enabled.  That would work, is
>> that what you meant?
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Mon, Jun 13, 2011 at 10:28 PM,  <jar@chromium.org> wrote:
>>>
>>> http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc
>>> File base/metrics/field_trial.cc (right):
>>>
>>>
http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc#...
>>> base/metrics/field_trial.cc:374: // randomization is supported, so we do
>>> not DCHECK.
>>> 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:
>>>>
>>>> On 2011/06/12 06:49:12, jar wrote:
>>>> > [...] I don't see the value of enabling field trials if we are not
>>>> > reporting results.
>>>
>>>> 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".
>>>
>>> http://codereview.chromium.org/7027040/
>>>
>>
>

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

LGTM with nit (TODO) added as noted below.

http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (left):

http://codereview.chromium.org/7027040/diff/5001/base/metrics/field_trial.cc#...
base/metrics/field_trial.cc:372: DCHECK(global_);
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).