Log suggestion scores in 10 discrete buckets.

components/ntp_snippets/content_suggestions_metrics.cc

Index: components/ntp_snippets/content_suggestions_metrics.cc
diff --git a/components/ntp_snippets/content_suggestions_metrics.cc b/components/ntp_snippets/content_suggestions_metrics.cc
index 4f61eed1fa9af77c9dbb0f6ea8386caaae14bbf1..26823ad7b31d054d0df0ea4d7e001fd912c7db84 100644
--- a/components/ntp_snippets/content_suggestions_metrics.cc
+++ b/components/ntp_snippets/content_suggestions_metrics.cc
@@ -4,10 +4,12 @@
 
 #include "components/ntp_snippets/content_suggestions_metrics.h"
 
+#include <cmath>
 #include <string>
 #include <type_traits>
 
 #include "base/metrics/histogram.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/strings/stringprintf.h"
@@ -25,18 +27,19 @@ const char kHistogramCountOnNtpOpened[] =
     "NewTabPage.ContentSuggestions.CountOnNtpOpened";
 const char kHistogramShown[] = "NewTabPage.ContentSuggestions.Shown";
 const char kHistogramShownAge[] = "NewTabPage.ContentSuggestions.ShownAge";
-const char kHistogramShownScore[] = "NewTabPage.ContentSuggestions.ShownScore";
+const char kHistogramShownScore[] =
+    "NewTabPage.ContentSuggestions.ShownScoreNormalized";
 const char kHistogramOpened[] = "NewTabPage.ContentSuggestions.Opened";
 const char kHistogramOpenedAge[] = "NewTabPage.ContentSuggestions.OpenedAge";
 const char kHistogramOpenedScore[] =
-    "NewTabPage.ContentSuggestions.OpenedScore";
+    "NewTabPage.ContentSuggestions.OpenedScoreNormalized";
 const char kHistogramOpenDisposition[] =
     "NewTabPage.ContentSuggestions.OpenDisposition";
 const char kHistogramMenuOpened[] = "NewTabPage.ContentSuggestions.MenuOpened";
 const char kHistogramMenuOpenedAge[] =
     "NewTabPage.ContentSuggestions.MenuOpenedAge";
 const char kHistogramMenuOpenedScore[] =
-    "NewTabPage.ContentSuggestions.MenuOpenedScore";
+    "NewTabPage.ContentSuggestions.MenuOpenedScoreNormalized";
 const char kHistogramDismissedUnvisited[] =
     "NewTabPage.ContentSuggestions.DismissedUnvisited";
 const char kHistogramDismissedVisited[] =
@@ -60,7 +63,7 @@ const char kPerCategoryHistogramFormat[] = "%s.%s";
 // and contains exactly the values to be recorded in UMA. Don't remove or
 // reorder elements, only add new ones at the end (before COUNT), and keep in
 // sync with ContentSuggestionsCategory in histograms.xml.
-enum class HistogramCategories {
+enum HistogramCategories {
   EXPERIMENTAL,
   RECENT_TABS,
   DOWNLOADS,
@@ -135,27 +138,6 @@ std::string GetCategoryHistogramName(const char* base_name, Category category) {
                             GetCategorySuffix(category).c_str());
 }
 
-// This corresponds to UMA_HISTOGRAM_ENUMERATION, for use with dynamic histogram
-// names.
-void UmaHistogramEnumeration(const std::string& name,
-                             int value,
-                             int boundary_value) {
-  base::LinearHistogram::FactoryGet(
-      name, 1, boundary_value, boundary_value + 1,
-      base::HistogramBase::kUmaTargetedHistogramFlag)
-      ->Add(value);
-}
-
-// This corresponds to UMA_HISTOGRAM_LONG_TIMES for use with dynamic histogram
-// names.
-void UmaHistogramLongTimes(const std::string& name,
-                           const base::TimeDelta& value) {
-  base::Histogram::FactoryTimeGet(
-      name, base::TimeDelta::FromMilliseconds(1), base::TimeDelta::FromHours(1),
-      50, base::HistogramBase::kUmaTargetedHistogramFlag)
-      ->AddTime(value);
-}
-
 // This corresponds to UMA_HISTOGRAM_CUSTOM_TIMES (with min/max appropriate
 // for the age of suggestions) for use with dynamic histogram names.
 void UmaHistogramAge(const std::string& name, const base::TimeDelta& value) {
@@ -165,29 +147,13 @@ void UmaHistogramAge(const std::string& name, const base::TimeDelta& value) {
       ->AddTime(value);
 }
 
-// This corresponds to UMA_HISTOGRAM_CUSTOM_COUNTS (with min/max appropriate
-// for the score of suggestions) for use with dynamic histogram names.
-void UmaHistogramScore(const std::string& name, float value) {
-  base::Histogram::FactoryGet(name, 1, 100000, 50,
-                              base::HistogramBase::kUmaTargetedHistogramFlag)
-      ->Add(value);
-}
-
-void LogCategoryHistogramEnumeration(const char* base_name,
-                                     Category category,
-                                     int value,
-                                     int boundary_value) {
-  std::string name = GetCategoryHistogramName(base_name, category);
-  // Since the histogram name is dynamic, we can't use the regular macro.
-  UmaHistogramEnumeration(name, value, boundary_value);
-}
-
-void LogCategoryHistogramLongTimes(const char* base_name,
-                                   Category category,
-                                   const base::TimeDelta& value) {
+void LogCategoryHistogramPosition(const char* base_name,
+                                  Category category,
+                                  int position,
+                                  int max_position) {
   std::string name = GetCategoryHistogramName(base_name, category);
   // Since the histogram name is dynamic, we can't use the regular macro.
-  UmaHistogramLongTimes(name, value);
+  base::UmaHistogramExactLinear(name, position, max_position);
 }
 
 void LogCategoryHistogramAge(const char* base_name,
@@ -202,8 +168,12 @@ void LogCategoryHistogramScore(const char* base_name,
                                Category category,
                                float score) {
   std::string name = GetCategoryHistogramName(base_name, category);
-  // Since the histogram name is dynamic, we can't use the regular macro.
-  UmaHistogramScore(name, score);
+  // Scores are typically reported in a range of (0,1]. As UMA does not support
+  // floats, we put them on a discrete scale of [1,10]. We keep the extra bucket
+  // 11 for unexpected over-flows as we want to distinguish them from scores
+  // close to 1. For instance, the discrete value 1 represents score values
+  // within (0.0, 0.1].
+  base::UmaHistogramExactLinear(name, ceil(score * 10), 11);
 }
 
 // Records ContentSuggestions usage. Therefore the day is sliced into 20min
@@ -222,10 +192,9 @@ void RecordContentSuggestionsUsage() {
   std::string histogram_name(
       base::StringPrintf("%s.%s", kHistogramArticlesUsageTimeLocal,
                          kWeekdayNames[now_exploded.day_of_week]));
-  UmaHistogramEnumeration(histogram_name, bucket, kNumBuckets);
-
-  UMA_HISTOGRAM_ENUMERATION(kHistogramArticlesUsageTimeLocal, bucket,
-                            kNumBuckets);
+  base::UmaHistogramExactLinear(histogram_name, bucket, kNumBuckets);
+  UMA_HISTOGRAM_EXACT_LINEAR(kHistogramArticlesUsageTimeLocal, bucket,
+                             kNumBuckets);
 
   base::RecordAction(
       base::UserMetricsAction("NewTabPage_ContentSuggestions_ArticlesUsage"));
@@ -237,13 +206,12 @@ void OnPageShown(
     const std::vector<std::pair<Category, int>>& suggestions_per_category) {
   int suggestions_total = 0;
   for (const std::pair<Category, int>& item : suggestions_per_category) {
-    LogCategoryHistogramEnumeration(kHistogramCountOnNtpOpened, item.first,
-                                    item.second, kMaxSuggestionsPerCategory);
+    LogCategoryHistogramPosition(kHistogramCountOnNtpOpened, item.first,
+                                 item.second, kMaxSuggestionsPerCategory);
     suggestions_total += item.second;
   }
-
-  UMA_HISTOGRAM_ENUMERATION(kHistogramCountOnNtpOpened, suggestions_total,
-                            kMaxSuggestionsTotal);
+  UMA_HISTOGRAM_EXACT_LINEAR(kHistogramCountOnNtpOpened, suggestions_total,
+                             kMaxSuggestionsTotal);
 }
 
 void OnSuggestionShown(int global_position,
@@ -252,10 +220,10 @@ void OnSuggestionShown(int global_position,
                        base::Time publish_date,
                        base::Time last_background_fetch_time,
                        float score) {
-  UMA_HISTOGRAM_ENUMERATION(kHistogramShown, global_position,
-                            kMaxSuggestionsTotal);
-  LogCategoryHistogramEnumeration(kHistogramShown, category, category_position,
-                                  kMaxSuggestionsPerCategory);
+  UMA_HISTOGRAM_EXACT_LINEAR(kHistogramShown, global_position,
+                             kMaxSuggestionsTotal);
+  LogCategoryHistogramPosition(kHistogramShown, category, category_position,
+                               kMaxSuggestionsPerCategory);
 
   base::TimeDelta age = base::Time::Now() - publish_date;
   LogCategoryHistogramAge(kHistogramShownAge, category, age);
@@ -286,21 +254,24 @@ void OnSuggestionOpened(int global_position,
                         base::Time publish_date,
                         float score,
                         WindowOpenDisposition disposition) {
-  UMA_HISTOGRAM_ENUMERATION(kHistogramOpened, global_position,
-                            kMaxSuggestionsTotal);
-  LogCategoryHistogramEnumeration(kHistogramOpened, category, category_position,
-                                  kMaxSuggestionsPerCategory);
+  UMA_HISTOGRAM_EXACT_LINEAR(kHistogramOpened, global_position,
+                             kMaxSuggestionsTotal);
+  LogCategoryHistogramPosition(kHistogramOpened, category, category_position,
+                               kMaxSuggestionsPerCategory);
 
   base::TimeDelta age = base::Time::Now() - publish_date;
   LogCategoryHistogramAge(kHistogramOpenedAge, category, age);
 
   LogCategoryHistogramScore(kHistogramOpenedScore, category, score);
 
-  UMA_HISTOGRAM_ENUMERATION(
+  // TODO(tschumann): Why do we convert to int and incremenet by one??
+  // If not, we could use UmaHistogramEnumeration().

[Alexei Svitkine (slow), 2017/01/16 17:37:10]

It's because WindowOpenDisposition::MAX_VALUE is inclusive whereas the macros &
functions expect it to be exclusive.

+  UMA_HISTOGRAM_EXACT_LINEAR(

[Marc Treib, 2017/01/16 14:35:12]

I don't understand this change - what was wrong with UMA_HISTOGRAM_ENUMERATION
here?

[tschumann, 2017/01/16 15:00:52]

well, we're not passing in an enum :-)
base::UmaHistogramEnumeration (which I actually wanted to use) enforces the
types being enums.

[Marc Treib, 2017/01/16 15:12:24]

But we *are* passing in an enum here...? I guess the function doesn't work with
enum classes? That seems like something to fix there...

[tschumann, 2017/01/16 15:27:32]

nope, at least strictly speaking we're passing two integers. The max_value
parameter is not within the enum range anymore.
This is very theoretical of course, as these macros also turn the values into
integers.

The macro is not working well with enum classes as they assume it's implicitly
cast-able to integers (they simply add 1). Not sure if the function
UmaHistogramEnumeration() works with enum classes.

I'm fine with having the casts here and using the ENUM version of the logging
macros/functions, but the special treatment of max_value made me wonder. If you
think it's safe to reduce the upper boundary, then I'm happy to change this to
use the enum version (and add a comment in case we need the cast).

Adding new enum values should always be ok and needs no special treatment
(https://codesearch.chromium.org/chromium/src/base/metrics/histogram_macros.h?...).

       kHistogramOpenDisposition, static_cast<int>(disposition),
       static_cast<int>(WindowOpenDisposition::MAX_VALUE) + 1);
-  LogCategoryHistogramEnumeration(
-      kHistogramOpenDisposition, category, static_cast<int>(disposition),
+  base::UmaHistogramExactLinear(
+      GetCategoryHistogramName(kHistogramOpenDisposition, category),
+      static_cast<int>(disposition),
       static_cast<int>(WindowOpenDisposition::MAX_VALUE) + 1);
 
   if (category.IsKnownCategory(KnownCategories::ARTICLES)) {
@@ -313,11 +284,10 @@ void OnSuggestionMenuOpened(int global_position,
                             int category_position,
                             base::Time publish_date,
                             float score) {
-  UMA_HISTOGRAM_ENUMERATION(kHistogramMenuOpened, global_position,
-                            kMaxSuggestionsTotal);
-  LogCategoryHistogramEnumeration(kHistogramMenuOpened, category,
-                                  category_position,
-                                  kMaxSuggestionsPerCategory);
+  UMA_HISTOGRAM_EXACT_LINEAR(kHistogramMenuOpened, global_position,
+                             kMaxSuggestionsTotal);
+  LogCategoryHistogramPosition(kHistogramMenuOpened, category,
+                               category_position, kMaxSuggestionsPerCategory);
 
   base::TimeDelta age = base::Time::Now() - publish_date;
   LogCategoryHistogramAge(kHistogramMenuOpenedAge, category, age);
@@ -330,42 +300,42 @@ void OnSuggestionDismissed(int global_position,
                            int category_position,
                            bool visited) {
   if (visited) {
-    UMA_HISTOGRAM_ENUMERATION(kHistogramDismissedVisited, global_position,
-                              kMaxSuggestionsTotal);
-    LogCategoryHistogramEnumeration(kHistogramDismissedVisited, category,
-                                    category_position,
-                                    kMaxSuggestionsPerCategory);
+    UMA_HISTOGRAM_EXACT_LINEAR(kHistogramDismissedVisited, global_position,
+                               kMaxSuggestionsTotal);
+    LogCategoryHistogramPosition(kHistogramDismissedVisited, category,
+                                 category_position, kMaxSuggestionsPerCategory);
   } else {
-    UMA_HISTOGRAM_ENUMERATION(kHistogramDismissedUnvisited, global_position,
-                              kMaxSuggestionsTotal);
-    LogCategoryHistogramEnumeration(kHistogramDismissedUnvisited, category,
-                                    category_position,
-                                    kMaxSuggestionsPerCategory);
+    UMA_HISTOGRAM_EXACT_LINEAR(kHistogramDismissedUnvisited, global_position,
+                               kMaxSuggestionsTotal);
+    LogCategoryHistogramPosition(kHistogramDismissedUnvisited, category,
+                                 category_position, kMaxSuggestionsPerCategory);
   }
 }
 
 void OnSuggestionTargetVisited(Category category, base::TimeDelta visit_time) {
-  LogCategoryHistogramLongTimes(kHistogramVisitDuration, category, visit_time);
+  std::string name =
+      GetCategoryHistogramName(kHistogramVisitDuration, category);
+  base::UmaHistogramLongTimes(name, visit_time);
 }
 
 void OnMoreButtonShown(Category category, int position) {
   // The "more" card can appear in addition to the actual suggestions, so add
   // one extra bucket to this histogram.
-  LogCategoryHistogramEnumeration(kHistogramMoreButtonShown, category, position,
-                                  kMaxSuggestionsPerCategory + 1);
+  LogCategoryHistogramPosition(kHistogramMoreButtonShown, category, position,
+                               kMaxSuggestionsPerCategory + 1);
 }
 
 void OnMoreButtonClicked(Category category, int position) {
   // The "more" card can appear in addition to the actual suggestions, so add
   // one extra bucket to this histogram.
-  LogCategoryHistogramEnumeration(kHistogramMoreButtonClicked, category,
-                                  position, kMaxSuggestionsPerCategory + 1);
+  LogCategoryHistogramPosition(kHistogramMoreButtonClicked, category, position,
+                               kMaxSuggestionsPerCategory + 1);
 }
 
 void OnCategoryDismissed(Category category) {
   UMA_HISTOGRAM_ENUMERATION(kHistogramCategoryDismissed,
-                            static_cast<int>(GetHistogramCategory(category)),
-                            static_cast<int>(HistogramCategories::COUNT));
+                            GetHistogramCategory(category),
+                            HistogramCategories::COUNT);
 }
 
 }  // namespace metrics