Replace SSLProtocolNegotiation histogram with SSLNegotiatedAlpnProtocol.

net/socket/ssl_client_socket_impl.cc

Index: net/socket/ssl_client_socket_impl.cc
diff --git a/net/socket/ssl_client_socket_impl.cc b/net/socket/ssl_client_socket_impl.cc
index 6252539a3d0044a929ad0de20c9c1b8e19e26858..183bcb4d0d5135b5cba5f6dbe4812c4ff1808d82 100644
--- a/net/socket/ssl_client_socket_impl.cc
+++ b/net/socket/ssl_client_socket_impl.cc
@@ -1222,7 +1222,7 @@ int SSLClientSocketImpl::DoHandshakeComplete(int result) {
     }
   }
 
-  RecordNegotiationExtension();
+  RecordNegotiatedProtocol();
   RecordChannelIDSupport();
 
   const uint8_t* ocsp_response_raw;
@@ -2342,26 +2342,31 @@ void SSLClientSocketImpl::LogConnectEndEvent(int rv) {
                     base::Bind(&NetLogSSLInfoCallback, base::Unretained(this)));
 }
 
-void SSLClientSocketImpl::RecordNegotiationExtension() const {
-  if (negotiation_extension_ == kExtensionUnknown)
-    return;
-  if (npn_status_ == kNextProtoUnsupported)
-    return;
-  base::HistogramBase::Sample sample =
-      static_cast<base::HistogramBase::Sample>(negotiated_protocol_);
-  // In addition to the protocol negotiated, we want to record which TLS
-  // extension was used, and in case of NPN, whether there was overlap between
-  // server and client list of supported protocols.
-  if (negotiation_extension_ == kExtensionNPN) {
-    if (npn_status_ == kNextProtoNoOverlap) {
-      sample += 1000;
-    } else {
-      sample += 500;
-    }
-  } else {
-    DCHECK_EQ(kExtensionALPN, negotiation_extension_);
+void SSLClientSocketImpl::RecordNegotiatedProtocol() const {
+  // Keep this enum in sync with Net.AlpnNegotiatedProtocol histogram.
+  // Do not change or re-use values.
+  enum {
+    ALPN_NOT_USED = 0,
+    ALPN_HTTP11_NEGOTIATED = 1,
+    ALPN_HTTP2_NEGOTIATED = 2,
+    ALPN_MAX
+  } protocol = ALPN_NOT_USED;
+
+  switch (negotiated_protocol_) {

[davidben, 2016/08/17 19:44:11]

Why not just log the kProtoFoo values?

[Bence, 2016/08/18 13:37:34]

Two reasons: (1) I want to fix the values here, close to the histogram logging,
and not in a different file, where a long comment would be needed to explain why
values should not be changed or reused.  (2) Not every enum NextProto value is a
valid ALPN protocol, like QUIC.  What if we add a new protocol, which then has
to go to the end of enum NextProto, and there will be a gap value in the
histogram?

[davidben, 2016/08/25 18:10:41]

The enum is small enough that I think it's fine to just use it. Fixing the
values here means that everyone using it in a histogram needs to carry a bunch
of extra code for no particularly good reason.

I did the same for Net.SSLVersion. There's a QUIC version in that enum that will
never get hit, and that's fine.

Besides, by using the same type, there is actually nothing stopping us from
sending kProtoQUIC1SPDY3 over ALPN. So the NOTREACHED() is, from the SSL layer's
perspective, not actually true. Kinda silly but so it goes.

[Bence, 2016/08/27 00:59:46]

Done.

+    case kProtoUnknown:
+      protocol = ALPN_NOT_USED;

[davidben, 2016/08/17 19:44:11]

[Note this will result in a larger histogram, but it's probably fine?]

[davidben, 2016/08/17 19:44:56]

By the way, this assumes we don't allow unknown ALPN values. But that should be
true now.

[Bence, 2016/08/18 13:37:34]

Acknowledged.

[Bence, 2016/08/18 13:37:34]

I actually consider this an important feature: I would really love to know what
percentage of TLS handshakes do not use ALPN.  This will give us a better sense
of overall HTTP/2 usage per-connection.  I was missing this from the previous
histogram a lot, especially whenever I had to explain to other people what
exactly our metrics show.

[davidben, 2016/08/25 18:10:41]

Sounds good.

+      break;
+    case kProtoHTTP11:
+      protocol = ALPN_HTTP11_NEGOTIATED;
+      break;
+    case kProtoHTTP2:
+      protocol = ALPN_HTTP2_NEGOTIATED;
+      break;
+    case kProtoQUIC1SPDY3:
+      NOTREACHED();
+      protocol = ALPN_NOT_USED;
   }
-  UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSLProtocolNegotiation", sample);
+  UMA_HISTOGRAM_ENUMERATION("Net.AlpnNegotiatedProtocol", protocol, ALPN_MAX);

[davidben, 2016/08/17 19:44:11]

Nit: I think it'd be clearest if all the SSL histograms began with SSL so we can
easily find them. ALPN is a very SSL-specific thing.

[Bence, 2016/08/18 13:37:34]

Done.

 }
 
 void SSLClientSocketImpl::RecordChannelIDSupport() const {