[Doodle] Record UMA for DoodleConfig download outcome and time

components/doodle/doodle_service.cc

Index: components/doodle/doodle_service.cc
diff --git a/components/doodle/doodle_service.cc b/components/doodle/doodle_service.cc
index 12a56200bb613a7a9dae0f583c444d2082489220..2e1edb3562e26940ee2f56084a29179c5a96a70b 100644
--- a/components/doodle/doodle_service.cc
+++ b/components/doodle/doodle_service.cc
@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/doodle/pref_names.h"
@@ -28,21 +29,26 @@ void DoodleService::RegisterProfilePrefs(PrefRegistrySimple* pref_registry) {
 DoodleService::DoodleService(PrefService* pref_service,
                              std::unique_ptr<DoodleFetcher> fetcher,
                              std::unique_ptr<base::OneShotTimer> expiry_timer,
-                             std::unique_ptr<base::Clock> clock)
+                             std::unique_ptr<base::Clock> clock,
+                             std::unique_ptr<base::TickClock> tick_clock)
     : pref_service_(pref_service),
       fetcher_(std::move(fetcher)),
       expiry_timer_(std::move(expiry_timer)),
-      clock_(std::move(clock)) {
+      clock_(std::move(clock)),
+      tick_clock_(std::move(tick_clock)) {
   DCHECK(pref_service_);
   DCHECK(fetcher_);
   DCHECK(expiry_timer_);
   DCHECK(clock_);
+  DCHECK(tick_clock_);
 
   base::Time expiry_date = base::Time::FromInternalValue(
       pref_service_->GetInt64(prefs::kCachedConfigExpiry));
   base::Optional<DoodleConfig> config = DoodleConfig::FromDictionary(
       *pref_service_->GetDictionary(prefs::kCachedConfig), base::nullopt);
-  UpdateCachedConfig(expiry_date - clock_->Now(), config);
+  DoodleState state =
+      config.has_value() ? DoodleState::AVAILABLE : DoodleState::NO_DOODLE;
+  HandleNewConfig(state, expiry_date - clock_->Now(), config);
 }
 
 DoodleService::~DoodleService() = default;
@@ -58,18 +64,93 @@ void DoodleService::RemoveObserver(Observer* observer) {
 }
 
 void DoodleService::Refresh() {
-  fetcher_->FetchDoodle(
-      base::BindOnce(&DoodleService::DoodleFetched, base::Unretained(this)));
+  fetcher_->FetchDoodle(base::BindOnce(&DoodleService::DoodleFetched,
+                                       base::Unretained(this),
+                                       tick_clock_->NowTicks()));
+}
+
+// static
+bool DoodleService::DownloadOutcomeIsSuccess(DownloadOutcome outcome) {
+  switch (outcome) {
+    case OUTCOME_NEW_DOODLE:
+    case OUTCOME_REVALIDATED_DOODLE:
+    case OUTCOME_CHANGED_DOODLE:
+    case OUTCOME_NO_DOODLE:
+      return true;
+    case OUTCOME_EXPIRED:
+    case OUTCOME_DOWNLOAD_ERROR:
+    case OUTCOME_PARSING_ERROR:
+      return false;
+    case OUTCOME_COUNT:
+      NOTREACHED();
+  }
+  return false;
+}
+
+// static
+void DoodleService::RecordDownloadMetrics(DownloadOutcome outcome,
+                                          base::TimeDelta download_time) {
+  UMA_HISTOGRAM_ENUMERATION("Doodle.ConfigDownloadOutcome", outcome,
+                            OUTCOME_COUNT);
+
+  if (DownloadOutcomeIsSuccess(outcome)) {
+    UMA_HISTOGRAM_MEDIUM_TIMES("Doodle.ConfigDownloadTime", download_time);
+  }
+}
+
+// static
+DoodleService::DownloadOutcome DoodleService::DetermineDownloadOutcome(
+    const base::Optional<DoodleConfig>& old_config,
+    const base::Optional<DoodleConfig>& new_config,
+    DoodleState state,
+    bool expired) {
+  // First, handle error cases: *_ERROR or EXPIRED override other outcomes.
+  switch (state) {
+    case DoodleState::AVAILABLE:
+      if (expired) {
+        return OUTCOME_EXPIRED;
+      }
+      break;
+
+    case DoodleState::NO_DOODLE:
+      break;
+
+    case DoodleState::DOWNLOAD_ERROR:
+      return OUTCOME_DOWNLOAD_ERROR;
+
+    case DoodleState::PARSING_ERROR:
+      return OUTCOME_PARSING_ERROR;
+  }
+
+  DownloadOutcome outcome = OUTCOME_COUNT;
+  if (!new_config.has_value()) {
+    outcome = OUTCOME_NO_DOODLE;
+  } else if (!old_config.has_value()) {
+    outcome = OUTCOME_NEW_DOODLE;
+  } else {
+    if (old_config.value() == new_config.value()) {

[fhorschig, 2017/03/21 17:09:00]

Wait, why is that if in an else and not another else if?
Am I missing something or is that just to show a separation
between checking optionals and checking their values?

[Marc Treib, 2017/03/21 17:16:05]

No particular reason; I just found it slightly easier to read like this. Anyway,
gone now.

+      outcome = OUTCOME_REVALIDATED_DOODLE;
+    } else {
+      outcome = OUTCOME_CHANGED_DOODLE;
+    }
+  }
+  DCHECK_NE(outcome, OUTCOME_COUNT);

[fhorschig, 2017/03/21 17:09:00]

Nit:
why not return every outcome= and just put a NOTREACHED here?
Or check it somewhere else?

[Marc Treib, 2017/03/21 17:16:05]

Good point - this was just a leftover from moving code around. Done.
Nice side effect: This gets rid of all the "else if"s :)

+
+  return outcome;
 }
 
 void DoodleService::DoodleFetched(
+    base::TimeTicks start_time,
     DoodleState state,
     base::TimeDelta time_to_live,
     const base::Optional<DoodleConfig>& doodle_config) {
-  UpdateCachedConfig(time_to_live, doodle_config);
+  base::TimeDelta download_time = tick_clock_->NowTicks() - start_time;
+  DownloadOutcome outcome = HandleNewConfig(state, time_to_live, doodle_config);
+  RecordDownloadMetrics(outcome, download_time);
 }
 
