Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
342863005:
QUIC - Record reject reasons for CHLO message. (Closed)
|
Created:
6 years, 6 months ago by ramant (doing other things) Modified:
6 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, wtc, avd, Mark P Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionQUIC - Record reject reasons for CHLO message.
Added "Net.QuicClientHelloRejectReasons" histogram to track
the reject reason in UMA. Currently it tracks only reject
reason.
Once we have more data, will add the multiple reject reasons
to histograms.xml (instead of listing all potential
combinations). We are guessing, that we would have few
failures due to multiple errors.
R=jar@chromium.org, rch@chromium.org, asvitkine@chromium.org, wtc@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283816
Patch Set 1 #Patch Set 2 : minor cleanup #Patch Set 3 : fixed lint errors #
Total comments: 4
Patch Set 4 : added COMPILE_ASSERT #
Total comments: 6
Patch Set 5 : mapping table for handshake failure to a shifted number #
Total comments: 2
Patch Set 6 : Minor cleanup - deleted local enum #Patch Set 7 : separated histograms.xml changes into a separate CL #
Total comments: 2
Patch Set 8 : Added a comment reject reasons can not be changed and it is uploaded to UMA #
Total comments: 8
Patch Set 9 : merge with internal source changes #Patch Set 10 : UMA histogram #
Total comments: 1
Patch Set 11 : merging with TOT #Messages
Total messages: 35 (0 generated)
lgtm https://codereview.chromium.org/342863005/diff/40001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/40001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:615: SOURCE_ADDRESS_TOKEN_SHIFT = 20, If you were really excited, you could add some compile_asserts that these values are large enough to include the number of codes in each category. Probably not worth it, though. https://codereview.chromium.org/342863005/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/342863005/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:14494: +<histogram name="Net.QuicClientHelloRejectReasons" enum="QuicRejectReasons"> I don't think that the units are correct here, since it's actually storing a binary OR of reasons. That being said, perhaps this is the best units you could use?
https://codereview.chromium.org/342863005/diff/40001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/40001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:615: SOURCE_ADDRESS_TOKEN_SHIFT = 20, On 2014/06/21 03:02:29, Ryan Hamilton wrote: > If you were really excited, you could add some compile_asserts that these values > are large enough to include the number of codes in each category. Probably not > worth it, though. Done. https://codereview.chromium.org/342863005/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/342863005/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:14494: +<histogram name="Net.QuicClientHelloRejectReasons" enum="QuicRejectReasons"> On 2014/06/21 03:02:29, Ryan Hamilton wrote: > I don't think that the units are correct here, since it's actually storing a > binary OR of reasons. That being said, perhaps this is the best units you could > use? Correct. jar suggested we could add the other enums after we get the data (instead on instantiating every possibility).
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/342863005/60001
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/342863005/80001
isherman@: jar is on vacation. Could you take a look at histograms.xml changes. thanks raman
https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:600: packed_error |= RejectReasonToPackedError(reason); Why this complicated scheme instead of logging |reason| itself in each iteration of this loop? Is it because you are actually interested in the combinations?
https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:600: packed_error |= RejectReasonToPackedError(reason); On 2014/06/23 18:33:08, Alexei Svitkine wrote: > Why this complicated scheme instead of logging |reason| itself in each iteration > of this loop? Is it because you are actually interested in the combinations? Correct.
https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:610: HandshakeFailureReason reason) { It's very confusing that this enum has the same name as the one in net/quic/quic_client_session.cc which is unrelated. Not for this CL, but consider renaming one of the two in a follow-up CL to make things less confusing. https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:612: CLIENT_NONCE_SHIFT = 5, TBH, I don't think this is very easy to follow. I think it would be simpler to have a simple mapping from each value in |HandshakeFailureReason| to a corresponding power of 2. Then, this can function can simply be a switch on all the values. This way, you won't have a problem of one of your pre-selected ranges becoming too small for a set of values, if too many values are added to that section. It would also be easier to decode IMHO, since the mapping will be in the code.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:610: HandshakeFailureReason reason) { On 2014/06/23 20:06:10, Alexei Svitkine wrote: > It's very confusing that this enum has the same name as the one in > net/quic/quic_client_session.cc which is unrelated. Not for this CL, but > consider renaming one of the two in a follow-up CL to make things less > confusing. Thanks. Will make that change in the internal code and will upload a separate CL. https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:612: CLIENT_NONCE_SHIFT = 5, On 2014/06/23 20:06:10, Alexei Svitkine wrote: > TBH, I don't think this is very easy to follow. > > I think it would be simpler to have a simple mapping from each value in > |HandshakeFailureReason| to a corresponding power of 2. Then, this can function > can simply be a switch on all the values. > > This way, you won't have a problem of one of your pre-selected ranges becoming > too small for a set of values, if too many values are added to that section. > > It would also be easier to decode IMHO, since the mapping will be in the code. Defined the mapping. Is this what you have in mind?
On 2014/06/23 22:08:03, ramant wrote: > https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... > File net/quic/crypto/quic_crypto_client_config.cc (right): > > https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... > net/quic/crypto/quic_crypto_client_config.cc:610: HandshakeFailureReason reason) > { > On 2014/06/23 20:06:10, Alexei Svitkine wrote: > > It's very confusing that this enum has the same name as the one in > > net/quic/quic_client_session.cc which is unrelated. Not for this CL, but > > consider renaming one of the two in a follow-up CL to make things less > > confusing. > > Thanks. Will make that change in the internal code and will upload a separate > CL. > > https://codereview.chromium.org/342863005/diff/80001/net/quic/crypto/quic_cry... > net/quic/crypto/quic_crypto_client_config.cc:612: CLIENT_NONCE_SHIFT = 5, > On 2014/06/23 20:06:10, Alexei Svitkine wrote: > > TBH, I don't think this is very easy to follow. > > > > I think it would be simpler to have a simple mapping from each value in > > |HandshakeFailureReason| to a corresponding power of 2. Then, this can > function > > can simply be a switch on all the values. > > > > This way, you won't have a problem of one of your pre-selected ranges becoming > > too small for a set of values, if too many values are added to that section. > > > > It would also be easier to decode IMHO, since the mapping will be in the code. > > Defined the mapping. Is this what you have in mind? rch@: could you please take another look at QUIC changes (will be back poring them into the internal source tree). thanks raman
lgtm https://codereview.chromium.org/342863005/diff/100001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/100001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:636: return HANDSHAKE_OK_SHIFTED; As discussed offline, might as well just make the return values explicit here.
Thanks Ryan. Deleted the enum. https://codereview.chromium.org/342863005/diff/100001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/100001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:636: return HANDSHAKE_OK_SHIFTED; On 2014/06/23 23:09:43, Ryan Hamilton wrote: > As discussed offline, might as well just make the return values explicit here. Done.
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/342863005/140001
https://codereview.chromium.org/342863005/diff/140001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/140001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:609: uint32 QuicCryptoClientConfig::RejectReasonToPackedError( Please explicitly comment that because these numbers are uploaded to UMA, they cannot be changed.
https://codereview.chromium.org/342863005/diff/140001/net/quic/crypto/quic_cr... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/140001/net/quic/crypto/quic_cr... net/quic/crypto/quic_crypto_client_config.cc:609: uint32 QuicCryptoClientConfig::RejectReasonToPackedError( On 2014/06/24 18:25:49, Mark P wrote: > Please explicitly comment that because these numbers are uploaded to UMA, they > cannot be changed. Done.
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/342863005/70005
The CQ bit was unchecked by rtenneti@chromium.org
Review comments on patch set 8: RejectReasonToPackedError has a bug: some output values have more than one 1 bit set. https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.h (right): https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.h:16: #include "net/quic/crypto/quic_crypto_server_config.h" Nit: it seems strange for quic_crypto_client_config.h to include quic_crypto_server_config.h. We should create a new header for the HandshakeFailureReason type definition, or define HandshakeFailureReason in some other header, such as crypto_protocol.h or crypto_handshake.h. https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.h:212: // Packs the |reason| into an uint32 value. Nit: an => a You may want to point out that only one bit is set in the output value, so the output values can be XOR'ed together. https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.h:213: uint32 RejectReasonToPackedError(HandshakeFailureReason reason); I guess this method is public just to make it available to the unit tests?
https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:615: case CLIENT_NONCE_UNKNOWN_FAILURE: Let's renumber these enum values to avoid the gaps and then simply do: return 1 << reason;
On 2014/06/24 21:09:24, Ryan Hamilton wrote: > https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... > File net/quic/crypto/quic_crypto_client_config.cc (right): > > https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... > net/quic/crypto/quic_crypto_client_config.cc:615: case > CLIENT_NONCE_UNKNOWN_FAILURE: > Let's renumber these enum values to avoid the gaps and then simply do: > > return 1 << reason; Will make the above change in the internal source tree first and then will update this CL. Will submit this for review after merging the internal changes CL into chromium. Will put this CL in ice box for a while. Many many thanks Wan-Teh and Ryan for the comments.
PTAL. Other CLs that this CL depends on: https://codereview.chromium.org/388333005/ https://codereview.chromium.org/354943002/ https://codereview.chromium.org/359823002/ https://codereview.chromium.org/345563009/ https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.cc (right): https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.cc:615: case CLIENT_NONCE_UNKNOWN_FAILURE: On 2014/06/24 21:09:24, Ryan Hamilton wrote: > Let's renumber these enum values to avoid the gaps and then simply do: > > return 1 << reason; Done. https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... File net/quic/crypto/quic_crypto_client_config.h (right): https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.h:16: #include "net/quic/crypto/quic_crypto_server_config.h" On 2014/06/24 21:02:16, wtc wrote: > > Nit: it seems strange for quic_crypto_client_config.h to include > quic_crypto_server_config.h. We should create a new header for the > HandshakeFailureReason type definition, or define HandshakeFailureReason in some > other header, such as crypto_protocol.h or crypto_handshake.h. Done. https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.h:212: // Packs the |reason| into an uint32 value. On 2014/06/24 21:02:16, wtc wrote: > > Nit: an => a > > You may want to point out that only one bit is set in the output value, so the > output values can be XOR'ed together. Deleted this method. https://codereview.chromium.org/342863005/diff/70005/net/quic/crypto/quic_cry... net/quic/crypto/quic_crypto_client_config.h:213: uint32 RejectReasonToPackedError(HandshakeFailureReason reason); On 2014/06/24 21:02:16, wtc wrote: > > I guess this method is public just to make it available to the unit tests? Deleted this method. https://codereview.chromium.org/342863005/diff/210001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/342863005/diff/210001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:45228: + <int value="1" label="CLIENT_NONCE_UNKNOWN_FAILURE"/> The reasons match the enums listed in net/quic/crypto/crypto_handshake.h
lgtm
Patch set 10 LGTM.
The CQ bit was checked by rtenneti@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rtenneti@chromium.org/342863005/230001
Message was sent while issue was closed.
Change committed as 283816 |
