Adding UMA for data reduction proxy page load size/savings

chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
index 50e1cdbaa1ab92012a541d2ad8bb643fb5b59d33..83ce4fa842d0633d5d3f518ec4914b6547b68116 100644
--- a/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/metrics/histogram_macros.h"
 #include "base/optional.h"
 #include "base/time/time.h"
 #include "chrome/browser/loader/chrome_navigation_data.h"
@@ -75,10 +76,32 @@ const char kHistogramParseBlockedOnScriptLoadSuffix[] =
     "ParseTiming.ParseBlockedOnScriptLoad";
 const char kHistogramParseDurationSuffix[] = "ParseTiming.ParseDuration";
 
+const char kRequestsPercentProxied[] =

[Bryan McQuade, 2017/01/19 18:44:18]

one challenge we've encountered with percent histograms is that it is difficult
to distinguish between e.g. 50% meaning 1 in 2 requests vs 50% meaning 50 of 100
requests - yet those cases are very different.

One possible way to address this would be to have a couple histograms here that
are segmented by total number of requests on the page, e.g.
Experimental.Bytes.Network.PercentProxied.LessThan10Requests, and
.AtLeast10Requests (maybe there are shorter ways to express this?). You could
also have an overall histo that collects stats for all page loads.

This is an optional suggestion - feel free to proceed w/o this. I've just found
it confusing to reason about these percentage histograms due to the above issue.

[RyanSturm, 2017/01/19 19:32:45]

Hmm. I'll open a bug to add a LessThan10Requests histogram later based on what
we see in the UMA initially. I think it is a good idea, but I don't want to add
too much new UMA.

Thanks for the suggestion. crbug.com/682765

+    "Experimental.Requests.Network.PercentProxied";
+const char kBytesPercentProxied[] = "Experimental.Bytes.Network.PercentProxied";
+const char kBytesCompressionRatio[] =
+    "Experimental.Bytes.Network.CompressionRatio";
+const char kBytesInflationPercent[] =
+    "Experimental.Bytes.Network.InflationPercent";
+const char kNetworkRequests[] = "Experimental.Requests.Network";
+const char kRequestsProxied[] = "Experimental.Requests.Network.Proxied";
+const char kRequestsNotProxied[] = "Experimental.Requests.Network.NonProxied";
+const char kNetworkBytes[] = "Experimental.Bytes.Network";
+const char kBytesProxied[] = "Experimental.Bytes.Network.Proxied";

[tbansal1, 2017/01/19 03:12:40]

Actually, I still do not understand why we can't replace Bytes by KB in the
histogram name. Are we sure that there is a need to precisely match the names of
the other existing histograms.

Searching for ".*KB.*" in the UMA dashboard shows plenty of histograms that use
KB in their name.

I would prefer if the histogram names are clear enough (or at least not
misleading).

[RyanSturm, 2017/01/19 19:32:45]

Acknowledged.

+const char kBytesNotProxied[] = "Experimental.Bytes.Network.NonProxied";
+const char kBytesOriginal[] = "Experimental.Bytes.Network.Original";
+const char kBytesSavings[] = "Experimental.Bytes.Network.Savings";
+const char kBytesInflation[] = "Experimental.Bytes.Network.Inflation";
+
 }  // namespace internal
 
 DataReductionProxyMetricsObserver::DataReductionProxyMetricsObserver()
-    : browser_context_(nullptr) {}
+    : browser_context_(nullptr),
+      num_data_reduction_proxy_requests_(0),
+      num_network_requests_(0),
+      original_network_bytes_(0),
+      network_bytes_proxied_(0),
+      network_bytes_(0) {}
 
 DataReductionProxyMetricsObserver::~DataReductionProxyMetricsObserver() {}
 
@@ -128,6 +151,7 @@ page_load_metrics::PageLoadMetricsObserver::ObservePolicy
 DataReductionProxyMetricsObserver::OnHidden(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
+  RecordPageSizeUMA();
   SendPingback(timing, info);
   return STOP_OBSERVING;
 }
@@ -140,6 +164,7 @@ DataReductionProxyMetricsObserver::FlushMetricsOnAppEnterBackground(
   // app is about to be backgrounded, as part of the Activity.onPause()
   // flow. After this method is invoked, Chrome may be killed without further
   // notification, so we send a pingback with data collected up to this point.
+  RecordPageSizeUMA();
   SendPingback(timing, info);
   return STOP_OBSERVING;
 }
@@ -147,9 +172,111 @@ DataReductionProxyMetricsObserver::FlushMetricsOnAppEnterBackground(
 void DataReductionProxyMetricsObserver::OnComplete(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
+  RecordPageSizeUMA();
   SendPingback(timing, info);
 }
 
+void DataReductionProxyMetricsObserver::RecordPageSizeUMA() const {
+  // If the first request didn't complete, don't record UMA.
+  if (num_network_requests_ == 0) {
+    return;
+  }
+
+  // The percent of requests that went through the data reduction proxy.
+  UMA_HISTOGRAM_PERCENTAGE(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append(internal::kRequestsPercentProxied),
+      (100 * num_data_reduction_proxy_requests_) / num_network_requests_);
+
+  // The percent of bytes that went through the data reduction proxy.
+  if (network_bytes_ > 0) {
+    UMA_HISTOGRAM_PERCENTAGE(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append(internal::kBytesPercentProxied),
+        static_cast<int>((100 * network_bytes_proxied_) / network_bytes_));

[Alexei Svitkine (slow), 2017/01/19 18:39:50]

Is static_cast needed here?

Seems you don't need it above on line 189 - if not I suggest removing it.

[RyanSturm, 2017/01/19 19:32:45]

For the percent ones, it might not be necessary, but I think I'd prefer to be
safe since this will be an int64_t after multiplication/division. Otherwise it
will be implicitly casted. 

+  }
+
+  // If the data reduction proxy caused savings, record the compression ratio;
+  // otherwise, record the inflation ratio.
+  if (original_network_bytes_ > 0 &&
+      original_network_bytes_ >= network_bytes_) {
+    UMA_HISTOGRAM_PERCENTAGE(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append(internal::kBytesCompressionRatio),
+        static_cast<int>((100 * network_bytes_) / original_network_bytes_));
+  } else if (original_network_bytes_ > 0) {
+    // Inflation should never be above one hundred percent.

[Bryan McQuade, 2017/01/19 18:44:18]

what happens if it is over 100%? does it just get recorded in the last bucket?

[RyanSturm, 2017/01/19 19:32:45]

Correct, which for now is an approach we are comfortable with. If we see that we
have a lot in that bucket, we will address the issue then.

+    UMA_HISTOGRAM_PERCENTAGE(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append(internal::kBytesInflationPercent),
+        static_cast<int>((100 * network_bytes_) / original_network_bytes_ -
+                         100));
+  }
+
+  // Record the number of network requests seen.
+  UMA_HISTOGRAM_COUNTS_10000(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append(internal::kNetworkRequests),
+      num_network_requests_);
+
+  // Record the number of requests that used data reduction proxy.
+  UMA_HISTOGRAM_COUNTS_10000(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append(internal::kRequestsProxied),

[Alexei Svitkine (slow), 2017/01/19 18:39:50]

Nit: How about making a helper function for this to make all of these take 1
line instead of two?

e.g. GetConstHistogramWithSuffix(internal::kRequestsProxied)?

[RyanSturm, 2017/01/19 19:32:45]

Done.

+      num_data_reduction_proxy_requests_);
+
+  // Record the number of requests that did not use data reduction proxy.
+  UMA_HISTOGRAM_COUNTS_10000(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append(internal::kRequestsNotProxied),
+      num_network_requests_ - num_data_reduction_proxy_requests_);
+
+  // Record the total KB of network bytes. Capped at 500 MB.
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append(internal::kNetworkBytes),
+      static_cast<int>(network_bytes_ / 1024), 1, 500 * 1024, 50);
+
+  // Record the total amount of bytes that went through the data reduction
+  // proxy. Capped at 500 MB.
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append(internal::kBytesProxied),
+      static_cast<int>(network_bytes_proxied_ / 1024), 1, 500 * 1024, 50);
+
+  // Record the total amount of bytes that did not go through the data reduction
+  // proxy. Capped at 500 MB.
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append(internal::kBytesNotProxied),
+      static_cast<int>((network_bytes_ - network_bytes_proxied_) / 1024), 1,
+      500 * 1024, 50);
+
+  // Record the total KB of network bytes that the user would have seen without
+  // using data reduction proxy. Capped at 500 MB.
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append(internal::kBytesOriginal),
+      static_cast<int>(original_network_bytes_ / 1024), 1, 500 * 1024, 50);
+
+  // Record the savings the user saw by using data reduction proxy. If there was
+  // inflation instead, record that. Capped at 500 MB.
+  if (network_bytes_ <= original_network_bytes_) {
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append(internal::kBytesSavings),
+        static_cast<int>((original_network_bytes_ - network_bytes_) / 1024), 1,
+        500 * 1024, 50);
+  } else {
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append(internal::kBytesInflation),
+        static_cast<int>((network_bytes_proxied_ - original_network_bytes_) /
+                         1024),
+        1, 500 * 1024, 50);
+  }
+}
+
 void DataReductionProxyMetricsObserver::SendPingback(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
@@ -284,6 +411,19 @@ void DataReductionProxyMetricsObserver::OnParseStop(
       internal::kHistogramParseBlockedOnScriptLoadSuffix);
 }
 
+void DataReductionProxyMetricsObserver::OnLoadedResource(
+    const page_load_metrics::ExtraRequestInfo& extra_request_info) {
+  if (extra_request_info.was_cached)
+    return;
+  original_network_bytes_ += extra_request_info.original_network_content_length;
+  network_bytes_ += extra_request_info.raw_body_bytes;
+  num_network_requests_++;
+  if (!extra_request_info.data_reduction_proxy_used)
+    return;
+  num_data_reduction_proxy_requests_++;
+  network_bytes_proxied_ += extra_request_info.raw_body_bytes;
+}
+
 DataReductionProxyPingbackClient*
 DataReductionProxyMetricsObserver::GetPingbackClient() const {
   return DataReductionProxyChromeSettingsFactory::GetForBrowserContext(