Add histogram to track NPN/ALPN.

Add histogram to track NPN/ALPN.

Add histogram to track
* which TLS extension (NPN or ALPN) is used for protocol negotiation,
* in case of NPN, is there overlap or do we fall back,
* which protocol is selected.

This CL is draft, I still do not know how to call set_protocol_negotiation() in
NSS.  I would love to do that in
SSLClientSocketNSS::Core::HandshakeSucceeded(), but that might not be the on
the same thread.

Committed: https://crrev.com/0d28ea5f8316e357734b8d7c8ac2d9a2d02c30b0
Cr-Commit-Position: refs/heads/master@{#299303}

[Bence, 2014-09-19 17:39:53]

bnc@chromium.org changed reviewers:
+ davidben@chromium.org

[Bence, 2014-09-19 17:39:54]

David, please help me to integrate this into NSS.  I do not know how to call
set_protocol_negotiation() from ssl_client_socket_nss.cc.  I would love to do
that in
SSLClientSocketNSS::Core::HandshakeSucceeded(), but that might not be the on the
same thread.  Any ideas?

[davidben, 2014-09-19 20:58:38]

On 2014/09/19 17:39:54, Bence wrote:
> David, please help me to integrate this into NSS.  I do not know how to call
> set_protocol_negotiation() from ssl_client_socket_nss.cc.  I would love to do
> that in
> SSLClientSocketNSS::Core::HandshakeSucceeded(), but that might not be the on
the
> same thread.  Any ideas?

Ah, right, you'll have to deal with the silly threading thing.
HandshakeSucceeded is on the NSS thread, which is the right one for querying the
extensions. I think you want to add it to HandshakeState and then, once the
handshake is done and you're back on the network thread, in
SSLClientSocketNSS::DoHandshakeComplete, call set_protocol_negotiation() based
on core_->state().

[davidben, 2014-09-19 21:09:17]

https://codereview.chromium.org/590513002/diff/1/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/590513002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:93: PN_NPN_NO_OVERLAP = 1000,
It seems strange to have this enum with large numbers and also have a value
(PN_NPN_NO_OVERLAP) that's not set externally. How about just

enum SSLProtocolNegotiation {
  kProtocolNone = 0,
  kProtocolALPN = 1,
  kProtocolNPN = 2,
};

(Our style seems slightly confused as to whether we do kConstantsLikeThis or
CONSTANTS_LIKE_THAT, but this is consistent with the other one in this class.)

And then munge into a single number internally if you want a single histogram.

https://codereview.chromium.org/590513002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:168: enum SSLProtocolNegotiation
protocol_negotiation);
No 'enum' prefix in C++.

[Bence, 2014-10-03 09:43:42]

Patchset #4 (id:60001) has been deleted

[Bence, 2014-10-03 10:50:22]

David, PTAL.

https://codereview.chromium.org/590513002/diff/1/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/590513002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:93: PN_NPN_NO_OVERLAP = 1000,
On 2014/09/19 21:09:17, David Benjamin wrote:
> It seems strange to have this enum with large numbers and also have a value
> (PN_NPN_NO_OVERLAP) that's not set externally. How about just
> 
> enum SSLProtocolNegotiation {
>   kProtocolNone = 0,
>   kProtocolALPN = 1,
>   kProtocolNPN = 2,
> };
> 
> (Our style seems slightly confused as to whether we do kConstantsLikeThis or
> CONSTANTS_LIKE_THAT, but this is consistent with the other one in this class.)
> 
> And then munge into a single number internally if you want a single histogram.

Done.

I changed to kExtension* to better distinguish TLS extensions (NPN, ALPN) from
protocols they negotiate (HTTP/1, SPDY, QUIC).

https://codereview.chromium.org/590513002/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:168: enum SSLProtocolNegotiation
protocol_negotiation);
On 2014/09/19 21:09:17, David Benjamin wrote:
> No 'enum' prefix in C++.

Done.

[davidben, 2014-10-03 17:55:08]

