Several fixes to the Net.ConnectionTypeCount histogram.

* Previously, the "CONNECTION_ANY" was incorrectly recorded.  It was recording
every Http *transaction*, not every Http connection.
* The histogram was vague about whether it was tracking successful or
unsuccessful connections.  In fact, it was recording all SSL connections (fail
or success), and yet only successful HTTP connections.  Modified to only apply
to successful connections.
* Added a Net.ConnectionTypeFailCount histogram which counts the number of
failed connections by type.

BUG=none
TEST=none

Review URL: http://codereview.chromium.org/519002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@35264 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 57 insertions, 25 deletions

diff --git a/net/base/connection_type_histograms.cc b/net/base/connection_type_histograms.cc
index d200bde..81affa5 100644
--- a/net/base/connection_type_histograms.cc
+++ b/net/base/connection_type_histograms.cc
@@ -20,26 +20,35 @@ namespace net {
 //
 // Each histogram has an unused bucket at the end to allow seamless future
 // expansion.
-void UpdateConnectionTypeHistograms(ConnectionType type) {
+void UpdateConnectionTypeHistograms(ConnectionType type, bool success) {
   static bool had_connection_type[NUM_OF_CONNECTION_TYPES];
-  static scoped_refptr<Histogram> counter1 =
-      LinearHistogram::LinearHistogramFactoryGet("Net.HadConnectionType",
+  static scoped_refptr<Histogram> had_histogram =
+      LinearHistogram::LinearHistogramFactoryGet("Net.HadConnectionType2",
                                                  1, NUM_OF_CONNECTION_TYPES,
                                                  NUM_OF_CONNECTION_TYPES + 1);
-  static scoped_refptr<Histogram> counter2 =
-      LinearHistogram::LinearHistogramFactoryGet("Net.ConnectionTypeCount",
+  static scoped_refptr<Histogram> success_histogram =
+      LinearHistogram::LinearHistogramFactoryGet("Net.ConnectionTypeCount2",
+                                                 1, NUM_OF_CONNECTION_TYPES,
+                                                 NUM_OF_CONNECTION_TYPES + 1);
+  static scoped_refptr<Histogram> failed_histogram =
+      LinearHistogram::LinearHistogramFactoryGet("Net.ConnectionTypeFailCount2",
                                                  1, NUM_OF_CONNECTION_TYPES,
                                                  NUM_OF_CONNECTION_TYPES + 1);
 
   if (type >= 0 && type < NUM_OF_CONNECTION_TYPES) {
     if (!had_connection_type[type]) {
       had_connection_type[type] = true;
-      counter1->SetFlags(kUmaTargetedHistogramFlag);
-      counter1->Add(type);
+      had_histogram->SetFlags(kUmaTargetedHistogramFlag);
+      had_histogram->Add(type);
     }
+
+    Histogram* histogram;
+    histogram = success ? success_histogram.get() : failed_histogram.get();
+    histogram->SetFlags(kUmaTargetedHistogramFlag);
+    histogram->Add(type);
+  } else {
+    NOTREACHED();  // Someone's logging an invalid type!
   }
-  counter2->SetFlags(kUmaTargetedHistogramFlag);
-  counter2->Add(type);
 }
 
 }  // namespace net
diff --git a/net/base/connection_type_histograms.h b/net/base/connection_type_histograms.h
index bea070c..c8517ff 100644
--- a/net/base/connection_type_histograms.h
+++ b/net/base/connection_type_histograms.h
@@ -15,7 +15,7 @@
 namespace net {
 
 enum ConnectionType {
-  CONNECTION_ANY = 0,      // Any connection, SSL or not
+  CONNECTION_ANY = 0,      // Any connection (SSL, HTTP, SPDY, etc)
   CONNECTION_SSL = 1,      // An SSL connection
   CONNECTION_SSL_MD5 = 2,  // An SSL connection with an MD5 certificate in
                            // the certificate chain (excluding root)
@@ -27,10 +27,14 @@ enum ConnectionType {
                               // in the certificate chain (excluding root)
   CONNECTION_SSL_MD2_CA = 6,  // An SSL connection with an MD2 CA certificate
                               // in the certificate chain (excluding root)
+  CONNECTION_HTTP = 7,     // An HTTP connection
+  CONNECTION_SPDY = 8,     // A SPDY connection
   NUM_OF_CONNECTION_TYPES
 };
 
-void UpdateConnectionTypeHistograms(ConnectionType type);
+// Update the connection type histograms.  |type| is the connection type.
+// |success| is whether or not the connection was successful or not.
+void UpdateConnectionTypeHistograms(ConnectionType type, bool success);
 
 }  // namespace net
 
diff --git a/net/flip/flip_network_transaction.cc b/net/flip/flip_network_transaction.cc
index 84831c6..440e666 100644
--- a/net/flip/flip_network_transaction.cc
+++ b/net/flip/flip_network_transaction.cc
@@ -7,6 +7,7 @@
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/scoped_ptr.h"
+#include "base/stats_counters.h"
 #include "net/base/host_resolver.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
@@ -47,6 +48,8 @@ int FlipNetworkTransaction::Start(const HttpRequestInfo* request_info,
   CHECK(request_info);
   CHECK(callback);
 
+  SIMPLE_STATS_COUNTER("FlipNetworkTransaction.Count");
+
   load_log_ = load_log;
   request_ = request_info;
   start_time_ = base::TimeTicks::Now();
diff --git a/net/flip/flip_session.cc b/net/flip/flip_session.cc
index b6abbe9..b1a288d 100644
--- a/net/flip/flip_session.cc
+++ b/net/flip/flip_session.cc
@@ -11,6 +11,7 @@
 #include "base/stats_counters.h"
 #include "base/stl_util-inl.h"
 #include "base/string_util.h"
+#include "net/base/connection_type_histograms.h"
 #include "net/base/load_flags.h"
 #include "net/base/load_log.h"
 #include "net/base/net_util.h"
@@ -418,6 +419,12 @@ LoadState FlipSession::GetLoadState() const {
 void FlipSession::OnTCPConnect(int result) {
   LOG(INFO) << "Flip socket connected (result=" << result << ")";
 
+  // We shouldn't be coming through this path if we didn't just open a fresh
+  // socket (or have an error trying to do so).
+  DCHECK(!connection_->socket() || !connection_->is_reused());
+
+  UpdateConnectionTypeHistograms(CONNECTION_SPDY, result >= 0);
+
   if (result != net::OK) {
     DCHECK_LT(result, 0);
     CloseSessionOnError(static_cast<net::Error>(result));
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 82fdff9..eb50716 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -9,6 +9,7 @@
 #include "base/compiler_specific.h"
 #include "base/field_trial.h"
 #include "base/histogram.h"
+#include "base/stats_counters.h"
 #include "base/string_util.h"
 #include "base/trace_event.h"
 #include "build/build_config.h"
@@ -164,7 +165,7 @@ void HttpNetworkTransaction::SetNextProtos(const std::string& next_protos) {
 int HttpNetworkTransaction::Start(const HttpRequestInfo* request_info,
                                   CompletionCallback* callback,
                                   LoadLog* load_log) {
-  UpdateConnectionTypeHistograms(CONNECTION_ANY);
+  SIMPLE_STATS_COUNTER("HttpNetworkTransaction.Count");
 
   load_log_ = load_log;
   request_ = request_info;
@@ -664,13 +665,15 @@ int HttpNetworkTransaction::DoInitConnection() {
 }
 
 int HttpNetworkTransaction::DoInitConnectionComplete(int result) {
-  if (result < 0)
+  if (result < 0) {
+    UpdateConnectionTypeHistograms(CONNECTION_HTTP, false);
     return ReconsiderProxyAfterError(result);
+  }
 
   DCHECK_EQ(OK, result);
 
   // If we don't have an initialized connection, that means we have a flip
-  // connection waiting for us. 
+  // connection waiting for us.
   if (!connection_->is_initialized()) {
     next_state_ = STATE_SPDY_SEND_REQUEST;
     return OK;
@@ -687,6 +690,7 @@ int HttpNetworkTransaction::DoInitConnectionComplete(int result) {
   } else {
     // Now we have a TCP connected socket.  Perform other connection setup as
     // needed.
+    UpdateConnectionTypeHistograms(CONNECTION_HTTP, true);
     if (proxy_mode_ == kSOCKSProxy)
       next_state_ = STATE_SOCKS_CONNECT;
     else if (using_ssl_ && proxy_mode_ == kDirectConnection) {
@@ -781,6 +785,7 @@ int HttpNetworkTransaction::DoSSLConnectComplete(int result) {
         100);
 
     if (use_spdy) {
+      UpdateConnectionTypeHistograms(CONNECTION_SPDY, true);
       next_state_ = STATE_SPDY_SEND_REQUEST;
     } else {
       next_state_ = STATE_SEND_REQUEST;
@@ -1081,7 +1086,7 @@ int HttpNetworkTransaction::DoSpdySendRequest() {
   const scoped_refptr<FlipSessionPool> spdy_pool =
       session_->flip_session_pool();
   scoped_refptr<FlipSession> spdy_session;
-  
+
   if (spdy_pool->HasSession(req_info)) {
     spdy_session = spdy_pool->Get(req_info, session_);
   } else {
diff --git a/net/socket/ssl_client_socket_win.cc b/net/socket/ssl_client_socket_win.cc
index 642652c..7c2e923 100644
--- a/net/socket/ssl_client_socket_win.cc
+++ b/net/socket/ssl_client_socket_win.cc
@@ -973,7 +973,7 @@ int SSLClientSocketWin::DoVerifyCertComplete(int result) {
       ssl_config_.IsAllowedBadCert(server_cert_))
     result = OK;
 
-  LogConnectionTypeMetrics();
+  LogConnectionTypeMetrics(result >= 0);
   if (renegotiating_) {
     DidCompleteRenegotiation();
     return result;
@@ -1313,18 +1313,18 @@ void SSLClientSocketWin::DidCompleteRenegotiation() {
   next_state_ = STATE_COMPLETED_RENEGOTIATION;
 }
 
-void SSLClientSocketWin::LogConnectionTypeMetrics() const {
-  UpdateConnectionTypeHistograms(CONNECTION_SSL);
+void SSLClientSocketWin::LogConnectionTypeMetrics(bool success) const {
+  UpdateConnectionTypeHistograms(CONNECTION_SSL, success);
   if (server_cert_verify_result_.has_md5)
-    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD5);
+    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD5, success);
   if (server_cert_verify_result_.has_md2)
-    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD2);
+    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD2, success);
   if (server_cert_verify_result_.has_md4)
-    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD4);
+    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD4, success);
   if (server_cert_verify_result_.has_md5_ca)
-    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD5_CA);
+    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD5_CA, success);
   if (server_cert_verify_result_.has_md2_ca)
-    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD2_CA);
+    UpdateConnectionTypeHistograms(CONNECTION_SSL_MD2_CA, success);
 }
 
 void SSLClientSocketWin::FreeSendBuffer() {
diff --git a/net/socket/ssl_client_socket_win.h b/net/socket/ssl_client_socket_win.h
index f0503bd..5a83a24 100644
--- a/net/socket/ssl_client_socket_win.h
+++ b/net/socket/ssl_client_socket_win.h
@@ -85,7 +85,7 @@ class SSLClientSocketWin : public SSLClientSocket {
   int DidCallInitializeSecurityContext();
   int DidCompleteHandshake();
   void DidCompleteRenegotiation();
-  void LogConnectionTypeMetrics() const;
+  void LogConnectionTypeMetrics(bool success) const;
   void FreeSendBuffer();
 
   // Internal callbacks as async operations complete.
diff --git a/net/socket/tcp_client_socket_win.cc b/net/socket/tcp_client_socket_win.cc
index 91a1212..3046659 100644
--- a/net/socket/tcp_client_socket_win.cc
+++ b/net/socket/tcp_client_socket_win.cc
@@ -11,6 +11,7 @@
 #include "base/string_util.h"
 #include "base/sys_info.h"
 #include "base/trace_event.h"
+#include "net/base/connection_type_histograms.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_log.h"
 #include "net/base/net_errors.h"
@@ -311,6 +312,7 @@ int TCPClientSocketWin::Connect(CompletionCallback* callback,
   } else {
     TRACE_EVENT_END("socket.connect", this, "");
     LoadLog::EndEvent(load_log, LoadLog::TYPE_TCP_CONNECT);
+    UpdateConnectionTypeHistograms(CONNECTION_ANY, rv >= 0);
   }
 
   return rv;
@@ -659,8 +661,10 @@ void TCPClientSocketWin::DidCompleteConnect() {
     load_log_ = NULL;
   }
 
-  if (result != ERR_IO_PENDING)
+  if (result != ERR_IO_PENDING) {
+    UpdateConnectionTypeHistograms(CONNECTION_ANY, result >= 0);
     DoReadCallback(result);
+  }
 }
 
 void TCPClientSocketWin::DidCompleteRead() {