[Feed] Add histograms to scheduler and user classifier.

As part of adding histograms, it seems that the preferred way to emit
enum histograms is with a kMaxValue sentinel and let the enum macro
detect that. All of the scheduler and user classifier histograms were
converted from SHOUTY_CASE to kCamelCase to facilitate a kMaxValue.

Bug: 831648
Change-Id: Ib24791fdb7bcafcfeb6cf754736fe6ba7298c7d7
Reviewed-on: https://chromium-review.googlesource.com/1133687
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: Gang Wu <gangwu@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576240}

components/feed/core/feed_scheduler_host.h

@@ -28,17 +28,19 @@ namespace feed {
 
 // The enum values and names are kept in sync with SchedulerApi.RequestBehavior
 // through Java unit tests, new values however must be manually added. If any
-// new values are added, also update FeedSchedulerBridgeTest.java.
+// new values are added, also update FeedSchedulerBridgeTest.java as well as
+// the corresponding definition in enums.xml.
 // A Java counterpart will be generated for this enum.
 // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.components.feed
 enum NativeRequestBehavior {
-  UNKNOWN = 0,
-  REQUEST_WITH_WAIT,
-  REQUEST_WITH_CONTENT,
-  REQUEST_WITH_TIMEOUT,
-  NO_REQUEST_WITH_WAIT,
-  NO_REQUEST_WITH_CONTENT,
-  NO_REQUEST_WITH_TIMEOUT
+  kUnknown = 0,
+  kRequestWithWait = 1,
+  kRequestWithContent = 2,
+  kRequestWithTimeout = 3,
+  kNoRequestWithWait = 4,
+  kNoRequestWithContent = 5,
+  kNoRequestWithTimeout = 6,
+  kMaxValue = kNoRequestWithTimeout
 };
 
 // Implementation of the Feed Scheduler Host API. The scheduler host decides
@@ -47,8 +49,14 @@ enum NativeRequestBehavior {
 class FeedSchedulerHost : web_resource::EulaAcceptedNotifier::Observer {
  public:
   // The TriggerType enum specifies values for the events that can trigger
-  // refreshing articles.
-  enum class TriggerType { NTP_SHOWN, FOREGROUNDED, FIXED_TIMER, COUNT };
+  // refreshing articles. When adding values, be certain to also update the
+  // corresponding definition in enums.xml.
+  enum class TriggerType {
+    kNtpShown = 0,
+    kForegrounded = 1,
+    kFixedTimer = 2,
+    kMaxValue = kFixedTimer
+  };
 
   FeedSchedulerHost(PrefService* profile_prefs,
                     PrefService* local_state,
@@ -168,6 +176,11 @@ class FeedSchedulerHost : web_resource::EulaAcceptedNotifier::Observer {
   // notify if IsEulaAccepted() is called and it returns false.
   std::unique_ptr<web_resource::EulaAcceptedNotifier> eula_accepted_notifier_;
 
+  // Variables to allow metrics to be reported the first time a given trigger
+  // occurs after a refresh.
+  bool time_until_first_shown_trigger_reported_ = false;
+  bool time_until_first_foregrounded_trigger_reported_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(FeedSchedulerHost);
 };
 

components/feed/core/user_classifier.cc

@@ -49,9 +49,15 @@ const double kRareUserOpensNTPAtMostOncePerHours = 96;
 const char kRareUserOpensNTPAtMostOncePerHoursParam[] =
     "user_classifier_rare_user_opens_ntp_at_most_once_per_hours";
 
+// Histograms for logging the estimated average hours to next event.
+const char kHistogramAverageHoursToOpenNTP[] =
+    "NewTabPage.UserClassifier.AverageHoursToOpenNTP";
+const char kHistogramAverageHoursToUseSuggestions[] =
+    "NewTabPage.UserClassifier.AverageHoursToUseSuggestions";
+
 // List of all Events used for iteration.
 const UserClassifier::Event kEvents[] = {
-    UserClassifier::Event::NTP_OPENED, UserClassifier::Event::SUGGESTIONS_USED};
+    UserClassifier::Event::kNtpOpened, UserClassifier::Event::kSuggestionsUsed};
 
 // Arrays of pref names, indexed by Event's int value.
 const char* kRateKeys[] = {prefs::kUserClassifierAverageNTPOpenedPerHour,
@@ -65,16 +71,19 @@ const char* kInitialHoursBetweenEventsParams[] = {
     "user_classifier_default_interval_ntp_opened",
     "user_classifier_default_interval_suggestions_used"};
 
+// This verifies that each of the arrays has exactly the same number of values
+// as the number of enum values in UserClassifier::Event. These arrays are all
+// indexed by the integer value of UserClassifier::Event values.
 static_assert(base::size(kEvents) ==
-                      static_cast<int>(UserClassifier::Event::COUNT) &&
+                      static_cast<int>(UserClassifier::Event::kMaxValue) + 1 &&
                   base::size(kRateKeys) ==
-                      static_cast<int>(UserClassifier::Event::COUNT) &&
+                      static_cast<int>(UserClassifier::Event::kMaxValue) + 1 &&
                   base::size(kLastTimeKeys) ==
-                      static_cast<int>(UserClassifier::Event::COUNT) &&
+                      static_cast<int>(UserClassifier::Event::kMaxValue) + 1 &&
                   base::size(kInitialHoursBetweenEvents) ==
-                      static_cast<int>(UserClassifier::Event::COUNT) &&
+                      static_cast<int>(UserClassifier::Event::kMaxValue) + 1 &&
                   base::size(kInitialHoursBetweenEventsParams) ==
-                      static_cast<int>(UserClassifier::Event::COUNT),
+                      static_cast<int>(UserClassifier::Event::kMaxValue) + 1,
               "Fill in info for all event types.");
 
 // Computes the discount rate.
@@ -218,13 +227,24 @@ void UserClassifier::RegisterProfilePrefs(PrefRegistrySimple* registry) {
 }
 
 void UserClassifier::OnEvent(Event event) {
-  DCHECK_NE(event, Event::COUNT);
-  UpdateRateOnEvent(event);
-  // TODO(skym): Record average hour for metric in a histogram.
+  double metric_value = UpdateRateOnEvent(event);
+  double avg = GetEstimateHoursBetweenEvents(
+      metric_value, discount_rate_per_hour_, min_hours_, max_hours_);
+  // We use kMaxHours as the max value below as the maximum value for the
+  // histograms must be constant.
+  switch (event) {
+    case Event::kNtpOpened:
+      UMA_HISTOGRAM_CUSTOM_COUNTS(kHistogramAverageHoursToOpenNTP, avg, 1,
+                                  kMaxHours, 50);
+      break;
+    case Event::kSuggestionsUsed:
+      UMA_HISTOGRAM_CUSTOM_COUNTS(kHistogramAverageHoursToUseSuggestions, avg,
+                                  1, kMaxHours, 50);
+      break;
+  }
 }
 
 double UserClassifier::GetEstimatedAvgTime(Event event) const {
-  DCHECK_NE(event, Event::COUNT);
   double rate = GetUpToDateRate(event);
   return GetEstimateHoursBetweenEvents(rate, discount_rate_per_hour_,
                                        min_hours_, max_hours_);
@@ -233,29 +253,29 @@ double UserClassifier::GetEstimatedAvgTime(Event event) const {
 UserClassifier::UserClass UserClassifier::GetUserClass() const {
   // The pref_service_ can be null in tests.
   if (!pref_service_) {
-    return UserClass::ACTIVE_NTP_USER;
+    return UserClass::kActiveNtpUser;
   }
 
-  if (GetEstimatedAvgTime(Event::NTP_OPENED) >=
+  if (GetEstimatedAvgTime(Event::kNtpOpened) >=
       rare_user_opens_ntp_at_most_once_per_hours_) {
-    return UserClass::RARE_NTP_USER;
+    return UserClass::kRareNtpUser;
   }
 
-  if (GetEstimatedAvgTime(Event::SUGGESTIONS_USED) <=
+  if (GetEstimatedAvgTime(Event::kSuggestionsUsed) <=
       active_consumer_clicks_at_least_once_per_hours_) {
-    return UserClass::ACTIVE_SUGGESTIONS_CONSUMER;
+    return UserClass::kActiveSuggestionsConsumer;
   }
 
-  return UserClass::ACTIVE_NTP_USER;
+  return UserClass::kActiveNtpUser;
 }
 
 std::string UserClassifier::GetUserClassDescriptionForDebugging() const {
   switch (GetUserClass()) {
-    case UserClass::RARE_NTP_USER:
+    case UserClass::kRareNtpUser:
       return "Rare user of the NTP";
-    case UserClass::ACTIVE_NTP_USER:
+    case UserClass::kActiveNtpUser:
       return "Active user of the NTP";
-    case UserClass::ACTIVE_SUGGESTIONS_CONSUMER:
+    case UserClass::kActiveSuggestionsConsumer:
       return "Active consumer of NTP articles";
   }
   NOTREACHED();

components/feed/core/user_classifier.h

@@ -27,9 +27,9 @@ class UserClassifier {
   // Different groupings of usage. A user will belong to exactly one of these at
   // any given point in time. Can change at runtime.
   enum class UserClass {
-    RARE_NTP_USER,                // Almost never opens the NTP.
-    ACTIVE_NTP_USER,              // Uses NTP but not articles.
-    ACTIVE_SUGGESTIONS_CONSUMER,  // Frequently opens news articles.
+    kRareNtpUser,                // Almost never opens the NTP.
+    kActiveNtpUser,              // Uses NTP but not articles.
+    kActiveSuggestionsConsumer,  // Frequently opens news articles.
   };
 
   // For estimating the average length of the intervals between two successive
@@ -44,11 +44,11 @@ class UserClassifier {
   // NOTE: if you add any element, add it also in the static arrays in .cc and
   // create another histogram.
   enum class Event {
-    NTP_OPENED,  // When the user opens a new NTP - this indicates potential
-                 // use of content suggestions.
-    SUGGESTIONS_USED,  // When the user clicks on some suggestions or on some
-                       // "More" button.
-    COUNT              // Keep this as the last element.
+    kNtpOpened = 0,  // When the user opens a new NTP - this indicates potential
+                     // use of content suggestions.
+    kSuggestionsUsed = 1,  // When the user clicks on some suggestions or on
+                           // some "More" button.
+    kMaxValue = kSuggestionsUsed
   };
 
   // The provided |pref_service| may be nullptr in unit-tests.

components/feed/core/user_classifier_unittest.cc

@@ -59,7 +59,7 @@ class FeedUserClassifierTest : public testing::Test {
 TEST_F(FeedUserClassifierTest, ShouldBeActiveNtpUserInitially) {
   UserClassifier* user_classifier = CreateUserClassifier();
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+              Eq(UserClassifier::UserClass::kActiveNtpUser));
 }
 
 TEST_F(FeedUserClassifierTest,
@@ -67,17 +67,17 @@ TEST_F(FeedUserClassifierTest,
   UserClassifier* user_classifier = CreateUserClassifier();
 
   // After one click still only an active user.
-  user_classifier->OnEvent(UserClassifier::Event::SUGGESTIONS_USED);
+  user_classifier->OnEvent(UserClassifier::Event::kSuggestionsUsed);
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+              Eq(UserClassifier::UserClass::kActiveNtpUser));
 
   // After a few more clicks, become an active consumer.
   for (int i = 0; i < 5; i++) {
     test_clock()->Advance(base::TimeDelta::FromHours(1));
-    user_classifier->OnEvent(UserClassifier::Event::SUGGESTIONS_USED);
+    user_classifier->OnEvent(UserClassifier::Event::kSuggestionsUsed);
   }
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER));
+              Eq(UserClassifier::UserClass::kActiveSuggestionsConsumer));
 }
 
 TEST_F(FeedUserClassifierTest,
@@ -91,17 +91,17 @@ TEST_F(FeedUserClassifierTest,
   UserClassifier* user_classifier = CreateUserClassifier();
 
   // After two clicks still only an active user.
-  user_classifier->OnEvent(UserClassifier::Event::SUGGESTIONS_USED);
+  user_classifier->OnEvent(UserClassifier::Event::kSuggestionsUsed);
   test_clock()->Advance(base::TimeDelta::FromHours(1));
-  user_classifier->OnEvent(UserClassifier::Event::SUGGESTIONS_USED);
+  user_classifier->OnEvent(UserClassifier::Event::kSuggestionsUsed);
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+              Eq(UserClassifier::UserClass::kActiveNtpUser));
 
   // One more click to become an active consumer.
   test_clock()->Advance(base::TimeDelta::FromHours(1));
-  user_classifier->OnEvent(UserClassifier::Event::SUGGESTIONS_USED);
+  user_classifier->OnEvent(UserClassifier::Event::kSuggestionsUsed);
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER));
+              Eq(UserClassifier::UserClass::kActiveSuggestionsConsumer));
 }
 
 TEST_F(FeedUserClassifierTest, ShouldBecomeRareNtpUserByNoActivity) {
@@ -110,12 +110,12 @@ TEST_F(FeedUserClassifierTest, ShouldBecomeRareNtpUserByNoActivity) {
   // After two days of waiting still an active user.
   test_clock()->Advance(base::TimeDelta::FromDays(2));
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+              Eq(UserClassifier::UserClass::kActiveNtpUser));
 
   // Two more days to become a rare user.
   test_clock()->Advance(base::TimeDelta::FromDays(2));
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::RARE_NTP_USER));
+              Eq(UserClassifier::UserClass::kRareNtpUser));
 }
 
 TEST_F(FeedUserClassifierTest,
@@ -130,17 +130,18 @@ TEST_F(FeedUserClassifierTest,
   // After one days of waiting still an active user.
   test_clock()->Advance(base::TimeDelta::FromDays(1));
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::ACTIVE_NTP_USER));
+              Eq(UserClassifier::UserClass::kActiveNtpUser));
 
   // One more day to become a rare user.
   test_clock()->Advance(base::TimeDelta::FromDays(1));
   EXPECT_THAT(user_classifier->GetUserClass(),
-              Eq(UserClassifier::UserClass::RARE_NTP_USER));
+              Eq(UserClassifier::UserClass::kRareNtpUser));
 }
 
 class FeedUserClassifierEventTest
     : public FeedUserClassifierTest,
