NetConnectivity - Collect stats for TCP/UDP network

chrome/browser/net/network_stats.cc

Index: chrome/browser/net/network_stats.cc
===================================================================
--- chrome/browser/net/network_stats.cc	(revision 132599)
+++ chrome/browser/net/network_stats.cc	(working copy)
@@ -16,11 +16,13 @@
 #include "base/tuple.h"
 #include "chrome/common/chrome_version_info.h"
 #include "content/public/browser/browser_thread.h"
+#include "googleurl/src/gurl.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/sys_addrinfo.h"
 #include "net/base/test_completion_callback.h"
+#include "net/proxy/proxy_service.h"
 #include "net/socket/tcp_client_socket.h"
 #include "net/udp/udp_client_socket.h"
 #include "net/udp/udp_server_socket.h"
@@ -85,6 +87,7 @@
       bytes_to_read_(0),
       bytes_to_send_(0),
       encoded_message_(""),
+      has_no_proxy_server_(true),
       start_time_(base::TimeTicks::Now()),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
@@ -96,11 +99,13 @@
 bool NetworkStats::Start(net::HostResolver* host_resolver,
                          const net::HostPortPair& server_host_port_pair,
                          HistogramPortSelector histogram_port,
+                         bool has_no_proxy_server,
                          uint32 bytes_to_send,
                          const net::CompletionCallback& finished_callback) {
   DCHECK(bytes_to_send);   // We should have data to send.
 
-  Initialize(bytes_to_send, histogram_port, finished_callback);
+  Initialize(
+      bytes_to_send, histogram_port, has_no_proxy_server, finished_callback);
 
   net::HostResolver::RequestInfo request(server_host_port_pair);
   int rv = host_resolver->Resolve(
@@ -116,6 +121,7 @@
 void NetworkStats::Initialize(
     uint32 bytes_to_send,
     HistogramPortSelector histogram_port,
+    bool has_no_proxy_server,
     const net::CompletionCallback& finished_callback) {
   DCHECK(bytes_to_send);   // We should have data to send.
   DCHECK_LE(bytes_to_send, kLargeTestBytesToSend);
@@ -127,6 +133,7 @@
       kKeyLength + load_size_;
 
   histogram_port_ = histogram_port;
+  has_no_proxy_server_ = has_no_proxy_server;
   finished_callback_ = finished_callback;
 }
 
@@ -409,14 +416,18 @@
 }
 
 // static
-void NetworkStats::GetHistogramNames(const ProtocolValue& protocol,
-                                     HistogramPortSelector port,
-                                     uint32 load_size,
-                                     int result,
-                                     std::string* rtt_histogram_name,
-                                     std::string* status_histogram_name) {
-  CHECK_GE(port, PORT_53);
-  CHECK_LE(port, HISTOGRAM_PORT_MAX);
+void NetworkStats::GetHistogramNames(
+    const ProtocolValue& protocol,
+    HistogramPortSelector histogram_port,
+    bool has_no_proxy_server,
+    uint32 load_size,
+    int result,
+    std::string* rtt_histogram_name,
+    std::string* rtt_no_proxy_histogram_name,
+    std::string* status_histogram_name,
+    std::string* status_no_proxy_histogram_name) {
+  CHECK_GE(histogram_port, PORT_53);
+  CHECK_LE(histogram_port, HISTOGRAM_PORT_MAX);
 
   // Build <protocol> string.
   const char* kTcpString = "TCP";
@@ -437,22 +448,44 @@
     load_size_string = kLargeLoadString;
 
   // Build "NetConnectivity.<protocol>.Success.<port>.<load_size>.RTT"
-  // histogram name. Total number of histograms are 2*5*2.
+  // and "NetConnectivity.<protocol>.NoProxy.Success.<port>.<load_size>.RTT"
+  // histogram names. Total number of histograms are 2*5*2*2.
   if (result == net::OK) {
     *rtt_histogram_name = base::StringPrintf(
         "NetConnectivity.%s.Success.%d.%s.RTT",
         protocol_string,
-        kPorts[port],
+        kPorts[histogram_port],
         load_size_string);
+    if (has_no_proxy_server) {
+      *rtt_no_proxy_histogram_name = base::StringPrintf(
+          "NetConnectivity.%s.NoProxy.Success.%d.%s.RTT",
+          protocol_string,
+          kPorts[histogram_port],
+          load_size_string);
+    } else {
+      rtt_no_proxy_histogram_name->clear();
+    }
+  } else {
+    rtt_histogram_name->clear();

[Ryan Sleevi, 2012/04/28 02:06:32]

random drive by: Since much of our error handling works via early-return style,
it seems like it might make more sense to check for failure in the if and then
let the else handle success. Otherwise, you have to continue reading and count
the columns to figure out
1) If there's error handling
2) What it does

eg:

if (result != net::OK) {
  rtt_histogram_name->clear();
} else {
  if (!has_no_proxy_server) {
    rtt_no_proxy_server_hisogram_name->clear();
  } else {
    ...
  }
}

I don't believe this will significantly impact branch mispredicts during runtime
(depending on how the compiler codegens), so if this is very perf-sensitive, the
readability might not be worth it.

[ramant (doing other things), 2012/04/30 17:17:00]

Done.

   }
 
-  // Build "NetConnectivity.<protocol>.Status.<port>.<load_size>" histogram
-  // name. Total number of histograms are 2*5*2.
+  // Build "NetConnectivity.<protocol>.Status.<port>.<load_size>" and
+  // "NetConnectivity.<protocol>.NoProxy.Status.<port>.<load_size>" histogram
+  // name. Total number of histograms are 2*2*5*2.
   *status_histogram_name =  base::StringPrintf(
       "NetConnectivity.%s.Status.%d.%s",
       protocol_string,
-      kPorts[port],
+      kPorts[histogram_port],
       load_size_string);
+  if (has_no_proxy_server) {
+    *status_no_proxy_histogram_name =  base::StringPrintf(
+        "NetConnectivity.%s.NoProxy.Status.%d.%s",
+        protocol_string,
+        kPorts[histogram_port],
+        load_size_string);
+  } else {
+    status_no_proxy_histogram_name->clear();
+  }
 }
 
 void NetworkStats::RecordHistograms(const ProtocolValue& protocol,
@@ -461,13 +494,18 @@
   base::TimeDelta duration = base::TimeTicks::Now() - start_time();
 
   std::string rtt_histogram_name;
+  std::string rtt_no_proxy_histogram_name;
   std::string status_histogram_name;
+  std::string status_no_proxy_histogram_name;
   GetHistogramNames(protocol,
                     histogram_port_,
+                    has_no_proxy_server_,
                     load_size_,
                     result,
                     &rtt_histogram_name,
-                    &status_histogram_name);
+                    &rtt_no_proxy_histogram_name,
+                    &status_histogram_name,
+                    &status_no_proxy_histogram_name);
 
   if (result == net::OK) {
     base::Histogram* rtt_histogram = base::Histogram::FactoryTimeGet(
@@ -476,12 +514,28 @@
         base::TimeDelta::FromSeconds(60), 50,
         base::Histogram::kUmaTargetedHistogramFlag);
     rtt_histogram->AddTime(duration);
+
+    if (has_no_proxy_server_) {
+      base::Histogram* rtt_no_proxy_histogram = base::Histogram::FactoryTimeGet(
+          rtt_no_proxy_histogram_name,
+          base::TimeDelta::FromMilliseconds(10),
+          base::TimeDelta::FromSeconds(60), 50,
+          base::Histogram::kUmaTargetedHistogramFlag);
+      rtt_no_proxy_histogram->AddTime(duration);
+    }
   }
 
   base::Histogram* status_histogram = base::LinearHistogram::FactoryGet(
       status_histogram_name, 1, STATUS_MAX, STATUS_MAX+1,
       base::Histogram::kUmaTargetedHistogramFlag);
   status_histogram->Add(status);
