Add network quality change events to net log

Add network quality change events to net log

If Network Quality Estimator (NQE) determines that network quality has
changed, an event is added to the net log. The event includes the
effective connection type, http rtt, transport rtt, and throughput.
The net log source is set to NETWORK_QUALITY_ESTIMATOR.

This is how the net log looks like:
t=295473 [st=295473]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput_kbps = 29
                      --> effective_connection_type = "Slow2G"
                      --> http_rtt_ms = 4691
                      --> transport_rtt_ms = 1822

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

Review-Url: https://codereview.chromium.org/2593243003
Cr-Commit-Position: refs/heads/master@{#445153}
Committed: https://chromium.googlesource.com/chromium/src/+/97e38a2f826fed6afd6422f44ca291d553adda8c

[tbansal1, 2016-12-22 04:58:23]

Description was changed from

==========
NQE net log

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

to

==========
NQE net log

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

[tbansal1, 2016-12-22 04:58:51]

Description was changed from

==========
NQE net log

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

to

==========
NQE net log

This is how the net log looks like:
t= 41857 [st= 41857]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "62 kbps"
                      --> effective_connection_type = "Slow2G"
                      --> http_rtt = "3979 msec"
                      --> transport_rtt = "1948 msec"
t= 57901 [st= 57901]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t= 62008 [st= 62008]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "2658 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "116 msec"
                      --> transport_rtt = "66 msec"

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

[tbansal1, 2016-12-22 04:59:25]

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

[commit-bot: I haz the power, 2016-12-22 04:59:39]

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

[tbansal1, 2016-12-22 04:59:41]

Description was changed from

==========
NQE net log

This is how the net log looks like:
t= 41857 [st= 41857]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "62 kbps"
                      --> effective_connection_type = "Slow2G"
                      --> http_rtt = "3979 msec"
                      --> transport_rtt = "1948 msec"
t= 57901 [st= 57901]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t= 62008 [st= 62008]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "2658 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "116 msec"
                      --> transport_rtt = "66 msec"

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

to

==========
NQE net log

This is how the net log looks like:
t= 57901 [st= 57901]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t= 62008 [st= 62008]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "2658 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "116 msec"
                      --> transport_rtt = "66 msec"

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

[tbansal1, 2016-12-22 05:14:23]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-12-22 05:14:43]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2016-12-22 05:14:47]

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

[commit-bot: I haz the power, 2016-12-22 05:15:02]

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

[tbansal1, 2016-12-22 05:28:30]

Description was changed from

==========
NQE net log

This is how the net log looks like:
t= 57901 [st= 57901]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t= 62008 [st= 62008]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "2658 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "116 msec"
                      --> transport_rtt = "66 msec"

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

to

==========
NQE net log

This is how the net log looks like:
t=408791 [st=408791]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t=410057 [st=410057]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "978 kbps"
                      --> effective_connection_type = "4G"

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

[tbansal1, 2016-12-22 05:28:48]

Description was changed from

==========
NQE net log

This is how the net log looks like:
t=408791 [st=408791]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t=410057 [st=410057]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "978 kbps"
                      --> effective_connection_type = "4G"

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

to

==========
NQE net log

This is how the net log looks like:
t=408791 [st=408791]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t=410057 [st=410057]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "978 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "64 msec"
                      --> transport_rtt = "10 msec"

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

[commit-bot: I haz the power, 2016-12-22 05:33:01]

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

[commit-bot: I haz the power, 2016-12-22 05:33:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2016-12-22 07:28:03]

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

[commit-bot: I haz the power, 2016-12-22 07:28:17]

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

[tbansal1, 2016-12-22 07:29:57]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-12-22 07:36:08]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2017-01-09 21:49:08]

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

[commit-bot: I haz the power, 2017-01-09 21:49:49]

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-09 22:19:11]

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

[commit-bot: I haz the power, 2017-01-09 22:19:12]

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

