Adds UMA to measure when the data reduction proxy via header is missing

components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc

Index: components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
diff --git a/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc b/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
index b24094763c503b3a30ef59cf9ebad7ba84a9127a..4f77f1a7d05f7adf460aa35f19d14318c1792b82 100644
--- a/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
+++ b/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc
@@ -11,6 +11,9 @@
 #include "base/metrics/sparse_histogram.h"
 #include "components/data_reduction_proxy/common/data_reduction_proxy_headers.h"
 #include "net/base/net_errors.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_status_code.h"
+#include "net/http/http_util.h"
 #include "net/proxy/proxy_retry_info.h"
 #include "net/proxy/proxy_server.h"
 #include "net/proxy/proxy_service.h"
@@ -72,6 +75,29 @@ void DataReductionProxyUsageStats::RecordDataReductionProxyBypassInfo(
   }
 }
 
+// static
+void DataReductionProxyUsageStats::DetectAndRecordMissingViaHeaderResponseCode(
+      bool is_primary, const net::HttpResponseHeaders* headers) {
+  if (HasDataReductionProxyViaHeader(headers, NULL)) {
+    // The data reduction proxy via header is present, so don't record anything.
+    return;
+  }
+
+  // Record the response code in the same way that net::HttpResponseHeaders
+  // records the response code for the histogram Net.HttpResponseCode.
+  if (is_primary) {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+        "DataReductionProxy.MissingViaHeaderResponseCodePrimary",
+        net::HttpUtil::MapStatusCodeForHistogram(headers->response_code()),
+        net::HttpUtil::GetStatusCodesForHistogram());
+  } else {
+    UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+        "DataReductionProxy.MissingViaHeaderResponseCodeFallback",
+        net::HttpUtil::MapStatusCodeForHistogram(headers->response_code()),
+        net::HttpUtil::GetStatusCodesForHistogram());
+  }
+}
+
 DataReductionProxyUsageStats::DataReductionProxyUsageStats(
     DataReductionProxyParams* params,
     MessageLoopProxy* ui_thread_proxy)
@@ -373,6 +399,32 @@ void DataReductionProxyUsageStats::RecordBypassedBytes(
   }
 }
 
+void DataReductionProxyUsageStats::RecordMissingViaHeaderBytes(
+    URLRequest* request) {
+  // Responses that were served from cache should have been filtered out
+  // already.
+  DCHECK(!request->was_cached());
+
+  if (!data_reduction_proxy_params_->WasDataReductionProxyUsed(request, NULL)
+      || HasDataReductionProxyViaHeader(request->response_headers(), NULL)) {

[mmenke, 2014/09/18 18:39:30]

Searching through the header every time we get a k or two seems needlessly heavy
weight.  Maybe use UserData to associate data-reduction-proxy information with
the request, when headers are first read?

[sclittle, 2014/09/19 01:05:01]

Aren't there a lot of things that search through all the headers multiple times
every time we get a k or two, such as the http caching logic? If searching
through the headers is so slow, then wouldn't it make more sense to make the
optimization in HttpResponseHeaders so that everything gets faster? Since every
via header value corresponds to a proxy or something that the response passed
through, the total number of via header values would typically be very small.

The complication with setting some UserData for the via header presence when the
headers are first read is that we would have to ensure that the via header is
not overridden or removed before the request completes. I don't see any reason
that something might want to remove it, but do you think that any performance
gains of doing this would be worth the additional complexity?

[mmenke, 2014/09/19 01:39:03]

Oh, sorry - was thinking this was being called on every read, not on complete,
which seemed like a ton of work purely for metrics (Just imagine a 4 GB download
in 1000k chunks...Probably wouldn't slow things down noticeably, but still seems
like a ton of completely pointless work).  Since we only do this on complete,
I'm fine with this.

+    // Only track requests that used the data reduction proxy and had responses
+    // that were missing the data reduction proxy via header.
+    return;
+  }
+
+  if (request->GetResponseCode() >= net::HTTP_BAD_REQUEST
+      && request->GetResponseCode() < net::HTTP_INTERNAL_SERVER_ERROR) {

[mmenke, 2014/09/18 18:39:30]

nit:  Not sure if the DRP code uses another style, but the generally preferred
style is the operator on the previous line.

[sclittle, 2014/09/19 01:05:01]

Done.

+    // Track 4xx responses that are missing via headers separately.
+    // separately.
+    UMA_HISTOGRAM_COUNTS("DataReductionProxy.MissingViaHeader4xxResponseBytes",
+                         request->received_response_content_length());
+  } else {
+    UMA_HISTOGRAM_COUNTS(
+        "DataReductionProxy.MissingViaHeaderOtherResponseBytes",
+        request->received_response_content_length());
+  }
+}
+
 }  // namespace data_reduction_proxy