dcsctp: Cleanup Metrics

This CL first restricts Metrics to be retrievable when the socket is
created. This avoids having most fields as optional and makes it
easier to add more metrics.

Secondly, the peer implementation is moved into Metrics.

Bug: webrtc:13052
Change-Id: I6cb53eeef3f84ac34f3efc883853338f903cc758
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/262256
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36888}

net/dcsctp/public/dcsctp_socket.h

@@ -170,43 +170,6 @@ enum class SendPacketStatus {
   kError,
 };
 
-// Tracked metrics, which is the return value of GetMetrics. Optional members
-// will be unset when they are not yet known.
-struct Metrics {
-  // Transmission stats and metrics.
-
-  // Number of packets sent.
-  size_t tx_packets_count = 0;
-
-  // Number of messages requested to be sent.
-  size_t tx_messages_count = 0;
-
-  // The current congestion window (cwnd) in bytes, corresponding to spinfo_cwnd
-  // defined in RFC6458.
-  absl::optional<size_t> cwnd_bytes = absl::nullopt;
-
-  // Smoothed round trip time, corresponding to spinfo_srtt defined in RFC6458.
-  absl::optional<int> srtt_ms = absl::nullopt;
-
-  // Number of data items in the retransmission queue that haven’t been
-  // acked/nacked yet and are in-flight. Corresponding to sstat_unackdata
-  // defined in RFC6458. This may be an approximation when there are messages in
-  // the send queue that haven't been fragmented/packetized yet.
-  size_t unack_data_count = 0;
-
-  // Receive stats and metrics.
-
-  // Number of packets received.
-  size_t rx_packets_count = 0;
-
-  // Number of messages received.
-  size_t rx_messages_count = 0;
-
-  // The peer’s last announced receiver window size, corresponding to
-  // sstat_rwnd defined in RFC6458.
-  absl::optional<uint32_t> peer_rwnd_bytes = absl::nullopt;
-};
-
 // Represent known SCTP implementations.
 enum class SctpImplementation {
   // There is not enough information toto determine any SCTP implementation.
@@ -232,6 +195,48 @@ inline constexpr absl::string_view ToString(SctpImplementation implementation) {
   }
 }
 
+// Tracked metrics, which is the return value of GetMetrics. Optional members
+// will be unset when they are not yet known.
+struct Metrics {
+  // Transmission stats and metrics.
+
+  // Number of packets sent.
+  size_t tx_packets_count = 0;
+
+  // Number of messages requested to be sent.
+  size_t tx_messages_count = 0;
+
+  // The current congestion window (cwnd) in bytes, corresponding to spinfo_cwnd
+  // defined in RFC6458.
+  size_t cwnd_bytes = 0;
+
+  // Smoothed round trip time, corresponding to spinfo_srtt defined in RFC6458.
+  int srtt_ms = 0;
+
+  // Number of data items in the retransmission queue that haven’t been
+  // acked/nacked yet and are in-flight. Corresponding to sstat_unackdata
+  // defined in RFC6458. This may be an approximation when there are messages in
+  // the send queue that haven't been fragmented/packetized yet.
+  size_t unack_data_count = 0;
+
+  // Receive stats and metrics.
+
+  // Number of packets received.
+  size_t rx_packets_count = 0;
+
+  // Number of messages received.
+  size_t rx_messages_count = 0;
+
+  // The peer’s last announced receiver window size, corresponding to
+  // sstat_rwnd defined in RFC6458.
+  uint32_t peer_rwnd_bytes = 0;
+
+  // Returns the detected SCTP implementation of the peer. As this is not
+  // explicitly signalled during the connection establishment, heuristics is
+  // used to analyze e.g. the state cookie in the INIT-ACK chunk.
+  SctpImplementation peer_implementation = SctpImplementation::kUnknown;
+};
+
 // Callbacks that the DcSctpSocket will call synchronously to the owning
 // client. It is allowed to call back into the library from callbacks that start
 // with "On". It has been explicitly documented when it's not allowed to call
@@ -467,8 +472,9 @@ class DcSctpSocketInterface {
   virtual void SetBufferedAmountLowThreshold(StreamID stream_id,
                                              size_t bytes) = 0;
 
-  // Retrieves the latest metrics.
-  virtual Metrics GetMetrics() const = 0;
+  // Retrieves the latest metrics. If the socket is not fully connected,
+  // `absl::nullopt` will be returned.
+  virtual absl::optional<Metrics> GetMetrics() const = 0;
 
   // Returns empty bitmask if the socket is in the state in which a snapshot of
   // the state can be made by `GetHandoverStateAndClose()`. Return value is
@@ -491,7 +497,10 @@ class DcSctpSocketInterface {
   // If this method is called too early (before
   // `DcSctpSocketCallbacks::OnConnected` has triggered), this will likely
   // return `SctpImplementation::kUnknown`.
-  virtual SctpImplementation peer_implementation() const = 0;
+  ABSL_DEPRECATED("See Metrics::peer_implementation instead")
+  virtual SctpImplementation peer_implementation() const {
+    return SctpImplementation::kUnknown;
+  }
 };
 }  // namespace dcsctp
 