+  if (has_no_proxy_server_) {
+    base::Histogram* status_no_proxy_histogram =
+        base::LinearHistogram::FactoryGet(
+            status_no_proxy_histogram_name, 1, STATUS_MAX, STATUS_MAX+1,
+            base::Histogram::kUmaTargetedHistogramFlag);
+    status_no_proxy_histogram->Add(status);
+  }
 }
 
 // UDPStatsClient methods and members.
@@ -604,6 +658,70 @@
   delete this;
 }
 
+// ProxyClient methods and members.
+ProxyClient::ProxyClient(const std::string& network_stats_server,
+                         NetworkStats::HistogramPortSelector histogram_port,
+                         IOThread* io_thread)
+    : network_stats_server_(network_stats_server),
+      histogram_port_(histogram_port),
+      io_thread_(io_thread) {
+}
+
+ProxyClient::~ProxyClient() {
+}
+
+void ProxyClient::ResolveProxy() {
+  std::string url = base::StringPrintf("http://%s:%d",
+                                       network_stats_server_.c_str(),
+                                       kPorts[histogram_port_]);

[Ryan Sleevi, 2012/04/28 02:06:32]

Would it make more sense to check for HTTPS here?

The reason I mention it is that HTTPS will give you an indicator as to whether
or not the proxy server supports CONNECT, which is effectively how outbound
connections can be mediated by the proxy.

[ramant (doing other things), 2012/04/30 17:17:00]

Done.

+  GURL gurl(url);
+  net::ProxyService* service =
+      io_thread_->globals()->system_proxy_service.get();
+
+  int rv = service->ResolveProxy(
+      gurl,
+      &proxy_info_,
+      base::Bind(&ProxyClient::OnResolveProxyComplete,
+                 base::Unretained(this)),
+      NULL,
+      net::BoundNetLog());
+  if (rv == net::OK)
+    OnResolveProxyComplete(rv);
+}
+
+void ProxyClient::OnResolveProxyComplete(int result) {
+  bool no_proxy_server = (!proxy_info_.proxy_server().is_valid() ||
+                          proxy_info_.proxy_server().is_direct());
+
+  net::HostResolver* host_resolver = io_thread_->globals()->host_resolver.get();
+  DCHECK(host_resolver);
+
+  DCHECK_GE(histogram_port_, NetworkStats::PORT_53);
+  DCHECK_LE(histogram_port_, NetworkStats::PORT_8080);
+  net::HostPortPair server_address(network_stats_server_,
+                                   kPorts[histogram_port_]);
+
+  UDPStatsClient* small_udp_stats = new UDPStatsClient();

[Ryan Sleevi, 2012/04/28 02:06:32]

Might be worth mentioning either here or in network_stats.h that the
(UDP/TCP)StatsClient will delete themselves when Finish() is called.

I had to check to find the "delete this" or if they were refcounted.

[ramant (doing other things), 2012/04/30 17:17:00]

Done.

+  small_udp_stats->Start(
+      host_resolver, server_address, histogram_port_, no_proxy_server,
+      kSmallTestBytesToSend, net::CompletionCallback());
+
+  UDPStatsClient* large_udp_stats = new UDPStatsClient();
+  large_udp_stats->Start(
+      host_resolver, server_address, histogram_port_, no_proxy_server,
+      kLargeTestBytesToSend, net::CompletionCallback());
+
+  TCPStatsClient* small_tcp_client = new TCPStatsClient();
+  small_tcp_client->Start(
+      host_resolver, server_address, histogram_port_, no_proxy_server,
+      kSmallTestBytesToSend, net::CompletionCallback());
+
+  TCPStatsClient* large_tcp_client = new TCPStatsClient();
+  large_tcp_client->Start(
+      host_resolver, server_address, histogram_port_, no_proxy_server,
+      kLargeTestBytesToSend, net::CompletionCallback());
+}
+
 // static
 void CollectNetworkStats(const std::string& network_stats_server,
                          IOThread* io_thread) {
@@ -630,8 +748,6 @@
   CR_DEFINE_STATIC_LOCAL(scoped_refptr<base::FieldTrial>, trial, ());
   static bool collect_stats = false;
 
-  static uint32 kTCPTestingPort;
-  static uint32 kUDPTestingPort;
   static NetworkStats::HistogramPortSelector histogram_port;
 
   if (!trial.get()) {
@@ -670,8 +786,6 @@
           base::RandInt(NetworkStats::PORT_53, NetworkStats::PORT_8080));
       DCHECK_GE(histogram_port, NetworkStats::PORT_53);
       DCHECK_LE(histogram_port, NetworkStats::PORT_8080);
-      kTCPTestingPort = kPorts[histogram_port];
-      kUDPTestingPort = kPorts[histogram_port];
     }
   }
 
