more uma stats

Add more UMA stats for AppCacheUpdateJob results and for response retrieval.

BUG=343620
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259792

[michaeln, 2014-03-21 01:05:53]

hi josh, i haven't run/tested this yet but here's what i got for those stats

[michaeln, 2014-03-21 01:21:39]

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_update_job.cc:1352:
LogHistogramStats(CANCELLED_ERROR, GURL());
When the job ends by this code path, if there are associated hosts, they aren't
told it. I should make a distinction between hosts vs nohosts here in the stats
(two different completion codes i guess). The latter is expected if the user
navigates away from a page with a job in progress. The former is what happens in
the forcible delete cases.

KILLED vs ABANDONED (or something)

[jsbell, 2014-03-21 16:41:02]

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_request_handler.cc:323:
host_->associated_cache()->owning_group()->is_being_deleted()) {
Is this a behavior change?

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_update_job.cc:134: job_->MadeProgress();
So here, a redirect - which is an error - calls MakeProgress

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_update_job.cc:146: job_->MadeProgress();
But here and below, MakeProgress is only called if the response was successful.

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_update_job.cc:489: is_valid_response_code =
(response_code / 100 == 2);;
Extra ; - was this going to MakeProgress too?

[michaeln, 2014-03-24 18:12:04]

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_request_handler.cc:323:
host_->associated_cache()->owning_group()->is_being_deleted()) {
On 2014/03/21 16:41:02, jsbell wrote:
> Is this a behavior change?

Yes, when is_being_deleted the request is doomed to fail, so the early bailout
is to avoid those being counted as failures in CountResponseRetrieval.

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_update_job.cc:134: job_->MadeProgress();
On 2014/03/21 16:41:02, jsbell wrote:
> So here, a redirect - which is an error - calls MakeProgress

Yes, MadeProgress() just means it wasn't dead in a hung state, having received a
response of any kind means the network was functioning and the algorithm was
moving forward (albeit not towards a successful completion).

Does that make sense?

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_update_job.cc:146: job_->MadeProgress();
On 2014/03/21 16:41:02, jsbell wrote:
> But here and below, MakeProgress is only called if the response was
successful.

see previous comment

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_update_job.cc:489: is_valid_response_code =
(response_code / 100 == 2);;
On 2014/03/21 16:41:02, jsbell wrote:
> Extra ; - was this going to MakeProgress too?

Done. idk, maybe i had it that way before the fetcher started updating the time
value for all network activity?

[jsbell, 2014-03-24 22:21:23]

lgtm

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/207163002/diff/60001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_update_job.cc:134: job_->MadeProgress();
Yep, thanks.

[jsbell, 2014-03-24 22:52:20]

This should have histograms.xml updates too, now that they're in the same repo.

[michaeln, 2014-03-24 23:05:37]

On 2014/03/24 22:52:20, jsbell wrote:
> This should have histograms.xml updates too, now that they're in the same
repo.

right! thnx

[michaeln, 2014-03-26 20:42:39]

@isherman, ptal at histograms.xml

[Ilya Sherman, 2014-03-26 21:51:30]

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:335: +  <owner>michaeln, jsbell</owner>
Please write this as multiple <owner> lines, and use email addresses for the
owners.

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:338: +    The boolean value indicates
success or failure.
This sentence is redundant with the enum labels.  Perhaps rephrase this as
something closer to "Tracks the success rate of retrieving a main resource from
the appcache"?

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:342: +<histogram
name="appcache.MainResourceResponseRetrieval.Docs"
Please use a <fieldtrial> element at the bottom of this file rather than listing
every histogram twice.

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:444: +    units="BooleanSuccess">
Firstly, BooleanSuccess is an enum, not a unit.  Secondly, BooleanSuccess is not
an appropriate label for this histogram.  Please define a custom label.  Please
double-check the other histograms as well.

https://codereview.chromium.org/207163002/diff/90001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_histograms.cc (right):

https://codereview.chromium.org/207163002/diff/90001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_histograms.cc:63: UMA_HISTOGRAM_BOOLEAN(label,
success);
This won't work.  The label needs to be a runtime constant in order to invoke
the UMA_HISTOGRAM_BOOLEAN macro.

