[Offline Pages] Adds UMA for number of started attempts and for network connection when Unsupported…

[Offline Pages] Adds UMA for number of started attempts and for network connection when Unsupported Scheme error encountered.

These changes are a follow-up to bug 642025 and the patch landed to mitigate it. They are meant to give us more visibility for tuning the policy for max number of started attempts for a request and whether we might use the network connection to better understand the nature of the Unsupported Scheme error.

Also this reduces the max number of start attempts from 5 to 4. I tried 2 EM URLs that took 3 attempts on GIN-2g on GIN-2g-poor and one took only 2 attempts while the other took 4 attempts so I don't want to lower this too much yet (and see what new UMA reveals).

BUG=642923
Committed: https://crrev.com/22647f96af79fb0d563644640689a8fc678f15ac
Cr-Commit-Position: refs/heads/master@{#416101}

[dougarnett, 2016-08-31 22:16:21]

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

[commit-bot: I haz the power, 2016-08-31 22:17: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-08-31 22:21:11]

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

[commit-bot: I haz the power, 2016-08-31 22:22:02]

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

[dougarnett, 2016-08-31 22:24:13]

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

[commit-bot: I haz the power, 2016-08-31 23:12:57]

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

[commit-bot: I haz the power, 2016-08-31 23:12:58]

Dry run: Exceeded global retry quota

[dougarnett, 2016-08-31 23:44:09]

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

[Pete Williamson, 2016-09-01 00:27:20]

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

https://codereview.chromium.org/2301703002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:50: void
RecordAttemptCount(const SavePageRequest& request,
Maybe this should be RecordStartedAttemptCount(), and we should have a
corresponding RecordCompletedAttemptCount().

https://codereview.chromium.org/2301703002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:171: void
RequestCoordinator::AbortRequestAttempt(SavePageRequest* request) {
This splits the responsibility for garbage collecting obsolete things from the
queue between RC and RequestPicker classes.  I wonder if it would be better to
do it all in the RequestPicker.  What do you think?

[dougarnett, 2016-09-01 19:27:37]

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

https://codereview.chromium.org/2301703002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:50: void
RecordAttemptCount(const SavePageRequest& request,
On 2016/09/01 00:27:20, Pete Williamson wrote:
> Maybe this should be RecordStartedAttemptCount(), and we should have a
> corresponding RecordCompletedAttemptCount().

Actually, my thought was to just add the completed attempt UMA in this same
method. Adding TODO to capture that intent.

https://codereview.chromium.org/2301703002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:171: void
RequestCoordinator::AbortRequestAttempt(SavePageRequest* request) {
On 2016/09/01 00:27:20, Pete Williamson wrote:
> This splits the responsibility for garbage collecting obsolete things from the
> queue between RC and RequestPicker classes.  I wonder if it would be better to
> do it all in the RequestPicker.  What do you think?

We are deleting the DoneCallback requests in other cases here so there is some
argument that it should be consistently done here as we know about it. Also, we
don't call the Picker for the abort path now, so they would linger in the queue
until next schedule window (and could cause confusion wrt queue size or showing
up in internals page).

I think the Picker garbage collection is a secondary yet important safeguard
against crash loops (as well as needed for expired requests). We might want
different UMA (and possibly different notification status) for the garbage
collected cases on starts to yield insights on potential crashes or being killed
by Android.

[dougarnett, 2016-09-01 19:27:41]

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

[commit-bot: I haz the power, 2016-09-01 19:28:07]

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

[dougarnett, 2016-09-01 19:28:53]

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

[dougarnett, 2016-09-01 19:28:53]

Steven, can you perform histogram review on this CL?

[Pete Williamson, 2016-09-01 20:43:33]

lgtm

[commit-bot: I haz the power, 2016-09-01 20:47:43]

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

[commit-bot: I haz the power, 2016-09-01 20:47:44]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2016-09-01 20:50:43]

histograms lgtm, but see note

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

https://codereview.chromium.org/2301703002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:54:
UMA_HISTOGRAM_CUSTOM_COUNTS(
If you want size one buckets, you'll need to use 11 buckets (otherwise you'll
have an 8-10 bucket).  Or just use UMA_HISTOGRAM_ENUMERATION with a max of 10.

[dougarnett, 2016-09-01 20:59:56]

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

https://codereview.chromium.org/2301703002/diff/40001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:54:
UMA_HISTOGRAM_CUSTOM_COUNTS(
On 2016/09/01 20:50:42, Steven Holte wrote:
> If you want size one buckets, you'll need to use 11 buckets (otherwise you'll
> have an 8-10 bucket).  Or just use UMA_HISTOGRAM_ENUMERATION with a max of 10.

Done. Thx

[dougarnett, 2016-09-01 21:45:01]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-09-01 21:45:02]

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/2301703002/#ps60001
(title: "Adjusted count histogram max arg")

[commit-bot: I haz the power, 2016-09-01 21:46:06]

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-09-01 22:55:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-01 22:58:50]

Description was changed from

==========
[Offline Pages] Adds UMA for number of started attempts and for network
connection when Unsupported Scheme error encountered.

These changes are a follow-up to bug 642025 and the patch landed to mitigate it.
They are meant to give us more visibility for tuning the policy for max number
of started attempts for a request and whether we might use the network
connection to better understand the nature of the Unsupported Scheme error.

Also this reduces the max number of start attempts from 5 to 4. I tried 2 EM
URLs that took 3 attempts on GIN-2g on GIN-2g-poor and one took only 2 attempts
while the other took 4 attempts so I don't want to lower this too much yet (and
see what new UMA reveals).

BUG=642923
==========

to

==========
[Offline Pages] Adds UMA for number of started attempts and for network
connection when Unsupported Scheme error encountered.

These changes are a follow-up to bug 642025 and the patch landed to mitigate it.
They are meant to give us more visibility for tuning the policy for max number
of started attempts for a request and whether we might use the network
connection to better understand the nature of the Unsupported Scheme error.

Also this reduces the max number of start attempts from 5 to 4. I tried 2 EM
URLs that took 3 attempts on GIN-2g on GIN-2g-poor and one took only 2 attempts
while the other took 4 attempts so I don't want to lower this too much yet (and
see what new UMA reveals).

BUG=642923

Committed: https://crrev.com/22647f96af79fb0d563644640689a8fc678f15ac
Cr-Commit-Position: refs/heads/master@{#416101}
==========

[commit-bot: I haz the power, 2016-09-01 22:58:51]

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