@@ -686,32 +800,9 @@
 
   ++number_of_tests_done;
 
-  net::HostResolver* host_resolver = io_thread->globals()->host_resolver.get();
-  DCHECK(host_resolver);
-
-  net::HostPortPair udp_server_address(network_stats_server, kUDPTestingPort);
-
-  UDPStatsClient* small_udp_stats = new UDPStatsClient();
-  small_udp_stats->Start(
-      host_resolver, udp_server_address, histogram_port,
-      kSmallTestBytesToSend, net::CompletionCallback());
-
-  UDPStatsClient* large_udp_stats = new UDPStatsClient();
-  large_udp_stats->Start(
-      host_resolver, udp_server_address, histogram_port,
-      kLargeTestBytesToSend, net::CompletionCallback());
-
-  net::HostPortPair tcp_server_address(network_stats_server, kTCPTestingPort);
-
-  TCPStatsClient* small_tcp_client = new TCPStatsClient();
-  small_tcp_client->Start(
-      host_resolver, tcp_server_address, histogram_port,
-      kSmallTestBytesToSend, net::CompletionCallback());
-
-  TCPStatsClient* large_tcp_client = new TCPStatsClient();
-  large_tcp_client->Start(
-      host_resolver, tcp_server_address, histogram_port,
-      kLargeTestBytesToSend, net::CompletionCallback());
+  ProxyClient* proxy_client = new ProxyClient(
+      network_stats_server, histogram_port, io_thread);
+  proxy_client->ResolveProxy();
 }
 
 }  // namespace chrome_browser_net