[Android NTP] Logging histograms for users that are part of a control group.

[Android NTP] Logging histograms for users that are part of a control group.

This adds the possibility of a control group for suggestions, where the user is served local TopSites suggestions, but would have received suggestions from the SuggestionsService. This enables us to avoid a population bias.

Regarding UMA_ prefix for histograms, will rebase once https://codereview.chromium.org/343643002/ gets in.

BUG=385159
TEST=SuggestionsServiceTest*

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

[Mathieu, 2014-06-18 17:30:25]

Hi,

Have a look please:

beaudoin: suggestions/ and overall control group logic
newt: most_visited_sites*

Thanks!

[beaudoin1, 2014-06-18 17:46:55]

LGTM if you confirm that the macro allows dynamic histogram names.

(Cute and short fix, btw, nice!)

https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:289: index);
I know many histogram macros do not support a dynamic histogram name. From a
cursory look it seems that this specific one may be good, but you may want to
double check.

If is_control_group_ ever changes and the macro doesn't support dynamic names I
believe it will trigger a DCHECK. This would make for a very strange behaviour.
From looking at the code, I don't think there is a strong guarantee that
is_control_group_ will never change.

https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:300:
UMA_HISTOGRAM_SPARSE_SLOWLY(kOpenedItemServerHistogramName, index);
Out of curiosity: What's the difference between the regular and UMA_ flavor of
the macro?

https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:367:
kImpressionControlHistogramName : kImpressionClientHistogramName, i);
Same remark here.

[Mathieu, 2014-06-18 17:55:09]

Thanks a lot. See my response.

https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:289: index);
On 2014/06/18 17:46:54, beaudoin1 wrote:
> I know many histogram macros do not support a dynamic histogram name. From a
> cursory look it seems that this specific one may be good, but you may want to
> double check.
> 
> If is_control_group_ ever changes and the macro doesn't support dynamic names
I
> believe it will trigger a DCHECK. This would make for a very strange
behaviour.
> From looking at the code, I don't think there is a strong guarantee that
> is_control_group_ will never change.

Ah good catch, I even have the function in this file to log dynamic histograms.
Changed.

https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:300:
UMA_HISTOGRAM_SPARSE_SLOWLY(kOpenedItemServerHistogramName, index);
On 2014/06/18 17:46:54, beaudoin1 wrote:
> Out of curiosity: What's the difference between the regular and UMA_ flavor of
> the macro?

The COUNTS macros don't work very well when the maximum is dynamic. It should be
the same across all callers of the macro. So on desktop the max is 8 but here it
varies. SPARSE works best.

https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:367:
kImpressionControlHistogramName : kImpressionClientHistogramName, i);
On 2014/06/18 17:46:54, beaudoin1 wrote:
> Same remark here.

Done.

[beaudoin, 2014-06-18 19:51:05]

On 2014/06/18 17:55:09, Mathieu Perreault wrote:
> Thanks a lot. See my response.
> 
>
https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
> File chrome/browser/android/most_visited_sites.cc (right):
> 
>
https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
> chrome/browser/android/most_visited_sites.cc:289: index);
> On 2014/06/18 17:46:54, beaudoin1 wrote:
> > I know many histogram macros do not support a dynamic histogram name. From a
> > cursory look it seems that this specific one may be good, but you may want
to
> > double check.
> > 
> > If is_control_group_ ever changes and the macro doesn't support dynamic
names
> I
> > believe it will trigger a DCHECK. This would make for a very strange
> behaviour.
> > From looking at the code, I don't think there is a strong guarantee that
> > is_control_group_ will never change.
> 
> Ah good catch, I even have the function in this file to log dynamic
histograms.
> Changed.
> 
>
https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
> chrome/browser/android/most_visited_sites.cc:300:
> UMA_HISTOGRAM_SPARSE_SLOWLY(kOpenedItemServerHistogramName, index);
> On 2014/06/18 17:46:54, beaudoin1 wrote:
> > Out of curiosity: What's the difference between the regular and UMA_ flavor
of
> > the macro?
> 
> The COUNTS macros don't work very well when the maximum is dynamic. It should
be
> the same across all callers of the macro. So on desktop the max is 8 but here
it
> varies. SPARSE works best.
> 
>
https://codereview.chromium.org/341943002/diff/20001/chrome/browser/android/m...
> chrome/browser/android/most_visited_sites.cc:367:
> kImpressionControlHistogramName : kImpressionClientHistogramName, i);
> On 2014/06/18 17:46:54, beaudoin1 wrote:
> > Same remark here.
> 
> Done.

LGTM again from my chromium.

[newt (away), 2014-06-18 21:25:09]

I think I'm missing some context here. In particular, how is this new control
group different from the existing group of users for whom the SuggestionsService
is disabled? Also, why are you logging different histograms depending on which
group the user is in?

https://codereview.chromium.org/341943002/diff/40001/chrome/browser/android/m...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/341943002/diff/40001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:287: const std::string histogram =
is_control_group_ ?
why log a different histogram depending on which group we're in? The finch
dashboard allows you to compare histograms between different trials, so it seems
like it'd be easier to compare histograms if you used the same name in all
cases.

https://codereview.chromium.org/341943002/diff/40001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:366: const std::string histogram =
is_control_group_ ?
pull the string assignment out of the loop

https://codereview.chromium.org/341943002/diff/40001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:386: is_control_group_ = size &&
SuggestionsService::IsControlGroup();
why check size here? why not just:

    is_control_group_ = SuggestionsService::IsControlGroup();

[Mathieu, 2014-06-18 21:55:18]

https://codereview.chromium.org/341943002/diff/40001/chrome/browser/android/m...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/341943002/diff/40001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:287: const std::string histogram =
is_control_group_ ?
On 2014/06/18 21:25:09, newt wrote:
> why log a different histogram depending on which group we're in? The finch
> dashboard allows you to compare histograms between different trials, so it
seems
> like it'd be easier to compare histograms if you used the same name in all
> cases.

See other comment, but we basically want to calculate clicks for "client" (non
syncing), client0 (syncing but local suggestions)  and server0 (syncing and
server suggestions.

https://codereview.chromium.org/341943002/diff/40001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:386: is_control_group_ = size &&
SuggestionsService::IsControlGroup();
On 2014/06/18 21:25:09, newt wrote:
> why check size here? why not just:
> 
>     is_control_group_ = SuggestionsService::IsControlGroup();

the main reason is that we have a population bias and we need to measure the
population that would have gotten our suggestions as the control group.

Hence the check for (size), which means the user is syncing and we have data for
them.

[newt (away), 2014-06-18 22:02:27]

Thanks for explaining. lgtm after string assignment fix.

[Mathieu, 2014-06-19 01:45:52]

Thanks! Submitting...

https://codereview.chromium.org/341943002/diff/40001/chrome/browser/android/m...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/341943002/diff/40001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:366: const std::string histogram =
is_control_group_ ?
On 2014/06/18 21:25:09, newt wrote:
> pull the string assignment out of the loop

Done.

[Mathieu, 2014-06-19 01:45:56]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-06-19 01:50:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/341943002/80001

[Mathieu, 2014-06-19 02:43:44]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-06-19 02:45:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/341943002/100001

[commit-bot: I haz the power, 2014-06-19 11:43:15]

Change committed as 278333