[Offline Pages] Adds UMA for result of attempts to immediately start background loading.

components/offline_pages/background/request_coordinator.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/offline_pages/background/request_coordinator.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
 #include "base/sys_info.h"
 #include "base/time/time.h"
 #include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
 #include "components/offline_pages/background/request_picker.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/offline_page_model.h"
 
 namespace offline_pages {
 
 namespace {
 const bool kUserRequest = true;
 
+enum class OfflinerImmediateStartStatus {
+  BUSY,

[fgorski, 2016/10/05 21:46:13]

Can you reorder these two?

Almost feels like it should be:
0 -  I Succeeded
1-5.. Here is why I didn't

WDYT?

[dougarnett, 2016/10/05 22:28:14]

Done.

+  STARTED,
+  NOT_ACCEPTED,
+  NO_CONNECTION,
+  NO_EFFECTIVE_CONNECTION,
+  NOT_STARTED_ON_SVELTE,
+  // NOTE: insert new values above this line and update histogram enum too.
+  STATUS_COUNT,
+};
+
+void RecordImmediateStart(OfflinerImmediateStartStatus immediate_start_status) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "OfflinePages.Background.ImmediateStartStatus",
+      static_cast<int>(immediate_start_status),

[fgorski, 2016/10/05 21:46:13]

holte@: should be using int vs. int32_t here? I have another patch where I have
the same dilemma. 

[dougarnett, 2016/10/05 22:28:15]

Pete just had CL that used int so followed that example.

+      static_cast<int>(OfflinerImmediateStartStatus::STATUS_COUNT));
+}
+
 // Records the final request status UMA for an offlining request. This should
 // only be called once per Offliner::LoadAndSave request.
 void RecordOfflinerResultUMA(const ClientId& client_id,
                              Offliner::RequestStatus request_status) {
   // TODO(dougarnett): Consider exposing AddHistogramSuffix from
   // offline_page_model_impl.cc as visible utility method.
   std::string histogram_name("OfflinePages.Background.OfflinerRequestStatus");
   if (!client_id.name_space.empty()) {
     histogram_name += "." + client_id.name_space;
   }
@@ skipping 325 matching lines @@
   operation_start_time_ = base::Time::Now();
 
   is_stopped_ = false;
   scheduler_callback_ = callback;
 
   TryNextRequest();
 
   return true;
 }
 
 void RequestCoordinator::StartProcessingIfConnected() {

[Pete Williamson, 2016/10/05 21:51:53]

Would it be better to have StartProcessingIfConnected just return the immediate
start status in a return value, and do the UMA from there?  If we could, it
would make the code easier to read.

It might also make sense to do the same for StartProcessing.

[dougarnett, 2016/10/05 22:28:14]

It is called in two places - so capture UMA in both or just for Adds not
Resumes? Or create a wrapping method? Improved later to get this in 55?

[Pete Williamson, 2016/10/05 22:45:03]

I'm happy if you want to improve this later in M55 instead of now, it's fine by
me to create a TODO.  I'm also open to any arguments if you think the code is
better as is.  I'd capture UMA for both, or creating a wrapping method is also
viable.

[dougarnett, 2016/10/05 23:20:37]

I tried wrapper - see if you like it better.

   // Make sure not already busy processing.
-  if (is_busy_) return;
+  if (is_busy_) {
+    RecordImmediateStart(OfflinerImmediateStartStatus::BUSY);
+    return;
+  }
 
   // Make sure we are not on svelte device to start immediately.
-  if (base::SysInfo::IsLowEndDevice()) return;
+  if (base::SysInfo::IsLowEndDevice()) {
+    RecordImmediateStart(OfflinerImmediateStartStatus::NOT_STARTED_ON_SVELTE);

[fgorski, 2016/10/05 21:46:13]

Just a thought: If you make this check above, you are effectively counting
svelte devices...

[dougarnett, 2016/10/05 22:28:14]

Happens anyway as far as unique devices goes on first Add (won't be busy).

+    return;
+  }
 
   // Make sure we have reasonable network quality (or at least a connection).
   if (network_quality_estimator_) {
     // TODO(dougarnett): Add UMA for quality type experienced.
     net::EffectiveConnectionType quality =
         network_quality_estimator_->GetEffectiveConnectionType();
     if (quality < net::EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_2G) {
+      RecordImmediateStart(
+          OfflinerImmediateStartStatus::NO_EFFECTIVE_CONNECTION);
       return;
     }
   } else if (GetConnectionType() ==
              net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE) {
+    RecordImmediateStart(OfflinerImmediateStartStatus::NO_CONNECTION);
     return;
   }
 
   // Start processing with manufactured conservative battery conditions
   // (i.e., assume no battery).
   // TODO(dougarnett): Obtain actual battery conditions (from Android/Java).
   DeviceConditions device_conditions(false, 0, GetConnectionType());
-  StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback));
+  if (StartProcessing(device_conditions, base::Bind(&EmptySchedulerCallback))) {

[fgorski, 2016/10/05 21:46:13]

nit: braces not needed

+    RecordImmediateStart(OfflinerImmediateStartStatus::STARTED);
+  } else {
+    RecordImmediateStart(OfflinerImmediateStartStatus::NOT_ACCEPTED);
+  }
 }
 
 void RequestCoordinator::TryNextRequest() {
   // If there is no time left in the budget, return to the scheduler.
   // We do not remove the pending task that was set up earlier in case
   // we run out of time, so the background scheduler will return to us
   // at the next opportunity to run background tasks.
   if (base::Time::Now() - operation_start_time_ >
       base::TimeDelta::FromSeconds(
           policy_->GetBackgroundProcessingTimeBudgetSeconds())) {
@@ skipping 189 matching lines @@
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages