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..e9ada2b4192d41b5473397e8c8d81a75222ef60e 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"
@@ -78,7 +79,12 @@ const char kHistogramParseDurationSuffix[] = "ParseTiming.ParseDuration";
 }  // 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 +134,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 +147,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 +155,112 @@ DataReductionProxyMetricsObserver::FlushMetricsOnAppEnterBackground(
 void DataReductionProxyMetricsObserver::OnComplete(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
+  RecordPageSizeUMA();
   SendPingback(timing, info);
 }
 
+void DataReductionProxyMetricsObserver::RecordPageSizeUMA() {

[tbansal1, 2017/01/18 22:35:19]

const method?

[RyanSturm, 2017/01/18 22:57:49]

Done.

+  if (num_network_requests_ == 0) {
+    return;

[tbansal1, 2017/01/18 22:35:19]

Why return? It might be useful to know if there are pages that are 100% cached
(may be PWA or due to SW)?

[RyanSturm, 2017/01/18 22:57:49]

This is happens when the main page request uses DRP but doesn't complete before
OnComplete (e.g., navigation away from the page)

+  }
+
+  // The percent of requests that went through the data reduction proxy.
+  int percent_proxied_requests =
+      100 * num_data_reduction_proxy_requests_ / num_network_requests_;

[tbansal1, 2017/01/18 22:35:19]

do we need parenthesis here? to get around the ambiguity of what happens first:
division or multiplication

similar comments below

[RyanSturm, 2017/01/18 22:57:49]

No, but I'll add it for you.

+  UMA_HISTOGRAM_PERCENTAGE(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append("Experimental.Requests.Network.PercentProxied"),
+      percent_proxied_requests);
+
+  // The percent of bytes that went through the data reduction proxy.
+  if (network_bytes_ > 0) {
+    int percent_network_bytes_proxied =
+        100 * network_bytes_proxied_ / network_bytes_;
+    UMA_HISTOGRAM_PERCENTAGE(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append("Experimental.Bytes.Network.PercentProxied"),
+        percent_network_bytes_proxied);
+  }
+
+  // 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_) {

[tbansal1, 2017/01/18 22:35:19]

s/>/>=/ for the second condition?
If the two are equal, I think we should record that as 0% savings rather than 0%
inflation.

[RyanSturm, 2017/01/18 22:57:49]

Done.

+    int compression_ratio = 100 * network_bytes_ / original_network_bytes_;
+    UMA_HISTOGRAM_PERCENTAGE(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append("Experimental.Bytes.Network.CompressionRatio"),

[tbansal1, 2017/01/18 22:35:19]

Should we record savings percent because that's what we use in all other
histograms?

[RyanSturm, 2017/01/18 22:57:49]

I'd prefer compression ratio because it shows when users get above 100% savings.

+        compression_ratio);
+  } else if (network_bytes_ > 0) {

[tbansal1, 2017/01/18 22:35:19]

Can network_bytes_ be 0 when num_network_requests_ > 0? The latter is already
checked at Line 163 above.

[RyanSturm, 2017/01/18 22:57:49]

I assume so. This is body bytes, and I believe body bytes could theoretically be
0.

+    int inflation_ratio = 100 * original_network_bytes_ / network_bytes_;

[tbansal1, 2017/01/18 22:35:19]

This histogram is difficult to parse.
eg., if original_n_b = 1, n_b = 10; then inflation_ratio is 10%
original_n_b = 1, n_b = 20; then inflation_ratio is 5%

whereas, clearly inflation was more in the second case.

Another possible way is to record:
(n_b - o_n_b)/o_n_b * 100

[RyanSturm, 2017/01/18 22:57:49]

Done.

+    UMA_HISTOGRAM_PERCENTAGE(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append("Experimental.Bytes.Network.InflationRatio"),
+        inflation_ratio);
+  }
+
+  // Record the number of network requests seen.
+  UMA_HISTOGRAM_COUNTS_10000(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append("Experimental.Requests.Network"),
+      num_network_requests_);
+
+  // Record the number of requests that used data reduction proxy.
+  UMA_HISTOGRAM_COUNTS_10000(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append("Experimental.Requests.Network.Proxied"),
+      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("Experimental.Requests.Network.NonProxied"),
+      num_network_requests_ - num_data_reduction_proxy_requests_);
+
+  // Record the total KB of network bytes.
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append("Experimental.Bytes.Network"),

[tbansal1, 2017/01/18 22:35:19]

Use KB or KiloBytes in the histogram name to avoid confusion

same comment below

[RyanSturm, 2017/01/18 22:57:49]

It's in the units on histograms.xml and I am standardizing on PLM exsiting
metrics.

+      static_cast<int>(network_bytes_) / 1024, 1, 500 * 1024, 50);
+
+  // Record the total amount of bytes that went through the data reduction
+  // proxy.
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append("Experimental.Bytes.Network.Proxied"),
+      static_cast<int>(network_bytes_proxied_) / 1024, 1, 500 * 1024, 50);

[tbansal1, 2017/01/18 22:35:19]

Can you add a comment that upper bucket is 500 MB. Here and below.

you can also use UMA_HISTOGRAM_COUNTS_1M if you are willing to use 1 GB as upper
bound.

[RyanSturm, 2017/01/18 22:57:49]

Done.

+
+  // Record the total amount of bytes that did not go through the data reduction
+  // proxy.
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append("Experimental.Bytes.Network.NonProxied"),
+      static_cast<int>(network_bytes_proxied_) / 1024, 1, 500 * 1024, 50);

[tbansal1, 2017/01/18 22:35:19]

s/network_bytes_proxied_/network_bytes_ - network_bytes_proxied_/

[RyanSturm, 2017/01/18 22:57:49]

Done.

+
+  // Record the total KB of network bytes that the user would have seen without
+  // using data reduction proxy.
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      std::string(internal::kHistogramDataReductionProxyPrefix)
+          .append("Experimental.Bytes.Network.Original"),
+      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.
+  if (network_bytes_ < original_network_bytes_) {

[tbansal1, 2017/01/18 22:35:19]

s/</<=/

[RyanSturm, 2017/01/18 22:57:49]

Done.

+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append("Experimental.Bytes.Network.Savings"),
+        static_cast<int>(original_network_bytes_ - network_bytes_) / 1024, 1,
+        500 * 1024, 50);
+  } else {
+    UMA_HISTOGRAM_CUSTOM_COUNTS(
+        std::string(internal::kHistogramDataReductionProxyPrefix)
+            .append("Experimental.Bytes.Network.Inflation"),
+        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 +395,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(