[Offline Pages] Adds UMA for number of started attempts and for network connection when Unsupported…

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"
@@ skipping 27 matching lines @@
   // macro adapted to allow for a dynamically suffixed histogram name.
   // Note: The factory creates and owns the histogram.
   base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
       histogram_name, 1,
       static_cast<int>(Offliner::RequestStatus::STATUS_COUNT),
       static_cast<int>(Offliner::RequestStatus::STATUS_COUNT) + 1,
       base::HistogramBase::kUmaTargetedHistogramFlag);
   histogram->Add(static_cast<int>(request_status));
 }
 
+// Records the number of started attempts for completed requests (whether
+// successful or not).
+void RecordAttemptCount(const SavePageRequest& request,

[Pete Williamson, 2016/09/01 00:27:20]

Maybe this should be RecordStartedAttemptCount(), and we should have a
corresponding RecordCompletedAttemptCount().

[dougarnett, 2016/09/01 19:27:37]

Actually, my thought was to just add the completed attempt UMA in this same
method. Adding TODO to capture that intent.

+                        RequestNotifier::SavePageStatus status) {
+  if (status == RequestNotifier::SavePageStatus::SUCCESS) {
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        "OfflinePages.Background.RequestSuccess.StartedAttemptCount",
+        request.started_attempt_count(), 1, 10, 10);
+  } else {
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        "OfflinePages.Background.RequestFailure.StartedAttemptCount",
+        request.started_attempt_count(), 1, 10, 10);
+  }
+}
+
 // This should use the same algorithm as we use for OfflinePageItem, so the IDs
 // are similar.
 int64_t GenerateOfflineId() {
   return base::RandGenerator(std::numeric_limits<int64_t>::max()) + 1;
 }
 
 // In case we start processing from SavePageLater, we need a callback, but there
 // is nothing for it to do.
 void EmptySchedulerCallback(bool started) {}
 
@@ skipping 54 matching lines @@
 
 void RequestCoordinator::GetQueuedRequestsCallback(
     const GetRequestsCallback& callback,
     RequestQueue::GetRequestsResult result,
     const std::vector<SavePageRequest>& requests) {
   callback.Run(requests);
 }
 
 void RequestCoordinator::StopPrerendering() {
   if (offliner_ && is_busy_) {
+    DCHECK(active_request_.get());
     offliner_->Cancel();
-    // Find current request and mark attempt aborted.
-    active_request_->MarkAttemptAborted();
-    queue_->UpdateRequest(*(active_request_.get()),
-                          base::Bind(&RequestCoordinator::UpdateRequestCallback,
-                                     weak_ptr_factory_.GetWeakPtr(),
-                                     active_request_->client_id()));
+    AbortRequestAttempt(active_request_.get());
   }
 
   // Stopping offliner means it will not call callback.
   last_offlining_status_ =
       Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED;
 
   if (active_request_) {
     RecordOfflinerResultUMA(active_request_->client_id(),
                             last_offlining_status_);
     is_busy_ = false;
@@ skipping 11 matching lines @@
     if (request_ids.end() != std::find(request_ids.begin(), request_ids.end(),
                                        active_request_->request_id())) {
       StopPrerendering();
       return true;
     }
   }
 
   return false;
 }
 
