QUIC - track CHLO's reject reason for secure QUIC vs insecure QUIC.

QUIC - track CHLO's reject reason for secure QUIC vs insecure QUIC.

R=rch@chromium.org, asvitkine@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286150

[ramant (doing other things), 2014-07-25 02:33:07]

Separated reject reasons for secure vs insecure QUIC per conversations with avd
and rch.

thanks
raman

[ramant (doing other things), 2014-07-25 02:49:06]

https://codereview.chromium.org/413363005/diff/1/net/quic/crypto/quic_crypto_...
File net/quic/crypto/quic_crypto_client_config.h (right):

https://codereview.chromium.org/413363005/diff/1/net/quic/crypto/quic_crypto_...
net/quic/crypto/quic_crypto_client_config.h:194: bool is_https,
Should we check if rej has kCertificateTag or not instead of passing is_https as
argument from server_id_.is_https()?

[ramant (doing other things), 2014-07-28 17:24:20]

Hi Ryan and Alexi,
  PTAL when you have some time.

thanks much
raman

[Alexei Svitkine (slow), 2014-07-28 17:46:51]

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

https://codereview.chromium.org/413363005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:14918: +<histogram
name="Net.QuicClientHelloRejectReasonsInSecure"
Nit: Suggest naming these:

Net.QuicClientHelloRejectReasons.InSecure

and

Net.QuicClientHelloRejectReasons.Secure

Then, you can actually keep the existing histogram definition about and simply
add a histogram_suffixes tag over it (see examples in this file) with suffixes
InSecure and Secure (and separator=".").

https://codereview.chromium.org/413363005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:14923: +    inscure QUIC.
Nit: insecure (typo)

[ramant (doing other things), 2014-07-28 18:43:33]

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

https://codereview.chromium.org/413363005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:14918: +<histogram
name="Net.QuicClientHelloRejectReasonsInSecure"
On 2014/07/28 17:46:50, Alexei Svitkine wrote:
> Nit: Suggest naming these:
> 
> Net.QuicClientHelloRejectReasons.InSecure
> 
> and
> 
> Net.QuicClientHelloRejectReasons.Secure
> 
> Then, you can actually keep the existing histogram definition about and simply
> add a histogram_suffixes tag over it (see examples in this file) with suffixes
> InSecure and Secure (and separator=".").

Thanks for the above suggestion.

Done.

https://codereview.chromium.org/413363005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:14923: +    inscure QUIC.
On 2014/07/28 17:46:50, Alexei Svitkine wrote:
> Nit: insecure (typo)

Done.

[Alexei Svitkine (slow), 2014-07-28 19:08:52]

lgtm

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

https://codereview.chromium.org/413363005/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:51085: +  <suffix name="InSecure"
label="for insecure QUIC."/>
Nit: Insecure is one word so maybe change from InSecure to Insecure (both here
and in the code).

[ramant (doing other things), 2014-07-28 19:25:47]

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

https://codereview.chromium.org/413363005/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:51085: +  <suffix name="InSecure"
label="for insecure QUIC."/>
On 2014/07/28 19:08:52, Alexei Svitkine wrote:
> Nit: Insecure is one word so maybe change from InSecure to Insecure (both here
> and in the code).

Done.

[wtc, 2014-07-29 01:37:10]

Patch set 4 LGTM.

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

https://codereview.chromium.org/413363005/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:14918: +    The reject reasons for
QUIC's CHLO (client hello) message from server

Did you delete the period accidentally?

If the label for the suffix will be appended, should the label begin with a
space?

https://codereview.chromium.org/413363005/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:51083: +<histogram_suffixes
name="NetQuicClientHelloRejectReasons" separator=".">

Nit: "NetQuicClientHelloRejectReasons" seems like a poor name for the suffix. It
seems that the suffix name should mention whether QUIC is secure or not.

[ramant (doing other things), 2014-07-29 03:55:37]

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

https://codereview.chromium.org/413363005/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:14918: +    The reject reasons for
QUIC's CHLO (client hello) message from server
On 2014/07/29 01:37:10, wtc wrote:
> 
> Did you delete the period accidentally?
> 
> If the label for the suffix will be appended, should the label begin with a
> space?

My understanding is, it adds a space when it concatenates histogram_suffixes

https://codereview.chromium.org/413363005/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:51083: +<histogram_suffixes
name="NetQuicClientHelloRejectReasons" separator=".">
On 2014/07/29 01:37:10, wtc wrote:
> 
> Nit: "NetQuicClientHelloRejectReasons" seems like a poor name for the suffix.
It
> seems that the suffix name should mention whether QUIC is secure or not.

Updated the name.

(sorry if already known)
name="NetQuicClientHelloRejectReasons" is not used in the code (my understanding
is, it is more to differentiate histogram_suffixes).

Histogram names are
  Net.QuicClientHelloRejectReasons.Secure
  Net.QuicClientHelloRejectReasons.Insecure

[ramant (doing other things), 2014-07-29 03:55:41]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-07-29 03:56:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/413363005/80001

[commit-bot: I haz the power, 2014-07-29 09:44:01]

Change committed as 286150