Expose packet loss counts from QUIC to NetworkQualityEstimator

net/base/network_quality_estimator.h

 // Copyright 2015 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.
 
 #ifndef NET_BASE_NETWORK_QUALITY_ESTIMATOR_H_
 #define NET_BASE_NETWORK_QUALITY_ESTIMATOR_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 83 matching lines @@
                                          ObservationSource source) = 0;
 
    protected:
     ThroughputObserver() {}
     virtual ~ThroughputObserver() {}
 
    private:
     DISALLOW_COPY_AND_ASSIGN(ThroughputObserver);
   };
 
+  // Observes measurements of packet loss.
+  class NET_EXPORT_PRIVATE PacketLossObserver {
+   public:
+    // Will be called when a new packet loss observation is available.

[bengr, 2016/02/16 17:02:02]

Use complete sentences. "This method will.."

[tbansal1, 2016/02/17 18:35:10]

Done.

+    // |num_packets_lost| is the number of packets lost.
+    // |num_packets_received_in_order| is the number of packets received in
+    // order. |num_packets_received_not_in_order| is the number of packets
+    // received out of order. See
+    // SocketPerformanceWatcherFactory::OnUpdatedPacketCountAvailable for more
+    // details on how these values are populated.
+    //
+    // Providing counts of lost, in-order and out-of-order packets provides
+    // consumers with more information. From the perspective of network quality,

[bengr, 2016/02/16 17:02:01]

More information than what?
How does network quality have perspective? :)

[tbansal1, 2016/02/17 18:35:10]

Done.

+    // loss of a packet indicates worse network quality than receival of an

[bengr, 2016/02/16 17:02:02]

receival -> reception

[tbansal1, 2016/02/17 18:35:10]

Removed that comment, it was confusing. Also, how to interpret the 3 counts
depends on the consumers. So, that comment was unnecessary.

+    // out-of-order packet. The latter in turns indicates worse network

[bengr, 2016/02/16 17:02:02]

 turns -> turn

[tbansal1, 2016/02/17 18:35:11]

Done.

+    // quality than receival of an in-order packet.

[bengr, 2016/02/16 17:02:02]

receival -> reception

[tbansal1, 2016/02/17 18:35:10]

Done.

+    virtual void OnPacketLossObservation(
+        uint64_t num_packets_lost,
+        uint64_t num_packets_received_in_order,
+        uint64_t num_packets_received_not_in_order,
+        const base::TimeTicks& timestamp,
+        ObservationSource source) = 0;
+
+   protected:
+    PacketLossObserver() {}
+    virtual ~PacketLossObserver() {}
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(PacketLossObserver);
+  };
+
   // Creates a new NetworkQualityEstimator.
   // |variation_params| is the map containing all field trial parameters
   // related to NetworkQualityEstimator field trial.
   // |external_estimates_provider| may be NULL.
   NetworkQualityEstimator(
       scoped_ptr<ExternalEstimateProvider> external_estimates_provider,
       const std::map<std::string, std::string>& variation_params);
 
   // Construct a NetworkQualityEstimator instance allowing for test
   // configuration. Registers for network type change notifications so estimates
@@ skipping 18 matching lines @@
 
   // Returns true if RTT is available and sets |rtt| to estimated RTT.
   // Virtualized for testing. |rtt| should not be null.
   virtual bool GetRTTEstimate(base::TimeDelta* rtt) const;
 
   // Returns true if downlink throughput is available and sets |kbps| to
   // estimated downlink throughput (in kilobits per second).
   // Virtualized for testing. |kbps| should not be null.
   virtual bool GetDownlinkThroughputKbpsEstimate(int32_t* kbps) const;
 
+  // Returns true if the estimated packet loss rate is available and sets
+  // |packet_loss| to the estimated packet loss rate. Virtualized for testing.
+  // |packet_loss| should not be NULL. |packet_loss| is always between 0.0
+  // and 1.0 with a higher value indicating higher packet loss rate.

[bengr, 2016/02/16 17:02:02]

inclusive?

[tbansal1, 2016/02/17 18:35:10]

Done.

