Log DRP UMAs for successful requests in network service

This fixes the last DRP browser test. Factors the code to a common
location so it can be used in the DRP throttle.

Bug: 721403
Change-Id: I47330f7e9cad75861787caa843d55147a42fb8a4
Reviewed-on: https://chromium-review.googlesource.com/c/1354653Reviewed-by: rajendrant <rajendrant@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612312}

chrome/browser/net/spdyproxy/data_reduction_proxy_browsertest.cc

@@ -22,6 +22,7 @@
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
+#include "components/data_reduction_proxy/core/common/uma_util.h"
 #include "components/data_reduction_proxy/proto/client_config.pb.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/network_service_instance.h"

components/data_reduction_proxy/content/common/data_reduction_proxy_url_loader_throttle.cc

@@ -10,6 +10,8 @@
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_server.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_throttle_manager.h"
+#include "components/data_reduction_proxy/core/common/uma_util.h"
+#include "net/base/load_flags.h"
 
 namespace net {
 class HttpRequestHeaders;
@@ -34,6 +36,8 @@ void DataReductionProxyURLLoaderThrottle::WillStartRequest(
   url_chain_.clear();
   url_chain_.push_back(request->url);
   request_method_ = request->method;
+  is_main_frame_ = request->resource_type == content::RESOURCE_TYPE_MAIN_FRAME;
+  final_load_flags_ = request->load_flags;
 
   MaybeSetAcceptTransformHeader(
       request->url, static_cast<content::ResourceType>(request->resource_type),
@@ -115,6 +119,19 @@ void DataReductionProxyURLLoaderThrottle::BeforeWillProcessResponse(
   }
 }
 
+void DataReductionProxyURLLoaderThrottle::WillProcessResponse(
+    const GURL& response_url,
+    network::ResourceResponseHead* response_head,
+    bool* defer) {
+  base::Optional<DataReductionProxyTypeInfo> proxy_info =
+      manager_->FindConfiguredDataReductionProxy(response_head->proxy_server);
+  if (!proxy_info || (final_load_flags_ & net::LOAD_BYPASS_PROXY) != 0)
+    return;
+
+  LogSuccessfulProxyUMAs(proxy_info.value(), response_head->proxy_server,
+                         is_main_frame_);
+}
+
 void DataReductionProxyURLLoaderThrottle::MarkProxiesAsBad(
     const std::vector<net::ProxyServer>& bad_proxies,
     base::TimeDelta bypass_duration) {
@@ -156,6 +173,7 @@ void DataReductionProxyURLLoaderThrottle::DoPendingRestart() {
 
   pending_restart_ = false;
   pending_restart_load_flags_ = 0;
+  final_load_flags_ |= load_flags;
 
   delegate_->RestartWithFlags(load_flags);
 }

components/data_reduction_proxy/content/common/data_reduction_proxy_url_loader_throttle.h

@@ -42,6 +42,9 @@ class DataReductionProxyURLLoaderThrottle : public content::URLLoaderThrottle {
       const GURL& response_url,
       const network::ResourceResponseHead& response_head,
       bool* defer) override;
+  void WillProcessResponse(const GURL& response_url,
+                           network::ResourceResponseHead* response_head,
+                           bool* defer) override;
 
  private:
   // Marks |bad_proxies| to be bypassed for |bypass_duration|. Once that action
@@ -69,6 +72,12 @@ class DataReductionProxyURLLoaderThrottle : public content::URLLoaderThrottle {
 
   // Set to true while waiting for OnMarkProxiesAsBadComplete to run.
   bool waiting_for_mark_proxies_ = false;
+
+  // Whether this throttle is intercepting a main frame request.
+  bool is_main_frame_ = false;
+
+  // The final load flags used to complete the request.
+  int final_load_flags_ = 0;
 };
 
 }  // namespace data_reduction_proxy

components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats.cc

@@ -12,6 +12,7 @@
 #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"
+#include "components/data_reduction_proxy/core/common/uma_util.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/proxy_server.h"
@@ -138,23 +139,9 @@ void DataReductionProxyBypassStats::OnUrlRequestCompleted(
   successful_requests_through_proxy_count_++;
   NotifyUnavailabilityIfChanged();
 
-  // Report the success counts.
-  UMA_HISTOGRAM_COUNTS_100(
-      "DataReductionProxy.SuccessfulRequestCompletionCounts",
-      proxy_info->proxy_index);
-
-  // It is possible that the scheme of request->proxy_server() is different
-  // from the scheme of proxy_info.proxy_servers.front(). The former may be set
-  // to QUIC by the network stack, while the latter may be set to HTTPS.
-  UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.ProxySchemeUsed",
-                            util::ConvertNetProxySchemeToProxyScheme(
-                                request->proxy_server().scheme()),
-                            PROXY_SCHEME_MAX);
-  if (request->load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED) {
-    UMA_HISTOGRAM_COUNTS_100(
-        "DataReductionProxy.SuccessfulRequestCompletionCounts.MainFrame",
-        proxy_info->proxy_index);
-  }
+  LogSuccessfulProxyUMAs(
+      proxy_info.value(), request->proxy_server(),
+      request->load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED);
 }
 
 void DataReductionProxyBypassStats::SetBypassType(

components/data_reduction_proxy/core/browser/data_reduction_proxy_util.cc

@@ -271,23 +271,6 @@ int64_t EstimateOriginalReceivedBytes(const net::URLRequest& request,
          EstimateOriginalBodySize(request, lofi_decider);
 }
 
-ProxyScheme ConvertNetProxySchemeToProxyScheme(
-    net::ProxyServer::Scheme scheme) {
-  switch (scheme) {
-    case net::ProxyServer::SCHEME_HTTP:
-      return PROXY_SCHEME_HTTP;
-    case net::ProxyServer::SCHEME_HTTPS:
-      return PROXY_SCHEME_HTTPS;
-    case net::ProxyServer::SCHEME_QUIC:
-      return PROXY_SCHEME_QUIC;
-    case net::ProxyServer::SCHEME_DIRECT:
-      return PROXY_SCHEME_DIRECT;
-    default:
-      NOTREACHED() << scheme;
-      return PROXY_SCHEME_UNKNOWN;
-  }
-}
-
 const char* GetSiteBreakdownOtherHostName() {
   return kOtherHostName;
 }

components/data_reduction_proxy/core/browser/data_reduction_proxy_util.h

@@ -48,16 +48,6 @@ enum class Client {
   CHROME_QNX,
 };
 
-// Scheme of the proxy used.
-enum ProxyScheme {
-  PROXY_SCHEME_UNKNOWN = 0,
-  PROXY_SCHEME_HTTP,
-  PROXY_SCHEME_HTTPS,
-  PROXY_SCHEME_QUIC,
-  PROXY_SCHEME_DIRECT,
-  PROXY_SCHEME_MAX
-};
-
 namespace util {
 
 // Returns the version of Chromium that is being used, e.g. "1.2.3.4".
@@ -119,9 +109,6 @@ int64_t EstimateOriginalBodySize(const net::URLRequest& request,
 int64_t EstimateOriginalReceivedBytes(const net::URLRequest& request,
                                       const LoFiDecider* lofi_decider);
 
-// Converts net::ProxyServer::Scheme to type ProxyScheme.
-ProxyScheme ConvertNetProxySchemeToProxyScheme(net::ProxyServer::Scheme scheme);
-
 // Returns the hostname used for the other bucket to record datause not scoped
 // to a page load such as chrome-services traffic, service worker, Downloads.
 const char* GetSiteBreakdownOtherHostName();

components/data_reduction_proxy/core/browser/warmup_url_fetcher.cc

@@ -14,6 +14,7 @@
 #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_server.h"
+#include "components/data_reduction_proxy/core/common/uma_util.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "content/public/browser/network_service_instance.h"
 #include "net/base/load_flags.h"
@@ -246,7 +247,7 @@ void WarmupURLFetcher::OnURLLoadComplete(
                                        nullptr /* has_intermediary */));
     UMA_HISTOGRAM_ENUMERATION(
         "DataReductionProxy.WarmupURL.ProxySchemeUsed",
-        util::ConvertNetProxySchemeToProxyScheme(proxy_server_.scheme()),
+        ConvertNetProxySchemeToProxyScheme(proxy_server_.scheme()),
         PROXY_SCHEME_MAX);
   }
 

components/data_reduction_proxy/core/browser/warmup_url_fetcher_unittest.cc

@@ -21,6 +21,7 @@
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_features.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_server.h"
+#include "components/data_reduction_proxy/core/common/uma_util.h"
 #include "net/base/proxy_server.h"
 #include "net/http/http_status_code.h"
 #include "net/socket/socket_test_util.h"
@@ -214,8 +215,7 @@ TEST(WarmupURLFetcherTest, TestSuccessfulFetchWarmupURLNoViaHeader) {
       "DataReductionProxy.WarmupURL.HasViaHeader", 0, 1);
   histogram_tester.ExpectUniqueSample(
       "DataReductionProxy.WarmupURL.ProxySchemeUsed",
-      util::ConvertNetProxySchemeToProxyScheme(net::ProxyServer::SCHEME_DIRECT),
-      1);
+      ConvertNetProxySchemeToProxyScheme(net::ProxyServer::SCHEME_DIRECT), 1);
 
   EXPECT_EQ(1u, warmup_url_fetcher.callback_received_count());
   EXPECT_EQ(net::ProxyServer::SCHEME_DIRECT,
@@ -268,8 +268,7 @@ TEST(WarmupURLFetcherTest, TestSuccessfulFetchWarmupURLWithViaHeader) {
       "DataReductionProxy.WarmupURL.HasViaHeader", 1, 1);
   histogram_tester.ExpectUniqueSample(
       "DataReductionProxy.WarmupURL.ProxySchemeUsed",
-      util::ConvertNetProxySchemeToProxyScheme(net::ProxyServer::SCHEME_DIRECT),
-      1);
+      ConvertNetProxySchemeToProxyScheme(net::ProxyServer::SCHEME_DIRECT), 1);
 
   EXPECT_EQ(1u, warmup_url_fetcher.callback_received_count());
   EXPECT_EQ(net::ProxyServer::SCHEME_DIRECT,
@@ -321,8 +320,7 @@ TEST(WarmupURLFetcherTest,
       "DataReductionProxy.WarmupURL.HasViaHeader", 1, 1);
   histogram_tester.ExpectUniqueSample(
       "DataReductionProxy.WarmupURL.ProxySchemeUsed",
-      util::ConvertNetProxySchemeToProxyScheme(net::ProxyServer::SCHEME_DIRECT),
-      1);
+      ConvertNetProxySchemeToProxyScheme(net::ProxyServer::SCHEME_DIRECT), 1);
 
   // The callback should be run.
   EXPECT_EQ(1u, warmup_url_fetcher.callback_received_count());
@@ -458,8 +456,7 @@ TEST(WarmupURLFetcherTest, TestSuccessfulFetchWarmupURLWithDelay) {
       "DataReductionProxy.WarmupURL.HasViaHeader", 1, 1);
   histogram_tester.ExpectUniqueSample(
       "DataReductionProxy.WarmupURL.ProxySchemeUsed",
-      util::ConvertNetProxySchemeToProxyScheme(net::ProxyServer::SCHEME_DIRECT),
-      1);
+      ConvertNetProxySchemeToProxyScheme(net::ProxyServer::SCHEME_DIRECT), 1);
 
   EXPECT_EQ(1u, warmup_url_fetcher.callback_received_count());
   EXPECT_EQ(net::ProxyServer::SCHEME_DIRECT,

components/data_reduction_proxy/core/common/BUILD.gn

@@ -35,6 +35,8 @@ template("common_tmpl") {
       "lofi_decider.h",
       "lofi_ui_service.h",
       "resource_type_provider.h",
+      "uma_util.cc",
+      "uma_util.h",
     ]
 
     public_deps = [

components/data_reduction_proxy/core/common/uma_util.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/data_reduction_proxy/core/common/uma_util.h"
+
+#include "base/metrics/histogram_macros.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_type_info.h"
+
+namespace data_reduction_proxy {
+
+ProxyScheme ConvertNetProxySchemeToProxyScheme(
+    net::ProxyServer::Scheme scheme) {
+  switch (scheme) {
+    case net::ProxyServer::SCHEME_HTTP:
+      return PROXY_SCHEME_HTTP;
+    case net::ProxyServer::SCHEME_HTTPS:
+      return PROXY_SCHEME_HTTPS;
+    case net::ProxyServer::SCHEME_QUIC:
+      return PROXY_SCHEME_QUIC;
+    case net::ProxyServer::SCHEME_DIRECT:
+      return PROXY_SCHEME_DIRECT;
+    default:
+      NOTREACHED() << scheme;
+      return PROXY_SCHEME_UNKNOWN;
+  }
+}
+
+void LogSuccessfulProxyUMAs(const DataReductionProxyTypeInfo& proxy_info,
+                            const net::ProxyServer& proxy_server,
+                            bool is_main_frame) {
+  // Report the success counts.
+  UMA_HISTOGRAM_COUNTS_100(
+      "DataReductionProxy.SuccessfulRequestCompletionCounts",
+      proxy_info.proxy_index);
+
+  // It is possible that the scheme of request->proxy_server() is different
+  // from the scheme of proxy_info.proxy_servers.front(). The former may be set
+  // to QUIC by the network stack, while the latter may be set to HTTPS.
+  UMA_HISTOGRAM_ENUMERATION(
+      "DataReductionProxy.ProxySchemeUsed",
+      ConvertNetProxySchemeToProxyScheme(proxy_server.scheme()),
+      PROXY_SCHEME_MAX);
+  if (is_main_frame) {
+    UMA_HISTOGRAM_COUNTS_100(
+        "DataReductionProxy.SuccessfulRequestCompletionCounts.MainFrame",
+        proxy_info.proxy_index);
+  }
+}
+
+}  // namespace data_reduction_proxy

components/data_reduction_proxy/core/common/uma_util.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_DATA_REDUCTION_PROXY_CORE_COMMON_UMA_UTIL_H_
+#define COMPONENTS_DATA_REDUCTION_PROXY_CORE_COMMON_UMA_UTIL_H_
+
+#include "net/base/proxy_server.h"
+
+namespace data_reduction_proxy {
+struct DataReductionProxyTypeInfo;
+
+// Scheme of the proxy used.
+enum ProxyScheme {
+  PROXY_SCHEME_UNKNOWN = 0,
+  PROXY_SCHEME_HTTP,
+  PROXY_SCHEME_HTTPS,
+  PROXY_SCHEME_QUIC,
+  PROXY_SCHEME_DIRECT,
+  PROXY_SCHEME_MAX
+};
+
+// Converts net::ProxyServer::Scheme to type ProxyScheme.
+ProxyScheme ConvertNetProxySchemeToProxyScheme(net::ProxyServer::Scheme scheme);
+
+// Logs UMAs for a successful request through a data reduction proxy.
+void LogSuccessfulProxyUMAs(const DataReductionProxyTypeInfo& proxy_info,
+                            const net::ProxyServer& proxy_server,
+                            bool is_main_frame);
+
+}  // namespace data_reduction_proxy
+
+#endif  // COMPONENTS_DATA_REDUCTION_PROXY_CORE_COMMON_UMA_UTIL_H_

testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

@@ -37,7 +37,6 @@
 
 # https://crbug.com/721403
 -ContextMenuBrowserTest.DataSaverOpenOrigImageInNewTab
--DataReductionProxyBrowsertest.UMAMetricsRecorded
 
 # NOTE: if adding an exclusion for an existing failure (e.g. additional test for
 # feature X that is already not working), please add it beside the existing