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

components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc

Index: components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
diff --git a/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc b/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
index 2ae49869536cc8f22df4b0c6ebd9bad61459c93a..35fbb944635091fd4e2f3e6cb3ffa52087594f84 100644
--- a/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
+++ b/components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats_unittest.cc
@@ -4,12 +4,21 @@
 
 #include "components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.h"
 
+#include <string>
+
 #include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/metrics/histogram.h"
+#include "base/test/histogram_tester.h"
+#include "components/data_reduction_proxy/common/data_reduction_proxy_headers_test_utils.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/request_priority.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
 #include "net/url_request/url_request.h"
+#include "net/url_request/url_request_job_factory_impl.h"
 #include "net/url_request/url_request_status.h"
+#include "net/url_request/url_request_test_job.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -49,6 +58,15 @@ class DataReductionProxyUsageStatsTest : public testing::Test {
         context_(true),
         unavailable_(false) {
     context_.Init();
+
+    // The |test_job_factory_| takes ownership of the interceptor.
+    test_job_interceptor_ = new net::TestJobInterceptor();
+    // This must be a CHECK, and not a DCHECK, so that Valgrind doesn't complain
+    // that the |test_job_interceptor_| is never freed.
+    CHECK(test_job_factory_.SetProtocolHandler(url::kHttpScheme,

[sclittle, 2014/09/24 04:22:58]

This line (and the comment above it) are the only differences between this CL
and https://codereview.chromium.org/577343002/. Changing this from a DCHECK to a
CHECK fixed Valgrind in release builds.

+                                                test_job_interceptor_));

[mmenke, 2014/09/24 15:31:12]

I think it's still a bad idea to put code with side effects in a CHECK statement
as well.  Maybe just use an ASSERT_TRUE, which would also abort the rest of the
test when called in the constructor?  Anyhow, just a suggestion, I'll defer to
you guys on this.

[sclittle, 2014/09/24 17:41:44]

Unfortunately, changing this to ASSERT_TRUE doesn't compile, since the macro
expansion of ASSERT_TRUE includes a return statement of a void expression, which
is not allowed in a constructor.

I've changed it to use an intermediate variable to hold the return value of
SetProtocolHandler, and DCHECK that it's true.

[bengr, 2014/09/24 17:46:47]

You shouldn't be using DCHECK in tests. See:
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

[sclittle, 2014/09/24 18:05:14]

OK, I've changed it to use EXPECT_TRUE.

+    context_.set_job_factory(&test_job_factory_);
+
     mock_url_request_ = context_.CreateRequest(GURL(), net::IDLE, &delegate_,
                                                NULL);
   }
@@ -57,6 +75,28 @@ class DataReductionProxyUsageStatsTest : public testing::Test {
     unavailable_ = unavailable;
   }
 
+  scoped_ptr<net::URLRequest> CreateURLRequestWithResponseHeaders(
+      const GURL& url,
+      const std::string& raw_response_headers) {
+    scoped_ptr<net::URLRequest> fake_request = context_.CreateRequest(
+        url, net::IDLE, &delegate_, NULL);
+
+    // Create a test job that will fill in the given response headers for the
+    // |fake_request|.
+    scoped_refptr<net::URLRequestTestJob> test_job(
+        new net::URLRequestTestJob(fake_request.get(),
+                                   context_.network_delegate(),
+                                   raw_response_headers, std::string(), true));
+
+    // Configure the interceptor to use the test job to handle the next request.
+    test_job_interceptor_->set_main_intercept_job(test_job.get());
+    fake_request->Start();
+    base::MessageLoop::current()->RunUntilIdle();
+
+    DCHECK(fake_request->response_headers() != NULL);
+    return fake_request.Pass();
+  }
+
   // Required for base::MessageLoopProxy::current().
   base::MessageLoopForUI loop_;
   base::MessageLoopProxy* loop_proxy_;
@@ -66,6 +106,9 @@ class DataReductionProxyUsageStatsTest : public testing::Test {
   net::TestDelegate delegate_;
   DataReductionProxyParamsMock mock_params_;
   scoped_ptr<net::URLRequest> mock_url_request_;
+  // |test_job_interceptor_| is owned by |test_job_factory_|.
+  net::TestJobInterceptor* test_job_interceptor_;
+  net::URLRequestJobFactoryImpl test_job_factory_;
   bool unavailable_;
 };
 
@@ -187,4 +230,215 @@ TEST_F(DataReductionProxyUsageStatsTest, ProxyReachableThenUnreachable) {
   EXPECT_TRUE(unavailable_);
 }
 
