NQE: Add more statistics to ObservationBuffer class

net/nqe/observation_buffer.h

Index: net/nqe/observation_buffer.h
diff --git a/net/nqe/observation_buffer.h b/net/nqe/observation_buffer.h
index 9a4ce3ab238c2bd1921f70daa232adb0c6a509db..9e4077839317468a4b85b1256d18e413d0fe260e 100644
--- a/net/nqe/observation_buffer.h
+++ b/net/nqe/observation_buffer.h
@@ -122,6 +122,90 @@ class NET_EXPORT_PRIVATE ObservationBuffer {
     return true;
   }
 
+  // Returns true iff the weighted average of the observations in this
+  // buffer is available. Sets |result| to the computed weighted average value
+  // among all observations since |begin_timestamp|. If the value is

[bengr, 2016/10/14 21:15:58]

among -> of?
since -> made on or after

[tbansal1, 2016/10/14 22:25:29]

Done.

+  // unavailable, false is returned and |result| is not modified. The unweighted
+  // average value is unavailable if all the values in observation buffer are
+  // older than |begin_timestamp|. |result| must not be null.
+  bool GetWeightedAverage(const base::TimeTicks begin_timestamp,

[bengr, 2016/10/14 21:15:58]

Did you mean for this parameter to be a const reference?

[tbansal1, 2016/10/14 22:25:30]

Done.

+                          const std::vector<NetworkQualityObservationSource>&
+                              disallowed_observation_sources,
+                          ValueType* result) const {

[bengr, 2016/10/14 21:15:58]

Why is this not a base::TimeDelta?

[tbansal1, 2016/10/14 22:25:30]

It is a template class. Caller may set it to base::TimeDelta (for RTT) or int32
(for throughput).

+    DCHECK(result);

[bengr, 2016/10/14 21:15:58]

No need for this DCHECK.

[tbansal1, 2016/10/14 22:25:30]

Done.

+    DCHECK_GE(Capacity(), Size());
+
+    // Stores WeightedObservation in increasing order of value.

[bengr, 2016/10/14 21:15:58]

I would say:
//Stores weighted observations in increasing order by value.

[tbansal1, 2016/10/14 22:25:30]

Done.

+    std::vector<WeightedObservation<ValueType>> weighted_observations;
+
+    // Total weight of all observations in |weighted_observations|.
+    double total_weight = 0.0;
+
+    ComputeWeightedObservations(begin_timestamp, weighted_observations,
+                                &total_weight, disallowed_observation_sources);
+    if (weighted_observations.empty())
+      return false;
+
+    DCHECK_LT(0.0, total_weight);

[bengr, 2016/10/14 21:15:58]

Why can't a weight be 0.0?

[tbansal1, 2016/10/14 22:25:30]

Because there is a divide by |total_weight| later on line 165, and that may
cause divide-by-zero. This DCHECK is more for documentation.

It is not expected to be zero because |(weighted_observations| has at least one
entry.

+
+    // |weighted_observations| may have a smaller size than |observations_|
+    // since the former contains only the observations later than
+    // |begin_timestamp|.
+    DCHECK_GE(observations_.size(), weighted_observations.size());
+
+    // Weighted average is the sum of observations times their respective
+    // weight, divided by the sum of weights of all observations.

[bengr, 2016/10/14 21:15:58]

weight -> weights
of weights -> of the weights

[tbansal1, 2016/10/14 22:25:29]

Done.

+    double total_weight_times_value = 0.0;
+    for (const auto& weighted_observation : weighted_observations) {
+      total_weight_times_value +=
+          (weighted_observation.weight *
+           ConvertValueTypeToDouble(weighted_observation.value));
+    }
+
+    ConvertDoubleToValueType(total_weight_times_value / total_weight, result);
+    return true;
+  }
+
+  // Returns true iff the unweighted average of the observations in this buffer
+  // is available. Sets |result| to the computed unweighted average value among
+  // all observations since |begin_timestamp|. If the value is unavailable,
+  // false is returned and |result| is not modified. The weighted average value
+  // is unavailable if all the values in observation buffer are older than

[bengr, 2016/10/14 21:15:58]

all the values in -> all values in the

[tbansal1, 2016/10/14 22:25:30]

Done.

+  // |begin_timestamp|. |result| must not be null.
+  bool GetUnweightedAverage(const base::TimeTicks begin_timestamp,

[bengr, 2016/10/14 21:15:58]

This method seems very similar to the previous one. Is it possible to extract
the common parts into an anonymous function that they share?

[tbansal1, 2016/10/14 22:25:29]

I have moved some of the common code to ComputeWeightedObservations().

+                            const std::vector<NetworkQualityObservationSource>&
+                                disallowed_observation_sources,
+                            ValueType* result) const {
+    DCHECK(result);

[bengr, 2016/10/14 21:15:58]

Remove this DCHECK.

[tbansal1, 2016/10/14 22:25:29]

Done.

+    DCHECK_GE(Capacity(), Size());
+
+    // Stores WeightedObservation in increasing order of value.

[bengr, 2016/10/14 21:15:58]

See previous comment on wording.

[tbansal1, 2016/10/14 22:25:29]

Done.

+    std::vector<WeightedObservation<ValueType>> weighted_observations;
+
+    // Total weight of all observations in |weighted_observations|.
+    double total_weight = 0.0;
+
+    ComputeWeightedObservations(begin_timestamp, weighted_observations,
+                                &total_weight, disallowed_observation_sources);
+    if (weighted_observations.empty())
+      return false;
+
+    // |weighted_observations| may have a smaller size than |observations_|
+    // since the former contains only the observations later than
+    // |begin_timestamp|.
+    DCHECK_GE(observations_.size(), weighted_observations.size());
+
+    // Unweighted average is the sum of all observations divided by the number

[bengr, 2016/10/14 21:15:58]

Unweighted -> The unweighted

[tbansal1, 2016/10/14 22:25:30]

Done.

+    // of observations.
+    double total_value = 0.0;
+    for (const auto& weighted_observation : weighted_observations)
+      total_value += ConvertValueTypeToDouble(weighted_observation.value);
+
+    ConvertDoubleToValueType(total_value / weighted_observations.size(),
+                             result);
+    return true;
+  }
+
   void SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock) {
     tick_clock_ = std::move(tick_clock);
   }
@@ -130,6 +214,21 @@ class NET_EXPORT_PRIVATE ObservationBuffer {
   // Maximum number of observations that can be held in the ObservationBuffer.
   static const size_t kMaximumObservationsBufferSize = 300;
 
+  // Convert different ValueTypes to double to make it possible to perform

[bengr, 2016/10/14 21:15:58]

It would be cleaner to not have to use any of these converters. I'm not sure why
they're necessary.

[tbansal1, 2016/10/14 22:25:29]

They are necessary for doing arithmetic operations on base::TimeDelta. Without
these, when computing the weighted average, the code will need to multiple
ValueType (which is base::TimeDelta) with double. That operation is not
supported. So, we have to temporarily convert base::TimeDelta to double.

+  // arithmetic operations on them.
+  double ConvertValueTypeToDouble(base::TimeDelta input) const {
+    return input.InMilliseconds();
+  }
+  double ConvertValueTypeToDouble(int32_t input) const { return input; }
+
+  // Convert double to different ValueTypes.
+  void ConvertDoubleToValueType(double input, base::TimeDelta* output) const {
+    *output = base::TimeDelta::FromMilliseconds(input);
+  }
+  void ConvertDoubleToValueType(double input, int32_t* output) const {
+    *output = input;
+  }
+
   // 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