0

kMinDifferenceInMetrics 100->20 for network latency metrics.

Currently, when looking at a detailed histogram of HTTP RTT, we see
an outsized histogram bin for 175ms on Android.
This is likely because even though a more accurate estimate for this
metric would be available, it's within 100ms of the initial value
(175ms), and thus not considered a meaningfully changed metric.
But, 175ms HTTP RTT and 80ms HTTP RTT really should be considered
as meaningfully different.

So, this change separates the treatment for the network latency
metrics and instead will consider them meaningfully changed if their
absolute value changes by at least 20ms - in addition to the
ratio which is left as is.

I'm hoping that after this takes effect, we'll see more evenly
distributed histogram bins for Android / mobile. Thanks much
for considering.

Bug: 332749214
Change-Id: I17caee39f62afd19c8554727dda62421b589921b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5386046
Reviewed-by: mmenke <mmenke@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Commit-Queue: Johannes Henkel <johannes@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1284136}
This commit is contained in:
Johannes Henkel
2024-04-08 22:30:26 +00:00
committed by Chromium LUCI CQ
parent 79e17af468
commit f896f3f479

@ -22,7 +22,13 @@
namespace {
// Returns true if |past_value| is significantly different from |current_value|.
bool MetricChangedMeaningfully(int32_t past_value, int32_t current_value) {
// Metric changed meaningfully only if (i) the difference between the two
// values exceed the threshold |min_difference_in_metrics|; and,
// (ii) the ratio of the values also exceeds the threshold |min_ratio|.
bool MetricChangedMeaningfully(int32_t past_value,
int32_t current_value,
int min_difference_in_metrics,
float min_ratio) {
// A negative value indicates that the value of the corresponding metric is
// unavailable. A difference in signature between the |past_value| and
// |current_value| indicates change in the availability of the value of that
@ -36,19 +42,13 @@ bool MetricChangedMeaningfully(int32_t past_value, int32_t current_value) {
if (past_value < 0 && current_value < 0)
return false;
// Metric changed meaningfully only if (i) the difference between the two
// values exceed the threshold; and, (ii) the ratio of the values also exceeds
// the threshold.
static const int kMinDifferenceInMetrics = 100;
static const float kMinRatio = 1.2f;
if (std::abs(past_value - current_value) < kMinDifferenceInMetrics) {
if (std::abs(past_value - current_value) < min_difference_in_metrics) {
// The absolute change in the value is not sufficient enough.
return false;
}
if (past_value < (kMinRatio * current_value) &&
current_value < (kMinRatio * past_value)) {
if (past_value < (min_ratio * current_value) &&
current_value < (min_ratio * past_value)) {
// The relative change in the value is not sufficient enough.
return false;
}
@ -56,6 +56,19 @@ bool MetricChangedMeaningfully(int32_t past_value, int32_t current_value) {
return true;
}
bool LatencyMetricChangedMeaningfully(int32_t past_value,
int32_t current_value) {
return MetricChangedMeaningfully(past_value, current_value,
/*min_difference_in_metrics=*/20,
/*min_ratio=*/1.2f);
}
bool BandwidthMetricChangedMeaningfully(int32_t past_value,
int32_t current_value) {
return MetricChangedMeaningfully(past_value, current_value,
/*min_difference_in_metrics=*/100,
/*min_ratio=*/1.2f);
}
} // namespace
namespace network {
@ -151,11 +164,11 @@ void NetworkQualityEstimatorManager::OnRTTOrThroughputEstimatesComputed(
int32_t downstream_throughput_kbps) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
bool http_rtt_changed_meaningfully = MetricChangedMeaningfully(
bool http_rtt_changed_meaningfully = LatencyMetricChangedMeaningfully(
http_rtt.InMilliseconds(), http_rtt_.InMilliseconds());
bool transport_rtt_changed_meaningfully = MetricChangedMeaningfully(
bool transport_rtt_changed_meaningfully = LatencyMetricChangedMeaningfully(
transport_rtt.InMilliseconds(), transport_rtt_.InMilliseconds());
bool downlink_changed_meaningfully = MetricChangedMeaningfully(
bool downlink_changed_meaningfully = BandwidthMetricChangedMeaningfully(
downstream_throughput_kbps, downstream_throughput_kbps_);
if (!http_rtt_changed_meaningfully && !transport_rtt_changed_meaningfully &&