ntp_tiles: Add UMA histograms to correlate icon type to position

components/ntp_tiles/metrics.cc

Index: components/ntp_tiles/metrics.cc
diff --git a/components/ntp_tiles/metrics.cc b/components/ntp_tiles/metrics.cc
index e407af25715fac4eb4370462e687af960b69d18e..030f14c17143993afc20a71baaf83b61931431c3 100644
--- a/components/ntp_tiles/metrics.cc
+++ b/components/ntp_tiles/metrics.cc
@@ -26,6 +26,11 @@ const char kHistogramServerName[] = "server";
 const char kHistogramPopularName[] = "popular";
 const char kHistogramWhitelistName[] = "whitelist";
 
+// Prefixes for the various icon types.

[Marc Treib, 2017/02/03 15:03:31]

nit: Suffixes

[mastiz, 2017/02/06 10:59:49]

Done.

+const char kIconTypeSuffixColor[] = "Color";
+const char kIconTypeSuffixGray[] = "Gray";
+const char kIconTypeSuffixReal[] = "Real";
+
 // Log an event for a given |histogram| at a given element |position|. This
 // routine exists because regular histogram macros are cached thus can't be used
 // if the name of the histogram will change at a given call site.
@@ -54,6 +59,23 @@ std::string GetSourceHistogramName(NTPTileSource source) {
   return std::string();
 }
 
+const char* GetIconTypeSuffix(MostVisitedTileType type) {
+  switch (type) {
+    case ICON_COLOR:
+      return kIconTypeSuffixColor;
+    case ICON_DEFAULT:
+      return kIconTypeSuffixGray;
+    case ICON_REAL:
+      return kIconTypeSuffixReal;
+    case NONE:                     // Fall through.
+    case NUM_RECORDED_TILE_TYPES:  // Fall through.
+    case THUMBNAIL:                // Fall through.
+    case UNKNOWN_TILE_TYPE:
+      break;
+  }
+  return nullptr;
+}
+
 }  // namespace
 
 void RecordPageImpression(const std::vector<TileImpression>& tiles,
@@ -89,25 +111,17 @@ void RecordPageImpression(const std::vector<TileImpression>& tiles,
         base::StringPrintf("NewTabPage.TileType.%s", source_name.c_str());
     LogHistogramEvent(tile_type_histogram, tile_type, NUM_RECORDED_TILE_TYPES);
 
-    switch (tile_type) {
-      case NONE:
-        break;
-      case ICON_COLOR:
-        rappor::SampleDomainAndRegistryFromGURL(
-            rappor_service, "NTP.SuggestionsImpressions.IconsColor", url);
-        break;
-      case ICON_DEFAULT:
-        rappor::SampleDomainAndRegistryFromGURL(
-            rappor_service, "NTP.SuggestionsImpressions.IconsGray", url);
-        break;
-      case ICON_REAL:
-        rappor::SampleDomainAndRegistryFromGURL(
-            rappor_service, "NTP.SuggestionsImpressions.IconsReal", url);
-        break;
-      case NUM_RECORDED_TILE_TYPES:  // Fall through.
-      case THUMBNAIL:  // Fall through.
-      case UNKNOWN_TILE_TYPE:
-        NOTREACHED();
+    const char* icon_type_suffix = GetIconTypeSuffix(tile_type);
+    if (icon_type_suffix) {
+      rappor::SampleDomainAndRegistryFromGURL(
+          rappor_service,
+          base::StringPrintf("NTP.SuggestionsImpressions.Icons%s",
+                             icon_type_suffix),
+          url);
+
+      std::string icon_impression_histogram = base::StringPrintf(
+          "NewTabPage.SuggestionsImpressionIcon.%s", icon_type_suffix);

[Marc Treib, 2017/02/03 15:03:31]

Hrm, somehow all our metrics use different naming conventions (here
"Icon.Color", below ".IconsColor"). But I like this naming scheme more than the
below one, so...eh.

[mastiz, 2017/02/06 10:59:49]

Acknowledged.

+      LogHistogramEvent(icon_impression_histogram, index, kMaxNumTiles);
     }
   }
 
@@ -130,6 +144,13 @@ void RecordTileClick(int index,
       "NewTabPage.MostVisited.%s", GetSourceHistogramName(source).c_str());
   LogHistogramEvent(histogram, index, kMaxNumTiles);
 
+  const char* icon_type_suffix = GetIconTypeSuffix(tile_type);
+  if (icon_type_suffix) {
+    std::string icon_histogram = base::StringPrintf(
+        "NewTabPage.MostVisitedClickIcon.%s", icon_type_suffix);

[Marc Treib, 2017/02/03 15:03:31]

Just MostVisitedIcon, to be slightly more consistent with the other MostVisited
histograms?

[mastiz, 2017/02/06 10:59:49]

Unless you feel strongly, I'd really like to put Click on the name. The old
histogram is very badly named and can be mistakenly interpreted as impression
(this one about icons even more).

[Marc Treib, 2017/02/06 11:09:54]

Eh.. I tend to value consistency over accuracy, but here I don't feel strongly
enough to argue.

[mastiz, 2017/02/07 08:27:41]

Thanks, I'll then stick to having Clicks in the name.

+    LogHistogramEvent(icon_histogram, index, kMaxNumTiles);
+  }
+
   if (tile_type < NUM_RECORDED_TILE_TYPES) {
     UMA_HISTOGRAM_ENUMERATION("NewTabPage.TileTypeClicked", tile_type,
                               NUM_RECORDED_TILE_TYPES);