Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2243453002:
Replace SSLProtocolNegotiation histogram with SSLNegotiatedAlpnProtocol. (Closed)
DescriptionReplace SSLProtocolNegotiation histogram with SSLNegotiatedAlpnProtocol.
* Replace sparse histogram recording ALPN/NPN usage and whether there was
overlap or fallback in case of NPN with a new histogram with much fewer
values. This was made possible by NPN being disabled.
* New histogram is recorded with all TLS handshakes, telling us what
percentage of total TLS connections use HTTP/2. (The old histogram was only
recorded when either NPN or ALPN was used, depriving us of this piece of
information.)
* Remove fixed values from enum NextProto; remove commented out historical
values. Note that git cl format puts the new enum on a single line.
BUG=526713
Committed: https://crrev.com/bd442c23edf32d09535f3f4d2fd67bb9f01b69ac
Cr-Commit-Position: refs/heads/master@{#418664}
Patch Set 1 #Patch Set 2 : Oops. #Patch Set 3 : Rebase. #
Total comments: 13
Patch Set 4 : Re: #11. #Patch Set 5 : Rebase. #Patch Set 6 : Rebase. #Patch Set 7 : Re: #18. #
Total comments: 6
Patch Set 8 : Re: #24. #
Total comments: 4
Patch Set 9 : Rebase. #Patch Set 10 : Nit. #Patch Set 11 : Rebase. #
Messages
Total messages: 44 (27 generated)
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bnc@chromium.org changed reviewers: + davidben@chromium.org
David: PTAL. Thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2355: switch (negotiated_protocol_) { Why not just log the kProtoFoo values? https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2357: protocol = ALPN_NOT_USED; [Note this will result in a larger histogram, but it's probably fine?] https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2369: UMA_HISTOGRAM_ENUMERATION("Net.AlpnNegotiatedProtocol", protocol, ALPN_MAX); 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. https://codereview.chromium.org/2243453002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2243453002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26507: + if so, the negotiated protocol. For each TLS handshake, whether ALPN was negotiated and, if so, the negotiated protocol. (The message is called ServerHello, but it's not even going to be in the ServerHello in TLS 1.3. Better to speak in terms of high-level things.)
https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2357: protocol = ALPN_NOT_USED; By the way, this assumes we don't allow unknown ALPN values. But that should be true now.
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
David: PTAL. Thank you. https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2355: switch (negotiated_protocol_) { On 2016/08/17 19:44:11, davidben wrote: > Why not just log the kProtoFoo values? 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? https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2357: protocol = ALPN_NOT_USED; On 2016/08/17 19:44:11, davidben wrote: > [Note this will result in a larger histogram, but it's probably fine?] 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. https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2357: protocol = ALPN_NOT_USED; On 2016/08/17 19:44:56, davidben wrote: > By the way, this assumes we don't allow unknown ALPN values. But that should be > true now. Acknowledged. https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2369: UMA_HISTOGRAM_ENUMERATION("Net.AlpnNegotiatedProtocol", protocol, ALPN_MAX); On 2016/08/17 19:44:11, davidben wrote: > 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. Done. https://codereview.chromium.org/2243453002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2243453002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:26507: + if so, the negotiated protocol. On 2016/08/17 19:44:11, davidben wrote: > For each TLS handshake, whether ALPN was negotiated and, if so, the negotiated > protocol. > > (The message is called ServerHello, but it's not even going to be in the > ServerHello in TLS 1.3. Better to speak in terms of high-level things.) Done.
https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2355: switch (negotiated_protocol_) { On 2016/08/18 13:37:34, Bence wrote: > On 2016/08/17 19:44:11, davidben wrote: > > Why not just log the kProtoFoo values? > > 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? 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. https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2357: protocol = ALPN_NOT_USED; On 2016/08/18 13:37:34, Bence wrote: > On 2016/08/17 19:44:11, davidben wrote: > > [Note this will result in a larger histogram, but it's probably fine?] > > 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. Sounds good.
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
David: PTAL. Thank you. https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2243453002/diff/40001/net/socket/ssl_client_s... net/socket/ssl_client_socket_impl.cc:2355: switch (negotiated_protocol_) { On 2016/08/25 18:10:41, davidben wrote: > On 2016/08/18 13:37:34, Bence wrote: > > On 2016/08/17 19:44:11, davidben wrote: > > > Why not just log the kProtoFoo values? > > > > 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? > > 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. Done.
lgtm with comments https://codereview.chromium.org/2243453002/diff/120001/net/http/http_response... File net/http/http_response_info.cc (right): https://codereview.chromium.org/2243453002/diff/120001/net/http/http_response... net/http/http_response_info.cc:410: case kProtoNumberOfNextProtos: If you keep the count value (see later comment), it should probably be NOTREACHED(). https://codereview.chromium.org/2243453002/diff/120001/net/socket/next_proto.h File net/socket/next_proto.h (right): https://codereview.chromium.org/2243453002/diff/120001/net/socket/next_proto.... net/socket/next_proto.h:21: kProtoNumberOfNextProtos = 4 Instead you can do: kProtoLast = kProtoQUIC1SPDY3, Since it has the same value, the compiler won't grump at you about missing switch-cases. It's also a bit less weird for when we need to stick a hole in there. (It's not really a count.) https://codereview.chromium.org/2243453002/diff/120001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2243453002/diff/120001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:2342: UMA_HISTOGRAM_ENUMERATION("SSL.AlpnNegotiatedProtocol", negotiated_protocol_, We don't have much UMA that starts with "SSL." It's usually Net.SSLBlah (or net.SSL_Blah, but that's apparently not the preferred style anymore. Net.SSLNegotiatedAlpnProtocol?
bnc@chromium.org changed reviewers: + jwd@chromium.org
jwd: PTAL at tools/metrics/histograms/. Thank you. davidben: Thank you. https://codereview.chromium.org/2243453002/diff/120001/net/http/http_response... File net/http/http_response_info.cc (right): https://codereview.chromium.org/2243453002/diff/120001/net/http/http_response... net/http/http_response_info.cc:410: case kProtoNumberOfNextProtos: On 2016/08/27 02:27:58, davidben wrote: > If you keep the count value (see later comment), it should probably be > NOTREACHED(). Changed to kProtoLast, moot. https://codereview.chromium.org/2243453002/diff/120001/net/socket/next_proto.h File net/socket/next_proto.h (right): https://codereview.chromium.org/2243453002/diff/120001/net/socket/next_proto.... net/socket/next_proto.h:21: kProtoNumberOfNextProtos = 4 On 2016/08/27 02:27:58, davidben wrote: > Instead you can do: > > kProtoLast = kProtoQUIC1SPDY3, > > Since it has the same value, the compiler won't grump at you about missing > switch-cases. It's also a bit less weird for when we need to stick a hole in > there. (It's not really a count.) Done. https://codereview.chromium.org/2243453002/diff/120001/net/socket/ssl_client_... File net/socket/ssl_client_socket_impl.cc (right): https://codereview.chromium.org/2243453002/diff/120001/net/socket/ssl_client_... net/socket/ssl_client_socket_impl.cc:2342: UMA_HISTOGRAM_ENUMERATION("SSL.AlpnNegotiatedProtocol", negotiated_protocol_, On 2016/08/27 02:27:58, davidben wrote: > We don't have much UMA that starts with "SSL." It's usually Net.SSLBlah (or > net.SSL_Blah, but that's apparently not the preferred style anymore. > > Net.SSLNegotiatedAlpnProtocol? I'm sorry, I have misunderstood your earlier comment at https://crrev.com/2243453002/diff/40001/net/socket/ssl_client_socket_impl.cc#.... My bad. Done.
LGTM with nit. https://codereview.chromium.org/2243453002/diff/140001/net/socket/next_proto.h File net/socket/next_proto.h (right): https://codereview.chromium.org/2243453002/diff/140001/net/socket/next_proto.... net/socket/next_proto.h:21: kProtoLast = kProtoQUIC1SPDY3 I'd prefer it if you kProtoLast was the boundary value, rather than doing the + 1 later. You can rename it to count or max or something, if you wish.
https://codereview.chromium.org/2243453002/diff/140001/net/socket/next_proto.h File net/socket/next_proto.h (right): https://codereview.chromium.org/2243453002/diff/140001/net/socket/next_proto.... net/socket/next_proto.h:21: kProtoLast = kProtoQUIC1SPDY3 On 2016/08/29 19:44:20, jwd wrote: > I'd prefer it if you kProtoLast was the boundary value, rather than doing the + > 1 later. You can rename it to count or max or something, if you wish. The nice thing about the boundary value is we don't need to stick a dummy case in every enum switch-case. 'count' is confusing if there are holes. 'max' is also a bit ambiguous as to whether it should be max(all_next_proto_values) or just some number such next_proto < max for all next_proto.
jwd: PTAL. Thank you. https://codereview.chromium.org/2243453002/diff/140001/net/socket/next_proto.h File net/socket/next_proto.h (right): https://codereview.chromium.org/2243453002/diff/140001/net/socket/next_proto.... net/socket/next_proto.h:21: kProtoLast = kProtoQUIC1SPDY3 On 2016/08/29 19:44:20, jwd wrote: > I'd prefer it if you kProtoLast was the boundary value, rather than doing the + > 1 later. You can rename it to count or max or something, if you wish. I agree with davidben@. See Patch Set 7 for all the places that a dummy value needs to be handled in every switch() to avoid compiler errors.
jwd: Since your l-g-t-m is contingent on a nit, and David and I have comments about that, I am waiting for your feedback or explicit re-l-g-t-m. Thank you.
Description was changed from ========== Replace SSLProtocolNegotiation histogram with AlpnNegotiatedProtocol. * Replace sparse histogram recording ALPN/NPN usage and whether there was overlap or fallback in case of NPN with a new histogram with much fewer values. This was made possible by NPN being disabled. * New histogram is recorded with all TLS handshakes, telling us what percentage of total TLS connections use HTTP/2. (The old histogram was only recorded when either NPN or ALPN was used, depriving us of this piece of information.) * Remove fixed values from enum NextProto; remove commented out historical values. Note that git cl format puts the new enum on a single line. BUG=526713 ========== to ========== Replace SSLProtocolNegotiation histogram with SSLNegotiatedAlpnProtocol. * Replace sparse histogram recording ALPN/NPN usage and whether there was overlap or fallback in case of NPN with a new histogram with much fewer values. This was made possible by NPN being disabled. * New histogram is recorded with all TLS handshakes, telling us what percentage of total TLS connections use HTTP/2. (The old histogram was only recorded when either NPN or ALPN was used, depriving us of this piece of information.) * Remove fixed values from enum NextProto; remove commented out historical values. Note that git cl format puts the new enum on a single line. BUG=526713 ==========
Ping jwd.
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2243453002/diff/140001/net/socket/next_proto.h File net/socket/next_proto.h (right): https://codereview.chromium.org/2243453002/diff/140001/net/socket/next_proto.... net/socket/next_proto.h:21: kProtoLast = kProtoQUIC1SPDY3 On 2016/08/30 00:51:06, Bence wrote: > On 2016/08/29 19:44:20, jwd wrote: > > I'd prefer it if you kProtoLast was the boundary value, rather than doing the > + > > 1 later. You can rename it to count or max or something, if you wish. > > I agree with davidben@. See Patch Set 7 for all the places that a dummy value > needs to be handled in every switch() to avoid compiler errors. Alright, that's fine.
The CQ bit was checked by bnc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/2243453002/#ps200001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Replace SSLProtocolNegotiation histogram with SSLNegotiatedAlpnProtocol. * Replace sparse histogram recording ALPN/NPN usage and whether there was overlap or fallback in case of NPN with a new histogram with much fewer values. This was made possible by NPN being disabled. * New histogram is recorded with all TLS handshakes, telling us what percentage of total TLS connections use HTTP/2. (The old histogram was only recorded when either NPN or ALPN was used, depriving us of this piece of information.) * Remove fixed values from enum NextProto; remove commented out historical values. Note that git cl format puts the new enum on a single line. BUG=526713 ========== to ========== Replace SSLProtocolNegotiation histogram with SSLNegotiatedAlpnProtocol. * Replace sparse histogram recording ALPN/NPN usage and whether there was overlap or fallback in case of NPN with a new histogram with much fewer values. This was made possible by NPN being disabled. * New histogram is recorded with all TLS handshakes, telling us what percentage of total TLS connections use HTTP/2. (The old histogram was only recorded when either NPN or ALPN was used, depriving us of this piece of information.) * Remove fixed values from enum NextProto; remove commented out historical values. Note that git cl format puts the new enum on a single line. BUG=526713 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Replace SSLProtocolNegotiation histogram with SSLNegotiatedAlpnProtocol. * Replace sparse histogram recording ALPN/NPN usage and whether there was overlap or fallback in case of NPN with a new histogram with much fewer values. This was made possible by NPN being disabled. * New histogram is recorded with all TLS handshakes, telling us what percentage of total TLS connections use HTTP/2. (The old histogram was only recorded when either NPN or ALPN was used, depriving us of this piece of information.) * Remove fixed values from enum NextProto; remove commented out historical values. Note that git cl format puts the new enum on a single line. BUG=526713 ========== to ========== Replace SSLProtocolNegotiation histogram with SSLNegotiatedAlpnProtocol. * Replace sparse histogram recording ALPN/NPN usage and whether there was overlap or fallback in case of NPN with a new histogram with much fewer values. This was made possible by NPN being disabled. * New histogram is recorded with all TLS handshakes, telling us what percentage of total TLS connections use HTTP/2. (The old histogram was only recorded when either NPN or ALPN was used, depriving us of this piece of information.) * Remove fixed values from enum NextProto; remove commented out historical values. Note that git cl format puts the new enum on a single line. BUG=526713 Committed: https://crrev.com/bd442c23edf32d09535f3f4d2fd67bb9f01b69ac Cr-Commit-Position: refs/heads/master@{#418664} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/bd442c23edf32d09535f3f4d2fd67bb9f01b69ac Cr-Commit-Position: refs/heads/master@{#418664} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
