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..9b710251f8a103bc045e8b11fbd897b53971ac7c 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,7 +25,6 @@ using std::string;
 namespace net {
 
 namespace {
-
 base::Value* NetLogQuicPacketCallback(const IPEndPoint* self_address,
                                       const IPEndPoint* peer_address,
                                       size_t packet_size,
@@ -55,12 +57,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 +126,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 +223,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),
+      connection_type_(NetworkChangeNotifier::GetConnectionType()) {
 }
 
 QuicConnectionLogger::~QuicConnectionLogger() {
@@ -231,6 +234,8 @@ QuicConnectionLogger::~QuicConnectionLogger() {
                        num_truncated_acks_sent_);
   UMA_HISTOGRAM_COUNTS("Net.QuicSession.TruncatedAcksReceived",
                        num_truncated_acks_received_);
+
+  RecordAckNackHistograms();
 }
 
 void QuicConnectionLogger::OnFrameAddedToPacket(const QuicFrame& frame) {
@@ -326,6 +331,9 @@ void QuicConnectionLogger::OnPacketHeader(const QuicPacketHeader& header) {
     }
     largest_received_packet_sequence_number_ = header.packet_sequence_number;
   }
+  if (header.packet_sequence_number < MAX_PACKET_STATUS) {
+    packets_received_[header.packet_sequence_number] = true;

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

Since you're comparing with <, and not indexing by sequence_number - 1, I think
this means that you'll actually only end up recoding 99 packets, not 100.  Seems
like indexing by -1 would be ideal.

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

Fixed per discussion.

+  }
   if (header.packet_sequence_number < last_received_packet_sequence_number_) {
     ++out_of_order_recieved_packet_count_;
     UMA_HISTOGRAM_COUNTS("Net.QuicSession.OutOfOrderGapReceived",
@@ -466,4 +474,41 @@ void QuicConnectionLogger::OnSuccessfulVersionNegotiation(
                     NetLog::StringCallback("version", &quic_version));
 }
 
+void QuicConnectionLogger::RecordAckNackHistograms() {
+  if (largest_received_packet_sequence_number_ == 0)
+    return;  // Connection was never used.
+  std::string prefix("Net.QuicSession.PacketReceived_");
+  std::string suffix = NetworkChangeNotifier::ConnectionTypeToString(
+      connection_type_);
+  base::HistogramBase* packet_ack_histogram(base::LinearHistogram::FactoryGet(
+      prefix + "Ack_" + suffix, 1, MAX_PACKET_STATUS, MAX_PACKET_STATUS + 1,

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

Can you extract this logic into a single method that's something like:

base::HistogramBase* GetAckHistogram("Ack_" | "Nack_");

Then you can say:

base::HistogramBase* ack_histogram = GetAckHistogram("Ack_");
base::HistogramBase* nack_histogram = GetAckHistogram("Nack_");

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

Done.

+      base::HistogramBase::kUmaTargetedHistogramFlag));
+  // Most of the time, we don't have any missing packets, so only cache the
+  // NACK histogram  when we need it (just in time).

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

Given that this code runs extremely infrequently, it seems unlikely that it's
performance sensitive at all.  Consequently, I don't think that it's worth the
complexity of doing this just-in-time initialization.

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

Done.

+  base::HistogramBase* packet_nack_histogram = NULL;
+  // largest_received_packet_sequence_number_ is a uint64, so it is easier
+  // to take the min() with a size_t MAX_PACKET_STATUS manually.
+  const size_t last_index =
+      (MAX_PACKET_STATUS - 1 < largest_received_packet_sequence_number_) ?
+          MAX_PACKET_STATUS - 1 :
+          static_cast<size_t>(largest_received_packet_sequence_number_);

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

I don't think I understand why doing this manually is "easier", since it
requires 4 lines to implement and a 2 line comment to explain :>

It would seem like this code should work just fine:

const size_t last_index = min(MAX_PACKET_STATUS - 1,
    static_cast<size_t>(largest_received_packet_sequence_number_));

Though I think the min<size_t> you had before would also work just fine.

Alternatively, how about just making MAX_PACKET_STATUS a unit64 which avoids all
casting problems.  (Or just #define. Ok, not really :>)

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

Per discussion... I changed to consistent typedefs that get uint64, ... and I'm
ignoring the cost for now.

...but that broke the arm build.... so I ended up using size_t for the const
static member, and explicitly selecting the template type here.

+  // We never get packet 0, so we skip recording stats for it, as it would
+  // always show as missing (Not ACKnowledged).
+  DCHECK(!packets_received_[0]);
+  for (size_t i = 1; i <= last_index; ++i) {
+    if (packets_received_[i]) {
+      packet_ack_histogram->Add(i);
+    } else {
+      if (!packet_nack_histogram) {
+        // Now we need the NACK histogram now.
+        packet_nack_histogram = base::LinearHistogram::FactoryGet(
+            prefix + "Nack_" + suffix, 1, MAX_PACKET_STATUS,
+            MAX_PACKET_STATUS + 1,
+            base::HistogramBase::kUmaTargetedHistogramFlag);
+      }
+      packet_nack_histogram->Add(i);
+    }
+  }
+}
+
 }  // namespace net