[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..4fe7deaa11d9960aa2f95b8ace94b9de6e98e6d4 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"
@@ -16,6 +17,41 @@
 
 namespace doodle {
 
+namespace {
+
+// Note: Keep in sync with the corresponding enum in histograms.xml. Never
+// remove values, and only insert new values at the end.
+enum DoodleConfigDownloadOutcome {
+  OUTCOME_NEW_DOODLE,
+  OUTCOME_REVALIDATED_DOODLE,
+  OUTCOME_CHANGED_DOODLE,
+  OUTCOME_NO_DOODLE,
+  OUTCOME_EXPIRED,
+  OUTCOME_DOWNLOAD_ERROR,
+  OUTCOME_PARSING_ERROR,
+  // Insert new values here!
+  OUTCOME_COUNT
+};
+
+bool DownloadOutcomeIsSuccess(DoodleConfigDownloadOutcome 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;
+}
+
+}  // namespace
+
 // static
 void DoodleService::RegisterProfilePrefs(PrefRegistrySimple* pref_registry) {
   pref_registry->RegisterDictionaryPref(prefs::kCachedConfig,
@@ -28,21 +64,27 @@ 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;
+  UpdateCachedConfig(/*download_time=*/base::nullopt, state,
+                     expiry_date - clock_->Now(), config);
 }
 
 DoodleService::~DoodleService() = default;
@@ -58,18 +100,23 @@ 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()));
 }
 
 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;
+  UpdateCachedConfig(download_time, state, time_to_live, doodle_config);
 }
 
 void DoodleService::UpdateCachedConfig(
+    base::Optional<base::TimeDelta> download_time,
+    DoodleState state,
     base::TimeDelta time_to_live,
     const base::Optional<DoodleConfig>& doodle_config) {

[fhorschig, 2017/03/21 15:26:27]

80 lines and nested ifs make this really hard to read. More below.

   // Handle the case where the new config is already expired.
@@ -77,18 +124,26 @@ void DoodleService::UpdateCachedConfig(
   const base::Optional<DoodleConfig>& new_config =
       expired ? base::nullopt : doodle_config;
 
+  DoodleConfigDownloadOutcome outcome = OUTCOME_COUNT;
+
   // 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) {

[fhorschig, 2017/03/21 15:26:27]

Can we consider putting this branch into a new function and replace the
nested ifs with guard clauses?
Outcome DoodleService::UpdateCache(new_config, TTL)

[Marc Treib, 2017/03/21 16:10:45]

Hm. Maaaybe there's a sensible way to split this up. I'll take a stab in the
next patch set.

+    bool had_config_before = cached_config_.has_value();
+
     cached_config_ = new_config;
 
     if (cached_config_.has_value()) {
+      outcome = had_config_before ? OUTCOME_CHANGED_DOODLE : OUTCOME_NEW_DOODLE;
+
       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 {
+      outcome = OUTCOME_NO_DOODLE;
+
       pref_service_->ClearPref(prefs::kCachedConfig);
       pref_service_->ClearPref(prefs::kCachedConfigExpiry);
     }
@@ -96,6 +151,39 @@ void DoodleService::UpdateCachedConfig(
     for (auto& observer : observers_) {
       observer.OnDoodleConfigUpdated(cached_config_);
     }
+  } else {
+    outcome = cached_config_.has_value() ? OUTCOME_REVALIDATED_DOODLE
+                                         : OUTCOME_NO_DOODLE;
+  }
+
+  // If the update came from the network, record metrics.
+  DCHECK(outcome != OUTCOME_COUNT);

[fhorschig, 2017/03/21 15:26:27]

Is there any chance this couldn't have happened?

[Marc Treib, 2017/03/21 16:10:46]

Nope, hence the DCHECK.

+  if (download_time.has_value()) {
+    // Determine the final outcome. ERROR or EXPIRED override NO_DOODLE.

[fhorschig, 2017/03/21 15:26:27]

So this complete branch is basically dead for one of two possible calls.
Can we make it an own function?
--> void ::RecordMetrics(outcome, state, download_time)

Then you could return |outcome| in UpdateCachedConfig.

[Marc Treib, 2017/03/21 16:10:45]

Good point.
Done (mostly). I left the "switch(state)" here, because I don't want to split up
the determination of outcome into two places. It's hard enough to follow as it
is :)
Done.

[fhorschig, 2017/03/21 16:41:34]

Not dead but very unused for one call. 
that's only a little bit better.

[Marc Treib, 2017/03/21 16:48:59]

Pulled it all out into a separate DetermineDownloadOutcome method.
It's still unused in some cases, but IMO that's fine.

+    switch (state) {
+      case DoodleState::AVAILABLE:
+      case DoodleState::NO_DOODLE:
+        if (expired) {
+          outcome = OUTCOME_EXPIRED;
+        }
+        break;
+
+      case DoodleState::DOWNLOAD_ERROR:
+        outcome = OUTCOME_DOWNLOAD_ERROR;
+        break;
+
+      case DoodleState::PARSING_ERROR:
+        outcome = OUTCOME_PARSING_ERROR;
+        break;
+    }
+
+    UMA_HISTOGRAM_ENUMERATION("Doodle.ConfigDownloadOutcome", outcome,
+                              OUTCOME_COUNT);
+
+    if (DownloadOutcomeIsSuccess(outcome)) {
+      UMA_HISTOGRAM_MEDIUM_TIMES("Doodle.ConfigDownloadTime",
+                                 download_time.value());
+    }
   }
 
   // Even if the configs are identical, the time-to-live might have changed.
@@ -111,7 +199,8 @@ void DoodleService::UpdateCachedConfig(
 
 void DoodleService::DoodleExpired() {
   DCHECK(cached_config_.has_value());
-  UpdateCachedConfig(base::TimeDelta(), base::nullopt);
+  UpdateCachedConfig(/*download_time=*/base::nullopt, DoodleState::NO_DOODLE,
+                     base::TimeDelta(), base::nullopt);
 }
 
 }  // namespace doodle