+  virtual bool GetPacketLossRateEstimate(float* packet_loss) const;
+
   // Notifies NetworkQualityEstimator that the response header of |request| has
   // been received.
   void NotifyHeadersReceived(const URLRequest& request);
 
   // Notifies NetworkQualityEstimator that the response body of |request| has
   // been received.
   void NotifyRequestCompleted(const URLRequest& request);
 
   // Returns true if median RTT is available and sets |rtt| to the median of
   // RTT observations since |begin_timestamp|.
   // Virtualized for testing. |rtt| should not be null.
   virtual bool GetRecentMedianRTT(const base::TimeTicks& begin_timestamp,
                                   base::TimeDelta* rtt) const;
 
   // Returns true if median downstream throughput is available and sets |kbps|
   // to the median of downstream throughput (in kilobits per second)
   // observations since |begin_timestamp|. Virtualized for testing. |kbps|
   // should not be null.
   virtual bool GetRecentMedianDownlinkThroughputKbps(
       const base::TimeTicks& begin_timestamp,
       int32_t* kbps) const;
 
   // SocketPerformanceWatcherFactory implementation:
   scoped_ptr<SocketPerformanceWatcher> CreateSocketPerformanceWatcher(
       const Protocol protocol) override;
   void OnUpdatedRTTAvailable(const Protocol protocol,
                              const base::TimeDelta& rtt) override;
+  void OnUpdatedPacketCountAvailable(
+      const Protocol protocol,
+      uint64_t num_packets_lost,
+      uint64_t num_packets_received_in_order,
+      uint64_t num_packets_received_not_in_order) override;
 
   // Adds |rtt_observer| to the list of round trip time observers. Must be
   // called on the IO thread.
   void AddRTTObserver(RTTObserver* rtt_observer);
 
   // Removes |rtt_observer| from the list of round trip time observers if it
   // is on the list of observers. Must be called on the IO thread.
   void RemoveRTTObserver(RTTObserver* rtt_observer);
 
   // Adds |throughput_observer| to the list of throughput observers. Must be
   // called on the IO thread.
   void AddThroughputObserver(ThroughputObserver* throughput_observer);
 
   // Removes |throughput_observer| from the list of throughput observers if it
   // is on the list of observers. Must be called on the IO thread.
   void RemoveThroughputObserver(ThroughputObserver* throughput_observer);
 
+  // Adds |packet_loss_observer| to the list of packet loss observers. Must be
+  // called on the IO thread.
+  void AddPacketLossObserver(PacketLossObserver* packet_loss_observer);
+
+  // Removes |packet_loss_observer| from the list of packet loss observers if it
+  // is on the list of observers. Must be called on the IO thread.
+  void RemovePacketLossObserver(PacketLossObserver* packet_loss_observer);
+
  protected:
   // NetworkID is used to uniquely identify a network.
   // For the purpose of network quality estimation and caching, a network is
   // uniquely identified by a combination of |type| and
   // |id|. This approach is unable to distinguish networks with
   // same name (e.g., different Wi-Fi networks with same SSID).
   // This is a protected member to expose it to tests.
   struct NET_EXPORT_PRIVATE NetworkID {
     NetworkID(NetworkChangeNotifier::ConnectionType type, const std::string& id)
         : type(type), id(id) {}
@@ skipping 31 matching lines @@
   // NetworkChangeNotifier::ConnectionTypeObserver implementation:
   void OnConnectionTypeChanged(
       NetworkChangeNotifier::ConnectionType type) override;
 
   // ExternalEstimateProvider::UpdatedEstimateObserver implementation.
   void OnUpdatedEstimateAvailable() override;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, StoreObservations);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, TestKbpsRTTUpdates);
+  FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest,
+                           TestPacketLossRateUpdates);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, TestAddObservation);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, ObtainOperatingParams);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, HalfLifeParam);
   FRIEND_TEST_ALL_PREFIXES(URLRequestTestHTTP, NetworkQualityEstimator);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest,
                            PercentileSameTimestamps);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest,
                            PercentileDifferentTimestamps);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, ComputedPercentiles);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, TestCaching);
@@ skipping 144 matching lines @@
     // buffer is available. Sets |result| to the computed |percentile|
     // value among all observations since |begin_timestamp|. If the value is
     // unavailable, false is returned and |result| is not modified. Percentile
     // value is unavailable if all the values in observation buffer are older
     // than |begin_timestamp|.
     // |result| must not be null.
     bool GetPercentile(const base::TimeTicks& begin_timestamp,
                        ValueType* result,
                        int percentile) const;
 
+    // Returns true iff the weighted average value of the observations in this
+    // buffer is available. Sets |result| to the computed average
+    // value among all observations since |begin_timestamp|. If the value is
+    // unavailable, false is returned and |result| is not modified. Average