[tbansal1, 2017-01-09 22:54:49]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2017-01-09 22:55:43]

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

[commit-bot: I haz the power, 2017-01-09 22:56:12]

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-09 22:56:17]

Description was changed from

==========
NQE net log

This is how the net log looks like:
t=408791 [st=408791]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t=410057 [st=410057]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "978 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "64 msec"
                      --> transport_rtt = "10 msec"

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

to

==========
NQE net log

This is how the net log looks like:
t=408791 [st=408791]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t=410057 [st=410057]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "978 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "64 msec"
                      --> transport_rtt = "10 msec"

BUG=676661

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

[tbansal1, 2017-01-09 22:56:28]

Patchset #1 (id:100001) has been deleted

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

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

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

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-01-10 01:16:09]

Description was changed from

==========
NQE net log

This is how the net log looks like:
t=408791 [st=408791]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t=410057 [st=410057]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "978 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "64 msec"
                      --> transport_rtt = "10 msec"

BUG=676661

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

to

==========
Add network quality change events to net log

If Network Quality Estimator (NQE) determines that network quality has
changed, an event is added to the net log. The event includes the
effective connection type, http rtt, transport rtt, and throughput.

This is how the net log looks like:
t=408791 [st=408791]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t=410057 [st=410057]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "978 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "64 msec"
                      --> transport_rtt = "10 msec"

BUG=676661

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

[tbansal1, 2017-01-10 01:16:55]

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

[tbansal1, 2017-01-10 01:16:55]

ryansturm: ptal. thanks.

[RyanSturm, 2017-01-10 16:22:38]

https://codereview.chromium.org/2593243003/diff/120001/chrome/browser/android...
File chrome/browser/android/net/external_estimate_provider_android_unittest.cc
(right):

https://codereview.chromium.org/2593243003/diff/120001/chrome/browser/android...
chrome/browser/android/net/external_estimate_provider_android_unittest.cc:11:
#include "base/memory/ptr_util.h"
Not seeing where ptr_util is used.

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

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.cc:20: std::unique_ptr<base::Value>
EffectiveConnectionTypeChangedNetLogCallback(
#include <memory>

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.cc:25: NetLogCaptureMode capture_mode) {
#include "net/log/net_log_capture_mode.h" since it's not a const ref.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.cc:35: return std::move(dict);
maybe #include <utility>

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

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.h:11: #include "net/base/net_export.h"
not sure if you need net_export

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.h:22: // Adds effective connection type changed event to
the net-internals log.
s/effective connection type changed/network quality changed/ for consistency
with the log output.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.h:23: // |http_rtt| is the estimate of the HTTP RTT.
|transport_rtt| is the estimate
units for both RTTs in the description (msec?)

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1570: base::TimeDelta http_rtt,
Why are you passing these values in? Can't you access them from
network_quality_?

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:182: // |external_estimates_provider| may be
NULL. |net_log| is guaranteed to be
Can you rephrase this to something like "The embedder guarantees that |net_log|
is non-null"

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:200: // network quality estimation.
|net_log| is guaranteed to be non-null.
same as above comment

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:516: // of the transport RTT.
|downstream_throughput_kbps| is the estimate of the
units for the RTTs (msec)

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_test_util.cc (right):

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_test_util.cc:43:
base::MakeUnique<BoundTestNetLog>()) {}
include ptr_util.h

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:569:
base::MakeUnique<BoundTestNetLog>());
include ptr_util.h

[tbansal1, 2017-01-10 18:27:53]

ryansturm: ptal. thanks.

https://codereview.chromium.org/2593243003/diff/120001/chrome/browser/android...
File chrome/browser/android/net/external_estimate_provider_android_unittest.cc
(right):

https://codereview.chromium.org/2593243003/diff/120001/chrome/browser/android...
chrome/browser/android/net/external_estimate_provider_android_unittest.cc:11:
#include "base/memory/ptr_util.h"
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> Not seeing where ptr_util is used.

