Add a dedicated SSL protocol version metric.

Remove the old SSL version gathering logic in ConnectionType. Those metrics
are very difficult to read because the denominator is wrong.

In the meantime, claim the ConnectionType metrics since they're unowned.
They'll be deprecated once this new one has gathered numbers and the
SPDY use case for them has also been accounted for.

BUG=459710

Review URL: https://codereview.chromium.org/941743002

Cr-Commit-Position: refs/heads/master@{#317161}

7 files changed, 30 insertions, 40 deletions

diff --git a/net/socket/ssl_client_socket.cc b/net/socket/ssl_client_socket.cc
index f41ab32..940c456 100644
--- a/net/socket/ssl_client_socket.cc
+++ b/net/socket/ssl_client_socket.cc
@@ -176,28 +176,6 @@ void SSLClientSocket::RecordChannelIDSupport(
 }
 
 // static
-void SSLClientSocket::RecordConnectionTypeMetrics(int ssl_version) {
-  UpdateConnectionTypeHistograms(CONNECTION_SSL);
-  switch (ssl_version) {
-    case SSL_CONNECTION_VERSION_SSL2:
-      UpdateConnectionTypeHistograms(CONNECTION_SSL_SSL2);
-      break;
-    case SSL_CONNECTION_VERSION_SSL3:
-      UpdateConnectionTypeHistograms(CONNECTION_SSL_SSL3);
-      break;
-    case SSL_CONNECTION_VERSION_TLS1:
-      UpdateConnectionTypeHistograms(CONNECTION_SSL_TLS1);
-      break;
-    case SSL_CONNECTION_VERSION_TLS1_1:
-      UpdateConnectionTypeHistograms(CONNECTION_SSL_TLS1_1);
-      break;
-    case SSL_CONNECTION_VERSION_TLS1_2:
-      UpdateConnectionTypeHistograms(CONNECTION_SSL_TLS1_2);
-      break;
-  }
-}
-
-// static
 bool SSLClientSocket::IsChannelIDEnabled(
     const SSLConfig& ssl_config,
     ChannelIDService* channel_id_service) {
diff --git a/net/socket/ssl_client_socket.h b/net/socket/ssl_client_socket.h
index 46461df..7de2611 100644
--- a/net/socket/ssl_client_socket.h
+++ b/net/socket/ssl_client_socket.h
@@ -201,9 +201,6 @@ class NET_EXPORT SSLClientSocket : public SSLSocket {
       bool channel_id_enabled,
       bool supports_ecc);
 
-  // Records ConnectionType histograms for a successful SSL connection.
-  static void RecordConnectionTypeMetrics(int ssl_version);
-
   // Returns whether TLS channel ID is enabled.
   static bool IsChannelIDEnabled(
       const SSLConfig& ssl_config,
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index 9692ea3..1cc156b 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -3537,15 +3537,6 @@ int SSLClientSocketNSS::DoVerifyCertComplete(int result) {
   // purposes.  See https://bugzilla.mozilla.org/show_bug.cgi?id=508081 and
   // http://crbug.com/15630 for more info.
 
-  // TODO(hclam): Skip logging if server cert was expected to be bad because
-  // |server_cert_verify_result_| doesn't contain all the information about
-  // the cert.
-  if (result == OK) {
-    int ssl_version =
-        SSLConnectionStatusToVersion(core_->state().ssl_connection_status);
-    RecordConnectionTypeMetrics(ssl_version);
-  }
-
   const CertStatus cert_status = server_cert_verify_result_.cert_status;
   if (transport_security_state_ &&
       (result == OK ||
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index da073bc..1518ef6 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -1211,8 +1211,6 @@ int SSLClientSocketOpenSSL::DoVerifyCertComplete(int result) {
   }
 
   if (result == OK) {
-    RecordConnectionTypeMetrics(GetNetSSLVersion(ssl_));
-
     if (SSL_session_reused(ssl_)) {
       // Record whether or not the server tried to resume a session for a
       // different version. See https://crbug.com/441456.
diff --git a/net/socket/ssl_client_socket_pool.cc b/net/socket/ssl_client_socket_pool.cc
index db28bec..64b6d67 100644
--- a/net/socket/ssl_client_socket_pool.cc
+++ b/net/socket/ssl_client_socket_pool.cc
@@ -501,6 +501,10 @@ int SSLConnectJob::DoSSLConnectComplete(int result) {
     SSLInfo ssl_info;
     ssl_socket_->GetSSLInfo(&ssl_info);
 
+    UMA_HISTOGRAM_ENUMERATION("Net.SSLVersion", SSLConnectionStatusToVersion(
+                                                    ssl_info.connection_status),
+                              SSL_CONNECTION_VERSION_MAX);
+
     UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSL_CipherSuite",
                                 SSLConnectionStatusToCipherSuite(
                                     ssl_info.connection_status));
diff --git a/net/ssl/ssl_connection_status_flags.h b/net/ssl/ssl_connection_status_flags.h
index d3bfed2..5d806ae 100644
--- a/net/ssl/ssl_connection_status_flags.h
+++ b/net/ssl/ssl_connection_status_flags.h
@@ -37,7 +37,8 @@ enum {
 };
 
 // NOTE: the SSL version enum constants must be between 0 and
-// SSL_CONNECTION_VERSION_MASK, inclusive.
+// SSL_CONNECTION_VERSION_MASK, inclusive. These values are persisted to disk
+// and used in UMA, so they must remain stable.
 enum {
   SSL_CONNECTION_VERSION_UNKNOWN = 0,  // Unknown SSL version.
   SSL_CONNECTION_VERSION_SSL2 = 1,
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 56132cd..2ef1fb2 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -16429,7 +16429,7 @@ Therefore, the affected-histogram name has to have at least one dot in it.
 </histogram>
 
 <histogram name="Net.ConnectionTypeCount3" enum="ConnectionType">
-  <owner>Please list the metric's owners. Add more owner tags as needed.</owner>
+  <owner>davidben@chromium.org</owner>
   <summary>
     Each bucket is the number of successful connections of a particular type
     that the user has had during the session.
@@ -17424,7 +17424,7 @@ Therefore, the affected-histogram name has to have at least one dot in it.
 </histogram>
 
 <histogram name="Net.HadConnectionType3" enum="ConnectionType">
-  <owner>Please list the metric's owners. Add more owner tags as needed.</owner>
+  <owner>davidben@chromium.org</owner>
   <summary>
     Each bucket is a boolean (0 or 1) indicating whether the user has had a
     successful connection of that type during the session.
@@ -19788,7 +19788,10 @@ Therefore, the affected-histogram name has to have at least one dot in it.
 <histogram name="Net.SSL_CipherSuite" enum="SSLCipherSuite">
   <owner>agl@chromium.org</owner>
   <owner>rsleevi@chromium.org</owner>
-  <summary>The SSL/TLS cipher suite that was negotiated.</summary>
+  <summary>
+    The SSL/TLS cipher suite that was negotiated. Recorded for each SSL/TLS
+    connection in the socket pool where Connect() succeeds.
+  </summary>
 </histogram>
 
 <histogram name="Net.SSL_Connection_Error" enum="NetErrorCodes">
@@ -19977,6 +19980,14 @@ Therefore, the affected-histogram name has to have at least one dot in it.
   <summary>Time saved by a speculative certificate vertification.</summary>
 </histogram>
 
+<histogram name="Net.SSLVersion" enum="SSLOrQUICVersion">
+  <owner>davidben@chromium.org</owner>
+  <summary>
+    The SSL/TLS version that was negotiated. Recorded for each SSL/TLS
+    connection in the socket pool where Connect() succeeds.
+  </summary>
+</histogram>
+
 <histogram name="Net.TCP_Connection_Idle_Sockets">
   <owner>Please list the metric's owners. Add more owner tags as needed.</owner>
   <summary>Number of idle sockets when the Connect() succeeded.</summary>
@@ -58834,6 +58845,16 @@ To add a new entry, add it with any value and run test to compute valid value.
   <int value="3" label="NOT_EXPIRED_AND_DO_NOT_PROCEED"/>
 </enum>
 
+<enum name="SSLOrQUICVersion" type="int">
+  <int value="0" label="Unknown"/>
+  <int value="1" label="SSL 2.0"/>
+  <int value="2" label="SSL 3.0"/>
+  <int value="3" label="TLS 1.0"/>
+  <int value="4" label="TLS 1.1"/>
+  <int value="5" label="TLS 1.2"/>
+  <int value="7" label="QUIC"/>
+</enum>
+
 <enum name="SSLProtocolNegotiation" type="int">
   <int value="1" label="ALPN, HTTP/1.1"/>
   <int value="100" label="ALPN, SPDY 2.0"/>