Create Ukm ReportingService implementation.

Create UKM ReportingService implementation.

* Moves UKM to having split rotation/upload schedulers
* Adds support for tracking + throttling UKM cellular data use.

BUG=693676
Review-Url: https://codereview.chromium.org/2770853002
Cr-Commit-Position: refs/heads/master@{#459886}
Committed: https://chromium.googlesource.com/chromium/src/+/77d815be2a13c86b5f4bc4152e248c2c491c21d3

[Steven Holte, 2017-03-22 21:23:18]

Description was changed from

==========
Create Ukm ReportingService implementation.

BUG=693676
==========

to

==========
Create UKM ReportingService implementation.

* Moves UKM to having split rotation/upload schedulers
* Adds support for tracking + throttling UKM cellular data use.

BUG=693676
==========

[Steven Holte, 2017-03-22 22:11:57]

holte@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-03-24 16:26:17]

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
File components/metrics/data_use_tracker.cc (right):

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:40: }
Nit: Add a NOTREACHED()?

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:41: return NULL;
nullptr

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:45: const
MetricsLogUploader::MetricServiceType service_type) {
Nit: No need for const on the param since it's an enum.

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:49: }
Nit: Add a NOTREACHED()?

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:50: return NULL;
nullptr

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:94: const
MetricsLogUploader::MetricServiceType service_type) {
Nit: Remove const

https://codereview.chromium.org/2770853002/diff/40001/components/ukm/ukm_repo...
File components/ukm/ukm_reporting_service.cc (right):

https://codereview.chromium.org/2770853002/diff/40001/components/ukm/ukm_repo...
components/ukm/ukm_reporting_service.cc:61: new
ukm::PersistedLogsMetricsImpl()),
base::MakeUnique

https://codereview.chromium.org/2770853002/diff/40001/components/ukm/ukm_repo...
components/ukm/ukm_reporting_service.cc:93:
UMA_HISTOGRAM_SPARSE_SLOWLY("UKM.Upload.ResponseCode", response_code);
I guess this needs rebasing on your other cl.

[Steven Holte, 2017-03-24 18:13:29]

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
File components/metrics/data_use_tracker.cc (right):

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:40: }
On 2017/03/24 16:26:16, Alexei Svitkine (slow) wrote:
> Nit: Add a NOTREACHED()?

It is reached, in most cases.

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:41: return NULL;
On 2017/03/24 16:26:17, Alexei Svitkine (slow) wrote:
> nullptr

Done.

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:45: const
MetricsLogUploader::MetricServiceType service_type) {
On 2017/03/24 16:26:17, Alexei Svitkine (slow) wrote:
> Nit: No need for const on the param since it's an enum.

Done.

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:49: }
On 2017/03/24 16:26:17, Alexei Svitkine (slow) wrote:
> Nit: Add a NOTREACHED()?

It is reached, in most cases.

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:50: return NULL;
On 2017/03/24 16:26:16, Alexei Svitkine (slow) wrote:
> nullptr

Done.

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:94: const
MetricsLogUploader::MetricServiceType service_type) {
On 2017/03/24 16:26:17, Alexei Svitkine (slow) wrote:
> Nit: Remove const

Done.

https://codereview.chromium.org/2770853002/diff/40001/components/ukm/ukm_repo...
File components/ukm/ukm_reporting_service.cc (right):

https://codereview.chromium.org/2770853002/diff/40001/components/ukm/ukm_repo...
components/ukm/ukm_reporting_service.cc:61: new
ukm::PersistedLogsMetricsImpl()),
On 2017/03/24 16:26:17, Alexei Svitkine (slow) wrote:
> base::MakeUnique

Done.

https://codereview.chromium.org/2770853002/diff/40001/components/ukm/ukm_repo...
components/ukm/ukm_reporting_service.cc:93:
UMA_HISTOGRAM_SPARSE_SLOWLY("UKM.Upload.ResponseCode", response_code);
On 2017/03/24 16:26:17, Alexei Svitkine (slow) wrote:
> I guess this needs rebasing on your other cl.

That CL is based on this one.

[Steven Holte, 2017-03-24 18:16:36]

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
File components/metrics/data_use_tracker.cc (right):

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:49: }
On 2017/03/24 18:13:29, Steven Holte wrote:
> On 2017/03/24 16:26:17, Alexei Svitkine (slow) wrote:
> > Nit: Add a NOTREACHED()?
> 
> It is reached, in most cases.

Actually, this one probably won't be reached.

[Steven Holte, 2017-03-24 18:20:13]

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
File components/metrics/data_use_tracker.cc (right):