Done.

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

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.cc:20: std::unique_ptr<base::Value>
EffectiveConnectionTypeChangedNetLogCallback(
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> #include <memory>

Done.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.cc:25: NetLogCaptureMode capture_mode) {
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> #include "net/log/net_log_capture_mode.h" since it's not a const ref.

Done.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.cc:35: return std::move(dict);
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> maybe #include <utility>

Done.

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

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.h:11: #include "net/base/net_export.h"
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> not sure if you need net_export

Done.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.h:22: // Adds effective connection type changed event to
the net-internals log.
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> s/effective connection type changed/network quality changed/ for consistency
> with the log output.

Done.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/event_creator....
net/nqe/event_creator.h:23: // |http_rtt| is the estimate of the HTTP RTT.
|transport_rtt| is the estimate
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> units for both RTTs in the description (msec?)

RTTs are in base::TimeDelta. So, units are not required.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1570: base::TimeDelta http_rtt,
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> Why are you passing these values in? Can't you access them from
> network_quality_?

Good catch. Done.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:182: // |external_estimates_provider| may be
NULL. |net_log| is guaranteed to be
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> Can you rephrase this to something like "The embedder guarantees that
|net_log|
> is non-null"

Done.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:200: // network quality estimation.
|net_log| is guaranteed to be non-null.
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> same as above comment

Done.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:516: // of the transport RTT.
|downstream_throughput_kbps| is the estimate of the
On 2017/01/10 16:22:37, Ryan Sturm wrote:
> units for the RTTs (msec)

RTTs are in base::TimeDelta. So, units are not required.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_test_util.cc (right):

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_test_util.cc:43:
base::MakeUnique<BoundTestNetLog>()) {}
On 2017/01/10 16:22:38, Ryan Sturm wrote:
> include ptr_util.h

Done.

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2593243003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:569:
base::MakeUnique<BoundTestNetLog>());
On 2017/01/10 16:22:38, Ryan Sturm wrote:
> include ptr_util.h

Done.

[tbansal1, 2017-01-10 18:28:43]

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

[commit-bot: I haz the power, 2017-01-10 18:29:17]

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-10 20:04:27]

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

[commit-bot: I haz the power, 2017-01-10 20:04:28]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-01-10 22:15:44]

lgtm

[tbansal1, 2017-01-10 22:18:29]

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

[tbansal1, 2017-01-10 22:18:29]

bengr: ptal. thanks.

[bengr, 2017-01-11 22:13:22]

https://codereview.chromium.org/2593243003/diff/140001/net/log/net_log_event_...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2593243003/diff/140001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3049: // quality of the network has changed.
How often can this happen? We shouldn't spam net log.

[tbansal1, 2017-01-11 23:25:39]

https://codereview.chromium.org/2593243003/diff/140001/net/log/net_log_event_...
File net/log/net_log_event_type_list.h (right):

https://codereview.chromium.org/2593243003/diff/140001/net/log/net_log_event_...
net/log/net_log_event_type_list.h:3049: // quality of the network has changed.
On 2017/01/11 22:13:22, bengr wrote:
> How often can this happen? We shouldn't spam net log.

ECT does not change too often. Also, NQE uses net log with source, so at any
time there is at most one NQE entry in the net-internals. In the worst case,
only that entry will get spammed will multiple events.

[bengr, 2017-01-12 18:48:39]

lgtm

[tbansal1, 2017-01-12 18:51:06]

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

[tbansal1, 2017-01-12 18:51:07]

