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,