Fix QUIC connection retransmission stats
Because QUIC retransmits data at the stream level (rather than packet level) since QUIC version > 39, the current implementation of bytes_retransmitted and packets_retransmitted is incorrect.
With this change:
- The packet now has a running total of "not retransmission" bytes. We use this new field to populate the `bytes_retransmitted` fields on the connection stats.
- The packet is considered a "retransmission" packet if ANY frame is a retransmission frame (rather than looking at the latest retransmittable frame).
PiperOrigin-RevId: 454625064
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc
index 92db728..2645c58 100644
--- a/quiche/quic/core/quic_connection.cc
+++ b/quiche/quic/core/quic_connection.cc
@@ -8,6 +8,7 @@
#include <sys/types.h>
#include <algorithm>
+#include <cstdint>
#include <iterator>
#include <limits>
#include <memory>
@@ -3655,10 +3656,24 @@
sent_packet_manager_.GetLeastPacketAwaitedByPeer(encryption_level_),
sent_packet_manager_.EstimateMaxPacketsInFlight(max_packet_length()));
- stats_.bytes_sent += result.bytes_written;
+ stats_.bytes_sent += encrypted_length;
++stats_.packets_sent;
+
+ QuicByteCount bytes_not_retransmitted =
+ packet->bytes_not_retransmitted.value_or(0);
if (packet->transmission_type != NOT_RETRANSMISSION) {
- stats_.bytes_retransmitted += result.bytes_written;
+ if (static_cast<uint64_t>(encrypted_length) < bytes_not_retransmitted) {
+ QUIC_BUG(quic_packet_bytes_written_lt_bytes_not_retransmitted)
+ << "Total bytes written to the packet should be larger than the "
+ "bytes in not-retransmitted frames. Bytes written: "
+ << encrypted_length
+ << ", bytes not retransmitted: " << bytes_not_retransmitted;
+ } else {
+ // bytes_retransmitted includes packet's headers and encryption
+ // overhead.
+ stats_.bytes_retransmitted +=
+ (encrypted_length - bytes_not_retransmitted);
+ }
++stats_.packets_retransmitted;
}
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc
index ff84538..b3c01d5 100644
--- a/quiche/quic/core/quic_connection_test.cc
+++ b/quiche/quic/core/quic_connection_test.cc
@@ -240,10 +240,19 @@
absl::string_view data,
QuicStreamOffset offset,
StreamSendingState state) {
+ return SaveAndSendStreamData(id, data, offset, state, NOT_RETRANSMISSION);
+ }
+
+ QuicConsumedData SaveAndSendStreamData(QuicStreamId id,
+ absl::string_view data,
+ QuicStreamOffset offset,
+ StreamSendingState state,
+ TransmissionType transmission_type) {
ScopedPacketFlusher flusher(this);
producer_.SaveStreamData(id, data);
if (notifier_ != nullptr) {
- return notifier_->WriteOrBufferData(id, data.length(), state);
+ return notifier_->WriteOrBufferData(id, data.length(), state,
+ transmission_type);
}
return QuicConnection::SendStreamData(id, data.length(), offset, state);
}
@@ -3545,6 +3554,86 @@
<< ". Actual time = " << actual_recorded_send_time.ToDebuggingValue();
}
+TEST_P(QuicConnectionTest, ConnectionStatsRetransmission_WithRetransmissions) {
+ // Send two stream frames in 1 packet by queueing them.
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+
+ {
+ QuicConnection::ScopedPacketFlusher flusher(&connection_);
+ connection_.SaveAndSendStreamData(
+ GetNthClientInitiatedStreamId(1, connection_.transport_version()),
+ "helloworld", 0, NO_FIN, PTO_RETRANSMISSION);
+ connection_.SaveAndSendStreamData(
+ GetNthClientInitiatedStreamId(2, connection_.transport_version()),
+ "helloworld", 0, NO_FIN, LOSS_RETRANSMISSION);
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1);
+ }
+
+ EXPECT_EQ(0u, connection_.NumQueuedPackets());
+ EXPECT_FALSE(connection_.HasQueuedData());
+
+ EXPECT_EQ(2u, writer_->frame_count());
+ for (auto& frame : writer_->stream_frames()) {
+ EXPECT_EQ(frame->data_length, 10u);
+ }
+
+ ASSERT_EQ(connection_.GetStats().packets_retransmitted, 1u);
+ ASSERT_GE(connection_.GetStats().bytes_retransmitted, 20u);
+}
+
+TEST_P(QuicConnectionTest, ConnectionStatsRetransmission_WithMixedFrames) {
+ // Send two stream frames in 1 packet by queueing them.
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+
+ {
+ QuicConnection::ScopedPacketFlusher flusher(&connection_);
+ // First frame is retransmission. Second is NOT_RETRANSMISSION but the
+ // packet retains the PTO_RETRANSMISSION type.
+ connection_.SaveAndSendStreamData(
+ GetNthClientInitiatedStreamId(1, connection_.transport_version()),
+ "helloworld", 0, NO_FIN, PTO_RETRANSMISSION);
+ connection_.SaveAndSendStreamData(
+ GetNthClientInitiatedStreamId(2, connection_.transport_version()),
+ "helloworld", 0, NO_FIN, NOT_RETRANSMISSION);
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1);
+ }
+
+ EXPECT_EQ(0u, connection_.NumQueuedPackets());
+ EXPECT_FALSE(connection_.HasQueuedData());
+
+ EXPECT_EQ(2u, writer_->frame_count());
+ for (auto& frame : writer_->stream_frames()) {
+ EXPECT_EQ(frame->data_length, 10u);
+ }
+
+ ASSERT_EQ(connection_.GetStats().packets_retransmitted, 1u);
+ ASSERT_GE(connection_.GetStats().bytes_retransmitted, 10u);
+}
+
+TEST_P(QuicConnectionTest, ConnectionStatsRetransmission_NoRetransmission) {
+ // Send two stream frames in 1 packet by queueing them.
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+
+ {
+ QuicConnection::ScopedPacketFlusher flusher(&connection_);
+ // Both frames are NOT_RETRANSMISSION
+ connection_.SaveAndSendStreamData(
+ GetNthClientInitiatedStreamId(1, connection_.transport_version()),
+ "helloworld", 0, NO_FIN, NOT_RETRANSMISSION);
+ connection_.SaveAndSendStreamData(
+ GetNthClientInitiatedStreamId(2, connection_.transport_version()),
+ "helloworld", 0, NO_FIN, NOT_RETRANSMISSION);
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1);
+ }
+
+ EXPECT_EQ(0u, connection_.NumQueuedPackets());
+ EXPECT_FALSE(connection_.HasQueuedData());
+
+ EXPECT_EQ(2u, writer_->frame_count());
+ ASSERT_EQ(connection_.GetStats().packets_retransmitted, 0u);
+ ASSERT_EQ(connection_.GetStats().bytes_retransmitted, 0u);
+}
+
TEST_P(QuicConnectionTest, FramePacking) {
// Send two stream frames in 1 packet by queueing them.
connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
diff --git a/quiche/quic/core/quic_packet_creator.cc b/quiche/quic/core/quic_packet_creator.cc
index 66c7d14..98d6314 100644
--- a/quiche/quic/core/quic_packet_creator.cc
+++ b/quiche/quic/core/quic_packet_creator.cc
@@ -476,6 +476,12 @@
QUIC_BUG_IF(quic_bug_12398_5, packet_.encrypted_buffer == nullptr)
<< ENDPOINT;
+ // Clear bytes_not_retransmitted for packets containing only
+ // NOT_RETRANSMISSION frames.
+ if (packet_.transmission_type == NOT_RETRANSMISSION) {
+ packet_.bytes_not_retransmitted.reset();
+ }
+
SerializedPacket packet(std::move(packet_));
ClearPacket();
RemoveSoftMaxPacketLength();
@@ -499,6 +505,7 @@
QUICHE_DCHECK(packet_.nonretransmittable_frames.empty()) << ENDPOINT;
packet_.largest_acked.Clear();
needs_full_padding_ = false;
+ packet_.bytes_not_retransmitted.reset();
}
size_t QuicPacketCreator::ReserializeInitialPacketInCoalescedPacket(
@@ -1795,9 +1802,13 @@
debug_delegate_->OnFrameAddedToPacket(frame);
}
- // Packet transmission type is determined by the last added retransmittable
- // frame.
- if (QuicUtils::IsRetransmittableFrame(frame.type)) {
+ if (transmission_type == NOT_RETRANSMISSION) {
+ packet_.bytes_not_retransmitted.emplace(
+ packet_.bytes_not_retransmitted.value_or(0) + frame_len);
+ } else if (QuicUtils::IsRetransmittableFrame(frame.type)) {
+ // Packet transmission type is determined by the last added retransmittable
+ // frame of a retransmission type. If a packet has no retransmittable
+ // retransmission frames, it has type NOT_RETRANSMISSION.
packet_.transmission_type = transmission_type;
}
return true;
diff --git a/quiche/quic/core/quic_packet_creator_test.cc b/quiche/quic/core/quic_packet_creator_test.cc
index c9f8ecc..f6a81b2 100644
--- a/quiche/quic/core/quic_packet_creator_test.cc
+++ b/quiche/quic/core/quic_packet_creator_test.cc
@@ -18,6 +18,7 @@
#include "quiche/quic/core/crypto/null_encrypter.h"
#include "quiche/quic/core/crypto/quic_decrypter.h"
#include "quiche/quic/core/crypto/quic_encrypter.h"
+#include "quiche/quic/core/frames/quic_frame.h"
#include "quiche/quic/core/frames/quic_stream_frame.h"
#include "quiche/quic/core/quic_connection_id.h"
#include "quiche/quic/core/quic_data_writer.h"
@@ -1927,6 +1928,10 @@
absl::string_view()));
ASSERT_TRUE(QuicUtils::IsRetransmittableFrame(stream_frame.type));
+ QuicFrame stream_frame_2(QuicStreamFrame(stream_id,
+ /*fin=*/false, 1u,
+ absl::string_view()));
+
QuicFrame padding_frame{QuicPaddingFrame()};
ASSERT_FALSE(QuicUtils::IsRetransmittableFrame(padding_frame.type));
@@ -1939,16 +1944,154 @@
EXPECT_TRUE(creator_.AddFrame(stream_frame, PTO_RETRANSMISSION));
ASSERT_EQ(serialized_packet_, nullptr);
- EXPECT_TRUE(creator_.AddFrame(padding_frame, LOSS_RETRANSMISSION));
+ EXPECT_TRUE(creator_.AddFrame(stream_frame_2, PATH_RETRANSMISSION));
+ ASSERT_EQ(serialized_packet_, nullptr);
+
+ EXPECT_TRUE(creator_.AddFrame(padding_frame, PTO_RETRANSMISSION));
creator_.FlushCurrentPacket();
ASSERT_TRUE(serialized_packet_->encrypted_buffer);
// The last retransmittable frame on packet is a stream frame, the packet's
// transmission type should be the same as the stream frame's.
- EXPECT_EQ(serialized_packet_->transmission_type, PTO_RETRANSMISSION);
+ EXPECT_EQ(serialized_packet_->transmission_type, PATH_RETRANSMISSION);
DeleteSerializedPacket();
}
+TEST_P(QuicPacketCreatorTest,
+ PacketBytesRetransmitted_AddFrame_Retransmission) {
+ creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE);
+
+ QuicAckFrame temp_ack_frame = InitAckFrame(1);
+ QuicFrame ack_frame(&temp_ack_frame);
+ EXPECT_TRUE(creator_.AddFrame(ack_frame, LOSS_RETRANSMISSION));
+
+ QuicStreamId stream_id = QuicUtils::GetFirstBidirectionalStreamId(
+ client_framer_.transport_version(), Perspective::IS_CLIENT);
+
+ QuicFrame stream_frame;
+ const std::string data("data");
+ // ConsumeDataToFillCurrentPacket calls AddFrame
+ ASSERT_TRUE(creator_.ConsumeDataToFillCurrentPacket(
+ stream_id, data, 0u, false, false, PTO_RETRANSMISSION, &stream_frame));
+ EXPECT_EQ(4u, stream_frame.stream_frame.data_length);
+
+ EXPECT_CALL(delegate_, OnSerializedPacket(_))
+ .WillOnce(Invoke(this, &QuicPacketCreatorTest::SaveSerializedPacket));
+
+ creator_.FlushCurrentPacket();
+ ASSERT_TRUE(serialized_packet_->encrypted_buffer);
+ ASSERT_FALSE(serialized_packet_->bytes_not_retransmitted.has_value());
+
+ DeleteSerializedPacket();
+}
+
+TEST_P(QuicPacketCreatorTest,
+ PacketBytesRetransmitted_AddFrame_NotRetransmission) {
+ creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE);
+
+ QuicAckFrame temp_ack_frame = InitAckFrame(1);
+ QuicFrame ack_frame(&temp_ack_frame);
+ EXPECT_TRUE(creator_.AddFrame(ack_frame, NOT_RETRANSMISSION));
+
+ QuicStreamId stream_id = QuicUtils::GetFirstBidirectionalStreamId(
+ client_framer_.transport_version(), Perspective::IS_CLIENT);
+
+ QuicFrame stream_frame;
+ const std::string data("data");
+ // ConsumeDataToFillCurrentPacket calls AddFrame
+ ASSERT_TRUE(creator_.ConsumeDataToFillCurrentPacket(
+ stream_id, data, 0u, false, false, NOT_RETRANSMISSION, &stream_frame));
+ EXPECT_EQ(4u, stream_frame.stream_frame.data_length);
+
+ EXPECT_CALL(delegate_, OnSerializedPacket(_))
+ .WillOnce(Invoke(this, &QuicPacketCreatorTest::SaveSerializedPacket));
+
+ creator_.FlushCurrentPacket();
+ ASSERT_TRUE(serialized_packet_->encrypted_buffer);
+ ASSERT_FALSE(serialized_packet_->bytes_not_retransmitted.has_value());
+
+ DeleteSerializedPacket();
+}
+
+TEST_P(QuicPacketCreatorTest, PacketBytesRetransmitted_AddFrame_MixedFrames) {
+ creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE);
+
+ QuicAckFrame temp_ack_frame = InitAckFrame(1);
+ QuicFrame ack_frame(&temp_ack_frame);
+ EXPECT_TRUE(creator_.AddFrame(ack_frame, NOT_RETRANSMISSION));
+
+ QuicStreamId stream_id = QuicUtils::GetFirstBidirectionalStreamId(
+ client_framer_.transport_version(), Perspective::IS_CLIENT);
+
+ QuicFrame stream_frame;
+ const std::string data("data");
+ // ConsumeDataToFillCurrentPacket calls AddFrame
+ ASSERT_TRUE(creator_.ConsumeDataToFillCurrentPacket(
+ stream_id, data, 0u, false, false, NOT_RETRANSMISSION, &stream_frame));
+ EXPECT_EQ(4u, stream_frame.stream_frame.data_length);
+
+ QuicFrame stream_frame2;
+ // ConsumeDataToFillCurrentPacket calls AddFrame
+ ASSERT_TRUE(creator_.ConsumeDataToFillCurrentPacket(
+ stream_id, data, 0u, false, false, LOSS_RETRANSMISSION, &stream_frame2));
+ EXPECT_EQ(4u, stream_frame2.stream_frame.data_length);
+
+ EXPECT_CALL(delegate_, OnSerializedPacket(_))
+ .WillOnce(Invoke(this, &QuicPacketCreatorTest::SaveSerializedPacket));
+
+ creator_.FlushCurrentPacket();
+ ASSERT_TRUE(serialized_packet_->encrypted_buffer);
+ ASSERT_TRUE(serialized_packet_->bytes_not_retransmitted.has_value());
+ ASSERT_GE(serialized_packet_->bytes_not_retransmitted.value(), 4u);
+
+ DeleteSerializedPacket();
+}
+
+TEST_P(QuicPacketCreatorTest,
+ PacketBytesRetransmitted_CreateAndSerializeStreamFrame_Retransmission) {
+ creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE);
+
+ const std::string data("test");
+ producer_.SaveStreamData(GetNthClientInitiatedStreamId(0), data);
+ EXPECT_CALL(delegate_, OnSerializedPacket(_))
+ .WillOnce(Invoke(this, &QuicPacketCreatorTest::SaveSerializedPacket));
+ size_t num_bytes_consumed;
+ // Retransmission frame adds to packet's bytes_retransmitted
+ creator_.CreateAndSerializeStreamFrame(
+ GetNthClientInitiatedStreamId(0), data.length(), 0, 0, true,
+ LOSS_RETRANSMISSION, &num_bytes_consumed);
+ EXPECT_EQ(4u, num_bytes_consumed);
+
+ ASSERT_TRUE(serialized_packet_->encrypted_buffer);
+ ASSERT_FALSE(serialized_packet_->bytes_not_retransmitted.has_value());
+ DeleteSerializedPacket();
+
+ EXPECT_FALSE(creator_.HasPendingFrames());
+}
+
+TEST_P(
+ QuicPacketCreatorTest,
+ PacketBytesRetransmitted_CreateAndSerializeStreamFrame_NotRetransmission) {
+ creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE);
+
+ const std::string data("test");
+ producer_.SaveStreamData(GetNthClientInitiatedStreamId(0), data);
+ EXPECT_CALL(delegate_, OnSerializedPacket(_))
+ .WillOnce(Invoke(this, &QuicPacketCreatorTest::SaveSerializedPacket));
+ size_t num_bytes_consumed;
+ // Non-retransmission frame does not add to packet's bytes_retransmitted
+ creator_.CreateAndSerializeStreamFrame(
+ GetNthClientInitiatedStreamId(0), data.length(), 0, 0, true,
+ NOT_RETRANSMISSION, &num_bytes_consumed);
+ EXPECT_EQ(4u, num_bytes_consumed);
+
+ ASSERT_TRUE(serialized_packet_->encrypted_buffer);
+ ASSERT_FALSE(serialized_packet_->bytes_not_retransmitted.has_value());
+ DeleteSerializedPacket();
+
+ EXPECT_FALSE(creator_.HasPendingFrames());
+}
+
TEST_P(QuicPacketCreatorTest, RetryToken) {
if (!GetParam().version_serialization ||
!QuicVersionHasLongHeaderLengths(client_framer_.transport_version())) {
diff --git a/quiche/quic/core/quic_packets.cc b/quiche/quic/core/quic_packets.cc
index 57d6ba7..369a32a 100644
--- a/quiche/quic/core/quic_packets.cc
+++ b/quiche/quic/core/quic_packets.cc
@@ -439,7 +439,8 @@
has_ack_frequency(other.has_ack_frequency),
has_message(other.has_message),
fate(other.fate),
- peer_address(other.peer_address) {
+ peer_address(other.peer_address),
+ bytes_not_retransmitted(other.bytes_not_retransmitted) {
if (this != &other) {
if (release_encrypted_buffer && encrypted_buffer != nullptr) {
release_encrypted_buffer(encrypted_buffer);
@@ -486,6 +487,7 @@
copy->has_message = serialized.has_message;
copy->fate = serialized.fate;
copy->peer_address = serialized.peer_address;
+ copy->bytes_not_retransmitted = serialized.bytes_not_retransmitted;
if (copy_buffer) {
copy->encrypted_buffer = CopyBuffer(serialized);
diff --git a/quiche/quic/core/quic_packets.h b/quiche/quic/core/quic_packets.h
index 5246f43..d4f04c8 100644
--- a/quiche/quic/core/quic_packets.h
+++ b/quiche/quic/core/quic_packets.h
@@ -376,6 +376,10 @@
bool has_message;
SerializedPacketFate fate;
QuicSocketAddress peer_address;
+ // Sum of bytes from frames that are not retransmissions. This field is only
+ // populated for packets with "mixed frames": at least one frame of a
+ // retransmission type and at least one frame of NOT_RETRANSMISSION type.
+ absl::optional<QuicByteCount> bytes_not_retransmitted;
};
// Make a copy of |serialized| (including the underlying frames). |copy_buffer|
diff --git a/quiche/quic/test_tools/simple_session_notifier.cc b/quiche/quic/test_tools/simple_session_notifier.cc
index 123eb6e..7a2e705 100644
--- a/quiche/quic/test_tools/simple_session_notifier.cc
+++ b/quiche/quic/test_tools/simple_session_notifier.cc
@@ -37,6 +37,12 @@
QuicConsumedData SimpleSessionNotifier::WriteOrBufferData(
QuicStreamId id, QuicByteCount data_length, StreamSendingState state) {
+ return WriteOrBufferData(id, data_length, state, NOT_RETRANSMISSION);
+}
+
+QuicConsumedData SimpleSessionNotifier::WriteOrBufferData(
+ QuicStreamId id, QuicByteCount data_length, StreamSendingState state,
+ TransmissionType transmission_type) {
if (!stream_map_.contains(id)) {
stream_map_[id] = StreamState();
}
@@ -53,7 +59,7 @@
return {0, false};
}
const size_t length = stream_state.bytes_total - stream_state.bytes_sent;
- connection_->SetTransmissionType(NOT_RETRANSMISSION);
+ connection_->SetTransmissionType(transmission_type);
QuicConsumedData consumed =
connection_->SendStreamData(id, length, stream_state.bytes_sent, state);
QUIC_DVLOG(1) << "consumed: " << consumed;
diff --git a/quiche/quic/test_tools/simple_session_notifier.h b/quiche/quic/test_tools/simple_session_notifier.h
index e846499..f1af586 100644
--- a/quiche/quic/test_tools/simple_session_notifier.h
+++ b/quiche/quic/test_tools/simple_session_notifier.h
@@ -28,6 +28,9 @@
// Tries to write stream data and returns data consumed.
QuicConsumedData WriteOrBufferData(QuicStreamId id, QuicByteCount data_length,
StreamSendingState state);
+ QuicConsumedData WriteOrBufferData(QuicStreamId id, QuicByteCount data_length,
+ StreamSendingState state,
+ TransmissionType transmission_type);
// Tries to write RST_STREAM_FRAME.
void WriteOrBufferRstStream(QuicStreamId id, QuicRstStreamErrorCode error,
