Record the data use by content type

Record the data use by content type

BUG=673820
Committed: https://crrev.com/a98850d939429e57c8a827adcb1e9f4760cb2db2
Cr-Commit-Position: refs/heads/master@{#441298}

[Raj, 2016-12-20 11:29:09]

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

[commit-bot: I haz the power, 2016-12-20 11:29:28]

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

[Raj, 2016-12-20 11:36:47]

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

[commit-bot: I haz the power, 2016-12-20 11:36:52]

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

[Raj, 2016-12-20 11:37:13]

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

[Raj, 2016-12-20 11:37:14]

ptal.

Will add unittests.

[commit-bot: I haz the power, 2016-12-20 11:59:30]

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

[commit-bot: I haz the power, 2016-12-20 11:59:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[RyanSturm, 2016-12-20 19:37:52]

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
File components/data_use_measurement/content/content_url_request_classifier.cc
(right):

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
components/data_use_measurement/content/content_url_request_classifier.cc:38:
(request.load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED)) {
Don't use the deprecated flag. Use ResourceRequestInfo and GetResourceType.

https://cs.chromium.org/chromium/src/content/public/browser/resource_request_...

Is using the ResorueceType in ResourceRequestInfo more accurate than MIME type?
I'm not sure which is better, but it might be worht investigating what happens
when they disagree.

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:69: enum
DataUseContenType {
nit: This seems duplicative. Maybe you could consolidate
DataUseUserData::ContentType and DataUseContenType.

If you change AUDIO_TABFOREGROUND to just AUDIO, you could return AUDIO from
GetContentType and in GetDataUseContentType you could leave it as AUDIO or
chagne it to AUDIO_TABBACKGROUND or AUDIO_APPBACKGROUND.

I'm sure there are other ways to consolidate them as well, but we are basically
defining two more content types on top of content::ResourceType.

If you think this way is clearer though, you can keep it, as it really costs the
user nothing.

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:272:
RecordContentTypeHistogram(attached_service_data->content_type(),
nit: I just noticed now that these are sort of strange in the sense that a
URLRequest could start in background and finish in foreground and be marked as
foreground (or vice versa). Maybe you should open a bug to resolve that later if
you agree that it is an issue.

[Raj, 2016-12-22 18:44:38]

ptal

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
File components/data_use_measurement/content/content_url_request_classifier.cc
(right):

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
components/data_use_measurement/content/content_url_request_classifier.cc:38:
(request.load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED)) {
On 2016/12/20 19:37:52, Ryan Sturm wrote:
> Don't use the deprecated flag. Use ResourceRequestInfo and GetResourceType.
> 
>
https://cs.chromium.org/chromium/src/content/public/browser/resource_request_...
> 
> Is using the ResorueceType in ResourceRequestInfo more accurate than MIME
type?
> I'm not sure which is better, but it might be worht investigating what happens
> when they disagree.

Thanks for pointing to ResourceType.
In my test of a few number of pages, ResourceType and MIME types match.
Moreover, for service requests MINE type needs to be used (ResourceType is not
populated). So I will stick with using MIME type for now. However ResourceType
provides more ways to break down. I will explore that.

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:69: enum
DataUseContenType {
On 2016/12/20 19:37:52, Ryan Sturm wrote:
> nit: This seems duplicative. Maybe you could consolidate
> DataUseUserData::ContentType and DataUseContenType.
> 
> If you change AUDIO_TABFOREGROUND to just AUDIO, you could return AUDIO from
> GetContentType and in GetDataUseContentType you could leave it as AUDIO or
> chagne it to AUDIO_TABBACKGROUND or AUDIO_APPBACKGROUND.
> 
> I'm sure there are other ways to consolidate them as well, but we are
basically
> defining two more content types on top of content::ResourceType.
> 
> If you think this way is clearer though, you can keep it, as it really costs
the
> user nothing.

Done.

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:272:
RecordContentTypeHistogram(attached_service_data->content_type(),
On 2016/12/20 19:37:52, Ryan Sturm wrote:
> nit: I just noticed now that these are sort of strange in the sense that a
> URLRequest could start in background and finish in foreground and be marked as
> foreground (or vice versa). Maybe you should open a bug to resolve that later
if
> you agree that it is an issue. 

Yes. That case is handled as follows.
These UMA are recorded on every NetworkBytes Sent/Received with the current
app/tab state. So if a URLRequest started in FG and moved to BG, part of the
bytes will be logged as FG and remaining as BG.

To explain in more detatil, data use is logged in 3 app states - FG, BG,
UNKNOWN. Unknown indicates FG->BG or BG->FG transitions.

[RyanSturm, 2016-12-22 20:00:42]

lgtm % a few nits/optional comments.

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
File components/data_use_measurement/content/content_url_request_classifier.cc
(right):

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
components/data_use_measurement/content/content_url_request_classifier.cc:38:
(request.load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED)) {
On 2016/12/22 18:44:37, Raj wrote:
> On 2016/12/20 19:37:52, Ryan Sturm wrote:
> > Don't use the deprecated flag. Use ResourceRequestInfo and GetResourceType.
> > 
> >
>
https://cs.chromium.org/chromium/src/content/public/browser/resource_request_...
> > 
> > Is using the ResorueceType in ResourceRequestInfo more accurate than MIME
> type?
> > I'm not sure which is better, but it might be worht investigating what
happens
> > when they disagree.
> 
> Thanks for pointing to ResourceType.
> In my test of a few number of pages, ResourceType and MIME types match.
> Moreover, for service requests MINE type needs to be used (ResourceType is not
> populated). So I will stick with using MIME type for now. However ResourceType
> provides more ways to break down. I will explore that.

Thanks for looking into it.

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2595503002/diff/20001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:272:
RecordContentTypeHistogram(attached_service_data->content_type(),
On 2016/12/22 18:44:37, Raj wrote:
> On 2016/12/20 19:37:52, Ryan Sturm wrote:
> > nit: I just noticed now that these are sort of strange in the sense that a
> > URLRequest could start in background and finish in foreground and be marked
as
> > foreground (or vice versa). Maybe you should open a bug to resolve that
later
> if
> > you agree that it is an issue. 
> 
> Yes. That case is handled as follows.
> These UMA are recorded on every NetworkBytes Sent/Received with the current
> app/tab state. So if a URLRequest started in FG and moved to BG, part of the
> bytes will be logged as FG and remaining as BG.
> 
> To explain in more detatil, data use is logged in 3 app states - FG, BG,
> UNKNOWN. Unknown indicates FG->BG or BG->FG transitions.

Awesome. Sounds like good enough granularity (FG->BG->FG in between calls to
NetworkBytesReceived is probably fine to treat as FG).

Can you add a comment with a similar explanation near if (old_app_state ==
CurrentAppState()) . When I first read that line, I misread it an missed the
subtlety of the tri-state machine.

https://codereview.chromium.org/2595503002/diff/60001/components/data_use_mea...
File components/data_use_measurement/content/content_url_request_classifier.cc
(right):

https://codereview.chromium.org/2595503002/diff/60001/components/data_use_mea...
components/data_use_measurement/content/content_url_request_classifier.cc:35:
content::ResourceRequestInfo::ForRequest(&request);
include ResourceRequestInfo, ResourceType, string

https://codereview.chromium.org/2595503002/diff/60001/components/data_use_mea...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2595503002/diff/60001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:182:
attached_service_data->set_app_state(CurrentAppState());
Maybe this should be called in OnBeforeURLRequest as well so the first set of
bytes isn't always marked as UNKNOWN.

[Raj, 2016-12-22 21:28:03]

rajendrant@chromium.org changed reviewers:
+ mpearson@google.com

[Raj, 2016-12-22 21:28:04]

mpearson: ptal histograms.xml

https://codereview.chromium.org/2595503002/diff/60001/components/data_use_mea...
File components/data_use_measurement/content/content_url_request_classifier.cc
(right):

https://codereview.chromium.org/2595503002/diff/60001/components/data_use_mea...
components/data_use_measurement/content/content_url_request_classifier.cc:35:
content::ResourceRequestInfo::ForRequest(&request);
On 2016/12/22 20:00:42, Ryan Sturm wrote:
> include ResourceRequestInfo, ResourceType, string

Done.

https://codereview.chromium.org/2595503002/diff/60001/components/data_use_mea...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2595503002/diff/60001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:182:
attached_service_data->set_app_state(CurrentAppState());
On 2016/12/22 20:00:42, Ryan Sturm wrote:
> Maybe this should be called in OnBeforeURLRequest as well so the first set of
> bytes isn't always marked as UNKNOWN.

Done.

[Mark P, 2016-12-22 21:40:28]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2016-12-22 21:40:29]

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:406:
STATIC_HISTOGRAM_POINTER_BLOCK(
Please use the standard UMA macro UMA_HISTOGRAM_ENUMERATION unless you have a
reason not to.  It doesn't appear you have a reason to avoid it here.

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:56: // with the enum
of the same name in metrics/histograms/histograms.xml.
Please follow the guidelines here:
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:70: OTHER,
Also, it's typical for people to put the unknown/other bucket first so that when
they add new elements later, it's not stuck in the middle somewhere.

https://codereview.chromium.org/2595503002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2595503002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9448: +    Data use of Chrome services
traffic by different content types.
Please follow the guidelines here, especially this one.
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

[Raj, 2016-12-22 23:14:37]

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
File components/data_use_measurement/core/data_use_measurement.cc (right):

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
components/data_use_measurement/core/data_use_measurement.cc:406:
STATIC_HISTOGRAM_POINTER_BLOCK(
On 2016/12/22 21:40:29, Mark P wrote:
> Please use the standard UMA macro UMA_HISTOGRAM_ENUMERATION unless you have a
> reason not to.  It doesn't appear you have a reason to avoid it here.

Added comments. UMA_HISTOGRAM_ENUMERATION cannot be used, since the histogram is
incremented 'byte' number of times.

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
File components/data_use_measurement/core/data_use_user_data.h (right):

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:56: // with the enum
of the same name in metrics/histograms/histograms.xml.
On 2016/12/22 21:40:29, Mark P wrote:
> Please follow the guidelines here:
>
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

Done.

https://codereview.chromium.org/2595503002/diff/80001/components/data_use_mea...
components/data_use_measurement/core/data_use_user_data.h:70: OTHER,
On 2016/12/22 21:40:29, Mark P wrote:
> Also, it's typical for people to put the unknown/other bucket first so that
when
> they add new elements later, it's not stuck in the middle somewhere.

Done.

https://codereview.chromium.org/2595503002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2595503002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:9448: +    Data use of Chrome services
traffic by different content types.
On 2016/12/22 21:40:29, Mark P wrote:
> Please follow the guidelines here, especially this one.
>
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

Done.

[Mark P, 2016-12-22 23:27:17]

histograms lgtm modulo two comments

I assume you've tested this using about:histograms?

--mark

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

https://codereview.chromium.org/2595503002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9449: +    when network bytes are
received by Chrome.
Please specify the unit somewhere here (bytes I assume).
ditto below

https://codereview.chromium.org/2595503002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9458: +    bytes are rece\   ived by
Chrome.
editing error here?

[Raj, 2016-12-23 00:47:39]

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

[commit-bot: I haz the power, 2016-12-23 00:47:54]

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-12-23 01:38:02]

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

[commit-bot: I haz the power, 2016-12-23 01:38:03]

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

[Raj, 2016-12-23 03:54:15]

Patchset #7 (id:120001) has been deleted

[Raj, 2016-12-23 03:55:06]

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

[commit-bot: I haz the power, 2016-12-23 03:55:16]

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

[Raj, 2016-12-23 05:31:10]

Patchset #7 (id:140001) has been deleted

[Raj, 2016-12-23 05:31:36]

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

[commit-bot: I haz the power, 2016-12-23 05:31:46]

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-12-23 06:13:14]

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

[commit-bot: I haz the power, 2016-12-23 06:13:16]

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...)

[Raj, 2016-12-23 06:39:44]

Patchset #7 (id:160001) has been deleted

[Raj, 2016-12-23 06:40:16]

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

[commit-bot: I haz the power, 2016-12-23 06:40:30]

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-12-23 07:09:57]

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

[commit-bot: I haz the power, 2016-12-23 07:10:01]

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_...)

[Raj, 2016-12-23 18:42:59]

Patchset #7 (id:180001) has been deleted

[Raj, 2016-12-24 02:43:50]

Patchset #7 (id:190001) has been deleted

[Raj, 2016-12-24 02:45:06]

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

[commit-bot: I haz the power, 2016-12-24 02:45:18]

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-12-24 03:50:17]

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

[commit-bot: I haz the power, 2016-12-24 03:50:18]

Dry run: This issue passed the CQ dry run.

[Raj, 2016-12-24 03:55:38]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-12-24 03:55:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2595503002/#ps210001
(title: "fixed nits")

[commit-bot: I haz the power, 2016-12-24 03:55:47]

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

[Raj, 2016-12-24 03:55:55]

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

https://codereview.chromium.org/2595503002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9449: +    when network bytes are
received by Chrome.
On 2016/12/22 23:27:17, Mark P wrote:
> Please specify the unit somewhere here (bytes I assume).
> ditto below

Done.

https://codereview.chromium.org/2595503002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:9458: +    bytes are rece\   ived by
Chrome.
On 2016/12/22 23:27:17, Mark P wrote:
> editing error here?

Done.

[commit-bot: I haz the power, 2016-12-24 04:02:00]

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

[commit-bot: I haz the power, 2016-12-24 04:02:02]

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...)

[Raj, 2016-12-24 04:53:20]

rajendrant@chromium.org changed reviewers:
+ nasko@chromium.org

[Raj, 2016-12-24 04:53:21]

nasko: ptal components/data_use_measurement/content/DEPS

[nasko, 2017-01-04 00:46:45]

components/data_use_measurement/content/DEPS LGTM

[Raj, 2017-01-04 01:03:46]

The CQ bit was checked by rajendrant@chromium.org

[commit-bot: I haz the power, 2017-01-04 01:04:18]

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-04 02:34:04]