[bengr, 2016/02/16 17:02:02]

Average -> The average

[tbansal1, 2016/02/17 18:35:11]

Done.

+    // value is unavailable if all the values in observation buffer are older

[bengr, 2016/02/16 17:02:02]

all the -> all of the
in observation -> in the observation

[tbansal1, 2016/02/17 18:35:11]

Done.

+    // than |begin_timestamp|. |result| must not be NULL.
+    bool GetWeightedAverage(const base::TimeTicks& begin_timestamp,
+                            ValueType* result) const;
+
    private:
     FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, StoreObservations);
     FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest,
                              ObtainOperatingParams);
     FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, HalfLifeParam);
 
     // Computes the weighted observations and stores them in
     // |weighted_observations| sorted by ascending |WeightedObservation.value|.
     // Only the observations with timestamp later than |begin_timestamp| are
     // considered. Also, sets |total_weight| to the total weight of all
@@ skipping 23 matching lines @@
       RttObservation;
   typedef net::NetworkQualityEstimator::ObservationBuffer<base::TimeDelta>
       RttObservationBuffer;
 
   // Value of throughput observations is in kilobits per second.
   typedef net::NetworkQualityEstimator::Observation<int32_t>
       ThroughputObservation;
   typedef net::NetworkQualityEstimator::ObservationBuffer<int32_t>
       ThroughputObservationBuffer;
 
+  // Value of a packet loss observation. A value of 0.0 indicates one packet
+  // was received successfully in-order, while a value of 1.0 indicates one
+  // packet was lost.

[bengr, 2016/02/16 17:02:02]

What does a value of 0.5 mean? Why can't this be a bool?

[tbansal1, 2016/02/17 18:35:11]

This is packet loss rate observation from the reception of a single packet.
Using a float makes it easier if in future, we record observation over multiple
packets (say 10 packets). Also, the current implementation of ObservationBuffer
computes the median/average with the same variable type as the observation
itself. So, using bool would make it complicated to compute median/average.

Renamed the variables a  bit to make it clearer.

+  typedef net::NetworkQualityEstimator::Observation<float>
+      PacketLossObservation;
+
   // This does not use a unordered_map or hash_map for code simplicity (key just
   // implements operator<, rather than hash and equality) and because the map is
   // tiny.
   typedef std::map<NetworkID, CachedNetworkQuality> CachedNetworkQualities;
 
   // Throughput is set to |kInvalidThroughput| if a valid value is
   // unavailable. Readers should discard throughput value if it is set to
   // |kInvalidThroughput|.
   static const int32_t kInvalidThroughput;
 
+  // Packet loss rate is set to |kInvalidPacketLossRate| if a valid value is
+  // unavailable. Readers should discard the packet loss rate value if it is
+  // set to |kInvalidPacketLossRate|.
+  static const float kInvalidPacketLossRate;

[bengr, 2016/02/16 17:02:02]

It might be better to provide an is_valid() method.

[tbansal1, 2016/02/17 18:35:11]

Changed to use a min valid value.

+
   // Tiny transfer sizes may give inaccurate throughput results.
   // Minimum size of the transfer over which the throughput is computed.
   static const int kMinTransferSizeInBytes = 10000;
 
   // Minimum duration (in microseconds) of the transfer over which the
   // throughput is computed.
   static const int kMinRequestDurationMicroseconds = 1000;
 
   // Minimum valid value of the variation parameter that holds RTT (in
   // milliseconds) values.
@@ skipping 38 matching lines @@
   // percentiles indicating less performant networks. For example, if
   // |percentile| is 90, then the network is expected to be faster than the
   // returned estimate with 0.9 probability. Similarly, network is expected to
   // be slower than the returned estimate with 0.1 probability.
   base::TimeDelta GetRTTEstimateInternal(const base::TimeTicks& begin_timestamp,
                                          int percentile) const;
   int32_t GetDownlinkThroughputKbpsEstimateInternal(
       const base::TimeTicks& begin_timestamp,
       int percentile) const;
 