+TEST_F(DataReductionProxyUsageStatsTest,
+       DetectAndRecordMissingViaHeaderResponseCode) {
+  const std::string kPrimaryHistogramName =
+      "DataReductionProxy.MissingViaHeader.ResponseCode.Primary";
+  const std::string kFallbackHistogramName =
+      "DataReductionProxy.MissingViaHeader.ResponseCode.Fallback";
+
+  struct TestCase {
+    bool is_primary;
+    const char* headers;
+    int expected_primary_sample;   // -1 indicates no expected sample.
+    int expected_fallback_sample;  // -1 indicates no expected sample.
+  };
+  const TestCase test_cases[] = {
+    {
+      true,
+      "HTTP/1.1 200 OK\n"
+      "Via: 1.1 Chrome-Compression-Proxy\n",
+      -1,
+      -1
+    },
+    {
+      false,
+      "HTTP/1.1 200 OK\n"
+      "Via: 1.1 Chrome-Compression-Proxy\n",
+      -1,
+      -1
+    },
+    {
+      true,
+      "HTTP/1.1 200 OK\n",
+      200,
+      -1
+    },
+    {
+      false,
+      "HTTP/1.1 200 OK\n",
+      -1,
+      200
+    },
+    {
+      true,
+      "HTTP/1.1 304 Not Modified\n",
+      304,
+      -1
+    },
+    {
+      false,
+      "HTTP/1.1 304 Not Modified\n",
+      -1,
+      304
+    },
+    {
+      true,
+      "HTTP/1.1 404 Not Found\n",
+      404,
+      -1
+    },
+    {
+      false,
+      "HTTP/1.1 404 Not Found\n",
+      -1,
+      404
+    }
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_cases); ++i) {
+    base::HistogramTester histogram_tester;
+    std::string raw_headers(test_cases[i].headers);
+    HeadersToRaw(&raw_headers);
+    scoped_refptr<net::HttpResponseHeaders> headers(
+        new net::HttpResponseHeaders(raw_headers));
+
+    DataReductionProxyUsageStats::DetectAndRecordMissingViaHeaderResponseCode(
+        test_cases[i].is_primary, headers.get());
+
+    if (test_cases[i].expected_primary_sample == -1) {
+      histogram_tester.ExpectTotalCount(kPrimaryHistogramName, 0);
+    } else {
+      histogram_tester.ExpectUniqueSample(
+          kPrimaryHistogramName, test_cases[i].expected_primary_sample, 1);
+    }
+
+    if (test_cases[i].expected_fallback_sample == -1) {
+      histogram_tester.ExpectTotalCount(kFallbackHistogramName, 0);
+    } else {
+      histogram_tester.ExpectUniqueSample(
+          kFallbackHistogramName, test_cases[i].expected_fallback_sample, 1);
+    }
+  }
+}
+
+TEST_F(DataReductionProxyUsageStatsTest, RecordMissingViaHeaderBytes) {
+  const std::string k4xxHistogramName =
+      "DataReductionProxy.MissingViaHeader.Bytes.4xx";
+  const std::string kOtherHistogramName =
+      "DataReductionProxy.MissingViaHeader.Bytes.Other";
+  const int64 kResponseContentLength = 100;
+
+  struct TestCase {
+    bool was_proxy_used;
+    const char* headers;
+    bool is_4xx_sample_expected;
+    bool is_other_sample_expected;
+  };
+  const TestCase test_cases[] = {
+    // Nothing should be recorded for requests that don't use the proxy.
+    {
+      false,
+      "HTTP/1.1 404 Not Found\n",
+      false,
+      false
+    },
+    {
+      false,
+      "HTTP/1.1 200 OK\n",
+      false,
+      false
+    },
+    // Nothing should be recorded for responses that have the via header.
+    {
+      true,
+      "HTTP/1.1 404 Not Found\n"
+      "Via: 1.1 Chrome-Compression-Proxy\n",
+      false,
+      false
+    },
+    {
+      true,
+      "HTTP/1.1 200 OK\n"
+      "Via: 1.1 Chrome-Compression-Proxy\n",
+      false,
+      false
+    },
+    // 4xx responses that used the proxy and don't have the via header should be
+    // recorded.
+    {
+      true,
+      "HTTP/1.1 404 Not Found\n",
+      true,
+      false
+    },
+    {
+      true,
+      "HTTP/1.1 400 Bad Request\n",
+      true,
+      false
+    },
+    {
+      true,
+      "HTTP/1.1 499 Big Client Error Response Code\n",
+      true,
+      false
+    },
+    // Non-4xx responses that used the proxy and don't have the via header
+    // should be recorded.
+    {
+      true,
+      "HTTP/1.1 200 OK\n",
+      false,
+      true
+    },
+    {
+      true,
+      "HTTP/1.1 399 Big Redirection Response Code\n",
+      false,
+      true
+    },
+    {
+      true,
+      "HTTP/1.1 500 Internal Server Error\n",
+      false,
+      true
+    }
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_cases); ++i) {
+    base::HistogramTester histogram_tester;
+    scoped_ptr<DataReductionProxyUsageStats> usage_stats(
+        new DataReductionProxyUsageStats(&mock_params_, loop_proxy_));
+
+    std::string raw_headers(test_cases[i].headers);
+    HeadersToRaw(&raw_headers);
+
+    scoped_ptr<net::URLRequest> fake_request(
+        CreateURLRequestWithResponseHeaders(GURL("http://www.google.com/"),
+                                            raw_headers));
+    fake_request->set_received_response_content_length(kResponseContentLength);
+
+    EXPECT_CALL(mock_params_,
+                WasDataReductionProxyUsed(fake_request.get(), NULL))
+        .WillRepeatedly(Return(test_cases[i].was_proxy_used));
+
+    usage_stats->RecordMissingViaHeaderBytes(fake_request.get());
+
+    if (test_cases[i].is_4xx_sample_expected) {
+      histogram_tester.ExpectUniqueSample(k4xxHistogramName,
+                                          kResponseContentLength, 1);
+    } else {
+      histogram_tester.ExpectTotalCount(k4xxHistogramName, 0);
+    }
+
+    if (test_cases[i].is_other_sample_expected) {
+      histogram_tester.ExpectUniqueSample(kOtherHistogramName,
+                                          kResponseContentLength, 1);
+    } else {
+      histogram_tester.ExpectTotalCount(kOtherHistogramName, 0);
+    }
+  }
+}
+
 }  // namespace data_reduction_proxy