Record bypass stats metric for network-servicification

This CL moves some common functions to core/common so that some bypass
stats is recorded for DRP servicification. No functional changes in this
CL other than that more metrics are recorded for DRP servicification.

Change-Id: If11df4bf59e0ebb2048c6c2f426cbe7c2ab7afc0

Bug: 900717
Change-Id: If11df4bf59e0ebb2048c6c2f426cbe7c2ab7afc0
Reviewed-on: https://chromium-review.googlesource.com/c/1433143
Commit-Queue: rajendrant <rajendrant@chromium.org>
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625832}

chrome/browser/data_reduction_proxy/data_reduction_proxy_browsertest.cc

@@ -435,11 +435,15 @@ class DataReductionProxyFallbackBrowsertest
 
 IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
                        FallbackProxyUsedOn500Status) {
+  base::HistogramTester histogram_tester;
   // Should fall back to the secondary proxy if a 500 error occurs.
   SetStatusCode(net::HTTP_INTERNAL_SERVER_ERROR);
   ui_test_utils::NavigateToURL(
       browser(), GURL("http://does.not.resolve/echoheader?Chrome-Proxy"));
   EXPECT_THAT(GetBody(), kSecondaryResponse);
+  histogram_tester.ExpectUniqueSample(
+      "DataReductionProxy.BypassTypePrimary",
+      BYPASS_EVENT_TYPE_STATUS_500_HTTP_INTERNAL_SERVER_ERROR, 1);
 
   // Bad proxy should still be bypassed.
   SetStatusCode(net::HTTP_OK);
@@ -450,11 +454,14 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
 
 IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
                        FallbackProxyUsedWhenBypassHeaderSent) {
+  base::HistogramTester histogram_tester;
   // Should fall back to the secondary proxy if the bypass header is set.
   SetHeader("bypass=100");
   ui_test_utils::NavigateToURL(
       browser(), GURL("http://does.not.resolve/echoheader?Chrome-Proxy"));
   EXPECT_THAT(GetBody(), kSecondaryResponse);
+  histogram_tester.ExpectUniqueSample("DataReductionProxy.BypassTypePrimary",
+                                      BYPASS_EVENT_TYPE_MEDIUM, 1);
 
   // Bad proxy should still be bypassed.
   SetHeader("");
@@ -465,10 +472,13 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
 
 IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
                        BadProxiesResetWhenDisabled) {
+  base::HistogramTester histogram_tester;
   SetHeader("bypass=100");
   ui_test_utils::NavigateToURL(
       browser(), GURL("http://does.not.resolve/echoheader?Chrome-Proxy"));
   EXPECT_THAT(GetBody(), kSecondaryResponse);
+  histogram_tester.ExpectUniqueSample("DataReductionProxy.BypassTypePrimary",
+                                      BYPASS_EVENT_TYPE_MEDIUM, 1);
 
   // Disabling and enabling DRP should clear the bypass.
   EnableDataSaver(false);
@@ -482,6 +492,7 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
 
 IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
                        NoProxyUsedWhenBlockOnceHeaderSent) {
+  base::HistogramTester histogram_tester;
   net::EmbeddedTestServer test_server;
   test_server.RegisterRequestHandler(
       base::BindRepeating(&BasicResponse, kDummyBody));
@@ -492,6 +503,9 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
   ui_test_utils::NavigateToURL(browser(),
                                GetURLWithMockHost(test_server, "/echo"));
   EXPECT_THAT(GetBody(), kDummyBody);
+  EXPECT_LE(
+      1, histogram_tester.GetBucketCount("DataReductionProxy.BlockTypePrimary",
+                                         BYPASS_EVENT_TYPE_CURRENT));
 
   // Proxy should no longer be blocked, and use first proxy.
   SetHeader("");
@@ -502,6 +516,7 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
 
 IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
                        FallbackProxyUsedWhenBlockHeaderSent) {
+  base::HistogramTester histogram_tester;
   net::EmbeddedTestServer test_server;
   test_server.RegisterRequestHandler(
       base::BindRepeating(&BasicResponse, kDummyBody));
@@ -512,6 +527,8 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
   ui_test_utils::NavigateToURL(browser(),
                                GetURLWithMockHost(test_server, "/echo"));
   EXPECT_THAT(GetBody(), kDummyBody);
+  histogram_tester.ExpectUniqueSample("DataReductionProxy.BlockTypePrimary",
+                                      BYPASS_EVENT_TYPE_MEDIUM, 1);
 
   // Request should still not use proxy.
   SetHeader("");
@@ -522,6 +539,7 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
 
 IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
                        FallbackProxyUsedWhenBlockZeroHeaderSent) {
+  base::HistogramTester histogram_tester;
   net::EmbeddedTestServer test_server;
   test_server.RegisterRequestHandler(
       base::BindRepeating(&BasicResponse, kDummyBody));
@@ -533,6 +551,8 @@ IN_PROC_BROWSER_TEST_F(DataReductionProxyFallbackBrowsertest,
   ui_test_utils::NavigateToURL(browser(),
                                GetURLWithMockHost(test_server, "/echo"));
   EXPECT_THAT(GetBody(), kDummyBody);
+  histogram_tester.ExpectUniqueSample("DataReductionProxy.BlockTypePrimary",
+                                      BYPASS_EVENT_TYPE_MEDIUM, 1);
 
   // Request should still not use proxy.
   SetHeader("");

components/data_reduction_proxy/content/common/data_reduction_proxy_url_loader_throttle.cc

@@ -94,10 +94,7 @@ void DataReductionProxyURLLoaderThrottle::BeforeWillProcessResponse(
 
   DataReductionProxyBypassType bypass_type = BYPASS_EVENT_TYPE_MAX;
 
-  // TODO(https://crbug.com/721403): Not logging stats.
-  DataReductionProxyBypassProtocol::Stats* stats = nullptr;
-
-  DataReductionProxyBypassProtocol protocol(stats);
+  DataReductionProxyBypassProtocol protocol;
   pending_restart_ = protocol.MaybeBypassProxyAndPrepareToRetry(
       request_method_, url_chain_, response_head.headers.get(),
       response_head.proxy_server, net_error, proxy_retry_info,

components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc

@@ -9,6 +9,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_util.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_bypass_protocol.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_type_info.h"
@@ -47,51 +48,6 @@ void RecordDataReductionProxyBypassOnNetworkError(
 
 }  // namespace
 
-// static
-void DataReductionProxyBypassStats::RecordDataReductionProxyBypassInfo(
-    bool is_primary,
-    bool bypass_all,
-    const net::ProxyServer& proxy_server,
-    DataReductionProxyBypassType bypass_type) {
-  if (bypass_all) {
-    if (is_primary) {
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.BlockTypePrimary",
-                                bypass_type, BYPASS_EVENT_TYPE_MAX);
-    } else {
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.BlockTypeFallback",
-                                bypass_type, BYPASS_EVENT_TYPE_MAX);
-    }
-  } else {
-    if (is_primary) {
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.BypassTypePrimary",
-                                bypass_type, BYPASS_EVENT_TYPE_MAX);
-    } else {
-      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.BypassTypeFallback",
-                                bypass_type, BYPASS_EVENT_TYPE_MAX);
-    }
-  }
-}
-
-// static
-void DataReductionProxyBypassStats::DetectAndRecordMissingViaHeaderResponseCode(
-    bool is_primary,
-    const net::HttpResponseHeaders& headers) {
-  if (HasDataReductionProxyViaHeader(headers, nullptr)) {
-    // The data reduction proxy via header is present, so don't record anything.
-    return;
-  }
-
-  if (is_primary) {
-    base::UmaHistogramSparse(
-        "DataReductionProxy.MissingViaHeader.ResponseCode.Primary",
-        headers.response_code());
-  } else {
-    base::UmaHistogramSparse(
-        "DataReductionProxy.MissingViaHeader.ResponseCode.Fallback",
-        headers.response_code());
-  }
-}
-
 DataReductionProxyBypassStats::DataReductionProxyBypassStats(
     DataReductionProxyConfig* config,
     UnreachableCallback unreachable_callback,
@@ -184,7 +140,7 @@ void DataReductionProxyBypassStats::OnProxyFallback(
   }
 
   const bool is_first_proxy = proxy_type_info->proxy_index == 0U;
-  RecordDataReductionProxyBypassInfo(is_first_proxy, false, bypassed_proxy,
+  RecordDataReductionProxyBypassInfo(is_first_proxy, false,
                                      BYPASS_EVENT_TYPE_NETWORK_ERROR);
   RecordDataReductionProxyBypassOnNetworkError(is_first_proxy, bypassed_proxy,
                                                net_error);

components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.h

@@ -17,7 +17,6 @@
 #include "services/network/public/cpp/network_connection_tracker.h"
 
 namespace net {
-class HttpResponseHeaders;
 class ProxyConfig;
 class ProxyServer;
 }
@@ -31,22 +30,6 @@ class DataReductionProxyBypassStats
  public:
   typedef base::Callback<void(bool /* unreachable */)> UnreachableCallback;
 
-  // Records a data reduction proxy bypass event as a "BlockType" if
-  // |bypass_all| is true and as a "BypassType" otherwise. Records the event as
-  // "Primary" if |is_primary| is true and "Fallback" otherwise.
-  static void RecordDataReductionProxyBypassInfo(
-      bool is_primary,
-      bool bypass_all,
-      const net::ProxyServer& proxy_server,
-      DataReductionProxyBypassType bypass_type);
-
-  // For the given response |headers| that are expected to include the data
-  // reduction proxy via header, records response code UMA if the data reduction
-  // proxy via header is not present.
-  static void DetectAndRecordMissingViaHeaderResponseCode(
-      bool is_primary,
-      const net::HttpResponseHeaders& headers);
-
   // |config| outlives this class instance. |unreachable_callback| provides a
   // hook to inform the user that the Data Reduction Proxy is unreachable.
   // |config| must not be null.

components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc

@@ -25,6 +25,7 @@
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_prefs.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_bypass_protocol.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers_test_utils.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h"
@@ -762,8 +763,8 @@ TEST_F(DataReductionProxyBypassStatsEndToEndTest,
     scoped_refptr<net::HttpResponseHeaders> headers(
         new net::HttpResponseHeaders(raw_headers));
 
-    DataReductionProxyBypassStats::DetectAndRecordMissingViaHeaderResponseCode(
-        test_cases[i].is_primary, *headers);
+    DetectAndRecordMissingViaHeaderResponseCode(test_cases[i].is_primary,
+                                                *headers);
 
     if (test_cases[i].expected_primary_sample == -1) {
       histogram_tester.ExpectTotalCount(kPrimaryHistogramName, 0);

components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.cc

@@ -40,27 +40,6 @@ void MarkProxiesAsBad(net::URLRequest* request,
       proxy_info, bypass_duration, bad_proxies, request->net_log());
 }
 
-class BypassStats : public DataReductionProxyBypassProtocol::Stats {
-  // TODO(http://crbug.com/721403): This interface only exists as an
-  // intermediary step to avoid depending on data_reduction_proxy/core/browser.
-  // The correct dependency is DataReductionProxyBypassStats.
-  void RecordDataReductionProxyBypassInfo(
-      bool is_primary,
-      bool bypass_all,
-      const net::ProxyServer& proxy_server,
-      DataReductionProxyBypassType bypass_type) override {
-    DataReductionProxyBypassStats::RecordDataReductionProxyBypassInfo(
-        is_primary, bypass_all, proxy_server, bypass_type);
-  }
-
-  void DetectAndRecordMissingViaHeaderResponseCode(
-      bool is_primary,
-      const net::HttpResponseHeaders& headers) override {
-    DataReductionProxyBypassStats::DetectAndRecordMissingViaHeaderResponseCode(
-        is_primary, headers);
-  }
-};
-
 }  // namespace
 
 DataReductionProxyInterceptor::DataReductionProxyInterceptor(
@@ -142,8 +121,7 @@ DataReductionProxyInterceptor::MaybeInterceptResponseOrRedirect(
     std::vector<net::ProxyServer> bad_proxies;
     int additional_load_flags_for_restart = 0;
 
-    BypassStats stats;
-    DataReductionProxyBypassProtocol protocol(&stats);
+    DataReductionProxyBypassProtocol protocol;
 
     should_retry = protocol.MaybeBypassProxyAndPrepareToRetry(
         request->method(), request->url_chain(), request->response_headers(),

components/data_reduction_proxy/core/common/data_reduction_proxy_bypass_protocol.cc

@@ -60,10 +60,49 @@ void ReportResponseProxyServerStatusHistogram(
 
 }  // namespace
 
-DataReductionProxyBypassProtocol::DataReductionProxyBypassProtocol(Stats* stats)
-    : stats_(stats) {}
+void RecordDataReductionProxyBypassInfo(
+    bool is_primary,
+    bool bypass_all,
+    DataReductionProxyBypassType bypass_type) {
+  if (bypass_all) {
+    if (is_primary) {
+      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.BlockTypePrimary",
+                                bypass_type, BYPASS_EVENT_TYPE_MAX);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.BlockTypeFallback",
+                                bypass_type, BYPASS_EVENT_TYPE_MAX);
+    }
+  } else {
+    if (is_primary) {
+      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.BypassTypePrimary",
+                                bypass_type, BYPASS_EVENT_TYPE_MAX);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.BypassTypeFallback",
+                                bypass_type, BYPASS_EVENT_TYPE_MAX);
+    }
+  }
+}
 
-DataReductionProxyBypassProtocol::Stats::~Stats() = default;
+void DetectAndRecordMissingViaHeaderResponseCode(
+    bool is_primary,
+    const net::HttpResponseHeaders& headers) {
+  if (HasDataReductionProxyViaHeader(headers, nullptr)) {
+    // The data reduction proxy via header is present, so don't record anything.
+    return;
+  }
+
+  if (is_primary) {
+    base::UmaHistogramSparse(
+        "DataReductionProxy.MissingViaHeader.ResponseCode.Primary",
+        headers.response_code());
+  } else {
+    base::UmaHistogramSparse(
+        "DataReductionProxy.MissingViaHeader.ResponseCode.Fallback",
+        headers.response_code());
+  }
+}
+
+DataReductionProxyBypassProtocol::DataReductionProxyBypassProtocol() = default;
 
 bool DataReductionProxyBypassProtocol::MaybeBypassProxyAndPrepareToRetry(
     const std::string& method,
@@ -207,10 +246,8 @@ bool DataReductionProxyBypassProtocol::HandleValidResponseHeadersCase(
 
   // At this point, the response is expected to have the data reduction proxy
   // via header, so detect and report cases where the via header is missing.
-  if (stats_) {
-    stats_->DetectAndRecordMissingViaHeaderResponseCode(
-        data_reduction_proxy_type_info.proxy_index == 0U, *response_headers);
-  }
+  DetectAndRecordMissingViaHeaderResponseCode(
+      data_reduction_proxy_type_info.proxy_index == 0U, *response_headers);
 
   // GetDataReductionProxyBypassType will only log a net_log event if a bypass
   // command was sent via the data reduction proxy headers.
@@ -231,11 +268,11 @@ bool DataReductionProxyBypassProtocol::HandleValidResponseHeadersCase(
           .proxy_server();
 
   // Only record UMA if the proxy isn't already on the retry list.
-  if (stats_ && !IsProxyBypassedAtTime(proxy_retry_info, proxy_server,
-                                       base::TimeTicks::Now(), nullptr)) {
-    stats_->RecordDataReductionProxyBypassInfo(
+  if (!IsProxyBypassedAtTime(proxy_retry_info, proxy_server,
+                             base::TimeTicks::Now(), nullptr)) {
+    RecordDataReductionProxyBypassInfo(
         data_reduction_proxy_type_info.proxy_index == 0U,
-        data_reduction_proxy_info->bypass_all, proxy_server, *bypass_type);
+        data_reduction_proxy_info->bypass_all, *bypass_type);
   }
   return true;
 }

components/data_reduction_proxy/core/common/data_reduction_proxy_bypass_protocol.h

@@ -15,29 +15,26 @@ namespace data_reduction_proxy {
 
 struct DataReductionProxyTypeInfo;
 
+// Records a data reduction proxy bypass event as a "BlockType" if
+// |bypass_all| is true and as a "BypassType" otherwise. Records the event as
+// "Primary" if |is_primary| is true and "Fallback" otherwise.
+void RecordDataReductionProxyBypassInfo(
+    bool is_primary,
+    bool bypass_all,
+    DataReductionProxyBypassType bypass_type);
+
+// For the given response |headers| that are expected to include the data
+// reduction proxy via header, records response code UMA if the data reduction
+// proxy via header is not present.
+void DetectAndRecordMissingViaHeaderResponseCode(
+    bool is_primary,
+    const net::HttpResponseHeaders& headers);
+
 // Class responsible for determining when a response should or should not cause
 // the data reduction proxy to be bypassed, and to what degree. Owned by the
 // DataReductionProxyInterceptor.
 class DataReductionProxyBypassProtocol {
  public:
-  // TODO(http://crbug.com/721403): This interface only exists as an
-  // intermediary step to avoid depending on data_reduction_proxy/core/browser.
-  // The correct dependency is DataReductionProxyBypassStats.
-  class Stats {
-   public:
-    virtual ~Stats();
-
-    virtual void RecordDataReductionProxyBypassInfo(
-        bool is_primary,
-        bool bypass_all,
-        const net::ProxyServer& proxy_server,
-        DataReductionProxyBypassType bypass_type) = 0;
-
-    virtual void DetectAndRecordMissingViaHeaderResponseCode(
-        bool is_primary,
-        const net::HttpResponseHeaders& headers) = 0;
-  };
-
   // Enum values that can be reported for the
   // DataReductionProxy.ResponseProxyServerStatus histogram. These values must
   // be kept in sync with their counterparts in histograms.xml. Visible here for
@@ -50,7 +47,7 @@ class DataReductionProxyBypassProtocol {
     RESPONSE_PROXY_SERVER_STATUS_MAX
   };
 
-  explicit DataReductionProxyBypassProtocol(Stats* stats);
+  DataReductionProxyBypassProtocol();
 
   // Decides whether to restart the request, whether to bypass proxies when
   // doing so, and whether to mark any data reduction proxies as bad based on
@@ -98,9 +95,6 @@ class DataReductionProxyBypassProtocol {
       DataReductionProxyInfo* data_reduction_proxy_info,
       DataReductionProxyBypassType* bypass_type);
 
-  // Must outlive |this|.
-  Stats* stats_;
-
   base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(DataReductionProxyBypassProtocol);