Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10375043:
Variations Service now creates field trials from variations seed. (Closed)
|
Created:
8 years, 7 months ago by jwd Modified:
8 years, 7 months ago CC:
chromium-reviews, MAD, Ilya Sherman, jar (doing other things), finch-team_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionVariations Service now creates field trials from variations seed.
BUG=121695
TEST=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135858
Patch Set 1 #
Total comments: 35
Patch Set 2 : MAD's comments and Fix unittests #
Total comments: 8
Patch Set 3 : Alexei's comments #
Total comments: 17
Patch Set 4 : MAD's and Alexei's comments fixed #Patch Set 5 : #
Messages
Total messages: 20 (0 generated)
First pass, still very messy.
Très cool! First round of comments... Thanks! BYE MAD... https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:55: LOG(INFO) << "Creating trials from seed"; Either remove all LOGs or use DVLOG(1)s https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:62: CreateTrialFromStudy(seed.study(i)); For single line block, we don't use {} https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:155: // An empty channel list matches all channels. Add {} if you have more than one line, including comments. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:158: for (int i = 0; i < study.channel_size(); i++) { i++ -> ++i https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:208: epoch + base::TimeDelta::FromSeconds(study.start_date()); Good catch, thanks for fixing... https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:266: bool has_default_group_number; Please initialize these two variables in case we don't enter the loop below... https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:275: default_group_number = study.experiment(i).experiment_id(); These are not related, the experiment_id, is the GWS ID... /methinks... https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:296: has_default_group_number ? &default_group_number : NULL)); This is not needed, you don't use it, and you won't need it... The default_group_number is only needed if one would like to compare the chosen group to this value... https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:303: if (study.experiment(i).name() != default_group_name) Please use {} (for both the for and the if) when there are more than one lines in the block, even if it's a single statement... https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:83: chrome_variations::TrialsSeed& seed); We don't use & for return argument, we use a * to avoid confusion.
Some style nits your way: https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:61: for (int i = 0; i<seed.study_size(); ++i) { Put spaces around the <. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:154: if (study.channel_size()==0) Put spaces around the ==. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:253: if (!study.has_default_experiment_name()){ Put a space before {. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:267: for (int i = 0; i<study.experiment_size(); ++i) { Put spaces around the <. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:299: study.consistency()==chrome_variations::Study_Consistency_PERMANENT) Put spaces around the ==. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:302: for (int i = 0; i<study.experiment_size(); ++i) Put spaces around the <. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:82: bool LoadVariationsSeed(PrefService* local_prefs, Better name: LoadTrialsSeedFromPrefs().
https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:305: study.experiment(i).probability_weight()); You also need to associate the study.experiment(i).experiment_id() here using the API SteveT added.
On 2012/05/08 03:25:26, Alexei Svitkine wrote: > https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... > File chrome/browser/metrics/variations_service.cc (right): > > https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... > chrome/browser/metrics/variations_service.cc:305: > study.experiment(i).probability_weight()); > You also need to associate the study.experiment(i).experiment_id() here using > the API SteveT added. See this CL for how to associate the trials with the experiments ids: http://codereview.chromium.org/10317011/
Thanks for the review. MAD's comments fixed, just noticed Alexei's. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:55: LOG(INFO) << "Creating trials from seed"; On 2012/05/08 03:09:00, MAD wrote: > Either remove all LOGs or use DVLOG(1)s Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:62: CreateTrialFromStudy(seed.study(i)); On 2012/05/08 03:09:00, MAD wrote: > For single line block, we don't use {} Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:158: for (int i = 0; i < study.channel_size(); i++) { On 2012/05/08 03:09:00, MAD wrote: > i++ -> ++i Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:275: default_group_number = study.experiment(i).experiment_id(); On 2012/05/08 03:09:00, MAD wrote: > These are not related, the experiment_id, is the GWS ID... /methinks... Ya, I think I was confused about this when I wrote it. I'm getting rid of the group number stuff. It looks like there is no way to specify the default group number in the protobuf. Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:296: has_default_group_number ? &default_group_number : NULL)); On 2012/05/08 03:09:00, MAD wrote: > This is not needed, you don't use it, and you won't need it... The > default_group_number is only needed if one would like to compare the chosen > group to this value... Ya, got rid of this now. Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.h:83: chrome_variations::TrialsSeed& seed); On 2012/05/08 03:09:00, MAD wrote: > We don't use & for return argument, we use a * to avoid confusion. Ah, oops, makes sense. DONE
Unfortunately we won't be able to support this just yet... The API doesn't support an overwrite... We'll wait for next round to fix both sides... On Mon, May 7, 2012 at 11:29 PM, <asvitkine@chromium.org> wrote: > On 2012/05/08 03:25:26, Alexei Svitkine wrote: > > https://chromiumcodereview.**appspot.com/10375043/diff/1/** > chrome/browser/metrics/**variations_service.cc<https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics/variations_service.cc> > >> File chrome/browser/metrics/**variations_service.cc (right): >> > > > https://chromiumcodereview.**appspot.com/10375043/diff/1/** > chrome/browser/metrics/**variations_service.cc#**newcode305<https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics/variations_service.cc#newcode305> > >> chrome/browser/metrics/**variations_service.cc:305: >> study.experiment(i).**probability_weight()); >> You also need to associate the study.experiment(i).**experiment_id() >> here using >> the API SteveT added. >> > > See this CL for how to associate the trials with the experiments ids: > > http://codereview.chromium.**org/10317011/<http://codereview.chromium.org/103... > > http://codereview.chromium.**org/10375043/<http://codereview.chromium.org/103... >
https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:275: default_group_number = study.experiment(i).experiment_id(); > I'm getting rid of the > group number stuff. It looks like there is no way to specify the default group > number in the protobuf. That parameter is an "out" parameter, so no need to use it at all (just pass NULL).
https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:61: for (int i = 0; i<seed.study_size(); ++i) { On 2012/05/08 03:22:54, Alexei Svitkine wrote: > Put spaces around the <. Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:154: if (study.channel_size()==0) On 2012/05/08 03:22:54, Alexei Svitkine wrote: > Put spaces around the ==. Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:155: // An empty channel list matches all channels. On 2012/05/08 03:09:00, MAD wrote: > Add {} if you have more than one line, including comments. Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:253: if (!study.has_default_experiment_name()){ On 2012/05/08 03:22:54, Alexei Svitkine wrote: > Put a space before {. Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:267: for (int i = 0; i<study.experiment_size(); ++i) { On 2012/05/08 03:22:54, Alexei Svitkine wrote: > Put spaces around the <. Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:275: default_group_number = study.experiment(i).experiment_id(); On 2012/05/08 03:31:27, Alexei Svitkine wrote: > > I'm getting rid of the > > group number stuff. It looks like there is no way to specify the default group > > number in the protobuf. > > That parameter is an "out" parameter, so no need to use it at all (just pass > NULL). Ya, I was very confused when I wrote that. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:299: study.consistency()==chrome_variations::Study_Consistency_PERMANENT) On 2012/05/08 03:22:54, Alexei Svitkine wrote: > Put spaces around the ==. Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:302: for (int i = 0; i<study.experiment_size(); ++i) On 2012/05/08 03:22:54, Alexei Svitkine wrote: > Put spaces around the <. Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:303: if (study.experiment(i).name() != default_group_name) On 2012/05/08 03:09:00, MAD wrote: > Please use {} (for both the for and the if) when there are more than one lines > in the block, even if it's a single statement... Done. https://chromiumcodereview.appspot.com/10375043/diff/1/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:305: study.experiment(i).probability_weight()); On 2012/05/08 03:25:26, Alexei Svitkine wrote: > You also need to associate the study.experiment(i).experiment_id() here using > the API SteveT added. MAD says that that's not important for M20, but yes I do.
LGTM with some more stylish nits on top of the spacing nits from Alexei, as well as a TODO comment request... Thanks again! BYE MAD... https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:246: found_default_group = true; No need for {} https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:274: trial->AppendGroup(study.experiment(i).name(), Also use {} for if since code block is on two lines... https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:277: Please add a TODO for the experiment_id assocition. https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... File chrome/browser/metrics/variations_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... chrome/browser/metrics/variations_service_unittest.cc:42: const bool expected = channel_added[j] || study.channel_size()==0; spaces around == https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... chrome/browser/metrics/variations_service_unittest.cc:60: const bool expected = channel_added[j] || study.channel_size()==0; spaces around ==
http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service.cc (right): http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:249: if (!found_default_group){ Space before {. http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:253: // valid. Nit remove space before "valid". http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:266: exploded_end_date.month, exploded_end_date.day_of_month, NULL)); Nit: remove space before NULL. http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:269: study.consistency() == chrome_variations::Study_Consistency_PERMANENT) Nit: Add {'s. http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.cc:275: study.experiment(i).probability_weight()); Nit: align. http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service_unittest.cc (right): http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:42: const bool expected = channel_added[j] || study.channel_size()==0; Spaces around ==. http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service_unittest.cc:60: const bool expected = channel_added[j] || study.channel_size()==0; Spaces around ==.
http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... File chrome/browser/metrics/variations_service.h (right): http://codereview.chromium.org/10375043/diff/3003/chrome/browser/metrics/vari... chrome/browser/metrics/variations_service.h:82: bool LoadVariationsSeed(PrefService* local_prefs, Also, see my previous comment: rename this to LoadTrialsSeedFromPrefs().
Drive-by nit. lgtm https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:275: study.experiment(i).probability_weight()); Nit: Braces around if body, since it spans 2 physical lines.
https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:246: found_default_group = true; On 2012/05/08 03:39:03, MAD wrote: > No need for {} Oop, missed that, not paying attention. Done https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:274: trial->AppendGroup(study.experiment(i).name(), On 2012/05/08 03:39:03, MAD wrote: > Also use {} for if since code block is on two lines... I do not generalize... DONE https://chromiumcodereview.appspot.com/10375043/diff/6/chrome/browser/metrics... chrome/browser/metrics/variations_service.cc:277: On 2012/05/08 03:39:03, MAD wrote: > Please add a TODO for the experiment_id assocition. Done. https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... File chrome/browser/metrics/variations_service.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:249: if (!found_default_group){ On 2012/05/08 03:40:13, Alexei Svitkine wrote: > Space before {. Aw man, I'm missing all of these... Done. https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:253: // valid. On 2012/05/08 03:40:13, Alexei Svitkine wrote: > Nit remove space before "valid". Done. https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:266: exploded_end_date.month, exploded_end_date.day_of_month, NULL)); On 2012/05/08 03:40:13, Alexei Svitkine wrote: > Nit: remove space before NULL. Done. https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:269: study.consistency() == chrome_variations::Study_Consistency_PERMANENT) On 2012/05/08 03:40:13, Alexei Svitkine wrote: > Nit: Add {'s. Done. https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... chrome/browser/metrics/variations_service.cc:275: study.experiment(i).probability_weight()); On 2012/05/08 03:40:13, Alexei Svitkine wrote: > Nit: align. Done. https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... File chrome/browser/metrics/variations_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... chrome/browser/metrics/variations_service_unittest.cc:42: const bool expected = channel_added[j] || study.channel_size()==0; On 2012/05/08 03:40:13, Alexei Svitkine wrote: > Spaces around ==. Done. https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... chrome/browser/metrics/variations_service_unittest.cc:60: const bool expected = channel_added[j] || study.channel_size()==0; On 2012/05/08 03:40:13, Alexei Svitkine wrote: > Spaces around ==. Done.
https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... File chrome/browser/metrics/variations_service.h (right): https://chromiumcodereview.appspot.com/10375043/diff/3003/chrome/browser/metr... chrome/browser/metrics/variations_service.h:82: bool LoadVariationsSeed(PrefService* local_prefs, On 2012/05/08 03:41:34, Alexei Svitkine wrote: > Also, see my previous comment: rename this to LoadTrialsSeedFromPrefs(). Done.
Excellent! LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jwd@chromium.org/10375043/6002
Try job failure for 10375043-6002 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jwd@chromium.org/10375043/6002
Try job failure for 10375043-6002 (retry) (retry) (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
