Add PLT measurement to Resource Prefetching for Mobile Web

chrome/browser/predictors/resource_prefetch_predictor.cc

Index: chrome/browser/predictors/resource_prefetch_predictor.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.cc b/chrome/browser/predictors/resource_prefetch_predictor.cc
index ea767daf69fe59792c6ebd121297aaacaf393aa4..a005f222fe0e700ed3057bf047139745bd9277ee 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor.cc
@@ -34,6 +34,7 @@
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/mime_util.h"
+#include "net/base/network_change_notifier.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -90,6 +91,12 @@ void RecordNavigationEvent(NavigationEvent event) {
                             NAVIGATION_EVENT_COUNT);
 }
 
+enum AdditionalConnectionType {
+    CONNECTION_CELLULAR = net::NetworkChangeNotifier::CONNECTION_LAST + 1,

[Alexei Svitkine (slow), 2014/10/14 19:27:27]

Nit: Indentation is wrong.

[Zhen Wang, 2014/10/16 18:12:22]

Done.

+    CONNECTION_ALL = net::NetworkChangeNotifier::CONNECTION_LAST + 2,
+    CONNECTION_COUNT = net::NetworkChangeNotifier::CONNECTION_LAST + 3
+  };
+
 }  // namespace
 
 namespace predictors {
@@ -525,6 +532,8 @@ void ResourcePrefetchPredictor::OnNavigationComplete(
   RecordNavigationEvent(NAVIGATION_EVENT_ONLOAD_TRACKED_URL);
 
   // Report any stats.
+  base::TimeDelta plt = base::TimeTicks::Now() - navigation_id.creation_time;
+  ReportPageLoadTimeStats(plt);
   if (prefetch_manager_.get()) {
     ResultsMap::iterator results_it = results_map_.find(navigation_id);
     bool have_prefetch_results = results_it != results_map_.end();
@@ -534,6 +543,9 @@ void ResourcePrefetchPredictor::OnNavigationComplete(
       ReportAccuracyStats(results_it->second->key_type,
                           *(nav_it->second),
                           results_it->second->requests.get());
+      ReportPageLoadTimePrefetchedStats(results_it->second->key_type, plt);
+    } else {
+      ReportPageLoadTimeNotPrefetchedStats(plt);
     }
   } else {
     scoped_ptr<ResourcePrefetcher::RequestVector> requests(
@@ -1003,7 +1015,249 @@ void ResourcePrefetchPredictor::LearnNavigation(
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// Accuracy measurement.
+// Page load time and accuracy measurement.
+
+#define REPORT_PLT(name, page_load_time) \
+  UMA_HISTOGRAM_CUSTOM_TIMES( \
+      name, \
+      page_load_time, \
+      base::TimeDelta::FromMilliseconds(10), \
+      base::TimeDelta::FromSeconds(60), \

[Alexei Svitkine (slow), 2014/10/14 19:27:27]

I don't think 60s is a good upper bound - there are probably pages that take
longer to load.

I suggest using UMA_HISTOGRAM_MEDIUM_TIMES(), which gives you an upper bound of
3minutes. It is 50 buckets, rather than 100, but I think given that you're
adding so many histograms, its wasteful to have them all be 100 buckets, in
terms of memory used on the client and data used in logs.

[Zhen Wang, 2014/10/16 18:12:22]

The granularity of 3 minutes with 50 buckets is too coarse (3.6s). The
granularity of 1 minute with 100 buckets (0.6s) seems most useful. For current
stage, we do not care too much about super long PLT pages. Although there are
many histograms for PLT, they are usually exclusive to each other because there
is only one network type for each page load.

And this is consistent with the implementation in prerender_histograms
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre...).

[Alexei Svitkine (slow), 2014/10/16 20:05:36]

Are you aware that these histograms will have bucket sizes that are
exponentially increasing? You can't just divide 3 minutes by 50, since it's not
a linear distribution. Instead, you'll get more granularity at the low range and
coarser granularity closer to the max value.

[Zhen Wang, 2014/10/16 21:57:34]

Oh, thanks for pointing it out. Using medium times now.

+      100)
+
+void ResourcePrefetchPredictor::ReportPageLoadTimeStats(
+    base::TimeDelta plt) const {
+  net::NetworkChangeNotifier::ConnectionType connection_type =
+      net::NetworkChangeNotifier::GetConnectionType();
+
+  REPORT_PLT("ResourcePrefetchPredictor.PLT", plt);
+  if (net::NetworkChangeNotifier::IsConnectionCellular(connection_type))
+    REPORT_PLT("ResourcePrefetchPredictor.PLT.Cellular", plt);
+  switch (connection_type) {
+    case net::NetworkChangeNotifier::CONNECTION_ETHERNET:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.Ethernet", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_WIFI:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.WiFi", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_2G:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.2G", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_3G:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.3G", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_4G:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.4G", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_NONE:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.None", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_BLUETOOTH:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.Bluetooth", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_UNKNOWN:
+    default:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.Unknown", plt);
+      break;
+  }
+}
+
+void ResourcePrefetchPredictor::ReportPageLoadTimePrefetchedStats(
+    PrefetchKeyType key_type,
+    base::TimeDelta plt) const {
+  net::NetworkChangeNotifier::ConnectionType connection_type =
+      net::NetworkChangeNotifier::GetConnectionType();
+  bool on_cellular =
+      net::NetworkChangeNotifier::IsConnectionCellular(connection_type);
+
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourcePrefetchPredictor.PagePrefetchedNetworkType",

[Alexei Svitkine (slow), 2014/10/14 19:27:27]

Please make a helper function for this histogram which takes the enum value as a
param. Since each histogram macro block expands to a lot of code, this will help
reduce some bloat.

[Zhen Wang, 2014/10/16 18:12:22]

Done.

+      CONNECTION_ALL,
+      CONNECTION_COUNT);
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourcePrefetchPredictor.PagePrefetchedNetworkType",
+      net::NetworkChangeNotifier::GetConnectionType(),
+      CONNECTION_COUNT);
+  if (on_cellular) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "ResourcePrefetchPredictor.PagePrefetchedNetworkType",
+        CONNECTION_CELLULAR,
+        CONNECTION_COUNT);
+  }
+
+  REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched", plt);
+  if (on_cellular)
+    REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.Cellular", plt);
+  switch (connection_type) {
+    case net::NetworkChangeNotifier::CONNECTION_ETHERNET:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.Ethernet", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_WIFI:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.WiFi", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_2G:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.2G", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_3G:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.3G", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_4G:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.4G", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_NONE:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.None", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_BLUETOOTH:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.Bluetooth", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_UNKNOWN:
+    default:
+      REPORT_PLT(
+          "ResourcePrefetchPredictor.PLT.PagePrefetched.Unknown", plt);
+      break;
+  }
+
+  if (key_type == PREFETCH_KEY_TYPE_HOST) {
+    REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.Host", plt);
+    if (on_cellular) {
+      REPORT_PLT(
+          "ResourcePrefetchPredictor.PLT.PagePrefetched.Host.Cellular", plt);
+    }
+    switch (connection_type) {
+      case net::NetworkChangeNotifier::CONNECTION_ETHERNET:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Host.Ethernet", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_WIFI:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Host.WiFi", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_2G:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Host.2G", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_3G:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Host.3G", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_4G:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Host.4G", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_NONE:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Host.None", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_BLUETOOTH:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Host.Bluetooth", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_UNKNOWN:
+      default:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Host.Unknown", plt);
+        break;
+    }
+  } else {
+    REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched.Url", plt);

[Alexei Svitkine (slow), 2014/10/14 19:27:27]

Honestly, I feel like it would be better to just use the underlying Histogram*
API directly, rather than going through the macros. Then, you can avoid all of
this duplication and just have helper functions that return suffix strings for
the particular enums.

[Zhen Wang, 2014/10/16 18:12:22]

I originally have a more concise version (patch 1). e.g.

REPORT_PLT("ResourcePrefetchPredictor.PLT.PagePrefetched." + histogram_type +
"." + GetNetworkTypeString(), plt);

But it turns out that each macro should records a const name. So I have to
expand it to this duplication. Or is there a better histogram API for me to use?

[Alexei Svitkine (slow), 2014/10/16 20:05:36]

Yeah, you can't use the macro. You need to use the FactoryGet() API of a
histogram directly. You can look at what the UMA macro expands to and use that
minus the static var that's used for caching.

[Zhen Wang, 2014/10/16 21:57:34]

Using self-defined RPP_HISTOGRAM_MEDIUM_TIMES now.

+    if (on_cellular) {
+      REPORT_PLT(
+          "ResourcePrefetchPredictor.PLT.PagePrefetched.Url.Cellular", plt);
+    }
+    switch (connection_type) {
+      case net::NetworkChangeNotifier::CONNECTION_ETHERNET:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Url.Ethernet", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_WIFI:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Url.WiFi", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_2G:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Url.2G", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_3G:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Url.3G", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_4G:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Url.4G", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_NONE:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Url.None", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_BLUETOOTH:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Url.Bluetooth", plt);
+        break;
+      case net::NetworkChangeNotifier::CONNECTION_UNKNOWN:
+      default:
+        REPORT_PLT(
+            "ResourcePrefetchPredictor.PLT.PagePrefetched.Url.Unknown", plt);
+        break;
+    }
+  }
+}
+
+void ResourcePrefetchPredictor::ReportPageLoadTimeNotPrefetchedStats(
+    base::TimeDelta plt) const {
+  net::NetworkChangeNotifier::ConnectionType connection_type =
+      net::NetworkChangeNotifier::GetConnectionType();
+  bool on_cellular =
+      net::NetworkChangeNotifier::IsConnectionCellular(connection_type);
+
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourcePrefetchPredictor.PageNotPrefetchedNetworkType",
+      CONNECTION_ALL,
+      CONNECTION_COUNT);
+  UMA_HISTOGRAM_ENUMERATION(
+      "ResourcePrefetchPredictor.PageNotPrefetchedNetworkType",
+      net::NetworkChangeNotifier::GetConnectionType(),
+      CONNECTION_COUNT);
+  if (on_cellular) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "ResourcePrefetchPredictor.PagePrefetchedNetworkType",
+        CONNECTION_CELLULAR,
+        CONNECTION_COUNT);
+  }
+
+  REPORT_PLT("ResourcePrefetchPredictor.PLT.PageNotPrefetched", plt);
+  if (on_cellular)
+    REPORT_PLT("ResourcePrefetchPredictor.PLT.PageNotPrefetched.Cellular", plt);
+  switch (connection_type) {
+    case net::NetworkChangeNotifier::CONNECTION_ETHERNET:
+      REPORT_PLT(
+          "ResourcePrefetchPredictor.PLT.PageNotPrefetched.Ethernet", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_WIFI:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PageNotPrefetched.WiFi", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_2G:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PageNotPrefetched.2G", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_3G:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PageNotPrefetched.3G", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_4G:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PageNotPrefetched.4G", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_NONE:
+      REPORT_PLT("ResourcePrefetchPredictor.PLT.PageNotPrefetched.None", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_BLUETOOTH:
+      REPORT_PLT(
+          "ResourcePrefetchPredictor.PLT.PageNotPrefetched.Bluetooth", plt);
+      break;
+    case net::NetworkChangeNotifier::CONNECTION_UNKNOWN:
+    default:
+      REPORT_PLT(
+          "ResourcePrefetchPredictor.PLT.PageNotPrefetched.Unknown", plt);
+      break;
+  }
+}
 
 void ResourcePrefetchPredictor::ReportAccuracyStats(
     PrefetchKeyType key_type,