[Remote suggestions] Report time distribution of triggers

components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc

Index: components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc b/components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
index 699c3b5a246cbceafee6b7c8c5cd0355eb9bd183..bfb0992154565535c03c46e2fea79f4cff321892 100644
--- a/components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
@@ -145,6 +145,66 @@ base::TimeDelta GetDesiredFetchingInterval(
   return base::TimeDelta::FromSecondsD(value_hours * 3600.0);
 }
 
+void ReportTimeUntilFirstTrigger(
+    UserClassifier::UserClass user_class,
+    const base::TimeDelta& time_until_first_failed_trigger) {

[Marc Treib, 2017/04/12 12:48:31]

s/time_until_first_failed_trigger/time_until_first_trigger/
(This records any, not just failed, right?)

And again, TimeDelta is fine to pass by value.

[jkrcal, 2017/04/12 13:26:44]

Done.

+  switch (user_class) {
+    case UserClassifier::UserClass::RARE_NTP_USER:
+      UMA_HISTOGRAM_CUSTOM_TIMES(
+          "NewTabPage.ContentSuggestions.TimeUntilFirstTrigger_RareNTPUser",
+          time_until_first_failed_trigger, base::TimeDelta::FromSeconds(1),
+          base::TimeDelta::FromDays(7),
+          /*bucket_count=*/100);
+      break;
+    case UserClassifier::UserClass::ACTIVE_NTP_USER:
+      UMA_HISTOGRAM_CUSTOM_TIMES(
+          "NewTabPage.ContentSuggestions.TimeUntilFirstTrigger_ActiveNTPUser",
+          time_until_first_failed_trigger, base::TimeDelta::FromSeconds(1),
+          base::TimeDelta::FromDays(7),
+          /*bucket_count=*/100);
+      break;
+    case UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER:
+      UMA_HISTOGRAM_CUSTOM_TIMES(
+          "NewTabPage.ContentSuggestions.TimeUntilFirstTrigger_"
+          "ActiveSuggestionsConsumer",
+          time_until_first_failed_trigger, base::TimeDelta::FromSeconds(1),
+          base::TimeDelta::FromDays(7),
+          /*bucket_count=*/100);
+      break;
+  }
+}
+
+void ReportTimeUntilFirstSuccessfulTrigger(
+    UserClassifier::UserClass user_class,
+    const base::TimeDelta& time_until_first_successful_trigger) {
+  switch (user_class) {
+    case UserClassifier::UserClass::RARE_NTP_USER:
+      UMA_HISTOGRAM_CUSTOM_TIMES(
+          "NewTabPage.ContentSuggestions.TimeUntilFirstSuccessfulTrigger_"
+          "RareNTPUser",
+          time_until_first_successful_trigger, base::TimeDelta::FromSeconds(1),
+          base::TimeDelta::FromDays(7),
+          /*bucket_count=*/100);
+      break;
+    case UserClassifier::UserClass::ACTIVE_NTP_USER:
+      UMA_HISTOGRAM_CUSTOM_TIMES(
+          "NewTabPage.ContentSuggestions.TimeUntilFirstSuccessfulTrigger_"
+          "ActiveNTPUser",
+          time_until_first_successful_trigger, base::TimeDelta::FromSeconds(1),
+          base::TimeDelta::FromDays(7),
+          /*bucket_count=*/100);
+      break;
+    case UserClassifier::UserClass::ACTIVE_SUGGESTIONS_CONSUMER:
+      UMA_HISTOGRAM_CUSTOM_TIMES(
+          "NewTabPage.ContentSuggestions.TimeUntilFirstSuccessfulTrigger_"
+          "ActiveSuggestionsConsumer",
+          time_until_first_successful_trigger, base::TimeDelta::FromSeconds(1),
+          base::TimeDelta::FromDays(7),
+          /*bucket_count=*/100);
+      break;
+  }
+}
+
 }  // namespace
 
 class EulaState : public web_resource::EulaAcceptedNotifier::Observer {
@@ -246,6 +306,7 @@ RemoteSuggestionsSchedulerImpl::RemoteSuggestionsSchedulerImpl(
           profile_prefs,
           RequestThrottler::RequestType::
               CONTENT_SUGGESTION_FETCHER_ACTIVE_SUGGESTIONS_CONSUMER),
+      time_until_first_trigger_reported_(false),
       eula_state_(base::MakeUnique<EulaState>(local_state_prefs, this)),
       profile_prefs_(profile_prefs),
       clock_(std::move(clock)),
@@ -438,8 +499,16 @@ void RemoteSuggestionsSchedulerImpl::RefetchInTheBackgroundIfAppropriate(
     return;
   }
 
+  const base::Time last_fetch_attempt_time = base::Time::FromInternalValue(

[Marc Treib, 2017/04/12 12:48:31]

What exactly does fetch *attempt* mean? (Does it matter for the metrics?)

[jkrcal, 2017/04/12 13:26:44]

It is the last time we sent a request to the server. We do not care whether the
request was successful or not so that we continue with the regular schedule and
do not burn the server by frequent retries whenever if it is overloaded.

This time is used as reference for ShouldRefetchInTheBackgroundNow() so this is
also the right reference time for the metrics.

+      profile_prefs_->GetInt64(prefs::kSnippetLastFetchAttempt));
+  if (!time_until_first_trigger_reported_) {

[Marc Treib, 2017/04/12 12:48:31]

Should we exclude PERSISTENT_SCHEDULER_WAKE_UP here?

[jkrcal, 2017/04/12 13:26:44]

Good point, done!

+    time_until_first_trigger_reported_ = true;
+    ReportTimeUntilFirstTrigger(user_classifier_->GetUserClass(),
+                                clock_->Now() - last_fetch_attempt_time);
+  }
+
   if (trigger != TriggerType::PERSISTENT_SCHEDULER_WAKE_UP &&
-      !ShouldRefetchInTheBackgroundNow()) {
+      !ShouldRefetchInTheBackgroundNow(last_fetch_attempt_time)) {
     return;
   }
 
@@ -447,6 +516,10 @@ void RemoteSuggestionsSchedulerImpl::RefetchInTheBackgroundIfAppropriate(
     return;
   }
 
+  ReportTimeUntilFirstSuccessfulTrigger(

[Marc Treib, 2017/04/12 12:48:31]

and here?

[jkrcal, 2017/04/12 13:26:44]

Good point! I've introduced another histogram for persistent fetches because we
never really understood how they get triggered out there.

+      user_classifier_->GetUserClass(),
+      clock_->Now() - last_fetch_attempt_time);
+
   UMA_HISTOGRAM_ENUMERATION(
       "NewTabPage.ContentSuggestions.BackgroundFetchTrigger",
       static_cast<int>(trigger), static_cast<int>(TriggerType::COUNT));
@@ -457,10 +530,8 @@ void RemoteSuggestionsSchedulerImpl::RefetchInTheBackgroundIfAppropriate(
       base::Unretained(this)));
 }
 
-bool RemoteSuggestionsSchedulerImpl::ShouldRefetchInTheBackgroundNow() {
-  const base::Time last_fetch_attempt_time = base::Time::FromInternalValue(
-      profile_prefs_->GetInt64(prefs::kSnippetLastFetchAttempt));
-
+bool RemoteSuggestionsSchedulerImpl::ShouldRefetchInTheBackgroundNow(
+    const base::Time& last_fetch_attempt_time) {
   // If we have no persistent scheduler to ask, err on the side of caution.
   bool wifi = false;
   if (persistent_scheduler_) {
@@ -521,6 +592,7 @@ void RemoteSuggestionsSchedulerImpl::RefetchInTheBackgroundFinished(
 void RemoteSuggestionsSchedulerImpl::OnFetchCompleted(Status fetch_status) {
   profile_prefs_->SetInt64(prefs::kSnippetLastFetchAttempt,
                            clock_->Now().ToInternalValue());
+  time_until_first_trigger_reported_ = false;
 
   // Reschedule after a fetch. The persistent schedule is applied only after a
   // successful fetch. After a failed fetch, we want to keep the previous