+  // Returns an estimate of the packet loss rate. Only the observations later
+  // than |begin_timestamp| are taken into account.
+  float GetPacketLossRateEstimateInternal(
+      const base::TimeTicks& begin_timestamp) const;
+
   // Returns the current network ID checking by calling the platform APIs.
   // Virtualized for testing.
   virtual NetworkID GetCurrentNetworkID() const;
 
   // Writes the estimated quality of the current network to the cache.
   void CacheNetworkQualityEstimate();
 
   void NotifyObserversOfRTT(const RttObservation& observation);
 
   void NotifyObserversOfThroughput(const ThroughputObservation& observation);
 
+  void NotifyObserversOfPacketLoss(uint64_t num_packets_lost,
+                                   uint64_t num_packets_received_in_order,
+                                   uint64_t num_packets_received_not_in_order,
+                                   const base::TimeTicks& timestamp,
+                                   ObservationSource source);
+
   // Records the UMA related to RTT.
   void RecordRTTUMA(int32_t estimated_value_msec,
                     int32_t actual_value_msec) const;
 
   // Returns true only if |request| can be used for network quality estimation.
   // Only the requests that go over network are considered to provide useful
   // observations.
   bool RequestProvidesUsefulObservations(const URLRequest& request) const;
 
   // Values of external estimate provider status. This enum must remain
   // synchronized with the enum of the same name in
   // metrics/histograms/histograms.xml.
   enum NQEExternalEstimateProviderStatus {
     EXTERNAL_ESTIMATE_PROVIDER_STATUS_NOT_AVAILABLE,
     EXTERNAL_ESTIMATE_PROVIDER_STATUS_AVAILABLE,
     EXTERNAL_ESTIMATE_PROVIDER_STATUS_QUERIED,
     EXTERNAL_ESTIMATE_PROVIDER_STATUS_QUERY_SUCCESSFUL,
     EXTERNAL_ESTIMATE_PROVIDER_STATUS_CALLBACK,
     EXTERNAL_ESTIMATE_PROVIDER_STATUS_BOUNDARY
   };
 
   // Records the metrics related to external estimate provider.
   void RecordExternalEstimateProviderMetrics(
       NQEExternalEstimateProviderStatus status) const;
 
+  // Sets  |observation_source| that corresponds to the given |protocol|, and

[bengr, 2016/02/16 17:02:02]

Do you mean "Gets the?" The entire comment is awkward, so please rewrite.

[tbansal1, 2016/02/17 18:35:11]

Done.

+  // returns true. If the corresponding ObservationSource is unknown, false is
+  // returned and |observation_source| is not modified.
+  bool GetObservationSourceForProtocol(
+      const Protocol protocol,
+      ObservationSource* observation_source) const;
+
   // Determines if the requests to local host can be used in estimating the
   // network quality. Set to true only for tests.
   const bool allow_localhost_requests_;
 
   // Determines if the responses smaller than |kMinTransferSizeInBytes|
   // or shorter than |kMinTransferSizeInBytes| can be used in estimating the
   // network quality. Set to true only for tests.
   const bool allow_small_responses_;
 
   // Time when last connection change was observed.
@@ skipping 13 matching lines @@
   // Cache that stores quality of previously seen networks.
   CachedNetworkQualities cached_network_qualities_;
 
   // Buffer that holds throughput observations (in kilobits per second) sorted
   // by timestamp.
   ThroughputObservationBuffer downstream_throughput_kbps_observations_;
 
   // Buffer that holds RTT observations sorted by timestamp.
   RttObservationBuffer rtt_msec_observations_;
 
+  // Buffer that holds packet loss observations sorted by timestamp.
+  ObservationBuffer<float> packet_loss_rate_observations_;
+
   // Default network quality observations obtained from the network quality
   // estimator field trial parameters. The observations are indexed by
   // ConnectionType.
   NetworkQuality
       default_observations_[NetworkChangeNotifier::CONNECTION_LAST + 1];
 
   // Estimated network quality. Updated on mainframe requests.
   NetworkQuality estimated_median_network_quality_;
 
   // ExternalEstimateProvider that provides network quality using operating
   // system APIs. May be NULL.
   const scoped_ptr<ExternalEstimateProvider> external_estimate_provider_;
 
-  // Observer lists for round trip times and throughput measurements.
+  // Observer lists for round trip times, throughput and packet loss
+  // measurements.
   base::ObserverList<RTTObserver> rtt_observer_list_;
   base::ObserverList<ThroughputObserver> throughput_observer_list_;
+  base::ObserverList<PacketLossObserver> packet_loss_observer_list_;
 
   base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkQualityEstimator);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_NETWORK_QUALITY_ESTIMATOR_H_