Gather histogram for received packets indicating losses.

net/quic/quic_connection_logger.cc

Index: net/quic/quic_connection_logger.cc
diff --git a/net/quic/quic_connection_logger.cc b/net/quic/quic_connection_logger.cc
index d52debe1faef89f99e2e39c4edfae149fc47a183..8c150e83e2ff5b58a81e20ea3fd4ef882e36766f 100644
--- a/net/quic/quic_connection_logger.cc
+++ b/net/quic/quic_connection_logger.cc
@@ -4,6 +4,9 @@
 
 #include "net/quic/quic_connection_logger.h"
 
+#include <algorithm>
+#include <string>
+
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/metrics/histogram.h"
@@ -22,6 +25,8 @@ 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,
@@ -55,12 +60,12 @@ 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,
@@ -124,7 +129,7 @@ base::Value* NetLogQuicCongestionFeedbackFrameCallback(
       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);
       }
@@ -221,7 +226,8 @@ QuicConnectionLogger::QuicConnectionLogger(const BoundNetLog& net_log)
       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() {
@@ -231,6 +237,19 @@ QuicConnectionLogger::~QuicConnectionLogger() {
                        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) {
@@ -325,6 +344,13 @@ void QuicConnectionLogger::OnPacketHeader(const QuicPacketHeader& header) {
       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_;