Add data usage tracking for feedback uploader, crash uploader, dom distiller

Add data usage tracking for feedback uploader, trace uploader, dom distiller

This CL tags the data use from Chrome services feedback uploader, trace uploader
and dom distiller

BUG=527304
Committed: https://crrev.com/06be1a78361e2243545ae1cce115f5bbe9739b1d
Cr-Commit-Position: refs/heads/master@{#425594}

[Raj, 2016-09-27 22:37:55]

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

[commit-bot: I haz the power, 2016-09-27 22:38:14]

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

[Raj, 2016-09-27 22:41:41]

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

[commit-bot: I haz the power, 2016-09-27 22:42:00]

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

[Raj, 2016-09-27 22:45:49]

Patchset #1 (id:1) has been deleted

[Raj, 2016-09-27 23:31:50]

rajendrant@chromium.org changed reviewers:
+ wychen@chromium.org, yfriedman@chromium.org, zork@chromium.org

[Raj, 2016-09-27 23:31:51]

zork: ptal components/feedback/* 

yfriedman: ptal  chrome/browser/android/feedback/connectivity_checker.cc

wychen: ptal components/dom_distiller/*

[Zachary Kuznia, 2016-09-27 23:56:57]

lgtm

[commit-bot: I haz the power, 2016-09-28 00:17:32]

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

[commit-bot: I haz the power, 2016-09-28 00:17:33]

Dry run: This issue passed the CQ dry run.

[wychen, 2016-09-28 07:36:02]

dom distiller lgtm

A side note: In normal use cases (where the original WebContents is the input of
DOM distiller), all the resources should have already been downloaded, and cause
no extra traffic. The only exception for now is srcset. The original page would
only download the displayed size, while DOM distiller extracts all sizes and
download them all.

[Raj, 2016-10-14 02:00:47]

rajendrant@chromium.org changed reviewers:
+ bauerb@chromium.org, holte@chromium.org, primiano@chromium.org

[Raj, 2016-10-14 02:00:48]

bauerb: ptal  chrome/browser/android/feedback/connectivity_checker.cc	
primiano: ptal  chrome/browser/tracing/crash_service_uploader.cc
holte: ptal histograms.xml

[Primiano Tucci (use gerrit), 2016-10-14 08:53:40]

primiano@chromium.org changed reviewers:
+ oysteine@chromium.org

[Primiano Tucci (use gerrit), 2016-10-14 08:53:40]

+oysteine
Code itself LG, but the name you are using here is extremely misleading.
What you are hooking there in components/tracing is NOT the crash uploader
service (despite the name of the class). That lives internally.
What you are seeing there is the service that uploads traces captured in
background via finch experiments. 
The tricky bit is that background tracing just happen to use the same backend
that the crash service uses. but those are *not* crashes.

P.S: DataUseUserData: are you really really sure you can't get a better name for
this?

[Bernhard Bauer, 2016-10-14 11:08:12]

lgtm

[Raj, 2016-10-14 17:23:57]

On 2016/10/14 08:53:40, Primiano Tucci wrote:
> +oysteine
> Code itself LG, but the name you are using here is extremely misleading.
> What you are hooking there in components/tracing is NOT the crash uploader
> service (despite the name of the class). That lives internally.
> What you are seeing there is the service that uploads traces captured in
> background via finch experiments. 
> The tricky bit is that background tracing just happen to use the same backend
> that the crash service uses. but those are *not* crashes.

Changed CRASH_UPLOADER to TRACING_CRASH_UPLOADER to indicate 'Tracing and Crash
Uploader'.
For now it is okay to have crash and tracing under one bucket.

> 
> P.S: DataUseUserData: are you really really sure you can't get a better name
for
> this?
Does DataUseServiceType sounds reasonable instead of DataUseUserData ?
I could revamp that in a separate CL.

[Primiano Tucci (use gerrit), 2016-10-14 17:26:07]

On 2016/10/14 17:23:57, Raj wrote:
> On 2016/10/14 08:53:40, Primiano Tucci wrote:
> > +oysteine
> > Code itself LG, but the name you are using here is extremely misleading.
> > What you are hooking there in components/tracing is NOT the crash uploader
> > service (despite the name of the class). That lives internally.
> > What you are seeing there is the service that uploads traces captured in
> > background via finch experiments. 
> > The tricky bit is that background tracing just happen to use the same
backend
> > that the crash service uses. but those are *not* crashes.
> 
> Changed CRASH_UPLOADER to TRACING_CRASH_UPLOADER to indicate 'Tracing and
Crash
> Uploader'.
> For now it is okay to have crash and tracing under one bucket.

I am missing just one point: where are you counting the bytes uploaded for
crashes?
That crash_service_uploader is NOT used by breakpad/crashpad (our crash uploader
mechanis).It is used only by tracing.
Unless I am missing something (did you hook crash in another CL?) this should be
called just TRACING_UPLOADER

[Steven Holte, 2016-10-14 22:13:37]

histograms lgtm

[oystein (OOO til 10th of July), 2016-10-14 22:15:59]

https://codereview.chromium.org/2375773002/diff/60001/chrome/browser/tracing/...
File chrome/browser/tracing/crash_service_uploader.cc (right):

https://codereview.chromium.org/2375773002/diff/60001/chrome/browser/tracing/...
chrome/browser/tracing/crash_service_uploader.cc:316:
data_use_measurement::DataUseUserData::TRACING_CRASH_UPLOADER);
Yeah as primiano mentioned, I would just call this "TRACE_UPLOADER". The "crash"
part is an implementation detail (my bad on the naming :) ).

[Raj, 2016-10-15 18:46:23]

On 2016/10/14 22:15:59, oystein wrote:
>
https://codereview.chromium.org/2375773002/diff/60001/chrome/browser/tracing/...
> File chrome/browser/tracing/crash_service_uploader.cc (right):
> 
>
https://codereview.chromium.org/2375773002/diff/60001/chrome/browser/tracing/...
> chrome/browser/tracing/crash_service_uploader.cc:316:
> data_use_measurement::DataUseUserData::TRACING_CRASH_UPLOADER);
> Yeah as primiano mentioned, I would just call this "TRACE_UPLOADER". The
"crash"
> part is an implementation detail (my bad on the naming :) ).

Thanks oystein, Primiano
I misunderstood Primiano's initial comment that the code is used by tracing and
crash.
Corrected it. ptal.

[Raj, 2016-10-15 18:46:35]

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

[commit-bot: I haz the power, 2016-10-15 18:46:44]

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-10-15 19:40:14]

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

[commit-bot: I haz the power, 2016-10-15 19:40:15]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-10-15 22:17:36]

LGTM this patch, this patch LGTM ;-) 

PS please update your commit message which still mentions crash. 
also note that the commit message should be formatted as follow :

one line description 
(blank line) 
longer description 

BUG=... 

it makes a  difference when you are sheriffing and looks at batches of hundred
cls, believe me

[Raj, 2016-10-16 19:07:39]

Description was changed from

==========
Add data usage tracking for feedback uploader, crash uploader, dom distiller

BUG=527304
==========

to

==========
Add data usage tracking for feedback uploader, trace uploader, dom distiller

This CL tags the data use from Chrome services feedback uploader, trace uploader
and dom distiller

BUG=527304
==========

[Raj, 2016-10-16 19:08:52]

Description was changed from

==========
Add data usage tracking for feedback uploader, trace uploader, dom distiller

This CL tags the data use from Chrome services feedback uploader, trace uploader
and dom distiller

BUG=527304
==========

to

==========
Add data usage tracking for feedback uploader, trace uploader, dom distiller

This CL tags the data use from Chrome services feedback uploader, trace uploader
and dom distiller

BUG=527304
==========

[Raj, 2016-10-16 19:09:04]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-10-16 19:09:05]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
wychen@chromium.org, zork@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2375773002/#ps80001
(title: "Change to TRACING_UPLOADER")

[commit-bot: I haz the power, 2016-10-16 19:09:10]

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-10-16 20:24:29]

Description was changed from

==========
Add data usage tracking for feedback uploader, trace uploader, dom distiller

This CL tags the data use from Chrome services feedback uploader, trace uploader
and dom distiller

BUG=527304
==========

to

==========
Add data usage tracking for feedback uploader, trace uploader, dom distiller

This CL tags the data use from Chrome services feedback uploader, trace uploader
and dom distiller

BUG=527304
==========

[commit-bot: I haz the power, 2016-10-16 20:24:30]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-10-16 20:27:05]

Description was changed from

==========
Add data usage tracking for feedback uploader, trace uploader, dom distiller

This CL tags the data use from Chrome services feedback uploader, trace uploader
and dom distiller

BUG=527304
==========

to

==========
Add data usage tracking for feedback uploader, trace uploader, dom distiller

This CL tags the data use from Chrome services feedback uploader, trace uploader
and dom distiller

BUG=527304

Committed: https://crrev.com/06be1a78361e2243545ae1cce115f5bbe9739b1d
Cr-Commit-Position: refs/heads/master@{#425594}
==========

[commit-bot: I haz the power, 2016-10-16 20:27:06]

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