QUIC - Record reject reasons for CHLO message.

net/quic/crypto/quic_crypto_client_config.cc

Index: net/quic/crypto/quic_crypto_client_config.cc
diff --git a/net/quic/crypto/quic_crypto_client_config.cc b/net/quic/crypto/quic_crypto_client_config.cc
index b37befea03e661e50600e9ec3b5b2136cac00d8d..87ab1bf08fc7807794e24d92a05f2c91462681f0 100644
--- a/net/quic/crypto/quic_crypto_client_config.cc
+++ b/net/quic/crypto/quic_crypto_client_config.cc
@@ -4,6 +4,7 @@
 
 #include "net/quic/crypto/quic_crypto_client_config.h"
 
+#include "base/metrics/sparse_histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "net/quic/crypto/cert_compressor.h"
@@ -592,16 +593,57 @@ QuicErrorCode QuicCryptoClientConfig::ProcessRejection(
   COMPILE_ASSERT(sizeof(QuicTag) == sizeof(uint32), header_out_of_sync);
   if (rej.GetTaglist(kRREJ, &reject_reasons,
                      &num_reject_reasons) == QUIC_NO_ERROR) {
-#if defined(DEBUG)
+    uint32 packed_error = 0;
     for (size_t i = 0; i < num_reject_reasons; ++i) {
-      DVLOG(1) << "Reasons for rejection: " << reject_reasons[i];
+      HandshakeFailureReason reason =
+          static_cast<HandshakeFailureReason>(reject_reasons[i]);
+      packed_error |= RejectReasonToPackedError(reason);

[Alexei Svitkine (slow), 2014/06/23 18:33:08]

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?

[ramant (doing other things), 2014/06/23 18:40:11]

Correct.

     }
-#endif
+    UMA_HISTOGRAM_SPARSE_SLOWLY("Net.QuicClientHelloRejectReasons",
+                                packed_error);
   }
 
   return QUIC_NO_ERROR;
 }
 
+uint32 QuicCryptoClientConfig::RejectReasonToPackedError(
+    HandshakeFailureReason reason) {

[Alexei Svitkine (slow), 2014/06/23 20:06:10]

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.

[ramant (doing other things), 2014/06/23 22:08:03]

Thanks. Will make that change in the internal code and will upload a separate
CL.

+  enum RejectReasonShift {
+    CLIENT_NONCE_SHIFT = 5,

[Alexei Svitkine (slow), 2014/06/23 20:06:10]

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.

[ramant (doing other things), 2014/06/23 22:08:03]

Defined the mapping. Is this what you have in mind?

+    SERVER_NONCE_SHIFT = 10,
+    SERVER_CONFIG_SHIFT = 15,
+    SOURCE_ADDRESS_TOKEN_SHIFT = 20,
+  };
+  COMPILE_ASSERT(CLIENT_NONCE_INVALID_FAILURE - CLIENT_NONCE_UNKNOWN_FAILURE <
+                 CLIENT_NONCE_SHIFT, client_nonce_failure_reasons_too_big);
+  COMPILE_ASSERT(SERVER_NONCE_NOT_UNIQUE_FAILURE -
+                 SERVER_NONCE_INVALID_FAILURE < SERVER_NONCE_SHIFT,
+                 server_nonce_failure_reasons_too_big);
+  COMPILE_ASSERT(SERVER_CONFIG_UNKNOWN_CONFIG_FAILURE -
+                 SERVER_CONFIG_INCHOATE_HELLO_FAILURE < SERVER_CONFIG_SHIFT,
+                 server_config_failure_reasons_too_big);
+  COMPILE_ASSERT(SOURCE_ADDRESS_TOKEN_EXPIRED_FAILURE -
+                 SOURCE_ADDRESS_TOKEN_INVALID_FAILURE <
+                 SOURCE_ADDRESS_TOKEN_SHIFT,
+                 source_address_token_failure_reasons_too_big);
+
+  if (reason < CLIENT_NONCE_UNKNOWN_FAILURE) {
+    return reason;
+  }
+  if (reason < SERVER_NONCE_INVALID_FAILURE) {
+    return (reason - CLIENT_NONCE_UNKNOWN_FAILURE + 1) << CLIENT_NONCE_SHIFT;
+  }
+  if (reason < SERVER_CONFIG_INCHOATE_HELLO_FAILURE) {
+    return (reason - SERVER_NONCE_INVALID_FAILURE + 1)  << SERVER_NONCE_SHIFT;
+  }
+  if (reason < SOURCE_ADDRESS_TOKEN_INVALID_FAILURE) {
+    return (reason - SERVER_CONFIG_INCHOATE_HELLO_FAILURE + 1) <<
+        SERVER_CONFIG_SHIFT;
+  }
+  return (reason - SOURCE_ADDRESS_TOKEN_INVALID_FAILURE + 1) <<
+      SOURCE_ADDRESS_TOKEN_SHIFT;
+}
+
 QuicErrorCode QuicCryptoClientConfig::ProcessServerHello(
     const CryptoHandshakeMessage& server_hello,
     QuicConnectionId connection_id,