Record UMA for total data usage consumed by Chromium

Record UMA for total data usage consumed by Chromium

The data usage count is recorded at the network delegate
layer. Also, the data usage count reported by operating system
is reported in different UMA metric.

BUG=640052
Committed: https://crrev.com/086197baf9f5fc856643818c95246f685fad6c3f
Cr-Commit-Position: refs/heads/master@{#420419}

[tbansal1, 2016-09-13 23:27:04]

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

[tbansal1, 2016-09-13 23:27:07]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-09-13 23:27:39]

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, 2016-09-13 23:48:58]

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

[commit-bot: I haz the power, 2016-09-13 23:48:59]

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

[tbansal1, 2016-09-14 00:57:44]

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

[tbansal1, 2016-09-14 00:58:06]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-09-14 00:58:14]

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

[tbansal1, 2016-09-14 01:05:49]

Description was changed from

==========
Record UMA for total data usage

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

to

==========
Record UMA for total data usage consumed by Chromium

The data usage count is recorded at the network delegate 
layer. Also, the data usage count reported by operating system
is reported in different UMA metric.

BUG=640052
==========

[tbansal1, 2016-09-14 01:08:04]

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

[commit-bot: I haz the power, 2016-09-14 01:08:32]

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

[tbansal1, 2016-09-14 01:08:47]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-09-14 01:22:53]

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

[tbansal1, 2016-09-14 01:23:00]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2016-09-14 01:23:10]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-09-14 01:23:27]

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

[tbansal1, 2016-09-14 01:24:05]

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

[tbansal1, 2016-09-14 01:24:20]

sclittle: ptal. Thanks.

[commit-bot: I haz the power, 2016-09-14 02:49:04]

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

[commit-bot: I haz the power, 2016-09-14 02:49:05]

Dry run: This issue passed the CQ dry run.

[sclittle, 2016-09-14 21:28:13]

https://codereview.chromium.org/2338563003/diff/100001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2338563003/diff/100001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:367:
data_use_measurement_.OnNetworkBytesReceived(*request, bytes_received);
Could you explain (e.g. in a comment in the code) why you can't just register
data_use_measurement_ as an observer on the DataUseAggregator? E.g., what's
different about this use case?

https://codereview.chromium.org/2338563003/diff/100001/components/data_use_me...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2338563003/diff/100001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:77:
RecordNetworkBytesReceivedOS();
Is it OK for performance to call this so often? Note that OnNetworkBytesReceived
is called whenever a chunk of a response is received, so e.g. if a large
resource is being downloaded, OnNetworkBytesReceived could be called a bunch of
times.

https://codereview.chromium.org/2338563003/diff/100001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:202: void
DataUseMeasurement::RecordNetworkBytesReceivedOS() {
Is there a timing issue here? E.g., suppose there's a whole bunch of data use
that happens at around the same time. Chrome would call into traffic stats
multiple times as it processes the received bytes, but traffic stats would
return the same value for each of these times. So all of those bytes would be
attributed to one of the chunks of Chrome bytes. Is this a problem?

https://codereview.chromium.org/2338563003/diff/100001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:207:
DCHECK_GE(bytes, rx_bytes_os_);
Are you sure you want to crash if this condition doesn't hold?

[tbansal1, 2016-09-15 01:15:13]

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

[commit-bot: I haz the power, 2016-09-15 01:15:50]

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, 2016-09-15 01:27:35]

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

[commit-bot: I haz the power, 2016-09-15 01:27:36]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  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-09-15 17:24:50]

Patchset #2 (id:120001) has been deleted

[tbansal1, 2016-09-15 17:37:00]

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

[tbansal1, 2016-09-15 17:37:04]

Patchset #2 (id:140001) has been deleted

[tbansal1, 2016-09-15 17:37:10]

Patchset #2 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-09-15 17:37:27]

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

[tbansal1, 2016-09-15 17:45:34]

Patchset #2 (id:180001) has been deleted

[tbansal1, 2016-09-15 17:45:43]

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

[tbansal1, 2016-09-15 17:46:04]

sclittle: ptal. thanks.

