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 {
-
 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 12 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),
+      connection_type_(NetworkChangeNotifier::GetConnectionType()) {
 }
 
 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_);
+
+  RecordAckNackHistograms();
 }
 
 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 75 matching lines @@
     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 (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",
                          last_received_packet_sequence_number_ -
                              header.packet_sequence_number);
   }
   last_received_packet_sequence_number_ = header.packet_sequence_number;
 }
 
 void QuicConnectionLogger::OnStreamFrame(const QuicStreamFrame& frame) {
@@ skipping 120 matching lines @@
       base::Bind(&NetLogQuicOnConnectionClosedCallback, error, from_peer));
 }
 
 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));
 }
 
+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