NQE: Add net log events when there is a significant change in metrics

NQE: Add net log events when there is a significant change in metrics

Add net log events for Network Quality Estimator (NQE) when there is a 
significant change in network quality metrics (e.g., RTT, bandwidth).
Currently, the net log event is added only when there is a change in
the effective connection type. This CL modifies the logic to add a
net log event when there is a significant change in other metrics.
Also, this CL refactors the code to make EventCreator a non-static
class.

BUG=676661
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2643213003
Cr-Commit-Position: refs/heads/master@{#446319}
Committed: https://chromium.googlesource.com/chromium/src/+/781262f98757b3e657a9089de7f98b9752482d09

[tbansal1, 2017-01-21 06:18:19]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2017-01-21 06:18:47]

Patchset #1 (id:1) has been deleted

[tbansal1, 2017-01-21 06:19:52]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2017-01-21 06:20:36]

Description was changed from

==========
Print net log

Add network quality net log events when NQE obtains an estimate of
RTTs
BUG=
==========

to

==========
Print net log

Add network quality net log events when NQE obtains an estimate of
RTTs
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-01-21 06:20:48]

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

[commit-bot: I haz the power, 2017-01-21 06:21:00]

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-01-21 07:29:19]

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

[commit-bot: I haz the power, 2017-01-21 07:29:20]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-01-23 20:41:28]

Description was changed from

==========
Print net log

Add network quality net log events when NQE obtains an estimate of
RTTs
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
NQE: Add net log events when there is a significant change in metrics

Add net log events for Network Quality Estimator (NQE) when there is a 
significant change in network quality metrics (e.g., RTT, bandwidth).
Currently, the net log event is added only when there is a change in
the effective connection type. This CL modifies the logic to add a
net log event when there is a significant change in other metrics.
Also, this CL refactors the code to make EventCreator a non-static
class.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-01-23 20:43:38]

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

[tbansal1, 2017-01-23 20:43:42]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-01-23 20:44:21]

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

[tbansal1, 2017-01-23 20:44:30]

Description was changed from

==========
NQE: Add net log events when there is a significant change in metrics

Add net log events for Network Quality Estimator (NQE) when there is a 
significant change in network quality metrics (e.g., RTT, bandwidth).
Currently, the net log event is added only when there is a change in
the effective connection type. This CL modifies the logic to add a
net log event when there is a significant change in other metrics.
Also, this CL refactors the code to make EventCreator a non-static
class.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
NQE: Add net log events when there is a significant change in metrics

Add net log events for Network Quality Estimator (NQE) when there is a 
significant change in network quality metrics (e.g., RTT, bandwidth).
Currently, the net log event is added only when there is a change in
the effective connection type. This CL modifies the logic to add a
net log event when there is a significant change in other metrics.
Also, this CL refactors the code to make EventCreator a non-static
class.

BUG=676661
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-01-23 20:44:49]

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

[tbansal1, 2017-01-23 20:44:49]

bengr: ptal. thanks.

[commit-bot: I haz the power, 2017-01-23 22:07:42]

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

[commit-bot: I haz the power, 2017-01-23 22:07:43]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-01-25 18:03:05]

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

[tbansal1, 2017-01-25 18:03:05]

ryansturm: ptal. Thanks.

[RyanSturm, 2017-01-25 22:56:51]