https://codereview.chromium.org/2338563003/diff/100001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2338563003/diff/100001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:367:
data_use_measurement_.OnNetworkBytesReceived(*request, bytes_received);
On 2016/09/14 21:28:13, sclittle wrote:
> Could you explain (e.g. in a comment in the code) why you can't just register
> data_use_measurement_ as an observer on the DataUseAggregator? E.g., what's
> different about this use case?

I want to record the data use by both the delegate and at TrafficStats layer.
DataUseAggregator provides measurements only at one of the two layers. Also, I
want to record overall data use (including incognito) which DataUseAggregator
does not provide. I have added comments to d_u_measurement.cc file.

https://codereview.chromium.org/2338563003/diff/100001/components/data_use_me...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2338563003/diff/100001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:77:
RecordNetworkBytesReceivedOS();
On 2016/09/14 21:28:13, sclittle wrote:
> Is it OK for performance to call this so often? Note that
OnNetworkBytesReceived
> is called whenever a chunk of a response is received, so e.g. if a large
> resource is being downloaded, OnNetworkBytesReceived could be called a bunch
of
> times.

Good catch. Fixed.

https://codereview.chromium.org/2338563003/diff/100001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:202: void
DataUseMeasurement::RecordNetworkBytesReceivedOS() {
On 2016/09/14 21:28:13, sclittle wrote:
> So all of those bytes would be
> attributed to one of the chunks of Chrome bytes. Is this a problem?
No, because I am not attributing traffic stats bytes to Chrome bytes. They are
recorded in completely separate histograms.

https://codereview.chromium.org/2338563003/diff/100001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:207:
DCHECK_GE(bytes, rx_bytes_os_);
On 2016/09/14 21:28:13, sclittle wrote:
> Are you sure you want to crash if this condition doesn't hold?

Yes, android provides guarantee that this is always increasing.
https://developer.android.com/reference/android/net/TrafficStats.html#getTota...

So, it shouldn't trip on DCHECK.

[commit-bot: I haz the power, 2016-09-15 17:46:22]

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, 2016-09-15 18:46:32]

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

[commit-bot: I haz the power, 2016-09-15 18:46:33]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-09-19 16:49:46]

ping. thanks.

[tbansal1, 2016-09-20 20:45:29]

sclittle: ping. thanks.

[sclittle, 2016-09-20 20:54:13]

https://codereview.chromium.org/2338563003/diff/200001/components/data_use_me...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2338563003/diff/200001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:197: //
background). This reduces the overhead of repeatedly calling the API.
I'm not sure 10k is much different here. If the user's on a fast connection then
this could be equivalent to calling into TrafficStats on every response chunk.

An alternative could be to have a timer here to call into traffic stats only
every so often.

An even easier approach might be to just call the TrafficStats-querying code
from OnComplete in the network delegate, since you're not too concerned with
timing anyways here.

https://codereview.chromium.org/2338563003/diff/200001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:198: static
const int64_t kMaxDelegateBytes = 10000;
nit: Should this be called kMinDelegateBytes, to match the comment above?

[tbansal1, 2016-09-21 01:55:29]

Patchset #3 (id:220001) has been deleted

[tbansal1, 2016-09-21 01:57:48]

Patchset #3 (id:240001) has been deleted

[tbansal1, 2016-09-21 02:02:33]

Patchset #3 (id:260001) has been deleted

[tbansal1, 2016-09-21 02:02:56]

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

[commit-bot: I haz the power, 2016-09-21 02:03:33]

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, 2016-09-21 02:17:15]

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

[commit-bot: I haz the power, 2016-09-21 02:17:16]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-09-21 04:24:13]

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

[commit-bot: I haz the power, 2016-09-21 04:24:25]

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

[tbansal1, 2016-09-21 04:24:35]

Patchset #3 (id:280001) has been deleted

[commit-bot: I haz the power, 2016-09-21 05:11:54]

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

[commit-bot: I haz the power, 2016-09-21 05:11:55]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-09-21 17:38:42]

sclittle: ptal. thanks.

https://codereview.chromium.org/2338563003/diff/200001/components/data_use_me...
File components/data_use_measurement/content/data_use_measurement.cc (right):

