[Android] Adding UMA for events in Background Task Scheduler

[Android] Adding UMA for events in Background Task Scheduler

Adds UMA for the following:
* Scheduling
* Canceling
* Starting a task
* Stopping a task
* Introduces the class to report uma: BackgroundTaskSchedulerUma

BUG=710630
Review-Url: https://codereview.chromium.org/2911503002
Cr-Commit-Position: refs/heads/master@{#476474}
Committed: https://chromium.googlesource.com/chromium/src/+/3f7a5c750ed5f1373506b11003f1834bea0a1d99

[fgorski, 2017-05-25 21:14:23]

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

[fgorski, 2017-05-25 21:15:27]

fgorski@chromium.org changed reviewers:
+ nyquist@chromium.org

[fgorski, 2017-05-25 21:15:27]

PTAL

[commit-bot: I haz the power, 2017-05-25 21:15: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, 2017-05-25 23:21:15]

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

[commit-bot: I haz the power, 2017-05-25 23:21:17]

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-05-26 00:53:43]

https://codereview.chromium.org/2911503002/diff/1/components/background_task_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java
(right):

https://codereview.chromium.org/2911503002/diff/1/components/background_task_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:22:
private static BackgroundTaskSchedulerUma sInstance;
Optional nit for reduction of static members:

    private static final class LazyHolder {
        private static final BackgroundTaskSchedulerUma INSTANCE = new
BackgroundTaskSchedulerUma();
    }

    public static BackgroundTaskSchedulerUma getInstance() {
      return LazyHolder.INSTANCE;
    }