lgtm % nits

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

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.c...
net/nqe/event_creator.cc:57: bool http_rtt_changed =
(past_network_quality_.http_rtt() == InvalidRTT() &&
nit: this pattern can be flattened a little bit:

instead of:

(a == invalid && b != invalid) || (a != invalid && b == invalid)

use:

(a == invalid) != (b == invalid)

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.c...
net/nqe/event_creator.cc:77: // Return since none of the metrics changed
significantly.
nit: s/significantly/meaningfully/ This might be me nit-picking with  too much
coffee in me, but increasing the RTT from 4G to slow2G is a significant change,
it's just not meaningful.

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.h
File net/nqe/event_creator.h (right):

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.h...
net/nqe/event_creator.h:32: // May add network quality changed event to the
net-internals log.
Add to the comment about either when it will add to the log or when it won't.

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.h...
net/nqe/event_creator.h:42: //  |past_effective_connection_type| is the
effective connection type when the
reformat these comments to not reference the variable name:

"The effective connection...."

[tbansal1, 2017-01-26 00:04:45]

Patchset #2 (id:100001) has been deleted

[tbansal1, 2017-01-26 00:05:46]

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

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.c...
net/nqe/event_creator.cc:57: bool http_rtt_changed =
(past_network_quality_.http_rtt() == InvalidRTT() &&
On 2017/01/25 22:56:50, Ryan Sturm wrote:
> nit: this pattern can be flattened a little bit:
> 
> instead of:
> 
> (a == invalid && b != invalid) || (a != invalid && b == invalid)
> 
> use:
> 
> (a == invalid) != (b == invalid)

Thanks, this cleaned up the code a lot. Done.

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.c...
net/nqe/event_creator.cc:77: // Return since none of the metrics changed
significantly.
On 2017/01/25 22:56:50, Ryan Sturm wrote:
> nit: s/significantly/meaningfully/ This might be me nit-picking with  too much
> coffee in me, but increasing the RTT from 4G to slow2G is a significant
change,
> it's just not meaningful.

Done.

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.h
File net/nqe/event_creator.h (right):

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.h...
net/nqe/event_creator.h:32: // May add network quality changed event to the
net-internals log.
On 2017/01/25 22:56:50, Ryan Sturm wrote:
> Add to the comment about either when it will add to the log or when it won't.

Done.

https://codereview.chromium.org/2643213003/diff/80001/net/nqe/event_creator.h...
net/nqe/event_creator.h:42: //  |past_effective_connection_type| is the
effective connection type when the
On 2017/01/25 22:56:50, Ryan Sturm wrote:
> reformat these comments to not reference the variable name:
> 
> "The effective connection...."

Done.

[tbansal1, 2017-01-26 00:05:50]

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

[commit-bot: I haz the power, 2017-01-26 00:06:31]

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-01-26 03:27:04]

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

[commit-bot: I haz the power, 2017-01-26 03:27:05]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tbansal1, 2017-01-26 03:44:47]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-01-26 03:44:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2643213003/#ps120001
(title: "ryansturm comments")

[commit-bot: I haz the power, 2017-01-26 03:45:19]

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-01-26 06:18:10]

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

[commit-bot: I haz the power, 2017-01-26 06:18:12]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tbansal1, 2017-01-26 13:49:22]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2017-01-26 13:49:39]

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-01-26 14:21:25]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485438562215720,
"parent_rev": "08d2857e7529247a40eb631e108147d8b8374485", "commit_rev":
"781262f98757b3e657a9089de7f98b9752482d09"}

[commit-bot: I haz the power, 2017-01-26 14:22:02]

Description was changed from

==========
NQE: Add net log events when there is a significant change in metrics

Add net log events for Network Quality Estimator (NQE) when there is a 
significant change in network quality metrics (e.g., RTT, bandwidth).
Currently, the net log event is added only when there is a change in
the effective connection type. This CL modifies the logic to add a
net log event when there is a significant change in other metrics.
Also, this CL refactors the code to make EventCreator a non-static
class.

BUG=676661
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
NQE: Add net log events when there is a significant change in metrics

Add net log events for Network Quality Estimator (NQE) when there is a 
significant change in network quality metrics (e.g., RTT, bandwidth).
Currently, the net log event is added only when there is a change in
the effective connection type. This CL modifies the logic to add a
net log event when there is a significant change in other metrics.
Also, this CL refactors the code to make EventCreator a non-static
class.

BUG=676661
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2643213003
Cr-Commit-Position: refs/heads/master@{#446319}
Committed:
https://chromium.googlesource.com/chromium/src/+/781262f98757b3e657a9089de7f9...
==========

[commit-bot: I haz the power, 2017-01-26 14:22:04]

Committed patchset #2 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/781262f98757b3e657a9089de7f9...