Add UMA to record the ratio of how many fetch events are not handled

Add UMA to record the ratio of how many fetch events are not handled

BUG=483354
TEST=n/a

Committed: https://crrev.com/384c156655a355bec5d1dd8e80165bc874051202
Cr-Commit-Position: refs/heads/master@{#330140}

[kinuko, 2015-05-14 17:20:09]

kinuko@chromium.org changed reviewers:
+ asvitkine@chromium.org, nhiroki@chromium.org

[kinuko, 2015-05-14 17:22:42]

asvitkine@: could you take a look at histogram changes?

nhiroki@: could you take a look?

[Alexei Svitkine (slow), 2015-05-14 17:49:59]

https://codereview.chromium.org/1133853007/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/1133853007/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_metrics.cc:113: 100.0 /
static_cast<double>(fired_events);
Nit: I don't think you actually need the double cast, you can just multiply by
100 (i..e. int) and divide by fired_events. The resulting value is logged as an
int anyway, and you're not doing any rounding anyway.

Though, you should probably guard against fired_events being zero, or you can
get a div by zero crash.

https://codereview.chromium.org/1133853007/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/1133853007/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_metrics.h:70: static void
RecordEventStatus(unsigned int fired_events,
Nit: Chromium style doesn't usually use unsigned ints except when interfacing
with external APIs that require it. int is probably fine for this.

[kinuko, 2015-05-14 23:47:19]

Patchset #2 (id:20001) has been deleted

[kinuko, 2015-05-15 00:05:37]

Thanks, updated.

https://codereview.chromium.org/1133853007/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/1133853007/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_metrics.cc:113: 100.0 /
static_cast<double>(fired_events);
On 2015/05/14 17:49:59, Alexei Svitkine wrote:
> Nit: I don't think you actually need the double cast, you can just multiply by
> 100 (i..e. int) and divide by fired_events. The resulting value is logged as
an
> int anyway, and you're not doing any rounding anyway.

Done.

> Though, you should probably guard against fired_events being zero, or you can
> get a div by zero crash.

Aha, that's right...  done.

https://codereview.chromium.org/1133853007/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/1133853007/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_metrics.h:70: static void
RecordEventStatus(unsigned int fired_events,
On 2015/05/14 17:49:59, Alexei Svitkine wrote:
> Nit: Chromium style doesn't usually use unsigned ints except when interfacing
> with external APIs that require it. int is probably fine for this.

Chromium guide says following sentence, is that what you mean?

'Do not use unsigned types to mean "this value should never be < 0".'

This is for counters, I think size_t is probably better? (Though int should be
also fine too)

[nhiroki, 2015-05-15 02:41:26]

https://codereview.chromium.org/1133853007/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/1133853007/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:113: if (handled_events
> 0u)
This should be |fired_events| to avoid divining by 0.

https://codereview.chromium.org/1133853007/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:116: unhandled_ratio);
Do we need to record in the case where |fired_events| is 0? This could record
noise because a version failed to start can go through here.

[kinuko, 2015-05-15 04:42:24]

https://codereview.chromium.org/1133853007/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/1133853007/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:113: if (handled_events
> 0u)
On 2015/05/15 02:41:25, nhiroki wrote:
> This should be |fired_events| to avoid divining by 0.

Done. I'm extremely careless these days.

https://codereview.chromium.org/1133853007/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:116: unhandled_ratio);
On 2015/05/15 02:41:25, nhiroki wrote:
> Do we need to record in the case where |fired_events| is 0? This could record
> noise because a version failed to start can go through here.

Done.

[nhiroki, 2015-05-15 05:09:52]

lgtm

[Alexei Svitkine (slow), 2015-05-15 14:35:58]

lgtm % nits

https://codereview.chromium.org/1133853007/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/1133853007/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:112: int
unhandled_ratio = 0;
Nit: No need to declare this at the top of the function - you can merge this
with line 115.

https://codereview.chromium.org/1133853007/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:115: unhandled_ratio =
(fired_events - handled_events) * 100.0 / fired_events;
Nit: No need for 100.0 - can just do integer multiplication - i.e. * 100.

[kinuko, 2015-05-15 15:22:31]

Thanks!

https://codereview.chromium.org/1133853007/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/1133853007/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:112: int
unhandled_ratio = 0;
On 2015/05/15 14:35:57, Alexei Svitkine wrote:
> Nit: No need to declare this at the top of the function - you can merge this
> with line 115.

Yeah, done.

https://codereview.chromium.org/1133853007/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:115: unhandled_ratio =
(fired_events - handled_events) * 100.0 / fired_events;
On 2015/05/15 14:35:57, Alexei Svitkine wrote:
> Nit: No need for 100.0 - can just do integer multiplication - i.e. * 100.

Done.

[kinuko, 2015-05-15 15:22:42]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2015-05-15 15:22:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1133853007/#ps100001
(title: "addressed comment")

[commit-bot: I haz the power, 2015-05-15 15:23:10]

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

[commit-bot: I haz the power, 2015-05-15 18:32:22]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-05-15 18:33:25]

Patchset 5 (id:??) landed as
https://crrev.com/384c156655a355bec5d1dd8e80165bc874051202
Cr-Commit-Position: refs/heads/master@{#330140}