Replace SSLProtocolNegotiation histogram with SSLNegotiatedAlpnProtocol.

net/socket/ssl_client_socket_impl.cc

 // Copyright (c) 2012 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 "net/socket/ssl_client_socket_impl.h"
 
 #include <errno.h>
 #include <openssl/bio.h>
 #include <openssl/bytestring.h>
 #include <openssl/err.h>
@@ skipping 1204 matching lines @@
     SSL_get0_alpn_selected(ssl_, &alpn_proto, &alpn_len);
     if (alpn_len > 0) {
       base::StringPiece proto(reinterpret_cast<const char*>(alpn_proto),
                               alpn_len);
       negotiated_protocol_ = NextProtoFromString(proto);
       npn_status_ = kNextProtoNegotiated;
       negotiation_extension_ = kExtensionALPN;
     }
   }
 
-  RecordNegotiationExtension();
+  RecordNegotiatedProtocol();
   RecordChannelIDSupport();
 
   const uint8_t* ocsp_response_raw;
   size_t ocsp_response_len;
   SSL_get0_ocsp_response(ssl_, &ocsp_response_raw, &ocsp_response_len);
   std::string ocsp_response;
   if (ocsp_response_len > 0) {
     ocsp_response_.assign(reinterpret_cast<const char*>(ocsp_response_raw),
                           ocsp_response_len);
   }
@@ skipping 1099 matching lines @@
 void SSLClientSocketImpl::LogConnectEndEvent(int rv) {
   if (rv != OK) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
     return;
   }
 
   net_log_.EndEvent(NetLog::TYPE_SSL_CONNECT,
                     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 {
   // Since this enum is used for a histogram, do not change or re-use values.
   enum {
     DISABLED = 0,
     CLIENT_ONLY = 1,
     CLIENT_AND_SERVER = 2,
     // CLIENT_NO_ECC is unused now.
     // CLIENT_BAD_SYSTEM_TIME is unused now.
@@ skipping 11 matching lines @@
   }
   UMA_HISTOGRAM_ENUMERATION("DomainBoundCerts.Support", supported,
                             CHANNEL_ID_USAGE_MAX);
 }
 
 bool SSLClientSocketImpl::IsChannelIDEnabled() const {
   return ssl_config_.channel_id_enabled && channel_id_service_;
 }
 
 }  // namespace net