+void RequestCoordinator::AbortRequestAttempt(SavePageRequest* request) {

[Pete Williamson, 2016/09/01 00:27:20]

This splits the responsibility for garbage collecting obsolete things from the
queue between RC and RequestPicker classes.  I wonder if it would be better to
do it all in the RequestPicker.  What do you think?

[dougarnett, 2016/09/01 19:27:37]

We are deleting the DoneCallback requests in other cases here so there is some
argument that it should be consistently done here as we know about it. Also, we
don't call the Picker for the abort path now, so they would linger in the queue
until next schedule window (and could cause confusion wrt queue size or showing
up in internals page).

I think the Picker garbage collection is a secondary yet important safeguard
against crash loops (as well as needed for expired requests). We might want
different UMA (and possibly different notification status) for the garbage
collected cases on starts to yield insights on potential crashes or being killed
by Android.

+  request->MarkAttemptAborted();
+  if (request->started_attempt_count() >= policy_->GetMaxStartedTries()) {
+    RemoveAttemptedRequest(*request, SavePageStatus::START_COUNT_EXCEEDED);
+  } else {
+    queue_->UpdateRequest(
+        *request,
+        base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                   weak_ptr_factory_.GetWeakPtr(), request->client_id()));
+  }
+}
+
+void RequestCoordinator::RemoveAttemptedRequest(const SavePageRequest& request,
+                                                SavePageStatus status) {
+  std::vector<int64_t> remove_requests;
+  remove_requests.push_back(request.request_id());
+  queue_->RemoveRequests(remove_requests,
+                         base::Bind(&RequestCoordinator::HandleRemovedRequests,
+                                    weak_ptr_factory_.GetWeakPtr(), status));
+  RecordAttemptCount(request, status);
+}
+
 void RequestCoordinator::RemoveRequests(
     const std::vector<int64_t>& request_ids,
     const RemoveRequestsCallback& callback) {
   bool canceled = CancelActiveRequestIfItMatches(request_ids);
   queue_->RemoveRequests(
       request_ids,
       base::Bind(&RequestCoordinator::HandleRemovedRequestsAndCallback,
                  weak_ptr_factory_.GetWeakPtr(), callback,
                  SavePageStatus::REMOVED));
   if (canceled)
@@ skipping 235 matching lines @@
 
   is_busy_ = false;
   active_request_.reset(nullptr);
 
   if (status == Offliner::RequestStatus::FOREGROUND_CANCELED ||
       status == Offliner::RequestStatus::PRERENDERING_CANCELED) {
     // Update the request for the canceled attempt.
     // TODO(dougarnett): See if we can conclusively identify other attempt
     // aborted cases to treat this way (eg, for Render Process Killed).
     SavePageRequest updated_request(request);
-    updated_request.MarkAttemptAborted();
-    queue_->UpdateRequest(updated_request,
-                          base::Bind(&RequestCoordinator::UpdateRequestCallback,
-                                     weak_ptr_factory_.GetWeakPtr(),
-                                     updated_request.client_id()));
+    AbortRequestAttempt(&updated_request);
     SavePageStatus notify_status =
         (status == Offliner::RequestStatus::FOREGROUND_CANCELED)
             ? SavePageStatus::FOREGROUND_CANCELED
             : SavePageStatus::PRERENDER_CANCELED;
     NotifyCompleted(updated_request, notify_status);
-
   } else if (status == Offliner::RequestStatus::SAVED) {
     // Remove the request from the queue if it succeeded.
-    std::vector<int64_t> remove_requests;
-    remove_requests.push_back(request.request_id());
-    queue_->RemoveRequests(
-        remove_requests,
-        base::Bind(&RequestCoordinator::HandleRemovedRequests,
-                   weak_ptr_factory_.GetWeakPtr(), SavePageStatus::SUCCESS));
+    RemoveAttemptedRequest(request, SavePageStatus::SUCCESS);
   } else if (request.completed_attempt_count() + 1 >=
              policy_->GetMaxCompletedTries()) {
-    // Remove from the request queue if we exceeeded max retries. The +1
+    // Remove from the request queue if we exceeded max retries. The +1
     // represents the request that just completed. Since we call
     // MarkAttemptCompleted within the if branches, the completed_attempt_count
     // has not yet been updated when we are checking the if condition.
-    std::vector<int64_t> remove_requests;
-    remove_requests.push_back(request.request_id());
-    queue_->RemoveRequests(
-        remove_requests, base::Bind(&RequestCoordinator::HandleRemovedRequests,
-                                    weak_ptr_factory_.GetWeakPtr(),
-                                    SavePageStatus::RETRY_COUNT_EXCEEDED));
+    RemoveAttemptedRequest(request, SavePageStatus::RETRY_COUNT_EXCEEDED);
   } else {
     // If we failed, but are not over the limit, update the request in the
     // queue.
     SavePageRequest updated_request(request);
     updated_request.MarkAttemptCompleted();
     queue_->UpdateRequest(updated_request,
                           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      updated_request.client_id()));
     NotifyChanged(updated_request);
@@ skipping 54 matching lines @@
   FOR_EACH_OBSERVER(Observer, observers_, OnChanged(request));
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages