Log suggestion scores in 10 discrete buckets.

components/ntp_snippets/content_suggestions_metrics.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "base/template_util.h"
 
 namespace ntp_snippets {
 namespace metrics {
 
 namespace {
 
 const int kMaxSuggestionsPerCategory = 10;
 const int kMaxSuggestionsTotal = 50;
 
 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[] =
     "NewTabPage.ContentSuggestions.DismissedVisited";
 const char kHistogramArticlesUsageTimeLocal[] =
     "NewTabPage.ContentSuggestions.UsageTimeLocal";
 const char kHistogramVisitDuration[] =
     "NewTabPage.ContentSuggestions.VisitDuration";
 const char kHistogramMoreButtonShown[] =
     "NewTabPage.ContentSuggestions.MoreButtonShown";
 const char kHistogramMoreButtonClicked[] =
     "NewTabPage.ContentSuggestions.MoreButtonClicked";
 const char kHistogramCategoryDismissed[] =
     "NewTabPage.ContentSuggestions.CategoryDismissed";
 const char kHistogramContentSuggestionsTimeSinceLastBackgroundFetch[] =
     "NewTabPage.ContentSuggestions.TimeSinceLastBackgroundFetch";
 
 const char kPerCategoryHistogramFormat[] = "%s.%s";
 
 // This mostly corresponds to the KnownCategories enum, but it is contiguous
 // 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,
   BOOKMARKS,
   PHYSICAL_WEB_PAGES,
   FOREIGN_TABS,
   ARTICLES,
   // Insert new values here!
   COUNT
 };
@@ skipping 54 matching lines @@
       break;
   }
   return std::string();
 }
 
 std::string GetCategoryHistogramName(const char* base_name, Category category) {
   return base::StringPrintf(kPerCategoryHistogramFormat, base_name,
                             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) {
   base::Histogram::FactoryTimeGet(
       name, base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(7), 100,
       base::HistogramBase::kUmaTargetedHistogramFlag)
       ->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) {
+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.
-  UmaHistogramEnumeration(name, value, boundary_value);
-}
-
-void LogCategoryHistogramLongTimes(const char* base_name,
-                                   Category category,
-                                   const base::TimeDelta& value) {
-  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,
                              Category category,
                              const base::TimeDelta& value) {
   std::string name = GetCategoryHistogramName(base_name, category);
   // Since the histogram name is dynamic, we can't use the regular macro.
   UmaHistogramAge(name, value);
 }
 
 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
 // buckets. Depending on the current local time the count of the corresponding
 // bucket is increased.
 void RecordContentSuggestionsUsage() {
   const int kBucketSizeMins = 20;
   const int kNumBuckets = 24 * 60 / kBucketSizeMins;
 
   base::Time::Exploded now_exploded;
   base::Time::Now().LocalExplode(&now_exploded);
   int bucket = (now_exploded.hour * 60 + now_exploded.minute) / kBucketSizeMins;
 
   const char* kWeekdayNames[] = {"Sunday",   "Monday", "Tuesday", "Wednesday",
                                  "Thursday", "Friday", "Saturday"};
   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"));
 }
 
 }  // namespace
 
 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,
                        Category category,
                        int category_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);
 
   LogCategoryHistogramScore(kHistogramShownScore, category, score);
 
   // TODO(markusheintz): Discuss whether the code below should be move into a
   // separate method called OnSuggestionsListShown.
   // When the first of the articles suggestions is shown, then we count this as
   // a single usage of content suggestions.
@@ skipping 10 matching lines @@
         /*bucket_count=*/100);
   }
 }
 
 void OnSuggestionOpened(int global_position,
                         Category category,
                         int category_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)) {
     RecordContentSuggestionsUsage();
   }
 }
 
 void OnSuggestionMenuOpened(int global_position,
                             Category category,
                             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);
 
   LogCategoryHistogramScore(kHistogramMenuOpenedScore, category, score);
 }
 
 void OnSuggestionDismissed(int global_position,
                            Category category,
                            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
 }  // namespace ntp_snippets