increase bucket size and break down video by http/https

components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc

Index: components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
index d444fbf3cfc42c930480d1b7e1a8d360643f0e7b..c047a26c10b2648ecb9fb3a5a8e4582555ebcc39 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
@@ -12,6 +12,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
+#include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h"
@@ -43,19 +44,66 @@ namespace data_reduction_proxy {
 
 namespace {
 
-// |lofi_low_header_added| is set to true iff Lo-Fi "q=low" request header can
-// be added to the Chrome proxy headers.
-// |received_content_length| is the number of prefilter bytes received.
-// |original_content_length| is the length of resource if accessed directly
-// without data saver proxy.
-// |freshness_lifetime| contains information on how long the resource will be
-// fresh for and how long is the usability.
+// Records the occurrence of |sample| in |name| histogram. Conventional UMA

[RyanSturm, 2017/05/24 23:15:37]

nit: s/Conventional UMA histograms/UMA macros/

[ajo1, 2017/05/25 18:19:09]

Done.

+// histograms are not used because the |name| is not static.
+void RecordNewContentLengthHistogram(const std::string& name, int64_t sample) {
+  base::HistogramBase* histogram_pointer = base::Histogram::FactoryGet(
+      name,
+      1,          // Minimum sample size in bytes.
+      128 << 20,  // Maximum sample size in bytes.
+      200,        // Bucket count.
+      base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram_pointer->Add(sample);
+}
+
+void RecordNewContentLengthHistograms(
+    bool is_https,
+    bool is_video,
+    DataReductionProxyRequestType request_type,
+    int64_t received_content_length) {
+  const char* prefix = "Net.HttpContentLength";
+  const char* connection_type = is_https ? ".Https" : ".Http";
+  const char* suffix;
+  switch (request_type) {
+    case VIA_DATA_REDUCTION_PROXY:
+      suffix = ".ViaDRP";
+      break;
+    case HTTPS:
+      suffix = ".Direct";
+      break;
+    case SHORT_BYPASS:
+    case LONG_BYPASS:
+      suffix = ".BypassedDRP";
+      break;
+    case UPDATE:
+    case UNKNOWN_TYPE:
+      suffix = ".Other";
+      break;
+  }
+  // Record aggregate of video and non-video traffic

[RyanSturm, 2017/05/24 23:15:37]

s/traffic/traffic./

[ajo1, 2017/05/25 18:19:09]

Done.

+  RecordNewContentLengthHistogram(
+      base::StringPrintf("%s%s%s", prefix, connection_type, suffix),
+      received_content_length);
+  if (is_video) {
+    RecordNewContentLengthHistogram(
+        base::StringPrintf("%s%s%s.Video", prefix, connection_type, suffix),
+        received_content_length);
+  }
+}
+
+// |lofi_low_header_added| is set to true iff Lo-Fi "q=low" request header

[RyanSturm, 2017/05/24 23:15:37]

Yikes, this is an old q=low comment. Would you mind changing it to remove
"q=low" so we can just read it as 'a "Lo-Fi" request header can be added...'

[ajo1, 2017/05/25 18:19:09]

Done.

+// can be added to the Chrome proxy header. |received_content_length| is
+// the number of prefilter bytes received. |original_content_length| is the
+// length of resource if accessed directly without data saver proxy.
+// |freshness_lifetime| specifies how long the resource will
+// be fresh for.
 void RecordContentLengthHistograms(bool lofi_low_header_added,
                                    bool is_https,
                                    bool is_video,
                                    int64_t received_content_length,
                                    int64_t original_content_length,
-                                   const base::TimeDelta& freshness_lifetime) {
+                                   const base::TimeDelta& freshness_lifetime,
+                                   DataReductionProxyRequestType request_type) {
   // Add the current resource to these histograms only when a valid
   // X-Original-Content-Length header is present.
   if (original_content_length >= 0) {
@@ -83,18 +131,13 @@ void RecordContentLengthHistograms(bool lofi_low_header_added,
     // length if the X-Original-Content-Header is not present.
     original_content_length = received_content_length;
   }
-  UMA_HISTOGRAM_COUNTS_1M("Net.HttpContentLength", received_content_length);
-  if (is_https) {
-    UMA_HISTOGRAM_COUNTS_1M("Net.HttpContentLength.Https",
-                            received_content_length);
-  } else {
-    UMA_HISTOGRAM_COUNTS_1M("Net.HttpContentLength.Http",
-                            received_content_length);
-  }
-  if (is_video) {
-    UMA_HISTOGRAM_COUNTS_1M("Net.HttpContentLength.Video",
-                            received_content_length);
-  }
+  std::string prefix = "Net.HttpContentLength";
+  UMA_HISTOGRAM_COUNTS_1M(prefix, received_content_length);
+
+  // Record the new histograms broken down by http/https and video/nonvideo

[RyanSturm, 2017/05/24 23:15:37]

s:/nonvideo/non-video./
s:/"http/https"/"HTTP/HTTPS"/

[ajo1, 2017/05/25 18:19:09]

Done.

+  RecordNewContentLengthHistograms(is_https, is_video, request_type,
+                                   received_content_length);
+
   UMA_HISTOGRAM_COUNTS_1M("Net.HttpOriginalContentLength",
                           original_content_length);
   UMA_HISTOGRAM_COUNTS_1M("Net.HttpContentLengthDifference",
@@ -102,8 +145,7 @@ void RecordContentLengthHistograms(bool lofi_low_header_added,
   UMA_HISTOGRAM_CUSTOM_COUNTS("Net.HttpContentFreshnessLifetime",
                               freshness_lifetime.InSeconds(),
                               base::TimeDelta::FromHours(1).InSeconds(),
-                              base::TimeDelta::FromDays(30).InSeconds(),
-                              100);
+                              base::TimeDelta::FromDays(30).InSeconds(), 100);
   if (freshness_lifetime.InSeconds() <= 0)
     return;
   UMA_HISTOGRAM_COUNTS_1M("Net.HttpContentLengthCacheable",
@@ -120,7 +162,7 @@ void RecordContentLengthHistograms(bool lofi_low_header_added,
 }
 
 // Estimate the size of the original headers of |request|. If |used_drp| is
-// true, then it's assumed that the original request would have used HTTP/1.1,
+// true, then it's assumed that the original request would have used  HTTP/1.1,
 // otherwise it assumes that the original request would have used the same
 // protocol as |request| did. This is to account for stuff like HTTP/2 header
 // compression.
@@ -570,7 +612,7 @@ void DataReductionProxyNetworkDelegate::RecordContentLength(
           data_reduction_proxy_io_data_->lofi_decider() &&
           data_reduction_proxy_io_data_->lofi_decider()->IsUsingLoFi(request),
       is_https, is_video, request.received_response_content_length(),
-      original_content_length, freshness_lifetime);
+      original_content_length, freshness_lifetime, request_type);

[RyanSturm, 2017/05/24 23:15:37]

This is sourced from:

https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro...

That implementation counts data saver being off as SHORT_BYPASS. Can you file a
bug to fix that (perhaps adding a new value to the enum for
DATA_SAVER_OFF/DATA_SAVER_EMPTY_CONFIG).

For now you can land this CL, but definitely need s a fix for these histograms
and a few others.

[ajo1, 2017/05/25 18:19:09]

crbug.com/726411

 
   if (data_reduction_proxy_io_data_ && data_reduction_proxy_bypass_stats_) {
     // Record BypassedBytes histograms for the request.