[OfflinePages] Adds TimeToStart UMA for background load requests.

[OfflinePages] Adds TimeToStart UMA for background load requests.

BUG=656099
Committed: https://crrev.com/d3b9a8de5aca133c4b7cbdf2edf254cd3f8702a5
Cr-Commit-Position: refs/heads/master@{#427186}

[dougarnett, 2016-10-20 16:51:50]

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

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

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

[dougarnett, 2016-10-20 16:57:50]

dougarnett@chromium.org changed reviewers:
+ dimich@chromium.org, petewil@chromium.org

[dougarnett, 2016-10-20 16:57:51]

Redo on the TimeToStart UMA with milliseconds instead of seconds and so using
time delta macro.

[commit-bot: I haz the power, 2016-10-20 18:19:49]

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

[commit-bot: I haz the power, 2016-10-20 18:19:50]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-10-20 18:43:20]

lgtm

[dougarnett, 2016-10-20 19:09:14]

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

[Steven Holte, 2016-10-20 20:44:28]

lgtm

[Steven Holte, 2016-10-20 20:46:31]

https://chromiumcodereview.appspot.com/2440733002/diff/1/components/offline_p...
File components/offline_pages/background/request_coordinator.cc (right):

https://chromiumcodereview.appspot.com/2440733002/diff/1/components/offline_p...
components/offline_pages/background/request_coordinator.cc:87:
base::TimeDelta::FromSeconds(kMaxDurationSeconds), kDurationBuckets);
You don't want this bound to change even if you change this constant for some
other reason (it will make buckets incompatible between new and old versions of
chrome), so it would be better to just inline the value here.

[Steven Holte, 2016-10-20 20:51:11]

https://chromiumcodereview.appspot.com/2440733002/diff/1/components/offline_p...
File components/offline_pages/background/request_coordinator.cc (right):

https://chromiumcodereview.appspot.com/2440733002/diff/1/components/offline_p...
components/offline_pages/background/request_coordinator.cc:84:
UMA_HISTOGRAM_CUSTOM_TIMES(
Also you shouldn't use this macro with a computed histogram name, since it uses
a static pointer to the histogram.  Instead, use
base::Histogram::FactoryTimeGet() (or split this into seperate macro invokations
for each histogram name)

[dougarnett, 2016-10-21 16:09:47]

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

[commit-bot: I haz the power, 2016-10-21 16:10:03]

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

[dougarnett, 2016-10-21 16:12:12]

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

[commit-bot: I haz the power, 2016-10-21 16:12:16]

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

[dougarnett, 2016-10-21 16:13:48]

https://codereview.chromium.org/2440733002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2440733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:84:
UMA_HISTOGRAM_CUSTOM_TIMES(
On 2016/10/20 20:51:11, Steven Holte wrote:
> Also you shouldn't use this macro with a computed histogram name, since it
uses
> a static pointer to the histogram.  Instead, use
> base::Histogram::FactoryTimeGet() (or split this into seperate macro
invokations
> for each histogram name)

Done. Thanks

https://codereview.chromium.org/2440733002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:87:
base::TimeDelta::FromSeconds(kMaxDurationSeconds), kDurationBuckets);
On 2016/10/20 20:46:31, Steven Holte wrote:
> You don't want this bound to change even if you change this constant for some
> other reason (it will make buckets incompatible between new and old versions
of
> chrome), so it would be better to just inline the value here.

Done.

[commit-bot: I haz the power, 2016-10-21 17:33:00]

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

[commit-bot: I haz the power, 2016-10-21 17:33:00]

Dry run: This issue passed the CQ dry run.

[Dmitry Titov, 2016-10-21 18:30:25]

https://codereview.chromium.org/2440733002/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2440733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:88:
base::TimeDelta::FromMilliseconds(100), base::TimeDelta::FromDays(7), 50,
This will have second bucket that would take everything from 100ms to >3hrs. And
50 is a good number of buckets already. Would log scale help here? I think we
want more resolution in the first couple hours and then less resolution...

[dougarnett, 2016-10-21 23:16:38]

https://codereview.chromium.org/2440733002/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2440733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:88:
base::TimeDelta::FromMilliseconds(100), base::TimeDelta::FromDays(7), 50,
On 2016/10/21 18:30:25, Dmitry Titov wrote:
> This will have second bucket that would take everything from 100ms to >3hrs.
And
> 50 is a good number of buckets already. Would log scale help here? I think we
> want more resolution in the first couple hours and then less resolution...

I thought this one would give exponential buckets vs. using
LinearHistogram::FactoryTimeGet. I will double check the reference I used.

[dougarnett, 2016-10-21 23:28:52]

https://codereview.chromium.org/2440733002/diff/40001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2440733002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:88:
base::TimeDelta::FromMilliseconds(100), base::TimeDelta::FromDays(7), 50,
On 2016/10/21 23:16:38, dougarnett wrote:
> On 2016/10/21 18:30:25, Dmitry Titov wrote:
> > This will have second bucket that would take everything from 100ms to >3hrs.
> And
> > 50 is a good number of buckets already. Would log scale help here? I think
we
> > want more resolution in the first couple hours and then less resolution...
> 
> I thought this one would give exponential buckets vs. using
> LinearHistogram::FactoryTimeGet. I will double check the reference I used.

Code doc here:
https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?rcl=0&l=136

OfflinePages.SavePageTime uses this and is not linear. I can certainly drop the
max though.

[Dmitry Titov, 2016-10-22 00:05:11]

Thanks for explanation!

LGTM

[dougarnett, 2016-10-24 14:50:03]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-10-24 14:50:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2440733002/#ps40001
(title: "Inlined histogram constants")

[commit-bot: I haz the power, 2016-10-24 14:50:22]

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-24 17:16:15]

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

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

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[dougarnett, 2016-10-24 21:32:39]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2016-10-24 21:33:39]

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-25 00:28:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-25 00:30:33]

Description was changed from

==========
[OfflinePages] Adds TimeToStart UMA for background load requests.


BUG=656099
==========

to

==========
[OfflinePages] Adds TimeToStart UMA for background load requests.

BUG=656099

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

[commit-bot: I haz the power, 2016-10-25 00:30:34]

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