Gather histogram for received packets indicating losses.

net/quic/quic_connection_logger.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/quic_connection_logger.h"
 
+#include <algorithm>
+#include <string>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "net/base/net_log.h"
 #include "net/quic/crypto/crypto_handshake_message.h"
 #include "net/quic/crypto/crypto_protocol.h"
 #include "net/quic/quic_address_mismatch.h"
 #include "net/quic/quic_socket_address_coder.h"
 
 using base::StringPiece;
 using std::string;
 
 namespace net {
 
 namespace {
+// The number of packets for which status is recorded.
+const size_t MAX_PACKET_STATUS = 100;

[Ryan Hamilton, 2014/03/05 03:48:00]

nit: kMaxPacketStatus, I think, right?

[jar (doing other things), 2014/03/06 03:34:17]

Chromium style guide seems to ask for this format:
http://www.chromium.org/developers/coding-style

[Ryan Hamilton, 2014/03/06 04:47:43]

Do you mean this line:

Though the Google C++ Style Guide now says to use kConstantNaming for enums,
Chromium was written using MACRO_STYLE naming.  Continue to use this style for
consistency.

That seems to be for enum not static variables.  Or are you referring to some
other section of the guide?

[jar (doing other things), 2014/03/07 17:11:48]

Done.

 
 base::Value* NetLogQuicPacketCallback(const IPEndPoint* self_address,
                                       const IPEndPoint* peer_address,
                                       size_t packet_size,
                                       NetLog::LogLevel /* log_level */) {
   base::DictionaryValue* dict = new base::DictionaryValue();
   dict->SetString("self_address", self_address->ToString());
   dict->SetString("peer_address", peer_address->ToString());
   dict->SetInteger("size", packet_size);
   return dict;
@@ skipping 13 matching lines @@
   if (result.status != WRITE_STATUS_OK) {
     dict->SetInteger("net_error", result.error_code);
   }
   return dict;
 }
 
 base::Value* NetLogQuicPacketRetransmittedCallback(
     QuicPacketSequenceNumber old_sequence_number,
     QuicPacketSequenceNumber new_sequence_number,
     NetLog::LogLevel /* log_level */) {
- base::DictionaryValue* dict = new base::DictionaryValue();
- dict->SetString("old_packet_sequence_number",
-                 base::Uint64ToString(old_sequence_number));
- dict->SetString("new_packet_sequence_number",
-                 base::Uint64ToString(new_sequence_number));
- return dict;
+  base::DictionaryValue* dict = new base::DictionaryValue();
+  dict->SetString("old_packet_sequence_number",
+                  base::Uint64ToString(old_sequence_number));
+  dict->SetString("new_packet_sequence_number",
+                  base::Uint64ToString(new_sequence_number));
+  return dict;
 }
 
 base::Value* NetLogQuicPacketHeaderCallback(const QuicPacketHeader* header,
                                             NetLog::LogLevel /* log_level */) {
   base::DictionaryValue* dict = new base::DictionaryValue();
   dict->SetString("guid",
                   base::Uint64ToString(header->public_header.guid));
   dict->SetInteger("reset_flag", header->public_header.reset_flag);
   dict->SetInteger("version_flag", header->public_header.version_flag);
   dict->SetString("packet_sequence_number",
@@ skipping 43 matching lines @@
     NetLog::LogLevel /* log_level */) {
   base::DictionaryValue* dict = new base::DictionaryValue();
   switch (frame->type) {
     case kInterArrival: {
       dict->SetString("type", "InterArrival");
       base::ListValue* received = new base::ListValue();
       dict->Set("received_packets", received);
       for (TimeMap::const_iterator it =
                frame->inter_arrival.received_packet_times.begin();
            it != frame->inter_arrival.received_packet_times.end(); ++it) {
-        std::string value = base::Uint64ToString(it->first) + "@" +
+        string value = base::Uint64ToString(it->first) + "@" +
             base::Uint64ToString(it->second.ToDebuggingValue());
         received->AppendString(value);
       }
       break;
     }
     case kFixRate:
       dict->SetString("type", "FixRate");
       dict->SetInteger("bitrate_in_bytes_per_second",
                        frame->fix_rate.bitrate.ToBytesPerSecond());
       break;
@@ skipping 76 matching lines @@
 
 }  // namespace
 
 QuicConnectionLogger::QuicConnectionLogger(const BoundNetLog& net_log)
     : net_log_(net_log),
       last_received_packet_sequence_number_(0),
       largest_received_packet_sequence_number_(0),
       largest_received_missing_packet_sequence_number_(0),
       out_of_order_recieved_packet_count_(0),
       num_truncated_acks_sent_(0),
-      num_truncated_acks_received_(0) {
+      num_truncated_acks_received_(0),
+      packets_received_(MAX_PACKET_STATUS) {
 }
 
 QuicConnectionLogger::~QuicConnectionLogger() {
   UMA_HISTOGRAM_COUNTS("Net.QuicSession.OutOfOrderPacketsReceived",
                        out_of_order_recieved_packet_count_);
   UMA_HISTOGRAM_COUNTS("Net.QuicSession.TruncatedAcksSent",
                        num_truncated_acks_sent_);
   UMA_HISTOGRAM_COUNTS("Net.QuicSession.TruncatedAcksReceived",
                        num_truncated_acks_received_);
+
+  const size_t last_index = std::min<size_t>(MAX_PACKET_STATUS - 1,

[Ryan Hamilton, 2014/03/05 03:48:00]

nit: We typically just say min(foo, bar);  Is the <size_t> required here?

[jar (doing other things), 2014/03/06 03:34:17]

I think the code may have been wrong, since it was handling uint64 as well as
size_t.  I wrote it out (with one explicit cast) so that there is no chance for
ambiguity.

+      largest_received_packet_sequence_number_);
+  // We never get packet 0, so it is just a sentinel.
+  for (size_t i = 1; i <= last_index; ++i) {
+    if (packets_received_[i]) {
+      UMA_HISTOGRAM_ENUMERATION("Net.QuicSessionPacketReceivedAck", i - 1,

[Ryan Hamilton, 2014/03/05 03:48:00]

Interesting!  I didn't realize ENUMERATION was applicable without actual enums. 
I'll have to remember this for next time.

[jar (doing other things), 2014/03/06 03:34:17]

Yes... it works nicely when what you really want is a linear histogram, with
buckets at every integer.

I ended up changing this to have to call for a LinearHistogram manually... since
I now dynamically form the name.

+                                MAX_PACKET_STATUS);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION("Net.QuicSessionPacketReceivedNack", i - 1,
+                                MAX_PACKET_STATUS);
+    }
+  }
 }
 
 void QuicConnectionLogger::OnFrameAddedToPacket(const QuicFrame& frame) {
   switch (frame.type) {
     case PADDING_FRAME:
       break;
     case STREAM_FRAME:
       net_log_.AddEvent(
           NetLog::TYPE_QUIC_SESSION_STREAM_FRAME_SENT,
           base::Bind(&NetLogQuicStreamFrameCallback, frame.stream_frame));
@@ skipping 74 matching lines @@
       header.packet_sequence_number) {
     QuicPacketSequenceNumber delta = header.packet_sequence_number -
         largest_received_packet_sequence_number_;
     if (delta > 1) {
       // There is a gap between the largest packet previously received and
       // the current packet.  This indicates either loss, or out-of-order
       // delivery.
       UMA_HISTOGRAM_COUNTS("Net.QuicSession.PacketGapReceived", delta - 1);
     }
     largest_received_packet_sequence_number_ = header.packet_sequence_number;
+    if (largest_received_packet_sequence_number_ < MAX_PACKET_STATUS) {
+      // We never get a packet of zero, but it is safer to handle this in
+      // production and avoiding reporting data from the 0th entry in the
+      // array.
+      DCHECK_GT(largest_received_packet_sequence_number_, 0);
+      packets_received_[largest_received_packet_sequence_number_] = true;

[Ryan Hamilton, 2014/03/05 03:48:00]

nit: i think the style guide says that DCHECK is only for situations that will
NEVER happen, and so handling the condition where the DCHECK is not satisfied is
prohibited.  If you want to use a DCHECK, then I think you should do use
largest_received_packet_sequence_number_ - 1 as the array index.  If you're
afraid of dealing with 0, then I would use an explicit if statement to deal with
this case. 

[Ryan Hamilton, 2014/03/05 03:48:00]

It looks like this code only records packets that arrive in order because of the
"if" on line 336.  Is that what you intended?

[jar (doing other things), 2014/03/06 03:34:17]

I moved around the DCHECK, and it now just checks when I'm recording things into
the histogram.  I think this may comply with the style rules.  I don't handle
the case where the DCHECK is false.

[jar (doing other things), 2014/03/06 03:34:17]

No. That was not intended... I messed this up in my refactors.  

I fixed it to record any packet arrival.

It might be interesting to look for "skipped" packets with another bitset... but
I'll leave that for future exploration.

+    }
   }
   if (header.packet_sequence_number < last_received_packet_sequence_number_) {
     ++out_of_order_recieved_packet_count_;
     UMA_HISTOGRAM_COUNTS("Net.QuicSession.OutOfOrderGapReceived",
                          last_received_packet_sequence_number_ -
                              header.packet_sequence_number);
   }
   last_received_packet_sequence_number_ = header.packet_sequence_number;
 }
 
@@ skipping 122 matching lines @@
 }
 
 void QuicConnectionLogger::OnSuccessfulVersionNegotiation(
     const QuicVersion& version) {
   string quic_version = QuicVersionToString(version);
   net_log_.AddEvent(NetLog::TYPE_QUIC_SESSION_VERSION_NEGOTIATED,
                     NetLog::StringCallback("version", &quic_version));
 }
 
 }  // namespace net