net/dcsctp/public/mock_dcsctp_socket.h

@@ -63,7 +63,7 @@ class MockDcSctpSocket : public DcSctpSocketInterface {
               (StreamID stream_id, size_t bytes),
               (override));
 
-  MOCK_METHOD(Metrics, GetMetrics, (), (const, override));
+  MOCK_METHOD(absl::optional<Metrics>, GetMetrics, (), (const, override));
 
   MOCK_METHOD(HandoverReadinessStatus,
               GetHandoverReadiness,
@@ -73,8 +73,6 @@ class MockDcSctpSocket : public DcSctpSocketInterface {
               GetHandoverStateAndClose,
               (),
               (override));
-
-  MOCK_METHOD(SctpImplementation, peer_implementation, (), (const));
 };
 
 }  // namespace dcsctp

net/dcsctp/socket/dcsctp_socket.cc

@@ -524,24 +524,24 @@ void DcSctpSocket::SetBufferedAmountLowThreshold(StreamID stream_id,
   send_queue_.SetBufferedAmountLowThreshold(stream_id, bytes);
 }
 
-Metrics DcSctpSocket::GetMetrics() const {
+absl::optional<Metrics> DcSctpSocket::GetMetrics() const {
   RTC_DCHECK_RUN_ON(&thread_checker_);
-  Metrics metrics = metrics_;
 
-  if (tcb_ != nullptr) {
-    // Update the metrics with some stats that are extracted from
-    // sub-components.
-    metrics.cwnd_bytes = tcb_->cwnd();
-    metrics.srtt_ms = tcb_->current_srtt().value();
-    size_t packet_payload_size =
-        options_.mtu - SctpPacket::kHeaderSize - DataChunk::kHeaderSize;
-    metrics.unack_data_count =
-        tcb_->retransmission_queue().outstanding_items() +
-        (send_queue_.total_buffered_amount() + packet_payload_size - 1) /
-            packet_payload_size;
-    metrics.peer_rwnd_bytes = tcb_->retransmission_queue().rwnd();
+  if (tcb_ == nullptr) {
+    return absl::nullopt;
   }
 
+  Metrics metrics = metrics_;
+  metrics.cwnd_bytes = tcb_->cwnd();
+  metrics.srtt_ms = tcb_->current_srtt().value();
+  size_t packet_payload_size =
+      options_.mtu - SctpPacket::kHeaderSize - DataChunk::kHeaderSize;
+  metrics.unack_data_count =
+      tcb_->retransmission_queue().outstanding_items() +
+      (send_queue_.total_buffered_amount() + packet_payload_size - 1) /
+          packet_payload_size;
+  metrics.peer_rwnd_bytes = tcb_->retransmission_queue().rwnd();
+
   return metrics;
 }
 
@@ -1187,7 +1187,7 @@ void DcSctpSocket::HandleInitAck(
   Capabilities capabilities = GetCapabilities(options_, chunk->parameters());
   t1_init_->Stop();
 
-  peer_implementation_ = DeterminePeerImplementation(cookie->data());
+  metrics_.peer_implementation = DeterminePeerImplementation(cookie->data());
 
   tcb_ = std::make_unique<TransmissionControlBlock>(
       timer_manager_, log_prefix_, options_, capabilities, callbacks_,

net/dcsctp/socket/dcsctp_socket.h

@@ -99,11 +99,11 @@ class DcSctpSocket : public DcSctpSocketInterface {
   size_t buffered_amount(StreamID stream_id) const override;
   size_t buffered_amount_low_threshold(StreamID stream_id) const override;
   void SetBufferedAmountLowThreshold(StreamID stream_id, size_t bytes) override;
-  Metrics GetMetrics() const override;
+  absl::optional<Metrics> GetMetrics() const override;
   HandoverReadinessStatus GetHandoverReadiness() const override;
   absl::optional<DcSctpSocketHandoverState> GetHandoverStateAndClose() override;
   SctpImplementation peer_implementation() const override {
-    return peer_implementation_;
+    return metrics_.peer_implementation;
   }
   // Returns this socket's verification tag, or zero if not yet connected.
   VerificationTag verification_tag() const {
@@ -282,8 +282,6 @@ class DcSctpSocket : public DcSctpSocketInterface {
   State state_ = State::kClosed;
   // If the connection is established, contains a transmission control block.
   std::unique_ptr<TransmissionControlBlock> tcb_;
-
-  SctpImplementation peer_implementation_ = SctpImplementation::kUnknown;
 };
 }  // namespace dcsctp
 

net/dcsctp/socket/dcsctp_socket_test.cc

@@ -1874,18 +1874,10 @@ TEST_P(DcSctpSocketParametrizedTest,
   MaybeHandoverSocketAndSendMessage(a, std::move(z));
 }
 
-TEST(DcSctpSocketTest, InitialMetricsAreZeroed) {
+TEST(DcSctpSocketTest, InitialMetricsAreUnset) {
   SocketUnderTest a("A");
 
-  Metrics metrics = a.socket.GetMetrics();
-  EXPECT_EQ(metrics.tx_packets_count, 0u);
-  EXPECT_EQ(metrics.tx_messages_count, 0u);
-  EXPECT_EQ(metrics.cwnd_bytes.has_value(), false);
-  EXPECT_EQ(metrics.srtt_ms.has_value(), false);
-  EXPECT_EQ(metrics.unack_data_count, 0u);
-  EXPECT_EQ(metrics.rx_packets_count, 0u);
-  EXPECT_EQ(metrics.rx_messages_count, 0u);
-  EXPECT_EQ(metrics.peer_rwnd_bytes.has_value(), false);
+  EXPECT_FALSE(a.socket.GetMetrics().has_value());
 }
 
 TEST(DcSctpSocketTest, RxAndTxPacketMetricsIncrease) {
@@ -1897,65 +1889,65 @@ TEST(DcSctpSocketTest, RxAndTxPacketMetricsIncrease) {
   const size_t initial_a_rwnd = a.options.max_receiver_window_buffer_size *
                                 ReassemblyQueue::kHighWatermarkLimit;
 
-  EXPECT_EQ(a.socket.GetMetrics().tx_packets_count, 2u);
-  EXPECT_EQ(a.socket.GetMetrics().rx_packets_count, 2u);
-  EXPECT_EQ(a.socket.GetMetrics().tx_messages_count, 0u);
-  EXPECT_EQ(*a.socket.GetMetrics().cwnd_bytes,
+  EXPECT_EQ(a.socket.GetMetrics()->tx_packets_count, 2u);
+  EXPECT_EQ(a.socket.GetMetrics()->rx_packets_count, 2u);
+  EXPECT_EQ(a.socket.GetMetrics()->tx_messages_count, 0u);
+  EXPECT_EQ(a.socket.GetMetrics()->cwnd_bytes,
             a.options.cwnd_mtus_initial * a.options.mtu);
-  EXPECT_EQ(a.socket.GetMetrics().unack_data_count, 0u);
+  EXPECT_EQ(a.socket.GetMetrics()->unack_data_count, 0u);
 
-  EXPECT_EQ(z.socket.GetMetrics().rx_packets_count, 2u);
-  EXPECT_EQ(z.socket.GetMetrics().rx_messages_count, 0u);
+  EXPECT_EQ(z.socket.GetMetrics()->rx_packets_count, 2u);
+  EXPECT_EQ(z.socket.GetMetrics()->rx_messages_count, 0u);
 
   a.socket.Send(DcSctpMessage(StreamID(1), PPID(53), {1, 2}), kSendOptions);
-  EXPECT_EQ(a.socket.GetMetrics().unack_data_count, 1u);
+  EXPECT_EQ(a.socket.GetMetrics()->unack_data_count, 1u);
 
   z.socket.ReceivePacket(a.cb.ConsumeSentPacket());  // DATA
   a.socket.ReceivePacket(z.cb.ConsumeSentPacket());  // SACK
-  EXPECT_EQ(*a.socket.GetMetrics().peer_rwnd_bytes, initial_a_rwnd);
-  EXPECT_EQ(a.socket.GetMetrics().unack_data_count, 0u);
+  EXPECT_EQ(a.socket.GetMetrics()->peer_rwnd_bytes, initial_a_rwnd);
+  EXPECT_EQ(a.socket.GetMetrics()->unack_data_count, 0u);
 
   EXPECT_TRUE(z.cb.ConsumeReceivedMessage().has_value());
 
-  EXPECT_EQ(a.socket.GetMetrics().tx_packets_count, 3u);
-  EXPECT_EQ(a.socket.GetMetrics().rx_packets_count, 3u);
-  EXPECT_EQ(a.socket.GetMetrics().tx_messages_count, 1u);
+  EXPECT_EQ(a.socket.GetMetrics()->tx_packets_count, 3u);
+  EXPECT_EQ(a.socket.GetMetrics()->rx_packets_count, 3u);
+  EXPECT_EQ(a.socket.GetMetrics()->tx_messages_count, 1u);
 
-  EXPECT_EQ(z.socket.GetMetrics().rx_packets_count, 3u);
-  EXPECT_EQ(z.socket.GetMetrics().rx_messages_count, 1u);
+  EXPECT_EQ(z.socket.GetMetrics()->rx_packets_count, 3u);
+  EXPECT_EQ(z.socket.GetMetrics()->rx_messages_count, 1u);
 
   // Send one more (large - fragmented), and receive the delayed SACK.
   a.socket.Send(DcSctpMessage(StreamID(1), PPID(53),
                               std::vector<uint8_t>(a.options.mtu * 2 + 1)),
                 kSendOptions);
-  EXPECT_EQ(a.socket.GetMetrics().unack_data_count, 3u);
+  EXPECT_EQ(a.socket.GetMetrics()->unack_data_count, 3u);
 
   z.socket.ReceivePacket(a.cb.ConsumeSentPacket());  // DATA
   z.socket.ReceivePacket(a.cb.ConsumeSentPacket());  // DATA
 
   a.socket.ReceivePacket(z.cb.ConsumeSentPacket());  // SACK
-  EXPECT_EQ(a.socket.GetMetrics().unack_data_count, 1u);
-  EXPECT_GT(*a.socket.GetMetrics().peer_rwnd_bytes, 0u);
-  EXPECT_LT(*a.socket.GetMetrics().peer_rwnd_bytes, initial_a_rwnd);
+  EXPECT_EQ(a.socket.GetMetrics()->unack_data_count, 1u);
+  EXPECT_GT(a.socket.GetMetrics()->peer_rwnd_bytes, 0u);
+  EXPECT_LT(a.socket.GetMetrics()->peer_rwnd_bytes, initial_a_rwnd);
 
   z.socket.ReceivePacket(a.cb.ConsumeSentPacket());  // DATA
 
   EXPECT_TRUE(z.cb.ConsumeReceivedMessage().has_value());
 
-  EXPECT_EQ(a.socket.GetMetrics().tx_packets_count, 6u);
-  EXPECT_EQ(a.socket.GetMetrics().rx_packets_count, 4u);
-  EXPECT_EQ(a.socket.GetMetrics().tx_messages_count, 2u);
+  EXPECT_EQ(a.socket.GetMetrics()->tx_packets_count, 6u);
+  EXPECT_EQ(a.socket.GetMetrics()->rx_packets_count, 4u);
+  EXPECT_EQ(a.socket.GetMetrics()->tx_messages_count, 2u);
 
-  EXPECT_EQ(z.socket.GetMetrics().rx_packets_count, 6u);
-  EXPECT_EQ(z.socket.GetMetrics().rx_messages_count, 2u);
+  EXPECT_EQ(z.socket.GetMetrics()->rx_packets_count, 6u);
+  EXPECT_EQ(z.socket.GetMetrics()->rx_messages_count, 2u);
 
   // Delayed sack
   AdvanceTime(a, z, a.options.delayed_ack_max_timeout);
 
   a.socket.ReceivePacket(z.cb.ConsumeSentPacket());  // SACK
-  EXPECT_EQ(a.socket.GetMetrics().unack_data_count, 0u);
-  EXPECT_EQ(a.socket.GetMetrics().rx_packets_count, 5u);
-  EXPECT_EQ(*a.socket.GetMetrics().peer_rwnd_bytes, initial_a_rwnd);
+  EXPECT_EQ(a.socket.GetMetrics()->unack_data_count, 0u);
+  EXPECT_EQ(a.socket.GetMetrics()->rx_packets_count, 5u);
+  EXPECT_EQ(a.socket.GetMetrics()->peer_rwnd_bytes, initial_a_rwnd);
 }
 
 TEST_P(DcSctpSocketParametrizedTest, UnackDataAlsoIncludesSendQueue) {
@@ -1980,10 +1972,10 @@ TEST_P(DcSctpSocketParametrizedTest, UnackDataAlsoIncludesSendQueue) {
 
   // Due to alignment, padding etc, it's hard to calculate the exact number, but
   // it should be in this range.
-  EXPECT_GE(a.socket.GetMetrics().unack_data_count,
+  EXPECT_GE(a.socket.GetMetrics()->unack_data_count,
             expected_sent_packets + expected_queued_packets);
 
-  EXPECT_LE(a.socket.GetMetrics().unack_data_count,
+  EXPECT_LE(a.socket.GetMetrics()->unack_data_count,
             expected_sent_packets + expected_queued_packets + 2);
 
   MaybeHandoverSocketAndSendMessage(a, std::move(z));