[michaeln, 2014-03-26 22:58:52]

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:342: +<histogram
name="appcache.MainResourceResponseRetrieval.Docs"
On 2014/03/26 21:51:31, Ilya Sherman wrote:
> Please use a <fieldtrial> element at the bottom of this file rather than
listing
> every histogram twice.

We've been listing per-domain histograms like this for a while.

How does the field trial stuff work? There's logic in the browser to log an
event to both labels. Would that logic need to change any? When viewing these
stats on uma.googleplex, do you need to look elsewhere to see the stats.

How is the field trial stuff better?

https://codereview.chromium.org/207163002/diff/90001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_histograms.cc (right):

https://codereview.chromium.org/207163002/diff/90001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_histograms.cc:63: UMA_HISTOGRAM_BOOLEAN(label,
success);
On 2014/03/26 21:51:31, Ilya Sherman wrote:
> This won't work.  The label needs to be a runtime constant in order to invoke
> the UMA_HISTOGRAM_BOOLEAN macro.

oops thank you, and the one below too

[jsbell, 2014-03-26 22:59:51]

https://codereview.chromium.org/207163002/diff/90001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_histograms.cc (right):

https://codereview.chromium.org/207163002/diff/90001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_histograms.cc:88: label =
"appcache.UpdateWasStalledAtPointOfFailure";
Presumably this pattern will fail as well then? Bleah.

[Ilya Sherman, 2014-03-26 23:01:22]

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:342: +<histogram
name="appcache.MainResourceResponseRetrieval.Docs"
On 2014/03/26 22:58:53, michaeln wrote:
> On 2014/03/26 21:51:31, Ilya Sherman wrote:
> > Please use a <fieldtrial> element at the bottom of this file rather than
> listing
> > every histogram twice.
> 
> We've been listing per-domain histograms like this for a while.
> 
> How does the field trial stuff work? There's logic in the browser to log an
> event to both labels. Would that logic need to change any? When viewing these
> stats on uma.googleplex, do you need to look elsewhere to see the stats.
> 
> How is the field trial stuff better?

It's purely a histograms.xml change that reduces repetition.  In this case,
you'd define

<fieldtrial name="appcacheSomethingOrOther" separator="">
  <group name="Docs" label="Only for docs.google.com"/>
  <affected-histogram name="..."/>
  <affected-histogram name="appcache.SubResourceResponseRetrieval"/>
  <affected-histogram name="..."/>
</fieldtrial>

and then not need to include all of the ".Docs" variants for the histograms.

[Ilya Sherman, 2014-03-26 23:02:19]

https://codereview.chromium.org/207163002/diff/90001/webkit/browser/appcache/...
File webkit/browser/appcache/appcache_histograms.cc (right):

https://codereview.chromium.org/207163002/diff/90001/webkit/browser/appcache/...
webkit/browser/appcache/appcache_histograms.cc:88: label =
"appcache.UpdateWasStalledAtPointOfFailure";
On 2014/03/26 22:59:52, jsbell wrote:
> Presumably this pattern will fail as well then? Bleah.

That's fine -- label is a runtime constant here.  That is, at line 89, the label
is always guaranteed to be "appcache.UpdateWasStalledAtPointOfFailure".

[michaeln, 2014-03-26 23:47:20]

ptal

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:335: +  <owner>michaeln, jsbell</owner>
On 2014/03/26 21:51:31, Ilya Sherman wrote:
> Please write this as multiple <owner> lines, and use email addresses for the
> owners.

Done.

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:338: +    The boolean value indicates
success or failure.
On 2014/03/26 21:51:31, Ilya Sherman wrote:
> This sentence is redundant with the enum labels.  Perhaps rephrase this as
> something closer to "Tracks the success rate of retrieving a main resource
from
> the appcache"?