lgtm with a handful of comments. You'll need to get a histogram OWNERS for the
histograms.xml change (who can probably also tell us if there's a better way to
correlate histograms than to do this funny addition thing).

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:260: sample += 500;
This is probably worth a comment in net/socket/next_proto.h that the values must
be below 500 now.

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:262: }
Nit: I'd put in an
  } else {
    DCHECK_EQ(kExtensionALPN, negotiation_extension_);
  }
just so you document that the other case is ALPN.

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.h:83: // or reuse values.
This comment seems no longer accurate (they don't seem to be record directly in
UMA anymore).

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.h:237: enum SSLNegotiationExtension
negotiation_extension_;
Nit: No 'enum' prefix in C++.

[Bence, 2014-10-10 15:26:10]

bnc@chromium.org changed reviewers:
+ jar@chromium.org

[Bence, 2014-10-10 15:26:11]

jar: PTAL at histograms.xml.  Also please let us know if there is an easier way
to do 2D histograms.
David: PTAL.
Thank you both.

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:260: sample += 500;
On 2014/10/03 17:55:07, David Benjamin wrote:
> This is probably worth a comment in net/socket/next_proto.h that the values
must
> be below 500 now.

Done.

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.cc:262: }
On 2014/10/03 17:55:07, David Benjamin wrote:
> Nit: I'd put in an
>   } else {
>     DCHECK_EQ(kExtensionALPN, negotiation_extension_);
>   }
> just so you document that the other case is ALPN.

Done.

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.h:83: // or reuse values.
On 2014/10/03 17:55:07, David Benjamin wrote:
> This comment seems no longer accurate (they don't seem to be record directly
in
> UMA anymore).

Done.

https://codereview.chromium.org/590513002/diff/80001/net/socket/ssl_client_so...
net/socket/ssl_client_socket.h:237: enum SSLNegotiationExtension
negotiation_extension_;
On 2014/10/03 17:55:07, David Benjamin wrote:
> Nit: No 'enum' prefix in C++.

Done.

[jar (doing other things), 2014-10-11 01:13:59]

histograms.xml and generation call LGTM.  Note: I have not reviewed any other
code, or validated how the data is fed to the sample in the histogram.

https://codereview.chromium.org/590513002/diff/140001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket.cc (right):

https://codereview.chromium.org/590513002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket.cc:265:
UMA_HISTOGRAM_SPARSE_SLOWLY("Net.SSLProtocolNegotiation", sample);
This is indeed the "right way" to get something akin to a 2D histogram.

https://codereview.chromium.org/590513002/diff/140001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/590513002/diff/140001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:52359: +  <int value="1200" label="NPN,
no overlap, fallback to QUIC/1 + SPDY/3"/>
This also LG.  You *might* want to be as concises (shorter setences?) as
possible, as this text will appear to the left of the histogram, and leave far
less room for counters (as I recall).  

YMMV.  This will work.... and you *might* choose to revisit if it is not as
readable and desired.

[Bence, 2014-10-13 14:54:15]

Thank you both.

https://codereview.chromium.org/590513002/diff/140001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/590513002/diff/140001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:52359: +  <int value="1200" label="NPN,
no overlap, fallback to QUIC/1 + SPDY/3"/>
On 2014/10/11 01:13:59, jar wrote:
> This also LG.  You *might* want to be as concises (shorter setences?) as
> possible, as this text will appear to the left of the histogram, and leave far
> less room for counters (as I recall).  
> 
> YMMV.  This will work.... and you *might* choose to revisit if it is not as
> readable and desired.

Thank you.  I removed the phrase "no overlap", histogram summary above explains
it anyway.

Done.

[Bence, 2014-10-13 14:54:20]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2014-10-13 14:54:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/590513002/300001

[commit-bot: I haz the power, 2014-10-13 15:15:56]

Committed patchset #7 (id:300001)

[commit-bot: I haz the power, 2014-10-13 15:16:25]

Patchset 7 (id:??) landed as
https://crrev.com/0d28ea5f8316e357734b8d7c8ac2d9a2d02c30b0
Cr-Commit-Position: refs/heads/master@{#299303}