https://codereview.chromium.org/2338563003/diff/200001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:197: //
background). This reduces the overhead of repeatedly calling the API.
On 2016/09/20 20:54:13, sclittle wrote:
> I'm not sure 10k is much different here. If the user's on a fast connection
then
> this could be equivalent to calling into TrafficStats on every response chunk.
> 
> An alternative could be to have a timer here to call into traffic stats only
> every so often.
> 
> An even easier approach might be to just call the TrafficStats-querying code
> from OnComplete in the network delegate, since you're not too concerned with
> timing anyways here.

Done.

https://codereview.chromium.org/2338563003/diff/200001/components/data_use_me...
components/data_use_measurement/content/data_use_measurement.cc:198: static
const int64_t kMaxDelegateBytes = 10000;
On 2016/09/20 20:54:13, sclittle wrote:
> nit: Should this be called kMinDelegateBytes, to match the comment above?

Obsolete.

[sclittle, 2016-09-21 17:58:26]

LGTM

[tbansal1, 2016-09-21 18:03:32]

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

[tbansal1, 2016-09-21 18:03:33]

mmenke: ptal at chrome/browser/net/*
gayane: ptal at histograms.xml
Thanks.

[mmenke, 2016-09-21 18:07:40]

On 2016/09/21 18:03:33, tbansal1 wrote:
> mmenke: ptal at chrome/browser/net/*
> gayane: ptal at histograms.xml
> Thanks.

LGTM

[gayane -on leave until 09-2017, 2016-09-21 19:41:20]

LGTM

https://codereview.chromium.org/2338563003/diff/300001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2338563003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:101330: +  <suffix name="OS" label="As
reported by the operating system."/>
This suffix seems to be used only for Android, should it me mentioned in the
description ?

[tbansal1, 2016-09-22 17:59:19]

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

[tbansal1, 2016-09-22 17:59:22]

https://codereview.chromium.org/2338563003/diff/300001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2338563003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:101330: +  <suffix name="OS" label="As
reported by the operating system."/>
On 2016/09/21 19:41:20, gayane wrote:
> This suffix seems to be used only for Android, should it me mentioned in the
> description ?

Done.

[commit-bot: I haz the power, 2016-09-22 17:59:45]

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, 2016-09-22 19:06:19]

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

[commit-bot: I haz the power, 2016-09-22 19:06:21]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-09-22 19:06:47]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-09-22 19:06:48]

The patchset sent to the CQ was uploaded after l-g-t-m from gayane@chromium.org,
mmenke@chromium.org, sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/2338563003/#ps320001
(title: "rebased, addressed gayane comment")

[commit-bot: I haz the power, 2016-09-22 19:07: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, 2016-09-22 19:13:29]

Description was changed from

==========
Record UMA for total data usage consumed by Chromium

The data usage count is recorded at the network delegate 
layer. Also, the data usage count reported by operating system
is reported in different UMA metric.

BUG=640052
==========

to

==========
Record UMA for total data usage consumed by Chromium

The data usage count is recorded at the network delegate 
layer. Also, the data usage count reported by operating system
is reported in different UMA metric.

BUG=640052
==========

[commit-bot: I haz the power, 2016-09-22 19:13:31]

Committed patchset #4 (id:320001)

[commit-bot: I haz the power, 2016-09-22 19:15:29]

Description was changed from

==========
Record UMA for total data usage consumed by Chromium

The data usage count is recorded at the network delegate 
layer. Also, the data usage count reported by operating system
is reported in different UMA metric.

BUG=640052
==========

to

==========
Record UMA for total data usage consumed by Chromium

The data usage count is recorded at the network delegate
layer. Also, the data usage count reported by operating system
is reported in different UMA metric.

BUG=640052

Committed: https://crrev.com/086197baf9f5fc856643818c95246f685fad6c3f
Cr-Commit-Position: refs/heads/master@{#420419}
==========

[commit-bot: I haz the power, 2016-09-22 19:15:31]

Patchset 4 (id:??) landed as
https://crrev.com/086197baf9f5fc856643818c95246f685fad6c3f
Cr-Commit-Position: refs/heads/master@{#420419}