Done.

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:342: +<histogram
name="appcache.MainResourceResponseRetrieval.Docs"
> > How is the field trial stuff better?
> 
> It's purely a histograms.xml change that reduces repetition.  In this case,
> you'd define
> 
> <fieldtrial name="appcacheSomethingOrOther" separator="">
>   <group name="Docs" label="Only for docs.google.com"/>
>   <affected-histogram name="..."/>
>   <affected-histogram name="appcache.SubResourceResponseRetrieval"/>
>   <affected-histogram name="..."/>
> </fieldtrial>
> 
> and then not need to include all of the ".Docs" variants for the histograms.

sgtm!

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:342: +<histogram
name="appcache.MainResourceResponseRetrieval.Docs"
On 2014/03/26 21:51:31, Ilya Sherman wrote:
> Please use a <fieldtrial> element at the bottom of this file rather than
listing
> every histogram twice.

Done.

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:444: +    units="BooleanSuccess">
On 2014/03/26 21:51:31, Ilya Sherman wrote:
> Firstly, BooleanSuccess is an enum, not a unit.  Secondly, BooleanSuccess is
not
> an appropriate label for this histogram.  Please define a custom label. 
Please
> double-check the other histograms as well.

Is there a semantically neutral bool enum defined? (poor bool, so burdened with
success). I'd really rather not add multiple new enum types and labels to this
monster file that are all variants of <TRUE, FALSE>.

(also, there's actually a good number of units="BooleanSuccess" misusages in
this file?)

[Ilya Sherman, 2014-03-27 00:02:45]

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:444: +    units="BooleanSuccess">
On 2014/03/26 23:47:21, michaeln wrote:
> On 2014/03/26 21:51:31, Ilya Sherman wrote:
> > Firstly, BooleanSuccess is an enum, not a unit.  Secondly, BooleanSuccess is
> not
> > an appropriate label for this histogram.  Please define a custom label. 
> Please
> > double-check the other histograms as well.
> 
> Is there a semantically neutral bool enum defined? (poor bool, so burdened
with
> success). I'd really rather not add multiple new enum types and labels to this
> monster file that are all variants of <TRUE, FALSE>.
> 
> (also, there's actually a good number of units="BooleanSuccess" misusages in
> this file?)

I think you're looking for the Boolean enum?

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

https://codereview.chromium.org/207163002/diff/110001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:338: +    The boolean value indicates
success or failure.
Please omit this sentence everywhere you use the BooleanSuccess enum, and
instead rephrase similarly to how you did for SubResourceResponseRetrieval.

[michaeln, 2014-03-27 00:44:06]

done and done

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/207163002/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:444: +    units="BooleanSuccess">
> I think you're looking for the Boolean enum?

indeed i am

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

https://codereview.chromium.org/207163002/diff/110001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:338: +    The boolean value indicates
success or failure.
On 2014/03/27 00:02:45, Ilya Sherman wrote:
> Please omit this sentence everywhere you use the BooleanSuccess enum, and
> instead rephrase similarly to how you did for SubResourceResponseRetrieval.

sry i missed this one and thnx for spotting it

[Ilya Sherman, 2014-03-27 00:45:38]

histograms LGTM, thanks

[michaeln, 2014-03-27 01:09:15]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-03-27 01:10:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/207163002/130001

[michaeln, 2014-03-27 01:10:40]

The CQ bit was unchecked by michaeln@chromium.org

[michaeln, 2014-03-27 01:15:33]

thnx, #9 has one addition that i forgot about, a new value for the JobResultType
enum

   <int value="7" label="Server Error"/>
+  <int value="8" label="Cancelled"/>
 </enum>

[michaeln, 2014-03-27 01:15:49]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-03-27 01:17:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/207163002/150001

[commit-bot: I haz the power, 2014-03-27 05:27:56]

Change committed as 259792