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

chrome/browser/android/most_visited_sites.cc

Index: chrome/browser/android/most_visited_sites.cc
diff --git a/chrome/browser/android/most_visited_sites.cc b/chrome/browser/android/most_visited_sites.cc
index 07854b8d0fd55881fb04fb81f6ab9e6c81b68259..e3c498fa79f2a6d33f883180ed08646b269e01a8 100644
--- a/chrome/browser/android/most_visited_sites.cc
+++ b/chrome/browser/android/most_visited_sites.cc
@@ -58,6 +58,8 @@ const char kNumEmptyTilesHistogramName[] = "NewTabPage.NumberOfGrayTiles";
 const char kNumServerTilesHistogramName[] = "NewTabPage.NumberOfExternalTiles";
 // Client suggestion opened.
 const char kOpenedItemClientHistogramName[] = "NewTabPage.MostVisited.client";
+// Control group suggestion opened.
+const char kOpenedItemControlHistogramName[] = "NewTabPage.MostVisited.client0";
 // Server suggestion opened, no provider.
 const char kOpenedItemServerHistogramName[] = "NewTabPage.MostVisited.server";
 // Server suggestion opened with provider.
@@ -66,6 +68,9 @@ const char kOpenedItemServerProviderHistogramFormat[] =
 // Client impression.
 const char kImpressionClientHistogramName[] =
     "NewTabPage.SuggestionsImpression.client";
+// Control group impression.
+const char kImpressionControlHistogramName[] =
+    "NewTabPage.SuggestionsImpression.client0";
 // Server suggestion impression, no provider.
 const char kImpressionServerHistogramName[] =
     "NewTabPage.SuggestionsImpression.server";
@@ -168,8 +173,9 @@ void LogHistogramEvent(const std::string& histogram, int position,
 }  // namespace
 
 MostVisitedSites::MostVisitedSites(Profile* profile)
-    : profile_(profile), num_sites_(0), num_local_thumbs_(0),
-      num_server_thumbs_(0), num_empty_thumbs_(0), weak_ptr_factory_(this) {
+    : profile_(profile), num_sites_(0), is_control_group_(false),
+      num_local_thumbs_(0), num_server_thumbs_(0), num_empty_thumbs_(0),
+      weak_ptr_factory_(this) {
   // Register the debugging page for the Suggestions Service and the thumbnails
   // debugging page.
   content::URLDataSource::Add(profile_,
@@ -278,7 +284,9 @@ void MostVisitedSites::RecordOpenedMostVisitedItem(JNIEnv* env,
                                                    jint index) {
   switch (mv_source_) {
     case TOP_SITES: {
-      HISTOGRAM_SPARSE_SLOWLY(kOpenedItemClientHistogramName, index);
+      const std::string histogram = is_control_group_ ?

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

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.

[Mathieu, 2014/06/18 21:55:17]

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. 

+          kOpenedItemControlHistogramName : kOpenedItemClientHistogramName;
+      LogHistogramEvent(histogram, index, num_sites_);
       break;
     }
     case SUGGESTIONS_SERVICE: {
@@ -289,7 +297,7 @@ void MostVisitedSites::RecordOpenedMostVisitedItem(JNIEnv* env,
               server_suggestions_.suggestions(index).providers(0));
           LogHistogramEvent(histogram, index, num_sites_);
         } else {
-          HISTOGRAM_SPARSE_SLOWLY(kOpenedItemServerHistogramName, index);
+          UMA_HISTOGRAM_SPARSE_SLOWLY(kOpenedItemServerHistogramName, index);
         }
       }
       break;
@@ -353,9 +361,11 @@ void MostVisitedSites::OnMostVisitedURLsAvailable(
   mv_source_ = TOP_SITES;
 
   int num_tiles = urls.size();
-  HISTOGRAM_SPARSE_SLOWLY(kNumTilesHistogramName, num_tiles);
+  UMA_HISTOGRAM_SPARSE_SLOWLY(kNumTilesHistogramName, num_tiles);
   for (int i = 0; i < num_tiles; ++i) {
-    HISTOGRAM_SPARSE_SLOWLY(kImpressionClientHistogramName, i);
+    const std::string histogram = is_control_group_ ?

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

pull the string assignment out of the loop

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

Done.

+        kImpressionControlHistogramName : kImpressionClientHistogramName;
+    LogHistogramEvent(histogram, i, num_sites_);
   }
 
   JNIEnv* env = AttachCurrentThread();
@@ -370,8 +380,14 @@ void MostVisitedSites::OnSuggestionsProfileAvailable(
     ScopedJavaGlobalRef<jobject>* j_observer,
     const SuggestionsProfile& suggestions_profile) {
   int size = suggestions_profile.suggestions_size();
-  if (size == 0) {
-    // No suggestions data available, initiate Top Sites query.
+
+  // Determine if the user is in a control group (they would have received
+  // suggestions, but are in a group where they shouldn't).
+  is_control_group_ = size && SuggestionsService::IsControlGroup();

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

why check size here? why not just:

    is_control_group_ = SuggestionsService::IsControlGroup();

[Mathieu, 2014/06/18 21:55:17]

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. 

+
+  // If no suggestions data is available or the user is in a control group,
+  // initiate Top Sites query.
+  if (is_control_group_ || !size) {
     InitiateTopSitesQuery();
     return;
   }
@@ -389,10 +405,10 @@ void MostVisitedSites::OnSuggestionsProfileAvailable(
           kImpressionServerHistogramFormat, suggestion.providers(0));
       LogHistogramEvent(histogram, i, num_sites_);
     } else {
-      HISTOGRAM_SPARSE_SLOWLY(kImpressionServerHistogramName, i);
+      UMA_HISTOGRAM_SPARSE_SLOWLY(kImpressionServerHistogramName, i);
     }
   }
-  HISTOGRAM_SPARSE_SLOWLY(kNumTilesHistogramName, i);
+  UMA_HISTOGRAM_SPARSE_SLOWLY(kNumTilesHistogramName, i);
 
   mv_source_ = SUGGESTIONS_SERVICE;
   // Keep a copy of the suggestions for eventual logging.
@@ -454,12 +470,12 @@ void MostVisitedSites::OnSuggestionsThumbnailAvailable(
 }
 
 void MostVisitedSites::RecordUMAMetrics() {
-  HISTOGRAM_SPARSE_SLOWLY(kNumLocalThumbnailTilesHistogramName,
+  UMA_HISTOGRAM_SPARSE_SLOWLY(kNumLocalThumbnailTilesHistogramName,
                           num_local_thumbs_);
   num_local_thumbs_ = 0;
-  HISTOGRAM_SPARSE_SLOWLY(kNumEmptyTilesHistogramName, num_empty_thumbs_);
+  UMA_HISTOGRAM_SPARSE_SLOWLY(kNumEmptyTilesHistogramName, num_empty_thumbs_);
   num_empty_thumbs_ = 0;
-  HISTOGRAM_SPARSE_SLOWLY(kNumServerTilesHistogramName, num_server_thumbs_);
+  UMA_HISTOGRAM_SPARSE_SLOWLY(kNumServerTilesHistogramName, num_server_thumbs_);
   num_server_thumbs_ = 0;
 }