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 e522a95f40892f89b50f81391f6a18320a066225..e9b4e961cf7d99dae784ae76bb40c5682d26d3db 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,11 +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/metrics/histogram_samples.h"
+#include "base/metrics/statistics_recorder.h"
+#include "components/data_reduction_proxy/common/data_reduction_proxy_headers_test_utils.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"
@@ -39,6 +49,38 @@ class DataReductionProxyParamsMock : public DataReductionProxyParams {
   DISALLOW_COPY_AND_ASSIGN(DataReductionProxyParamsMock);
 };
 
+scoped_ptr<base::HistogramSamples> GetHistogramSamples(
+    const char* histogram_name) {

[Alexei Svitkine (slow), 2014/09/23 15:12:13]

Pass these as const std::string&.

[sclittle, 2014/09/23 18:13:41]

Removed functions.

+  base::HistogramBase* histogram =
+      base::StatisticsRecorder::FindHistogram(histogram_name);

[Alexei Svitkine (slow), 2014/09/23 15:12:13]

Out of curiosity, have you looked at the histogram_tester.cc class to check if
it would be a better fit in this case? (FWIW, I'm okay with not changing this
existing code if it doesn't provide substantial benefit, just wondering whether
that class didn't fit your nits or whether you didn't know about it.)

[sclittle, 2014/09/23 18:13:41]

Thanks, I had no idea histogram_tester existed. I've changed these tests to use
it.

+  EXPECT_NE(static_cast<base::HistogramBase*>(NULL), histogram);
+  return histogram->SnapshotSamples().Pass();
+}
+
+scoped_ptr<base::HistogramSamples> GetDeltaHistogramSamples(
+    const base::HistogramSamples& initial_samples, const char* histogram_name) {

[Alexei Svitkine (slow), 2014/09/23 15:12:13]

Nit: 1 param per line if the first param is on a new line in function signature.

[sclittle, 2014/09/23 18:13:41]

Removed functions.

+  scoped_ptr<base::HistogramSamples> delta_samples(
+      GetHistogramSamples(histogram_name));
+  delta_samples->Subtract(initial_samples);
+  return delta_samples.Pass();
+}
+
+void ExpectNoNewSamples(const base::HistogramSamples& initial_samples,
+                        const char* histogram_name) {
+  scoped_ptr<base::HistogramSamples> delta_samples(
+      GetDeltaHistogramSamples(initial_samples, histogram_name));
+  EXPECT_EQ(0, delta_samples->TotalCount());
+}
+
+void ExpectOneNewSample(const base::HistogramSamples& initial_samples,
+                        const char* histogram_name,
+                        base::HistogramBase::Sample sample) {
+  scoped_ptr<base::HistogramSamples> delta_samples(
+      GetDeltaHistogramSamples(initial_samples, histogram_name));
+  EXPECT_EQ(1, delta_samples->TotalCount());
+  EXPECT_EQ(1, delta_samples->GetCount(sample));
+}
+
 }  // namespace
 
 namespace data_reduction_proxy {
@@ -49,7 +91,15 @@ class DataReductionProxyUsageStatsTest : public testing::Test {
       : loop_proxy_(MessageLoopProxy::current().get()),
         context_(true),
         unavailable_(false) {
+    base::StatisticsRecorder::Initialize();
     context_.Init();
+
+    // The |test_job_factory_| takes ownership of the interceptor.
+    test_job_interceptor_ = new net::TestJobInterceptor();
+    DCHECK(test_job_factory_.SetProtocolHandler(url::kHttpScheme,
+                                                test_job_interceptor_));
+    context_.set_job_factory(&test_job_factory_);
+
     mock_url_request_ = context_.CreateRequest(GURL(), net::IDLE, &delegate_,
                                                NULL);
   }
@@ -58,6 +108,28 @@ class DataReductionProxyUsageStatsTest : public testing::Test {
     unavailable_ = unavailable;
   }
 
+  scoped_ptr<URLRequest> CreateURLRequestWithResponseHeaders(
+      const GURL& url, const std::string& raw_response_headers) {

[Alexei Svitkine (slow), 2014/09/23 15:12:13]

Nit: 1 param per line if the first param is on a new line in function signature.

[sclittle, 2014/09/23 18:13:41]

Done.

+    scoped_ptr<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();
+    MessageLoop::current()->RunUntilIdle();
+
+    DCHECK(fake_request->response_headers() != NULL);
+    return fake_request.Pass();
+  }
+
   // Required for MessageLoopProxy::current().
   base::MessageLoopForUI loop_;
   MessageLoopProxy* loop_proxy_;
@@ -67,6 +139,8 @@ class DataReductionProxyUsageStatsTest : public testing::Test {
   TestDelegate delegate_;
   DataReductionProxyParamsMock mock_params_;
   scoped_ptr<URLRequest> mock_url_request_;
+  net::TestJobInterceptor* test_job_interceptor_;
+  net::URLRequestJobFactoryImpl test_job_factory_;
   bool unavailable_;
 };
 
@@ -117,4 +191,235 @@ TEST_F(DataReductionProxyUsageStatsTest, IsDataReductionProxyUnreachable) {
   }
 }
 
+TEST_F(DataReductionProxyUsageStatsTest,
+       DetectAndRecordMissingViaHeaderResponseCode) {
+  const char kPrimaryHistogramName[] =
+      "DataReductionProxy.MissingViaHeaderResponseCodePrimary";
+  const char kFallbackHistogramName[] =
+      "DataReductionProxy.MissingViaHeaderResponseCodeFallback";
+
+  // Log a sample for each histogram, to ensure that they are both created.
+  UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+      kPrimaryHistogramName, net::HttpUtil::MapStatusCodeForHistogram(200),
+      net::HttpUtil::GetStatusCodesForHistogram());
+  UMA_HISTOGRAM_CUSTOM_ENUMERATION(
+      kFallbackHistogramName, net::HttpUtil::MapStatusCodeForHistogram(200),
+      net::HttpUtil::GetStatusCodesForHistogram());
+
+  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) {
+    scoped_ptr<base::HistogramSamples> initial_primary_samples(
+        GetHistogramSamples(kPrimaryHistogramName));
+    scoped_ptr<base::HistogramSamples> initial_fallback_samples(
+        GetHistogramSamples(kFallbackHistogramName));
+
+    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) {
+      ExpectNoNewSamples(*initial_primary_samples, kPrimaryHistogramName);
+    } else {
+      ExpectOneNewSample(*initial_primary_samples, kPrimaryHistogramName,
+                         test_cases[i].expected_primary_sample);
+    }
+
+    if (test_cases[i].expected_fallback_sample == -1) {
+      ExpectNoNewSamples(*initial_fallback_samples, kFallbackHistogramName);
+    } else {
+      ExpectOneNewSample(*initial_fallback_samples, kFallbackHistogramName,
+                         test_cases[i].expected_fallback_sample);
+    }
+  }
+}
+
+TEST_F(DataReductionProxyUsageStatsTest, RecordMissingViaHeaderBytes) {
+  const char k4xxHistogramName[] =
+      "DataReductionProxy.MissingViaHeader4xxResponseBytes";
+  const char kOtherHistogramName[] =
+      "DataReductionProxy.MissingViaHeaderOtherResponseBytes";
+  const int64 kResponseContentLength = 100;
+
+  // Log a sample for each histogram to ensure that they're created.
+  UMA_HISTOGRAM_COUNTS(k4xxHistogramName, 0);
+  UMA_HISTOGRAM_COUNTS(kOtherHistogramName, 0);
+
+  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) {
+    scoped_ptr<base::HistogramSamples> initial_4xx_samples(
+        GetHistogramSamples(k4xxHistogramName));
+    scoped_ptr<base::HistogramSamples> initial_other_samples(
+        GetHistogramSamples(kOtherHistogramName));
+
+    scoped_ptr<DataReductionProxyUsageStats> usage_stats(
+        new DataReductionProxyUsageStats(&mock_params_, loop_proxy_));
+
+    std::string raw_headers(test_cases[i].headers);
+    HeadersToRaw(&raw_headers);
+
+    scoped_ptr<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) {
+      ExpectOneNewSample(*initial_4xx_samples, k4xxHistogramName,
+                         kResponseContentLength);
+    } else {
+      ExpectNoNewSamples(*initial_4xx_samples, k4xxHistogramName);
+    }
+
+    if (test_cases[i].is_other_sample_expected) {
+      ExpectOneNewSample(*initial_other_samples, kOtherHistogramName,
+                         kResponseContentLength);
+    } else {
+      ExpectNoNewSamples(*initial_other_samples, kOtherHistogramName);
+    }
+  }
+}
+
 }  // namespace data_reduction_proxy