[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..0fff60d301bfba37cfca172266b8a1e02e21d402 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);
+      UMA_HISTOGRAM_SPARSE_SLOWLY(is_control_group_ ?
+          kOpenedItemControlHistogramName : kOpenedItemClientHistogramName,
+          index);

[beaudoin1, 2014/06/18 17:46:54]

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.

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

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

       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);

[beaudoin1, 2014/06/18 17:46:54]

Out of curiosity: What's the difference between the regular and UMA_ flavor of
the macro?

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

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. 

         }
       }
       break;
@@ -353,9 +361,10 @@ 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);
+    UMA_HISTOGRAM_SPARSE_SLOWLY(is_control_group_ ?
+        kImpressionControlHistogramName : kImpressionClientHistogramName, i);

[beaudoin1, 2014/06/18 17:46:54]

Same remark here.

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

Done.

   }
 
   JNIEnv* env = AttachCurrentThread();
@@ -370,8 +379,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();
+
+  // 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 +404,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 +469,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;
 }