https://codereview.chromium.org/2770853002/diff/40001/components/metrics/data...
components/metrics/data_use_tracker.cc:49: }
On 2017/03/24 18:16:36, Steven Holte wrote:
> On 2017/03/24 18:13:29, Steven Holte wrote:
> > On 2017/03/24 16:26:17, Alexei Svitkine (slow) wrote:
> > > Nit: Add a NOTREACHED()?
> > 
> > It is reached, in most cases.
> 
> Actually, this one probably won't be reached.

Added a DCHECK for non-null return to ShouldUploadLogOnCellular, which is where
this is called.

[Alexei Svitkine (slow), 2017-03-24 18:27:21]

https://codereview.chromium.org/2770853002/diff/60001/components/metrics/data...
File components/metrics/data_use_tracker.cc (right):

https://codereview.chromium.org/2770853002/diff/60001/components/metrics/data...
components/metrics/data_use_tracker.cc:103: if
(!GetUmaWeeklyQuota(&uma_weekly_quota_bytes))
This function and variable names still say "uma". Please rename them.

Also, this is currently still using the same thresholds as UMA - I assume you're
OK with this for now, but please add TODOs around it, so we revise it. Also,
this is effectively doubling the allowed data use - since both UMA and UKM use
the same threshold but track their data separately. Mention this in a TODO.
....

Separately I also wonder if we should still track them together - e.g. both are
types of metrics and probably it makes sense to have an overall limit for Chrome
metrics data - rather than separate for UMA and UKM. Maybe it would be better to
start off that way until we discuss in a meeting with us and Rob and others? If
so, I suggest reverting the changes to this file for now in this CL other than
making UpdateMetricsUsagePrefs() check for both "UMA" and "UKM" - so their use
is tracked in the same pref for now.

[Steven Holte, 2017-03-24 19:24:25]

On 2017/03/24 18:27:21, Alexei Svitkine (slow) wrote:
>
https://codereview.chromium.org/2770853002/diff/60001/components/metrics/data...
> File components/metrics/data_use_tracker.cc (right):
> 
>
https://codereview.chromium.org/2770853002/diff/60001/components/metrics/data...
> components/metrics/data_use_tracker.cc:103: if
> (!GetUmaWeeklyQuota(&uma_weekly_quota_bytes))
> This function and variable names still say "uma". Please rename them.
> 
> Also, this is currently still using the same thresholds as UMA - I assume
you're
> OK with this for now, but please add TODOs around it, so we revise it. Also,
> this is effectively doubling the allowed data use - since both UMA and UKM use
> the same threshold but track their data separately. Mention this in a TODO.
> ....
> 
> Separately I also wonder if we should still track them together - e.g. both
are
> types of metrics and probably it makes sense to have an overall limit for
Chrome
> metrics data - rather than separate for UMA and UKM. Maybe it would be better
to
> start off that way until we discuss in a meeting with us and Rob and others?
If
> so, I suggest reverting the changes to this file for now in this CL other than
> making UpdateMetricsUsagePrefs() check for both "UMA" and "UKM" - so their use
> is tracked in the same pref for now.

Made the tracker treat UKM + UMA as one thing for now.

[Alexei Svitkine (slow), 2017-03-24 19:35:27]

lgtm

[Steven Holte, 2017-03-24 19:38:56]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2017-03-24 19:39:52]

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-03-24 19:48:07]

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

[commit-bot: I haz the power, 2017-03-24 19:48:08]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[Steven Holte, 2017-03-24 21:37:25]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2017-03-24 21:37:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2770853002/#ps120001
(title: "Also revert datatracker unittest change")

[commit-bot: I haz the power, 2017-03-24 21:38:00]

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-03-24 23:14:22]

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

[commit-bot: I haz the power, 2017-03-24 23:14:24]

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

[Steven Holte, 2017-03-27 20:04:04]

The CQ bit was checked by holte@chromium.org

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

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-03-27 21:27:52]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490645044297220,
"parent_rev": "77d17a744d78d3d35b51fcbe0119bd5dd99bbd46", "commit_rev":
"77d815be2a13c86b5f4bc4152e248c2c491c21d3"}

[commit-bot: I haz the power, 2017-03-27 21:29:59]

Description was changed from

==========
Create UKM ReportingService implementation.

* Moves UKM to having split rotation/upload schedulers
* Adds support for tracking + throttling UKM cellular data use.

BUG=693676
==========

to

==========
Create UKM ReportingService implementation.

* Moves UKM to having split rotation/upload schedulers
* Adds support for tracking + throttling UKM cellular data use.

BUG=693676

Review-Url: https://codereview.chromium.org/2770853002
Cr-Commit-Position: refs/heads/master@{#459886}
Committed:
https://chromium.googlesource.com/chromium/src/+/77d815be2a13c86b5f4bc4152e24...
==========

[commit-bot: I haz the power, 2017-03-27 21:30:00]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/77d815be2a13c86b5f4bc4152e24...