NQE: Add net log event if the metric changes substantially

NQE: Add net log event if the metric changes substantially

A Network Quality changed event is added to the net log if (i) RTT 
changes by more than 100 msec or if bandwidth changes by more than 100 
kbps; (ii) percent change is more than 20%.

BUG=696143
TBR=bengr@chromium.org

Review-Url: https://codereview.chromium.org/2724403004
Cr-Commit-Position: refs/heads/master@{#454736}
Committed: https://chromium.googlesource.com/chromium/src/+/f38471f3b0c824e14fa8c25a7cbab0746e056f63

[tbansal1, 2017-03-03 20:44:32]

Description was changed from

==========
Add net log events

BUG=
==========

to

==========
NQE: Add net log event if the metric changes by more than 100

A Network Quality changed event is added to the net log if RTT changes
by more than 100 msec or if bandwidth changes by more than 100 kbps.

BUG=696143
==========

[tbansal1, 2017-03-03 20:46:20]

tbansal@chromium.org changed reviewers:
+ ryansturm@chromium.org

[tbansal1, 2017-03-03 20:46:20]

ryansturm: ptal. Thanks.

[RyanSturm, 2017-03-03 22:10:21]

https://codereview.chromium.org/2724403004/diff/1/net/nqe/event_creator.cc
File net/nqe/event_creator.cc (right):

https://codereview.chromium.org/2724403004/diff/1/net/nqe/event_creator.cc#ne...
net/nqe/event_creator.cc:31: static const int kMinDifferenceInMetrics = 100;
This is fine, but you might consider having the difference be a proportion
instead. Like if the RTT changes by 20%. I imagine Slow2G will hit the net log
every time a new estimate is made, but that won't at all be the case for 4G as
is.

https://codereview.chromium.org/2724403004/diff/1/net/nqe/event_creator.cc#ne...
net/nqe/event_creator.cc:48: bool MetricChangedMeaningfully(int32_t past_value,
int32_t current_value) {
nit: the behavior is already correct, but adding a separate case for both being
INVALID_RTT_THROUGHPUT may be more clear.

https://codereview.chromium.org/2724403004/diff/1/net/nqe/network_quality_est...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2724403004/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator_unittest.cc:414: EXPECT_LE(
why is this LE now?

[tbansal1, 2017-03-03 23:05:41]

Description was changed from

==========
NQE: Add net log event if the metric changes by more than 100

A Network Quality changed event is added to the net log if RTT changes
by more than 100 msec or if bandwidth changes by more than 100 kbps.

BUG=696143
==========

to

==========
NQE: Add net log event if the metric changes substantially

A Network Quality changed event is added to the net log if (i) RTT 
changes by more than 100 msec or if bandwidth changes by more than 100 
kbps; (ii) percent change is more than 20%.

BUG=696143
==========

[tbansal1, 2017-03-03 23:06:36]

ptal. Thanks.

https://codereview.chromium.org/2724403004/diff/1/net/nqe/event_creator.cc
File net/nqe/event_creator.cc (right):

https://codereview.chromium.org/2724403004/diff/1/net/nqe/event_creator.cc#ne...
net/nqe/event_creator.cc:31: static const int kMinDifferenceInMetrics = 100;
On 2017/03/03 22:10:21, Ryan Sturm wrote:
> This is fine, but you might consider having the difference be a proportion
> instead. Like if the RTT changes by 20%. I imagine Slow2G will hit the net log
> every time a new estimate is made, but that won't at all be the case for 4G as
> is.

Yeah, percentage difference is one way but then we do not want to add net log if
it changes from 10 msec to 20 msec.

https://codereview.chromium.org/2724403004/diff/1/net/nqe/event_creator.cc#ne...
net/nqe/event_creator.cc:48: bool MetricChangedMeaningfully(int32_t past_value,
int32_t current_value) {
On 2017/03/03 22:10:21, Ryan Sturm wrote:
> nit: the behavior is already correct, but adding a separate case for both
being
> INVALID_RTT_THROUGHPUT may be more clear.

Done.

https://codereview.chromium.org/2724403004/diff/1/net/nqe/network_quality_est...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2724403004/diff/1/net/nqe/network_quality_est...
net/nqe/network_quality_estimator_unittest.cc:414: EXPECT_LE(
On 2017/03/03 22:10:21, Ryan Sturm wrote:
> why is this LE now?

With this CL, it is difficult to precisely determine the number of entries that
will be added to the net log.

[RyanSturm, 2017-03-03 23:10:25]

lgtm

[tbansal1, 2017-03-03 23:10:49]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 23:11:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 00:04:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-04 00:04:10]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-03-04 00:05:41]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2017-03-04 00:06:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 00:15:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-04 00:15:37]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tbansal1, 2017-03-04 01:01:28]

Description was changed from

==========
NQE: Add net log event if the metric changes substantially

A Network Quality changed event is added to the net log if (i) RTT 
changes by more than 100 msec or if bandwidth changes by more than 100 
kbps; (ii) percent change is more than 20%.

BUG=696143
==========

to

==========
NQE: Add net log event if the metric changes substantially

A Network Quality changed event is added to the net log if (i) RTT 
changes by more than 100 msec or if bandwidth changes by more than 100 
kbps; (ii) percent change is more than 20%.

BUG=696143
TBR=bengr@chromium.org
==========

[tbansal1, 2017-03-04 01:01:28]

tbansal@chromium.org changed reviewers:
+ bengr@chromium.org

[tbansal1, 2017-03-04 01:01:47]

TBR'ing bengr@chromium.org for BUILD.gn.

[tbansal1, 2017-03-04 01:02:05]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2017-03-04 01:02:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 01:07:08]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488589325934320,
"parent_rev": "f9cdb557485c0186a1f4520c966628736465b84c", "commit_rev":
"f38471f3b0c824e14fa8c25a7cbab0746e056f63"}

[commit-bot: I haz the power, 2017-03-04 01:08:10]

Description was changed from

==========
NQE: Add net log event if the metric changes substantially

A Network Quality changed event is added to the net log if (i) RTT 
changes by more than 100 msec or if bandwidth changes by more than 100 
kbps; (ii) percent change is more than 20%.

BUG=696143
TBR=bengr@chromium.org
==========

to

==========
NQE: Add net log event if the metric changes substantially

A Network Quality changed event is added to the net log if (i) RTT 
changes by more than 100 msec or if bandwidth changes by more than 100 
kbps; (ii) percent change is more than 20%.

BUG=696143
TBR=bengr@chromium.org

Review-Url: https://codereview.chromium.org/2724403004
Cr-Commit-Position: refs/heads/master@{#454736}
Committed:
https://chromium.googlesource.com/chromium/src/+/f38471f3b0c824e14fa8c25a7cba...
==========

[commit-bot: I haz the power, 2017-03-04 01:08:11]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f38471f3b0c824e14fa8c25a7cba...