Add page load abort metrics to PageLoadMetrics system

components/page_load_metrics/browser/metrics_web_contents_observer.cc

 // Copyright 2015 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/page_load_metrics/browser/metrics_web_contents_observer.h"
 
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "components/page_load_metrics/common/page_load_metrics_messages.h"
 #include "components/page_load_metrics/common/page_load_timing.h"
@@ skipping 42 matching lines @@
 }
 
 }  // namespace
 
 #define PAGE_LOAD_HISTOGRAM(name, sample)                           \
   UMA_HISTOGRAM_CUSTOM_TIMES(name, sample,                          \
                              base::TimeDelta::FromMilliseconds(10), \
                              base::TimeDelta::FromMinutes(10), 100)
 
 PageLoadTracker::PageLoadTracker(bool in_foreground)
-    : has_commit_(false), started_in_foreground_(in_foreground) {}
+    : has_commit_(false), started_in_foreground_(in_foreground) {
+  RecordEvent(STARTED_PROVISIONAL_LOAD);
+}
 
 PageLoadTracker::~PageLoadTracker() {
-  if (has_commit_)
+  if (has_commit_) {
     RecordTimingHistograms();
+    if (timing_.first_layout.is_zero())
+      RecordEvent(ABORTED_LOAD_BEFORE_FIRST_LAYOUT);
+    else
+      RecordEvent(SUCCESSFUL_FIRST_LAYOUT);
+  }
 }
 
 void PageLoadTracker::WebContentsHidden() {
   // Only log the first time we background in a given page load.
   if (background_time_.is_null()) {
     background_time_ = base::TimeTicks::Now();
   }
 }
 
 void PageLoadTracker::Commit() {
@@ skipping 50 matching lines @@
     if (timing_.first_layout < background_delta) {
       PAGE_LOAD_HISTOGRAM("PageLoad.Timing2.NavigationToFirstLayout",
                           timing_.first_layout);
     } else {
       PAGE_LOAD_HISTOGRAM("PageLoad.Timing2.NavigationToFirstLayout.BG",
                           timing_.first_layout);
     }
   }
 }
 
+void PageLoadTracker::RecordEvent(PageLoadEvent event) {
+  UMA_HISTOGRAM_ENUMERATION("PageLoad.EventCounts", event, EVENT_COUNT);

[Bryan McQuade, 2015/10/01 14:57:38]

is EVENT_COUNT supposed to be the value of the last enum entry plus one, or the
value of the last valid entry?

also is it valid to add additional entries to such an enum used with
UMA_HISTOGRAM_ENUMERATION (where EVENT_COUNT will increase)?

I've also seen people use UMA_HISTOGRAM_SPARSE_SLOWLY for enums - I'd be
interested to understand the differences just so we know which to choose in the
future. Can you investigate?

[Charlie Harrison, 2015/10/01 15:26:20]

The comments on LOCAL_HISTOGRAM_ENUMERATION clarify this:
// Support histograming of an enumerated value.  The samples should always be
// strictly less than |boundary_value| -- this prevents you from running into
// problems down the line if you add additional buckets to the histogram.

So EVENT_COUNT bounds the enum values, and allows us to add elements later.
HISTOGRAM_SPARSE_SLOWLY I gathered is for sparse histograms, with tons of
buckets. This doesn't really fit our use case and I think this enum histograms
are a better fit.

+}
+
 MetricsWebContentsObserver::MetricsWebContentsObserver(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents), in_foreground_(false) {}
 
 MetricsWebContentsObserver::~MetricsWebContentsObserver() {}
 
 bool MetricsWebContentsObserver::OnMessageReceived(
     const IPC::Message& message,
     content::RenderFrameHost* render_frame_host) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ skipping 20 matching lines @@
 
 void MetricsWebContentsObserver::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame())
     return;
 
   scoped_ptr<PageLoadTracker> finished_nav(
       provisional_loads_.take_and_erase(navigation_handle));
   DCHECK(finished_nav);
 
-  // TODO(csharrison) handle the two error cases:
-  // 1. Error pages that replace the previous page.
-  // 2. Error pages that leave the user on the previous page.
-  // For now these cases will be ignored.
-  if (!navigation_handle->HasCommitted())
+  // Handle a pre-commit error here. Navigations that result in an error page
+  // will be ignored.
+  // TODO(csharrison) filter out error codes that shouldn't result in us logging
+  // an aborted provisional load.
+  if (!navigation_handle->HasCommitted()) {
+    finished_nav->RecordEvent(ABORTED_PROVISIONAL_LOAD);

[Bryan McQuade, 2015/10/01 14:57:38]

could we call this PROVISIONAL_LOAD_FAILED, which captures all types of errors,
even if not user-initiated abort? we can add a separate ABORTED_PROVISIONAL_LOAD
later which tracks just user-initiated aborts.

     return;
+  }
 
   // Don't treat a same-page nav as a new page load.
   if (navigation_handle->IsSamePage())
     return;
 
   // Eagerly log the previous UMA even if we don't care about the current
   // navigation.
   committed_load_.reset();
 
   if (!IsRelevantNavigation(navigation_handle))
@@ skipping 51 matching lines @@
   // sure here that we aren't logging UMA for internal pages.
   const GURL& browser_url = web_contents()->GetLastCommittedURL();
   return navigation_handle->IsInMainFrame() &&
          !navigation_handle->IsSamePage() &&
          !navigation_handle->IsErrorPage() &&
          navigation_handle->GetURL().SchemeIsHTTPOrHTTPS() &&
          browser_url.SchemeIsHTTPOrHTTPS();
 }
 
 }  // namespace page_load_metrics