-      public ::testing::WithParamInterface<UserClassifier::Event> {
+      public ::testing::WithParamInterface<
+          std::pair<UserClassifier::Event, std::string>> {
  public:
   FeedUserClassifierEventTest() {}
 
@@ -149,7 +150,7 @@ class FeedUserClassifierEventTest
 };
 
 TEST_P(FeedUserClassifierEventTest, ShouldDecreaseEstimateAfterEvent) {
-  UserClassifier::Event event = GetParam();
+  UserClassifier::Event event = GetParam().first;
   UserClassifier* user_classifier = CreateUserClassifier();
 
   // The initial event. does not decrease the estimate.
@@ -163,8 +164,18 @@ TEST_P(FeedUserClassifierEventTest, ShouldDecreaseEstimateAfterEvent) {
   }
 }
 
+TEST_P(FeedUserClassifierEventTest, ShouldReportToUmaOnEvent) {
+  UserClassifier::Event event = GetParam().first;
+  const std::string& histogram_name = GetParam().second;
+  base::HistogramTester histogram_tester;
+  UserClassifier* user_classifier = CreateUserClassifier();
+
+  user_classifier->OnEvent(event);
+  EXPECT_THAT(histogram_tester.GetAllSamples(histogram_name), SizeIs(1));
+}
+
 TEST_P(FeedUserClassifierEventTest, ShouldConvergeTowardsPattern) {
-  UserClassifier::Event event = GetParam();
+  UserClassifier::Event event = GetParam().first;
   UserClassifier* user_classifier = CreateUserClassifier();
 
   // Have the pattern of an event every five hours and start changing it towards
@@ -189,7 +200,7 @@ TEST_P(FeedUserClassifierEventTest, ShouldConvergeTowardsPattern) {
 }
 
 TEST_P(FeedUserClassifierEventTest, ShouldIgnoreSubsequentEventsForHalfAnHour) {
-  UserClassifier::Event event = GetParam();
+  UserClassifier::Event event = GetParam().first;
   UserClassifier* user_classifier = CreateUserClassifier();
 
   // The initial event.
@@ -210,7 +221,7 @@ TEST_P(FeedUserClassifierEventTest, ShouldIgnoreSubsequentEventsForHalfAnHour) {
 
 TEST_P(FeedUserClassifierEventTest,
        ShouldIgnoreSubsequentEventsWithIncreasedLimit) {
-  UserClassifier::Event event = GetParam();
+  UserClassifier::Event event = GetParam().first;
   // Increase the min_hours to 1.0, i.e. 60 minutes.
   variations::testing::VariationParamsManager variation_params(
       kInterestFeedContentSuggestions.name,
@@ -235,7 +246,7 @@ TEST_P(FeedUserClassifierEventTest,
 }
 
 TEST_P(FeedUserClassifierEventTest, ShouldCapDelayBetweenEvents) {
-  UserClassifier::Event event = GetParam();
+  UserClassifier::Event event = GetParam().first;
   UserClassifier* user_classifier = CreateUserClassifier();
 
   // The initial event.
@@ -258,7 +269,7 @@ TEST_P(FeedUserClassifierEventTest, ShouldCapDelayBetweenEvents) {
 
 TEST_P(FeedUserClassifierEventTest,
        ShouldCapDelayBetweenEventsWithDecreasedLimit) {
-  UserClassifier::Event event = GetParam();
+  UserClassifier::Event event = GetParam().first;
   // Decrease the max_hours to 72, i.e. 3 days.
   variations::testing::VariationParamsManager variation_params(
       kInterestFeedContentSuggestions.name,
@@ -288,9 +299,12 @@ INSTANTIATE_TEST_CASE_P(
     ,  // An empty prefix for the parametrized tests names (no need to
        // distinguish the only instance we make here).
     FeedUserClassifierEventTest,
-    testing::Values(UserClassifier::Event::NTP_OPENED,
-                    UserClassifier::Event::SUGGESTIONS_USED));
-
+    testing::Values(
+        std::make_pair(UserClassifier::Event::kNtpOpened,
+                       "NewTabPage.UserClassifier.AverageHoursToOpenNTP"),
+        std::make_pair(
+            UserClassifier::Event::kSuggestionsUsed,
+            "NewTabPage.UserClassifier.AverageHoursToUseSuggestions")));
 }  // namespace
 
 }  // namespace feed

tools/metrics/histograms/enums.xml

@@ -40332,6 +40332,12 @@ Called by update_net_trust_anchors.py.-->
   <int value="1" label="Increment"/>
 </enum>
 
+<enum name="RefreshTrigger">
+  <int value="0" label="NTP shown"/>
+  <int value="1" label="Foregrounded"/>
+  <int value="2" label="Fixed timer"/>
+</enum>
+
 <enum name="RelaunchNotificationShowResult">
   <int value="0" label="Shown"/>
   <int value="1" label="Not shown for unknown reason"/>
@@ -40854,6 +40860,16 @@ Called by update_net_trust_anchors.py.-->
   <int value="4" label="SET_AUTH_CREDENTIALS"/>
 </enum>
 
+<enum name="RequestBehavior">
+  <int value="0" label="Unknown"/>
+  <int value="1" label="Request with wait"/>
+  <int value="2" label="Request with content"/>
+  <int value="3" label="Request with timeout"/>
+  <int value="4" label="No request with wait"/>
+  <int value="5" label="No request with content"/>
+  <int value="6" label="No request with timeout"/>
+</enum>
+
 <enum name="RequestedImageMimeType">
   <int value="0" label="Png"/>
   <int value="1" label="Jpeg"/>

tools/metrics/histograms/histograms.xml

@@ -30331,6 +30331,26 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="Feed.Scheduler.RefreshTrigger" enum="RefreshTrigger"
+    expires_after="2019-07-11">
+  <owner>skym@chromium.org</owner>
+  <summary>
+    The scheduler watches for various triggers, which cause it to decide if a
+    refresh is currently warranted or not. This histogram is emitted when a
+    trigger causes a refresh.
+  </summary>
+</histogram>
+
+<histogram name="Feed.Scheduler.RequestBehavior" enum="RequestBehavior"
+    expires_after="2019-07-11">
+  <owner>skym@chromium.org</owner>
+  <summary>
+    When NTP is opened, the scheduler host tells the Feed library how to act, if
+    the existing content should be shown, immediately or with timeout, and if a
+    refresh request should be started.
+  </summary>
+</histogram>
+
 <histogram name="Feedback.RequestSource" enum="FeedbackSource">
   <owner>afakhry@chromium.org</owner>
   <summary>Records the source that requested showing the feedback app.</summary>