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..78e6cbe28266fb968544c19ab6baa9a187915c14 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,68 @@ 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. UMA macros 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.

[RyanSturm, 2017/05/25 21:05:41]

I just noticed you missed my comment from before about bucket size and max
bounds, can you look at them  (patch set 10)

[ajo1, 2017/05/25 22:10:37]

Done.

[Ilya Sherman, 2017/05/25 22:12:31]

200 is a huge number of buckets.  Are you sure you need so many?  50 is a more
typical number of buckets.

[Ilya Sherman, 2017/05/25 22:14:16]

(Nvm, you've addressed this comment already. Thanks!)

+      base::HistogramBase::kUmaTargetedHistogramFlag);
+  histogram_pointer->Add(sample);

[Ilya Sherman, 2017/05/25 22:12:31]

Please use base::UmaHistogramCustomCounts() rather than calling FactoryGet() and
Add() directly.

[ajo1, 2017/05/25 23:42:19]

Done.

+}
+
+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;
+  // TODO(crbug.com/726411): Differentiate between a bypass and a disabled
+  // proxy config.
+  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/25 21:05:41]

nit: sort of weird to say "record aggregate" when you aren't summing anything,
but rather recording traffic for each request regardless of video/non-video.
Feel free to leave it if it makes more sense to you than it does to me.

[ajo1, 2017/05/25 22:10:37]

Clarified the language.

+  RecordNewContentLengthHistogram(
+      base::StringPrintf("%s%s%s", prefix, connection_type, suffix),

[Ilya Sherman, 2017/05/25 22:12:32]

Optional nit: I suspect that using std::string rather than char* and a+b+c
rather than StringPrintf is at least equally efficient, and IMO easier to read.

[ajo1, 2017/05/25 23:42:18]

Done.

+      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 request header
+// 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 +133,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";

[Ilya Sherman, 2017/05/25 22:12:31]

nit: This is an odd choice of variable name, given how it's used below.

[ajo1, 2017/05/25 23:42:19]

This naming was chosen in an earlier revision. I removed the variable as it
doesn't provide any useful information in the current patch.

+  UMA_HISTOGRAM_COUNTS_1M(prefix, received_content_length);
+
+  // Record the new histograms broken down by HTTP/HTTPS and video/non-video
+  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 +147,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 +164,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,

[Ilya Sherman, 2017/05/25 22:12:32]

nit: Spurious diff?

[ajo1, 2017/05/25 23:42:19]

Erroneous space snuck in. I cleaned it up.

 // 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 +614,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);
 
   if (data_reduction_proxy_io_data_ && data_reduction_proxy_bypass_stats_) {
     // Record BypassedBytes histograms for the request.