mmenke: ptal at chrome/browser/*, components/cronet/*, net/log/.
Thanks.

[tbansal1, 2017-01-12 18:51:56]

On 2017/01/12 18:51:07, tbansal1 wrote:
> mmenke: ptal at chrome/browser/*, components/cronet/*, net/log/.
> Thanks.

Also, mmenke ptal at ios/chrome/browser/ios_chrome_io_thread.mm.
Thanks.

[mmenke, 2017-01-12 21:40:03]

Just one suggestion

https://codereview.chromium.org/2593243003/diff/140001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2593243003/diff/140001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:231: NetLogWithSource::Make(net_log,
net::NetLogSourceType::NONE)) {}
Seems like these should be top level events - i.e., just use NetLog::AddEvent,
rather than creating a NetLogSourceType.  Making a NetLogWithSource with a type
of NONE really wasn't an intended use case of NetLogWithSource.

[tbansal1, 2017-01-12 23:12:31]

https://codereview.chromium.org/2593243003/diff/140001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2593243003/diff/140001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:231: NetLogWithSource::Make(net_log,
net::NetLogSourceType::NONE)) {}
On 2017/01/12 21:40:02, mmenke wrote:
> Seems like these should be top level events - i.e., just use NetLog::AddEvent,
> rather than creating a NetLogSourceType.  Making a NetLogWithSource with a
type
> of NONE really wasn't an intended use case of NetLogWithSource.

My only concern with making this a top level event is that I might add more net
log NQE events, and would prefer not spamming the top-level net-log event list.
Is it better to use something other than NONE here?

[tbansal1, 2017-01-13 21:31:35]

mmenke, ptal. I have added a net log source. wdyt?

[tbansal1, 2017-01-13 21:39:16]

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

[commit-bot: I haz the power, 2017-01-13 21:39:45]

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

[mmenke, 2017-01-13 21:39:51]

On 2017/01/13 21:31:35, tbansal1 wrote:
> mmenke, ptal. I have added a net log source. wdyt?

Sorry for not responding, this came up on another review recently, and was
hoping eroman would weigh in on the pattern before I responded.  I'll take a
look (Maybe not tonight, meeting soon)

[commit-bot: I haz the power, 2017-01-13 22:55:36]

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

[commit-bot: I haz the power, 2017-01-13 22:55:37]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-18 19:43:06]

On 2017/01/13 21:39:51, mmenke wrote:
> On 2017/01/13 21:31:35, tbansal1 wrote:
> > mmenke, ptal. I have added a net log source. wdyt?
> 
> Sorry for not responding, this came up on another review recently, and was
> hoping eroman would weigh in on the pattern before I responded.  I'll take a
> look (Maybe not tonight, meeting soon)

So I'm still waiting on eroman (The other main NetLog person's) opinion on this
pattern on another CL.  Once we work things out there, we can move ahead here,
or change the pattern.

[tbansal1, 2017-01-18 20:03:11]

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

[tbansal1, 2017-01-18 20:03:12]

Adding eroman@ to reviewer list so  that the CL shows up prominently on the
codereview dashboard.

[eroman, 2017-01-19 23:57:25]

LGTM.

The pattern of using a separate Source for NQE looks fine to me. More commentary
in the other CL.

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

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/event_creator....
net/nqe/event_creator.cc:32: base::IntToString(http_rtt.InMilliseconds()) + "
msec");
Rather than setting this as a string parameter, can you set it as an integer
parameter (and include units in the name). For instance:

dict->SetDouble("http_rtt_ms", http_rtt.InMilliseconds());

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/event_creator....
net/nqe/event_creator.cc:36: base::IntToString(downstream_throughput_kbps) + "
kbps");
Same thing here and throughout.

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_test_util.cc (right):

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_test_util.cc:208: for (auto entry : entries) {
const auto&

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:524: 0,
estimator.GetEntriesCount(NetLogEventType::NETWORK_QUALITY_CHANGED));
Is it not possible to expect an exact number of events?

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

Patchset #4 (id:180001) has been deleted

[tbansal1, 2017-01-20 01:30:34]

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

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/event_creator....
net/nqe/event_creator.cc:32: base::IntToString(http_rtt.InMilliseconds()) + "
msec");
On 2017/01/19 23:57:24, eroman (slow) wrote:
> Rather than setting this as a string parameter, can you set it as an integer
> parameter (and include units in the name). For instance:
> 
> dict->SetDouble("http_rtt_ms", http_rtt.InMilliseconds());

Done.

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/event_creator....
net/nqe/event_creator.cc:36: base::IntToString(downstream_throughput_kbps) + "
kbps");
On 2017/01/19 23:57:24, eroman (slow) wrote:
> Same thing here and throughout.

Done.

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_test_util.cc (right):

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_test_util.cc:208: for (auto entry : entries) {
On 2017/01/19 23:57:25, eroman (slow) wrote:
> const auto&

Done.

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2593243003/diff/160001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:524: 0,
estimator.GetEntriesCount(NetLogEventType::NETWORK_QUALITY_CHANGED));
On 2017/01/19 23:57:25, eroman (slow) wrote:
> Is it not possible to expect an exact number of events?

Done.

[eroman, 2017-01-20 01:45:20]

lgtm

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

https://codereview.chromium.org/2593243003/diff/200001/net/nqe/event_creator....
net/nqe/event_creator.cc:31: dict->SetInteger("http_rtt_ms",
http_rtt.InMilliseconds());
[optional] if you care about sub-millisecond precision use InMillisecondsF()
along with SetDouble().

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

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

https://codereview.chromium.org/2593243003/diff/200001/net/nqe/event_creator....
net/nqe/event_creator.cc:31: dict->SetInteger("http_rtt_ms",
http_rtt.InMilliseconds());
On 2017/01/20 01:45:20, eroman (slow) wrote:
> [optional] if you care about sub-millisecond precision use InMillisecondsF()
> along with SetDouble().

No. I think milliseconds is enough. (There is lot of other NQE code which rounds
up/down to milliseconds).

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

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

[commit-bot: I haz the power, 2017-01-20 02:06:52]

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-20 02:08:09]

mmenke: ptal at 
chrome/browser/io_thread.cc
components/cronet/android/cronet_url_request_context_adapter.cc
ios/chrome/browser/ios_chrome_io_thread.mm.

Thanks.

[tbansal1, 2017-01-20 02:41:29]

Description was changed from

==========
Add network quality change events to net log

If Network Quality Estimator (NQE) determines that network quality has
changed, an event is added to the net log. The event includes the
effective connection type, http rtt, transport rtt, and throughput.

This is how the net log looks like:
t=408791 [st=408791]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "-1 kbps"
                      --> effective_connection_type = "Offline"
                      --> http_rtt = "-1 msec"
                      --> transport_rtt = "-1 msec"
t=410057 [st=410057]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput = "978 kbps"
                      --> effective_connection_type = "4G"
                      --> http_rtt = "64 msec"
                      --> transport_rtt = "10 msec"

BUG=676661

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

to

==========
Add network quality change events to net log

If Network Quality Estimator (NQE) determines that network quality has
changed, an event is added to the net log. The event includes the
effective connection type, http rtt, transport rtt, and throughput.
The net log source is set to NETWORK_QUALITY_ESTIMATOR.

This is how the net log looks like:
t=295473 [st=295473]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput_kbps = 29
                      --> effective_connection_type = "Slow2G"
                      --> http_rtt_ms = 4691
                      --> transport_rtt_ms = 1822

BUG=676661

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

[commit-bot: I haz the power, 2017-01-20 03:27:17]

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

[commit-bot: I haz the power, 2017-01-20 03:27:18]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-20 18:28:03]

On 2017/01/20 02:08:09, tbansal1 wrote:
> mmenke: ptal at 
> chrome/browser/io_thread.cc
> components/cronet/android/cronet_url_request_context_adapter.cc
> ios/chrome/browser/ios_chrome_io_thread.mm.
> 
> Thanks.

So...erm...it was brought up on the other CL that NetLog logs to tracing, and
having a bunch of subevents of a parent event (apparently) makes a tracing entry
that spans for the entire duration of the trace.  (Disclaimer:  I haven't
verified this)

[tbansal1, 2017-01-20 20:21:22]

On 2017/01/20 18:28:03, mmenke wrote:
> On 2017/01/20 02:08:09, tbansal1 wrote:
> > mmenke: ptal at 
> > chrome/browser/io_thread.cc
> > components/cronet/android/cronet_url_request_context_adapter.cc
> > ios/chrome/browser/ios_chrome_io_thread.mm.
> > 
> > Thanks.
> 
> So...erm...it was brought up on the other CL that NetLog logs to tracing, and
> having a bunch of subevents of a parent event (apparently) makes a tracing
entry
> that spans for the entire duration of the trace.  (Disclaimer:  I haven't
> verified this)

AFAICT, this is not true. I only checked the NQE log.
Screenshots: https://drive.google.com/open?id=0B-KiRb6NuIF-SVJwLWwwWUpzMDA

[mmenke, 2017-01-20 20:26:08]

On 2017/01/20 20:21:22, tbansal1 wrote:
> On 2017/01/20 18:28:03, mmenke wrote:
> > On 2017/01/20 02:08:09, tbansal1 wrote:
> > > mmenke: ptal at 
> > > chrome/browser/io_thread.cc
> > > components/cronet/android/cronet_url_request_context_adapter.cc
> > > ios/chrome/browser/ios_chrome_io_thread.mm.
> > > 
> > > Thanks.
> > 
> > So...erm...it was brought up on the other CL that NetLog logs to tracing,
and
> > having a bunch of subevents of a parent event (apparently) makes a tracing
> entry
> > that spans for the entire duration of the trace.  (Disclaimer:  I haven't
> > verified this)
> 
> AFAICT, this is not true. I only checked the NQE log.
> Screenshots: https://drive.google.com/open?id=0B-KiRb6NuIF-SVJwLWwwWUpzMDA

THanks for checking!  And with that, io_thread, cronet, ios all LGTM.

[tbansal1, 2017-01-20 20:32:42]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-01-20 20:32:42]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
ryansturm@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2593243003/#ps220001
(title: "rebased")

[commit-bot: I haz the power, 2017-01-20 20:33:15]

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-20 20:43:07]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484944362002730,
"parent_rev": "7ad5b48b37bf61094051d98198a705ce8eeafe44", "commit_rev":
"97e38a2f826fed6afd6422f44ca291d553adda8c"}

[commit-bot: I haz the power, 2017-01-20 20:43:43]

Description was changed from

==========
Add network quality change events to net log

If Network Quality Estimator (NQE) determines that network quality has
changed, an event is added to the net log. The event includes the
effective connection type, http rtt, transport rtt, and throughput.
The net log source is set to NETWORK_QUALITY_ESTIMATOR.

This is how the net log looks like:
t=295473 [st=295473]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput_kbps = 29
                      --> effective_connection_type = "Slow2G"
                      --> http_rtt_ms = 4691
                      --> transport_rtt_ms = 1822

BUG=676661

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

to

==========
Add network quality change events to net log

If Network Quality Estimator (NQE) determines that network quality has
changed, an event is added to the net log. The event includes the
effective connection type, http rtt, transport rtt, and throughput.
The net log source is set to NETWORK_QUALITY_ESTIMATOR.

This is how the net log looks like:
t=295473 [st=295473]  NETWORK_QUALITY_CHANGED
                      --> downstream_throughput_kbps = 29
                      --> effective_connection_type = "Slow2G"
                      --> http_rtt_ms = 4691
                      --> transport_rtt_ms = 1822

BUG=676661

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2593243003
Cr-Commit-Position: refs/heads/master@{#445153}
Committed:
https://chromium.googlesource.com/chromium/src/+/97e38a2f826fed6afd6422f44ca2...
==========

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

Committed patchset #5 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/97e38a2f826fed6afd6422f44ca2...