Record precache periodic task interval

Record precache periodic task interval

Precache periodic task is invoked by GCMNetworkManager when the
conditions are right for precaching. The interval between successive
task invocations should be logged.

BUG=618386
Committed: https://crrev.com/1a867255fe4aa1f86136315520a26cfa44f9515e
Cr-Commit-Position: refs/heads/master@{#398743}

[Raj, 2016-06-08 18:30:46]

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

[Raj, 2016-06-08 18:30:46]

ptal. Will add reviewers for histogram.xml, after your review.

[sclittle, 2016-06-08 19:05:16]

lgtm with nit

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:517:
int interval_mins =
nit: minutes seems a little odd to use for a metric - why not use seconds?

[Raj, 2016-06-08 19:42:55]

Description was changed from

==========
Record precache periodic task interval

Precache periodic task is invoked by GCMNetworkManager when the
conditions are right for precaching. The interval between successive
task invocations should be logged.

BUG=618386
==========

to

==========
Record precache periodic task interval

Precache periodic task is invoked by GCMNetworkManager when the
conditions are right for precaching. The interval between successive
task invocations should be logged.

BUG=618386
==========

[Raj, 2016-06-08 19:42:55]

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

[Raj, 2016-06-08 19:43:21]

rajendrant@chromium.org changed reviewers:
- rkaplow@chromium.org

[Raj, 2016-06-08 21:35:24]

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

[Raj, 2016-06-08 21:35:24]

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:517:
int interval_mins =
On 2016/06/08 19:05:16, sclittle wrote:
> nit: minutes seems a little odd to use for a metric - why not use seconds?

For the precache task interval, we do not care about exact seconds, and minutes
is ok. Also the task interval will be ~6hour in general, and it corresponds to
UMA bucket 21156-27864(6-7.7 hours). So we are loosing some precision/accuracy.
Logging in minutes does not buy lot of accuracy either. WDYT.

[sclittle, 2016-06-08 21:36:57]

still lgtm

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:517:
int interval_mins =
On 2016/06/08 21:35:24, Raj wrote:
> On 2016/06/08 19:05:16, sclittle wrote:
> > nit: minutes seems a little odd to use for a metric - why not use seconds?
> 
> For the precache task interval, we do not care about exact seconds, and
minutes
> is ok. Also the task interval will be ~6hour in general, and it corresponds to
> UMA bucket 21156-27864(6-7.7 hours). So we are loosing some
precision/accuracy.
> Logging in minutes does not buy lot of accuracy either. WDYT.

SGTM, whichever you think is best.

[Alexei Svitkine (slow), 2016-06-08 21:39:45]

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:519:
RecordHistogram.recordCountHistogram("Precache.PeriodicTaskInterval",
interval_mins);
Default counts histogram has a max value of 1000000 which is ~700 days. I don't
that makes much sense here. Maybe the equivalent of UMA_HISTOGRAM_COUNTS_10000()
would be better? Max value of 10k is about 7 days...

[Raj, 2016-06-08 21:48:20]

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java
(right):

https://codereview.chromium.org/2047573007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:519:
RecordHistogram.recordCountHistogram("Precache.PeriodicTaskInterval",
interval_mins);
On 2016/06/08 21:39:45, Alexei Svitkine (slow) wrote:
> Default counts histogram has a max value of 1000000 which is ~700 days. I
don't
> that makes much sense here. Maybe the equivalent of
UMA_HISTOGRAM_COUNTS_10000()
> would be better? Max value of 10k is about 7 days...

Done.

[Alexei Svitkine (slow), 2016-06-08 21:49:41]

lgtm % comment

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

https://codereview.chromium.org/2047573007/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:41635: +    The time between successive
precache periodic GCM task invocations.
Nit: Clarify when this is logged.

[Raj, 2016-06-09 00:01:41]

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

https://codereview.chromium.org/2047573007/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:41635: +    The time between successive
precache periodic GCM task invocations.
On 2016/06/08 21:49:41, Alexei Svitkine (slow) wrote:
> Nit: Clarify when this is logged.

Done.

[Raj, 2016-06-09 00:01:46]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-06-09 00:01:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
sclittle@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2047573007/#ps40001
(title: "Addressed nits")

[commit-bot: I haz the power, 2016-06-09 00:02:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047573007/40001

[commit-bot: I haz the power, 2016-06-09 00:09:03]

Description was changed from

==========
Record precache periodic task interval

Precache periodic task is invoked by GCMNetworkManager when the
conditions are right for precaching. The interval between successive
task invocations should be logged.

BUG=618386
==========

to

==========
Record precache periodic task interval

Precache periodic task is invoked by GCMNetworkManager when the
conditions are right for precaching. The interval between successive
task invocations should be logged.

BUG=618386
==========

[commit-bot: I haz the power, 2016-06-09 00:09:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-09 00:12:23]

Description was changed from

==========
Record precache periodic task interval

Precache periodic task is invoked by GCMNetworkManager when the
conditions are right for precaching. The interval between successive
task invocations should be logged.

BUG=618386
==========

to

==========
Record precache periodic task interval

Precache periodic task is invoked by GCMNetworkManager when the
conditions are right for precaching. The interval between successive
task invocations should be logged.

BUG=618386

Committed: https://crrev.com/1a867255fe4aa1f86136315520a26cfa44f9515e
Cr-Commit-Position: refs/heads/master@{#398743}
==========

[commit-bot: I haz the power, 2016-06-09 00:12:24]

Patchset 3 (id:??) landed as
https://crrev.com/1a867255fe4aa1f86136315520a26cfa44f9515e
Cr-Commit-Position: refs/heads/master@{#398743}