Add a dedicated SSL protocol version metric.

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
Committed: https://crrev.com/9ff250e0023b2f87e0fddd7eadbef7be3ecc3882
Cr-Commit-Position: refs/heads/master@{#317161}

[davidben, 2015-02-19 17:53:01]

davidben@chromium.org changed reviewers:
+ isherman@chromium.org, rsleevi@chromium.org

[davidben, 2015-02-19 17:53:02]

rsleevi: net/

isherman: histograms.xml. Is the normal procedure here to obsolete the old
histogram now or later? I thought I'd claim it just so I could see it in the
dashboard. The old histogram is nominally still useful for HTTP vs SPDY (this CL
doesn't remove all places we record ConnectionType). For SSL versions, the data
is fine, just very difficult to use because the denominator is wrong. I figured
it'd be useful to keep it visible in UMA until the new one has cycled through
enough that we don't need to look at the old one.

[Ryan Sleevi, 2015-02-19 18:40:48]

https://codereview.chromium.org/941743002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/941743002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58539: +  <int value="7">QUIC</int>
Why QUIC? That's not an SSL version.

[davidben, 2015-02-19 19:09:10]

https://codereview.chromium.org/941743002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/941743002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58539: +  <int value="7">QUIC</int>
On 2015/02/19 18:40:48, Ryan Sleevi wrote:
> Why QUIC? That's not an SSL version.

The enum includes an entry for QUIC, so I figured I'd just transcribe it, in
case we reuse the enum for another metric.

https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_connec...

[Ryan Sleevi, 2015-02-19 19:13:24]

On 2015/02/19 19:09:10, David Benjamin wrote:
>
https://codereview.chromium.org/941743002/diff/20001/tools/metrics/histograms...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/941743002/diff/20001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:58539: +  <int value="7">QUIC</int>
> On 2015/02/19 18:40:48, Ryan Sleevi wrote:
> > Why QUIC? That's not an SSL version.
> 
> The enum includes an entry for QUIC, so I figured I'd just transcribe it, in
> case we reuse the enum for another metric.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_connec...

I... what.... hrm... "Square hole, round peg" vs "if it fits, I sits!"

In that case, perhaps renaming the histogram SSLOrQuicVersion is more...
appropriate?

[davidben, 2015-02-19 19:19:35]

On 2015/02/19 19:13:24, Ryan Sleevi wrote:
> On 2015/02/19 19:09:10, David Benjamin wrote:
> >
>
https://codereview.chromium.org/941743002/diff/20001/tools/metrics/histograms...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/941743002/diff/20001/tools/metrics/histograms...
> > tools/metrics/histograms/histograms.xml:58539: +  <int value="7">QUIC</int>
> > On 2015/02/19 18:40:48, Ryan Sleevi wrote:
> > > Why QUIC? That's not an SSL version.
> > 
> > The enum includes an entry for QUIC, so I figured I'd just transcribe it, in
> > case we reuse the enum for another metric.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_connec...
> 
> I... what.... hrm... "Square hole, round peg" vs "if it fits, I sits!"
> 
> In that case, perhaps renaming the histogram SSLOrQuicVersion is more...
> appropriate?

Done.

[Ryan Sleevi, 2015-02-19 19:24:02]

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_pool.cc:504: UMA_HISTOGRAM_ENUMERATION(
BUG: You're now not histograming for HPKP mismatches.

[davidben, 2015-02-19 19:32:20]

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_pool.cc:504: UMA_HISTOGRAM_ENUMERATION(
On 2015/02/19 19:24:02, Ryan Sleevi wrote:
> BUG: You're now not histograming for HPKP mismatches.

Is that a problem? Seems like a bug in the old code. We're also not measuring
Net.SSL_CipherSuite for such connections either. If the connection wasn't going
to be  allowed anyway, we probably don't want to count it was succeeding under
some version. The original metrics predate key pinning.

(If the user later clicks through---HPKP not allowing click-through
aside---there'll be a new connection and we'll count it there.)

[Ryan Sleevi, 2015-02-19 20:12:06]

On 2015/02/19 19:32:20, David Benjamin wrote:
>
https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
> File net/socket/ssl_client_socket_pool.cc (right):
> 
>
https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
> net/socket/ssl_client_socket_pool.cc:504: UMA_HISTOGRAM_ENUMERATION(
> On 2015/02/19 19:24:02, Ryan Sleevi wrote:
> > BUG: You're now not histograming for HPKP mismatches.
> 
> Is that a problem? Seems like a bug in the old code. We're also not measuring
> Net.SSL_CipherSuite for such connections either. If the connection wasn't
going
> to be  allowed anyway, we probably don't want to count it was succeeding under
> some version. The original metrics predate key pinning.

Well, you're trying to fix the original metrics, aren't you? :) Seems like you
should fix it to ensure we're always logging :)

[Ilya Sherman, 2015-02-19 20:18:35]

On 2015/02/19 17:53:02, David Benjamin wrote:
> isherman: histograms.xml. Is the normal procedure here to obsolete the old
> histogram now or later? I thought I'd claim it just so I could see it in the
> dashboard. The old histogram is nominally still useful for HTTP vs SPDY (this
CL
> doesn't remove all places we record ConnectionType). For SSL versions, the
data
> is fine, just very difficult to use because the denominator is wrong. I
figured
> it'd be useful to keep it visible in UMA until the new one has cycled through
> enough that we don't need to look at the old one.

Either way is fine.  Note that you can still view <obsolete> (deprecated)
histograms on the UMA dashboard -- you just need to check a checkbox to do so.

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_pool.cc:507: SSL_CONNECTION_VERSION_MAX);
It looks like you're re-using an enum that's currently used outside of UMA. 
Please either annotate that enum documenting that it's used to back an UMA
histogram, and hence should be treated as append-only; or create a new enum
specifically to back the UMA histogram.

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19724: +<histogram
name="Net.SSL_Version" enum="SSLOrQUICVersion">
Nit: Please use CamelCase, i.e. no underscore in "SSLVersion".

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19726: +  <summary>The SSL/TLS version
that was negotiated.</summary>
Please document when this is recorded.

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58456: +  <summary>SSL/TLS protocol
versions</summary>
nit: You can leave this summary if you want to, but it's not really going to be
shown on the dashboard (except on mouse hover).  Most enums omit a summary.

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58457: +  <int value="0">Unknown</int>
nit: Please use label attributes.  As you've written it, the dashboard would
list values 0-7, and strings like "Unknown" would be shown only on hover.

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58463: +  <int value="7">QUIC</int>
What happened to value 6?

[davidben, 2015-02-19 21:22:34]

> > > BUG: You're now not histograming for HPKP mismatches.
> > 
> > Is that a problem? Seems like a bug in the old code. We're also not
measuring
> > Net.SSL_CipherSuite for such connections either. If the connection wasn't
going
> > to be  allowed anyway, we probably don't want to count it was succeeding
under
> > some version. The original metrics predate key pinning.
> 
> Well, you're trying to fix the original metrics, aren't you? :) Seems like you
> should fix it to ensure we're always logging :)

Sorry, that was unclear.

I'm trying to replace the ConnectionType metrics because they're screwy. The
currently DO NOT log for bad certificates, DO log for click-through, and DO log
for pinning failure. These metrics predate key pinning, so I wouldn't take their
behavior w.r.t. key pinning as canonical.

Net.SSL_CipherSuite and the metrics near it work differently. They log based on
whether Connect returns net::OK[*]. So they DO NOT log for bad certificates, DO
log for click-through, and DO NOT log for pinning failure.

The ConnectionType metrics make no sense. We should either log only when all
certificate checks pass or ignore all of them. I think the Net.SSL_CipherSuite
semantics are pretty reasonable---we log for successful SSLClientSocket
connections, which includes all steps of that puzzle. If we were trying to
watch, say, SSL 3.0 usage to determine if we can remove it, we don't really care
if there's a HUGE number of SSL 3.0 connections in the wild that fail HPKP
checks. They didn't work anyway. So I've made this new metric consistent with
those.

[*] Except that, looking at that more closely, there seems to be a bug in that
code. That's not what you were referring to, is it? The check is result == OK ||
IgnoreCertError(result, flags), but IgnoreCertError's implementation never
actually checks IsCertificateError(result). That seems pretty bogus, though it
only affects --ignore-certificate-errors. I'll go fix that separately since it
affects the entire block of code and probably requires more testing and review
of how IgnoreCertError is used.
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli...

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_pool.cc:507: SSL_CONNECTION_VERSION_MAX);
On 2015/02/19 20:18:35, Ilya Sherman wrote:
> It looks like you're re-using an enum that's currently used outside of UMA. 
> Please either annotate that enum documenting that it's used to back an UMA
> histogram, and hence should be treated as append-only; or create a new enum
> specifically to back the UMA histogram.

Done.

Ryan: I figured it would be okay to stabilize these flags since they're already
persisted to disk. But conceivably we could version them on disk. Do you agree
that this is fine?

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19724: +<histogram
name="Net.SSL_Version" enum="SSLOrQUICVersion">
On 2015/02/19 20:18:35, Ilya Sherman wrote:
> Nit: Please use CamelCase, i.e. no underscore in "SSLVersion".

Done. (I was wondering which was the right convention... //net's got a mix.)

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19726: +  <summary>The SSL/TLS version
that was negotiated.</summary>
On 2015/02/19 20:18:35, Ilya Sherman wrote:
> Please document when this is recorded.

Done. Also added to the metric I copied this text from.

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58456: +  <summary>SSL/TLS protocol
versions</summary>
On 2015/02/19 20:18:35, Ilya Sherman wrote:
> nit: You can leave this summary if you want to, but it's not really going to
be
> shown on the dashboard (except on mouse hover).  Most enums omit a summary.

Done.

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58457: +  <int value="0">Unknown</int>
On 2015/02/19 20:18:35, Ilya Sherman wrote:
> nit: Please use label attributes.  As you've written it, the dashboard would
> list values 0-7, and strings like "Unknown" would be shown only on hover.

Oops. Not sure where I got that spelling from. Done.

https://codereview.chromium.org/941743002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:58463: +  <int value="7">QUIC</int>
On 2015/02/19 20:18:35, Ilya Sherman wrote:
> What happened to value 6?

It's apparently reserved for TLS 1.3 but not actually allocated yet? Yeah, I
dunno. I didn't write that kooky enum.

https://code.google.com/p/chromium/codesearch#chromium/src/net/ssl/ssl_connec...

[Ilya Sherman, 2015-02-19 22:08:23]

Histograms LGMT -- thanks.

https://codereview.chromium.org/941743002/diff/50001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/941743002/diff/50001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19908: +<histogram name="Net.SSLVersion"
enum="SSLOrQUICVersion">
Sorry, I see now that you were matching the convention of the other Net.SSL
histograms by using an underscore.  I suppose that matching the local convention
is more important than matching the general style preference, so I retract my
request to drop the underscore.

[Ryan Sleevi, 2015-02-19 22:18:51]

lgtm

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_pool.cc (right):

https://codereview.chromium.org/941743002/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_pool.cc:507: SSL_CONNECTION_VERSION_MAX);
On 2015/02/19 21:22:34, David Benjamin wrote:
> On 2015/02/19 20:18:35, Ilya Sherman wrote:
> > It looks like you're re-using an enum that's currently used outside of UMA. 
> > Please either annotate that enum documenting that it's used to back an UMA
> > histogram, and hence should be treated as append-only; or create a new enum
> > specifically to back the UMA histogram.
> 
> Done.
> 
> Ryan: I figured it would be okay to stabilize these flags since they're
already
> persisted to disk. But conceivably we could version them on disk. Do you agree
> that this is fine?

Yeah.

[davidben, 2015-02-19 22:19:07]

> Histograms LGMT -- thanks.

(Did you mean to swap some characters there? :-) )

https://codereview.chromium.org/941743002/diff/50001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/941743002/diff/50001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:19908: +<histogram name="Net.SSLVersion"
enum="SSLOrQUICVersion">
On 2015/02/19 22:08:23, Ilya Sherman wrote:
> Sorry, I see now that you were matching the convention of the other Net.SSL
> histograms by using an underscore.  I suppose that matching the local
convention
> is more important than matching the general style preference, so I retract my
> request to drop the underscore.

Eh, we have some of both. It got chopped off by the diff, but there's a bunch of
underscore-less ones. As long as we have a mix, may as well make the new ones
match the general preference.

[Ilya Sherman, 2015-02-19 23:00:19]

LGTM for really reals.

[davidben, 2015-02-19 23:01:55]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-02-19 23:02:49]

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

[commit-bot: I haz the power, 2015-02-19 23:07:10]

Committed patchset #4 (id:50001)

[commit-bot: I haz the power, 2015-02-19 23:08:08]

Patchset 4 (id:??) landed as
https://crrev.com/9ff250e0023b2f87e0fddd7eadbef7be3ecc3882
Cr-Commit-Position: refs/heads/master@{#317161}