CQ is committing da patch.

Bot data: {"patchset_id": 210001, "attempt_start_ts": 1483491826313880,
"parent_rev": "d4bbd07beb41eaad87f7ee2198a570ede7b05299", "commit_rev":
"903c0201f80af9289bf811b26dfeae7412ec3024"}

[commit-bot: I haz the power, 2017-01-04 02:34:37]

Description was changed from

==========
Record the data use by content type

BUG=673820
==========

to

==========
Record the data use by content type

BUG=673820

Review-Url: https://codereview.chromium.org/2595503002
==========

[commit-bot: I haz the power, 2017-01-04 02:34:44]

Committed patchset #7 (id:210001)

[commit-bot: I haz the power, 2017-01-04 02:37:06]

Description was changed from

==========
Record the data use by content type

BUG=673820

Review-Url: https://codereview.chromium.org/2595503002
==========

to

==========
Record the data use by content type

BUG=673820

Committed: https://crrev.com/a98850d939429e57c8a827adcb1e9f4760cb2db2
Cr-Commit-Position: refs/heads/master@{#441298}
==========

[commit-bot: I haz the power, 2017-01-04 02:37:07]

Patchset 7 (id:??) landed as
https://crrev.com/a98850d939429e57c8a827adcb1e9f4760cb2db2
Cr-Commit-Position: refs/heads/master@{#441298}