https://codereview.chromium.org/2911503002/diff/1/components/background_task_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:36:
public int toUmaEnumValueFromTaskId(int taskId) {
Should we refer to this somewhere in the documentation or in the TaskIds class
so people remember to update?

https://codereview.chromium.org/2911503002/diff/1/components/background_task_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:63:
RecordHistogram.recordEnumeratedHistogram(
Does this work even if native has not loaded yet?

[fgorski, 2017-05-30 21:18:47]

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

[commit-bot: I haz the power, 2017-05-30 21:19: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, 2017-05-30 23:48:57]

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

[commit-bot: I haz the power, 2017-05-30 23:48:58]

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

[fgorski, 2017-05-31 06:20:20]

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

[commit-bot: I haz the power, 2017-05-31 06:20:29]

Dry run: CQ is trying da patch.

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

[fgorski, 2017-05-31 06:20:40]

PTAL

https://codereview.chromium.org/2911503002/diff/1/components/background_task_...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java
(right):

https://codereview.chromium.org/2911503002/diff/1/components/background_task_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:22:
private static BackgroundTaskSchedulerUma sInstance;
On 2017/05/26 00:53:43, nyquist (nychthemeron ping) wrote:
> Optional nit for reduction of static members:
> 
>     private static final class LazyHolder {
>         private static final BackgroundTaskSchedulerUma INSTANCE = new
> BackgroundTaskSchedulerUma();
>     }
> 
>     public static BackgroundTaskSchedulerUma getInstance() {
>       return LazyHolder.INSTANCE;
>     }

will it still make sense if the INSTANCE is not final, so that I can inject it
for testing?

https://codereview.chromium.org/2911503002/diff/1/components/background_task_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:36:
public int toUmaEnumValueFromTaskId(int taskId) {
On 2017/05/26 00:53:43, nyquist (nychthemeron ping) wrote:
> Should we refer to this somewhere in the documentation or in the TaskIds class
> so people remember to update?

Done.

https://codereview.chromium.org/2911503002/diff/1/components/background_task_...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:63:
RecordHistogram.recordEnumeratedHistogram(
On 2017/05/26 00:53:43, nyquist (nychthemeron ping) wrote:
> Does this work even if native has not loaded yet?

Updated to cache on shared preferences.

[commit-bot: I haz the power, 2017-05-31 07:19:23]

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

[commit-bot: I haz the power, 2017-05-31 07:19:23]

Dry run: This issue passed the CQ dry run.

[nyquist, 2017-06-01 00:55:38]

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java
(right):

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:39:
/** Parses a cached UMA entry from a string. */
Nit: I guess this warrants a @return or something explaining that it returns
null on parse errors?

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:141:
public void flushStats() {
Is it possible to assert that native has loaded? Maybe not here?

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:142:
Set<String> cachedUmaStrings =
getCachedUmaEntries(ContextUtils.getAppSharedPreferences());
Should we assert UI thread maybe, just so we don't mess up later?

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:159:
ContextUtils.getAppSharedPreferences().edit().remove(KEY_CACHED_UMA).apply();
assert UI thread for thread safety?

[fgorski, 2017-06-01 04:47:18]

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

[commit-bot: I haz the power, 2017-06-01 04:47:31]

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-06-01 06:07:46]

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

[commit-bot: I haz the power, 2017-06-01 06:07:46]

Dry run: This issue passed the CQ dry run.

[fgorski, 2017-06-01 16:25:41]

PTAL. All addressed.
I do have a question about DeferredStartupHandler thou, in context.

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
File
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java
(right):

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:39:
/** Parses a cached UMA entry from a string. */
On 2017/06/01 00:55:38, nyquist (nychthemeron ping) wrote:
> Nit: I guess this warrants a @return or something explaining that it returns
> null on parse errors?

Done.

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:141:
public void flushStats() {
On 2017/06/01 00:55:38, nyquist (nychthemeron ping) wrote:
> Is it possible to assert that native has loaded? Maybe not here?

Done. I think we can add an assert. This is only invoked from checkOSUpgrade,
called from DeferredStartupHandler, so it should be done after startup.

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:142:
Set<String> cachedUmaStrings =
getCachedUmaEntries(ContextUtils.getAppSharedPreferences());
On 2017/06/01 00:55:38, nyquist (nychthemeron ping) wrote:
> Should we assert UI thread maybe, just so we don't mess up later?

Done. Do you know if DeferredStartupHandler invokes the deferred tasks on UI
Thread?
If not I am happily wrapping this with runnable and posting to UI thread.

https://codereview.chromium.org/2911503002/diff/40001/components/background_t...
components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java:159:
ContextUtils.getAppSharedPreferences().edit().remove(KEY_CACHED_UMA).apply();
On 2017/06/01 00:55:38, nyquist (nychthemeron ping) wrote:
> assert UI thread for thread safety?

Done. I've added a few more. I'll test it tomorrow.

[fgorski, 2017-06-01 16:27:16]

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

[fgorski, 2017-06-01 16:27:17]

Alexei, PTAL at histograms.

[Alexei Svitkine (slow), 2017-06-01 17:03:34]

lgtm % comment

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

https://codereview.chromium.org/2911503002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4701: +<histogram
name="BackgroundTaskScheduler.TaskCanceled" enum="BackgroundTaskId">
Nit: Suggest prefixing these with "Android." since the BackgroundTaskScheduler
is an Android thing.

So Android.BackgroundTaskSchedular.TaskCanceled

[fgorski, 2017-06-01 21:20:23]

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

[commit-bot: I haz the power, 2017-06-01 21:20:51]

Dry run: CQ is trying da patch.

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

[fgorski, 2017-06-01 21:21:33]

Tommy, PTAL.

[nyquist, 2017-06-01 22:43:27]

lgtm

[fgorski, 2017-06-01 22:58:29]

The CQ bit was unchecked by fgorski@chromium.org

[fgorski, 2017-06-01 22:58:32]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2017-06-01 22:58:32]

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/2911503002/#ps80001
(title: "Addressing CR feedback: Renaming metrics")

[fgorski, 2017-06-01 22:58:40]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2017-06-01 22:59: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-06-01 23:13:24]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496357920690290,
"parent_rev": "adb3fe930f151b63798729d3e43f37f6097a7875", "commit_rev":
"3f7a5c750ed5f1373506b11003f1834bea0a1d99"}

[commit-bot: I haz the power, 2017-06-01 23:13:32]

Description was changed from

==========
[Android] Adding UMA for events in Background Task Scheduler

Adds UMA for the following:
* Scheduling
* Canceling
* Starting a task
* Stopping a task
* Introduces the class to report uma: BackgroundTaskSchedulerUma

BUG=710630
==========

to

==========
[Android] Adding UMA for events in Background Task Scheduler

Adds UMA for the following:
* Scheduling
* Canceling
* Starting a task
* Stopping a task
* Introduces the class to report uma: BackgroundTaskSchedulerUma

BUG=710630

Review-Url: https://codereview.chromium.org/2911503002
Cr-Commit-Position: refs/heads/master@{#476474}
Committed:
https://chromium.googlesource.com/chromium/src/+/3f7a5c750ed5f1373506b11003f1...
==========

[commit-bot: I haz the power, 2017-06-01 23:13:33]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3f7a5c750ed5f1373506b11003f1...