QUIC - disable QUIC if packet loss rate is bad for a connection.

QUIC - disable QUIC if packet loss rate is bad for a connection.

Goal - Improve quality of service (QOS) in chromium when QUIC is
enabled. For some users when QUIC is enabled, their access to QUIC
based servers doesn't work well. This could be due to either high loss
rate while using UDP or service providers blacklisting ports 80/443 for
QUIC traffic.

+ One simple approach (which would detect breakage quickly) would be
  to look at the packet loss (received) when the crypto handshake
  is confirmed. When packet loss is "really bad", say over
  50% for 5 handshakes in a row, we will disable QUIC.

+ For a connection is packet loss is higher than the packet loss
  threshold, then we mark that port as recently broken. This would
  require crypto handshake confirmation.

+ Added two parameters - number of "lossy" connections (defaults to 0)
  and packet loss threshold (defaults to 1000%) params to determine
  when to disable QUIC. These parameters will be controlled by finch
  in the next CL.

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

Committed: https://crrev.com/85dcfac2d1c911beb4c1d25caebf679581a9283f
Cr-Commit-Position: refs/heads/master@{#322630}

[ramant (doing other things), 2015-03-20 20:07:47]

Patchset #1 (id:1) has been deleted

[ramant (doing other things), 2015-03-20 21:21:26]

Patchset #1 (id:20001) has been deleted

[ramant (doing other things), 2015-03-20 21:23:54]

rtenneti@chromium.org changed reviewers:
+ asvitkine@chromium.org

[ramant (doing other things), 2015-03-20 21:28:51]

Hi Ryan,
  This is a first cut at what You, Alyssa, Ian and I had discussed.

  Had couple of questions: should we close the connection if handshake is
confirmed (but packet loss rate is high). Should we require_confirmation if we
disable 0-RTT (we don't load server config from disk cache, but it is possible,
highly unlikely that during the first 5 handshakes, one of the handshake was
successful and we have it in the memory cache).

thanks
raman

[ramant (doing other things), 2015-03-20 22:50:59]

https://codereview.chromium.org/1025573002/diff/40001/net/quic/quic_connectio...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/1025573002/diff/40001/net/quic/quic_connectio...
net/quic/quic_connection_logger.cc:772: int
QuicConnectionLogger::PacketLossRate() const {
Is there a better place to find the packet loss rate? Saw we were already
calculating packet loss and used that code.

[Ryan Hamilton, 2015-03-21 00:04:47]

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:630: ++number_of_handshakes_;
I'm not sure that I understand what this member does. Perhaps the factory should
be computing this?

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_client_se...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_client_se...
net/quic/quic_client_session.h:178: int PacketLossRate() const;
I wonder if a float would be more clear?

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:341: int number_of_lossy_handshakes_;
I wonder if this should be a count of "Connections" instead of handshakes. Not
sure.

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:346: bool disable_zero_rtt_;
As we discussed, I don't think we need this.

[ramant (doing other things), 2015-03-21 03:33:27]

Patchset #3 (id:80001) has been deleted

[ramant (doing other things), 2015-03-21 03:46:05]

Hi Ryan,
  Implemented what we had talked offline. Defaulted packet loss rate to 1000%
(this new code will not be used).

  Currently this code doesn't do consecutive connections with high packet loss
rate (it is a TBD).

PTAL.

thanks very much
raman

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_client_se...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_client_se...
net/quic/quic_client_session.cc:630: ++number_of_handshakes_;
On 2015/03/21 00:04:46, Ryan Hamilton wrote:
> I'm not sure that I understand what this member does. Perhaps the factory
should
> be computing this?

Done.

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_client_se...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_client_se...
net/quic/quic_client_session.h:178: int PacketLossRate() const;
On 2015/03/21 00:04:46, Ryan Hamilton wrote:
> I wonder if a float would be more clear?

Done.

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_stream_fa...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:341: int number_of_lossy_handshakes_;
On 2015/03/21 00:04:46, Ryan Hamilton wrote:
> I wonder if this should be a count of "Connections" instead of handshakes. Not
> sure.

Done.

https://codereview.chromium.org/1025573002/diff/60001/net/quic/quic_stream_fa...
net/quic/quic_stream_factory.h:346: bool disable_zero_rtt_;
On 2015/03/21 00:04:46, Ryan Hamilton wrote:
> As we discussed, I don't think we need this.

Done.

[ramant (doing other things), 2015-03-22 01:06:35]

Patchset #3 (id:100001) has been deleted

[Ryan Hamilton, 2015-03-23 02:31:07]

Great progress.

https://codereview.chromium.org/1025573002/diff/120001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/1025573002/diff/120001/net/http/http_network_...
net/http/http_network_session.cc:96: quic_packet_loss_threshold(1000.0f),
Is this value a percent or a fraction?

https://codereview.chromium.org/1025573002/diff/120001/net/http/http_network_...
net/http/http_network_session.cc:266: quic_stream_factory_.quic_is_disabled());
I think we need to track this per-port (or perhaps secure vs insecure).

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_client_s...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_client_s...
net/quic/quic_client_session.cc:597: float packet_loss_rate =
logger_->PacketLossRate() / 10;
Why / 10?

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_client_s...
net/quic/quic_client_session.cc:599:
stream_factory_->OnBadPacketLoss(server_id_);
I wonder if it might make more sense for this to be in the factory instead of
the session since it looks like the factory knows the loss threshold.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_client_s...
net/quic/quic_client_session.cc:606: NotifyFactoryOfSessionClosedLater();
Is there a method the factory could call which would do all of this? Maybe
CloseSessionOnError?

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
net/quic/quic_connection_logger.cc:772: float
QuicConnectionLogger::PacketLossRate() const {
This is the received packet loss rate, right? (As opposed to the sent packet
loss rate). We should probably put it into the method name.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
net/quic/quic_connection_logger.cc:782: return numerator / divisor;
So the reason this code uses * 1000 or / 1000 is because it was going directly
into a histogram as an int. Since you're returning a float, I think you can just
do x / y and then multiply by 1000 down at line 799 when adding to the
histogram.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.h (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
net/quic/quic_connection_logger.h:80: // Return connection's overall packet loss
rate in 1/10th of percent.
Since it's a float, I wonder if this should simply be a percent? Or possibly a
fraction?

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:620: return ERR_QUIC_PROTOCOL_ERROR;
If you do QUIC_HANDSHAKE_FAILED then HttpNetworkTransaction will retry the
request. It also occurs to me that you might consider hooking into the
HttpStreamFactoryImpl to not use QUIC at all if it's disabled.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:196: bool quic_is_disabled() const { return
quic_is_disabled_; }
I think we need this per-port.

[ramant (doing other things), 2015-03-24 02:34:53]

Patchset #4 (id:140001) has been deleted

[ramant (doing other things), 2015-03-24 03:07:22]

Thanks very much Ryan for explaining why we want to do by port. PTAL.

Tested chrome by artificially returning high packet loss rate and verified QUIC
was disabled and HTTP2 is being used.

https://codereview.chromium.org/1025573002/diff/120001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/1025573002/diff/120001/net/http/http_network_...
net/http/http_network_session.cc:96: quic_packet_loss_threshold(1000.0f),
On 2015/03/23 02:31:05, Ryan Hamilton wrote:
> Is this value a percent or a fraction?

Defined it as fraction.

https://codereview.chromium.org/1025573002/diff/120001/net/http/http_network_...
net/http/http_network_session.cc:266: quic_stream_factory_.quic_is_disabled());
On 2015/03/23 02:31:06, Ryan Hamilton wrote:
> I think we need to track this per-port (or perhaps secure vs insecure).

Tracked it by port.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_client_s...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_client_s...
net/quic/quic_client_session.cc:597: float packet_loss_rate =
logger_->PacketLossRate() / 10;
On 2015/03/23 02:31:06, Ryan Hamilton wrote:
> Why / 10?

Remove the percent. Used fraction.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_client_s...
net/quic/quic_client_session.cc:599:
stream_factory_->OnBadPacketLoss(server_id_);
On 2015/03/23 02:31:06, Ryan Hamilton wrote:
> I wonder if it might make more sense for this to be in the factory instead of
> the session since it looks like the factory knows the loss threshold.

Done.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_client_s...
net/quic/quic_client_session.cc:606: NotifyFactoryOfSessionClosedLater();
On 2015/03/23 02:31:06, Ryan Hamilton wrote:
> Is there a method the factory could call which would do all of this? Maybe
> CloseSessionOnError?

Called CloseSessionOnError from QuicStreamFactory.

Done.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
net/quic/quic_connection_logger.cc:772: float
QuicConnectionLogger::PacketLossRate() const {
On 2015/03/23 02:31:06, Ryan Hamilton wrote:
> This is the received packet loss rate, right? (As opposed to the sent packet
> loss rate). We should probably put it into the method name.

Done.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
net/quic/quic_connection_logger.cc:782: return numerator / divisor;
On 2015/03/23 02:31:06, Ryan Hamilton wrote:
> So the reason this code uses * 1000 or / 1000 is because it was going directly
> into a histogram as an int. Since you're returning a float, I think you can
just
> do x / y and then multiply by 1000 down at line 799 when adding to the
> histogram.

Done.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.h (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_connecti...
net/quic/quic_connection_logger.h:80: // Return connection's overall packet loss
rate in 1/10th of percent.
On 2015/03/23 02:31:06, Ryan Hamilton wrote:
> Since it's a float, I wonder if this should simply be a percent? Or possibly a
> fraction?

Changed it to return a fraction.

Done.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:620: return ERR_QUIC_PROTOCOL_ERROR;
On 2015/03/23 02:31:06, Ryan Hamilton wrote:
> If you do QUIC_HANDSHAKE_FAILED then HttpNetworkTransaction will retry the
> request. It also occurs to me that you might consider hooking into the
> HttpStreamFactoryImpl to not use QUIC at all if it's disabled.

As we had talked offline, hooked it to HttpStreamFactoryImpl.

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1025573002/diff/120001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:196: bool quic_is_disabled() const { return
quic_is_disabled_; }
On 2015/03/23 02:31:06, Ryan Hamilton wrote:
> I think we need this per-port.

Done.

[Ryan Hamilton, 2015-03-24 03:43:00]

Looking good! Thanks for doing this!

Can you ping alyssar or ianswett about if we want to do simply total number of
bad connections, or if we want to do N-in-a-row, or some other metric?

Just a few small comments.

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_client_s...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_client_s...
net/quic/quic_client_session.cc:597: (stream_factory_->OnBadPacketLoss(this,
server_id_,
This looks much cleaner now. I like it! I think I might use a different name
than OnBadPacketLoss since that suggests that we already know that the loss is
bad. Perhaps something generic like OnHandshakeConfirmed() ? Naming is hard!

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_connecti...
net/quic/quic_connection_logger.cc:776:
(largest_received_packet_sequence_number_ - num_packets_received_);
Is this inverted? I would expect the denominator to be larger than the
numerator, otherwise the loss would be greater than 1.

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:783:
http_server_properties_->MarkAlternativeServiceRecentlyBroken(
Can you add a comment which explains that this disables 0-RTT?

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:134: const QuicServerId& server_id,
I think |session| has a server_id() method so perhaps you can drop the
|server_id| argument.

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:138: // greater than
|max_number_of_lossy_connections_|.
I think I would simply say
  // Returns true if QUIC is disabled for this port.
Typically it's best to document the interface here, and the implementation in
the method in the .cc file if it needs to be documented.

[ramant (doing other things), 2015-03-24 17:19:24]

Pinged alyssar and Ian. Will submit another CL with finch trial changes after
their recommendation about "simply total number of bad connections vs N-in-a-row
or something else".

PTAL. Thanks much.

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_client_s...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_client_s...
net/quic/quic_client_session.cc:597: (stream_factory_->OnBadPacketLoss(this,
server_id_,
On 2015/03/24 03:43:00, Ryan Hamilton wrote:
> This looks much cleaner now. I like it! I think I might use a different name
> than OnBadPacketLoss since that suggests that we already know that the loss is
> bad. Perhaps something generic like OnHandshakeConfirmed() ? Naming is hard!

+1. Thanks very much for the suggestion (me culpa)

Done.

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_connecti...
File net/quic/quic_connection_logger.cc (right):

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_connecti...
net/quic/quic_connection_logger.cc:776:
(largest_received_packet_sequence_number_ - num_packets_received_);
On 2015/03/24 03:43:00, Ryan Hamilton wrote:
> Is this inverted? I would expect the denominator to be larger than the
> numerator, otherwise the loss would be greater than 1.

facepalm. Goofed up. thanks much.

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:783:
http_server_properties_->MarkAlternativeServiceRecentlyBroken(
On 2015/03/24 03:43:00, Ryan Hamilton wrote:
> Can you add a comment which explains that this disables 0-RTT?

Done.

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.h (right):

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:134: const QuicServerId& server_id,
On 2015/03/24 03:43:00, Ryan Hamilton wrote:
> I think |session| has a server_id() method so perhaps you can drop the
> |server_id| argument.

Done.

https://codereview.chromium.org/1025573002/diff/160001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.h:138: // greater than
|max_number_of_lossy_connections_|.
On 2015/03/24 03:43:00, Ryan Hamilton wrote:
> I think I would simply say
>   // Returns true if QUIC is disabled for this port.
> Typically it's best to document the interface here, and the implementation in
> the method in the .cc file if it needs to be documented.

Done.

[ramant (doing other things), 2015-03-25 00:24:15]

Hi Ryan,
  Added code to disable QUIC if N connections in a row are lossy (as suggested
by Ian).

thanks
raman

[ramant (doing other things), 2015-03-26 17:36:30]

rtenneti@chromium.org changed reviewers:
+ mpearson@chromium.org

[ramant (doing other things), 2015-03-26 17:37:01]

On 2015/03/25 00:24:15, ramant wrote:
> Hi Ryan,
>   Added code to disable QUIC if N connections in a row are lossy (as suggested
> by Ian).
> 
> thanks
> raman

Hi Ryan,
  Would appreciate if you could take another look.

thanks much
raman

[Mark P, 2015-03-26 20:21:43]

https://codereview.chromium.org/1025573002/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1025573002/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19794: +<histogram
name="Net.QuicStreamFactory.QuicIsDisabled">
enum=?

https://codereview.chromium.org/1025573002/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19798: +    and when number of lossy
connections goes over a threshold.
I assume this is "or", not "and", right?

https://codereview.chromium.org/1025573002/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19798: +    and when number of lossy
connections goes over a threshold.
It might be good to say roughly how long this disabling lasts.  Until next
reboot of Chrome?  Permanently?  Until refresh of the page?

[Mark P, 2015-03-26 20:25:00]

https://codereview.chromium.org/1025573002/diff/200001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1025573002/diff/200001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:799:
UMA_HISTOGRAM_CUSTOM_COUNTS("Net.QuicStreamFactory.QuicIsDisabled", port, 0,
Also, is there any way to get the information you need without creating a
histogram with ~1024 buckets?  That's a lot.

If most buckets with be zero AND this is not on the critical path, you can
consider using a SPARSE_HISTOGRAM as an alternative.  Note that this macro is
quite a bit slower than the regular histograms macros, so if you have a solution
that uses the regular macros with fewer buckets, you may want to consider that
first.

[ramant (doing other things), 2015-03-26 21:03:35]

PTAL.

https://codereview.chromium.org/1025573002/diff/200001/net/quic/quic_stream_f...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/1025573002/diff/200001/net/quic/quic_stream_f...
net/quic/quic_stream_factory.cc:799:
UMA_HISTOGRAM_CUSTOM_COUNTS("Net.QuicStreamFactory.QuicIsDisabled", port, 0,
On 2015/03/26 20:25:00, Mark P wrote:
> Also, is there any way to get the information you need without creating a
> histogram with ~1024 buckets?  That's a lot.
> 
> If most buckets with be zero AND this is not on the critical path, you can
> consider using a SPARSE_HISTOGRAM as an alternative.  Note that this macro is
> quite a bit slower than the regular histograms macros, so if you have a
solution
> that uses the regular macros with fewer buckets, you may want to consider that
> first.

Good point. Changed it to UMA_HISTOGRAM_SPARSE_SLOWLY.

https://codereview.chromium.org/1025573002/diff/200001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1025573002/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19794: +<histogram
name="Net.QuicStreamFactory.QuicIsDisabled">
On 2015/03/26 20:21:42, Mark P wrote:
> enum=?

Added "Ports" enum. Does that sound reasonable. Generally we should see port 80
and 443 only. Will add as needed.

https://codereview.chromium.org/1025573002/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19798: +    and when number of lossy
connections goes over a threshold.
On 2015/03/26 20:21:42, Mark P wrote:
> I assume this is "or", not "and", right?

It is an and. We need to have high packet loss for 5 consecutive QUIC
handshakes. Then only we will disable.

https://codereview.chromium.org/1025573002/diff/200001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19798: +    and when number of lossy
connections goes over a threshold.
On 2015/03/26 20:21:42, Mark P wrote:
> It might be good to say roughly how long this disabling lasts.  Until next
> reboot of Chrome?  Permanently?  Until refresh of the page?

Done.

[Mark P, 2015-03-26 21:23:06]

https://codereview.chromium.org/1025573002/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1025573002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19901: +    and when number of
consecutive lossy connections goes over a threshold. QUIC
"It is an and. We need to have high packet loss for 5 consecutive QUIC
handshakes. Then only we will disable."
Then perhaps write:
... is disabled for a port due to repeated lossy connections (high packet loss
on multiple consecutive connections).

(Feel free to edit.  Or drop "on multiple consecutive connections" if you think
it's too redundant.)

https://codereview.chromium.org/1025573002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19902: +    is disabled until next
reboot of Chrome. Recorded once per chrome session.
either omit this last sentence (it's obvious given the two sentence before) or
clarify it (is this on a per-profile basis? is that why you're using the term
session?).

[ramant (doing other things), 2015-03-26 22:10:31]

Thanks for the text (updated histograms.xml).

https://codereview.chromium.org/1025573002/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1025573002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19901: +    and when number of
consecutive lossy connections goes over a threshold. QUIC
On 2015/03/26 21:23:05, Mark P wrote:
> "It is an and. We need to have high packet loss for 5 consecutive QUIC
> handshakes. Then only we will disable."
> Then perhaps write:
> ... is disabled for a port due to repeated lossy connections (high packet loss
> on multiple consecutive connections).
> 
> (Feel free to edit.  Or drop "on multiple consecutive connections" if you
think
> it's too redundant.)

Done.

https://codereview.chromium.org/1025573002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:19902: +    is disabled until next
reboot of Chrome. Recorded once per chrome session.
On 2015/03/26 21:23:05, Mark P wrote:
> either omit this last sentence (it's obvious given the two sentence before) or
> clarify it (is this on a per-profile basis? is that why you're using the term
> session?).

omitted "Recorded once per chrome session."

Done.

[Mark P, 2015-03-26 22:20:35]

histograms.xml lgtm

[Ryan Hamilton, 2015-03-27 03:04:36]

lgtm

[ramant (doing other things), 2015-03-27 16:36:59]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2015-03-27 16:37:26]

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

[commit-bot: I haz the power, 2015-03-27 20:22:35]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2015-03-27 20:23:45]

Patchset 8 (id:??) landed as
https://crrev.com/85dcfac2d1c911beb4c1d25caebf679581a9283f
Cr-Commit-Position: refs/heads/master@{#322630}