-void DoodleService::UpdateCachedConfig(
+DoodleService::DownloadOutcome DoodleService::HandleNewConfig(
+    DoodleState state,
     base::TimeDelta time_to_live,
     const base::Optional<DoodleConfig>& doodle_config) {
   // Handle the case where the new config is already expired.
@@ -77,21 +158,15 @@ void DoodleService::UpdateCachedConfig(
   const base::Optional<DoodleConfig>& new_config =
       expired ? base::nullopt : doodle_config;
 
+  // Determine the download outcome *before* updating the cached config.
+  DownloadOutcome outcome =
+      DetermineDownloadOutcome(cached_config_, new_config, state, expired);
+
   // If the config changed, update our cache and notify observers.
   // Note that this checks both for existence changes as well as changes of the
   // configs themselves.
   if (cached_config_ != new_config) {
-    cached_config_ = new_config;
-
-    if (cached_config_.has_value()) {
-      pref_service_->Set(prefs::kCachedConfig, *cached_config_->ToDictionary());
-      base::Time expiry_date = clock_->Now() + time_to_live;
-      pref_service_->SetInt64(prefs::kCachedConfigExpiry,
-                              expiry_date.ToInternalValue());
-    } else {
-      pref_service_->ClearPref(prefs::kCachedConfig);
-      pref_service_->ClearPref(prefs::kCachedConfigExpiry);
-    }
+    UpdateCachedConfig(time_to_live, new_config);
 
     for (auto& observer : observers_) {
       observer.OnDoodleConfigUpdated(cached_config_);
@@ -107,11 +182,31 @@ void DoodleService::UpdateCachedConfig(
   } else {
     expiry_timer_->Stop();
   }
+
+  return outcome;
+}
+
+void DoodleService::UpdateCachedConfig(
+    base::TimeDelta time_to_live,
+    const base::Optional<DoodleConfig>& new_config) {
+  DCHECK(cached_config_ != new_config);
+
+  cached_config_ = new_config;
+
+  if (cached_config_.has_value()) {
+    pref_service_->Set(prefs::kCachedConfig, *cached_config_->ToDictionary());
+    base::Time expiry_date = clock_->Now() + time_to_live;
+    pref_service_->SetInt64(prefs::kCachedConfigExpiry,
+                            expiry_date.ToInternalValue());
+  } else {
+    pref_service_->ClearPref(prefs::kCachedConfig);
+    pref_service_->ClearPref(prefs::kCachedConfigExpiry);
+  }
 }
 
 void DoodleService::DoodleExpired() {
   DCHECK(cached_config_.has_value());
-  UpdateCachedConfig(base::TimeDelta(), base::nullopt);
+  HandleNewConfig(DoodleState::NO_DOODLE, base::TimeDelta(), base::nullopt);
 }
 
 }  // namespace doodle