Update ServiceWorker event handled status histogram

Update ServiceWorker event handled status histogram

Currently ServiceWorker.UnhandledEventRatio is a bit noisy and not
very informative. This CL changes the UMA as following:

- Simplify per-worker event handled status. Probably we don't need to
  record the handled ratio in percentage, but just record status in
  some buckets (e.g. 'all events' or 'no events' are handled etc)
  would be enough.

- Reset the metrics when the SW's stopped, so that the metrics won't be
  skewed by the lifetime of versions.

- Exclude ServiceWorker stats for NTP like scope, as it tends to dominate
  the stats and make the resutls largely skewed.

BUG=483354
TEST=(manually tested locally with chrome://histograms)

Committed: https://crrev.com/7a488959b8dc39bc610ce0dbd736a20de96786e3
Cr-Commit-Position: refs/heads/master@{#337576}

[kinuko, 2015-06-11 08:46:04]

kinuko@chromium.org changed reviewers:
+ falken@chromium.org

[kinuko, 2015-06-11 08:47:44]

I suspected the way I record the event handled status might be wrong, but it
doesn't seem to be the case in my local testing.  This CL tries to simplify the
UMA a bit, could you take a look?  (Not high priority)

[falken, 2015-06-12 05:50:29]

Would it make sense to have separate histograms for each event type, like
FetchEventHandledStatus vs PushEventHandledStatus?

Right now I think we only care about Fetch (since I assume anyone that
registered for Push would handle the Push events). But I'm wondering if this
could change later, and we'll want to distinguish the event types.

https://codereview.chromium.org/1178043002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1178043002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:37227: -<histogram
name="ServiceWorker.UnhandledEventRatio" units="percent">
I don't think we're allowed to rm histograms, they are just marked obsolete. But
histogram OWNER would know for sure.

[kinuko, 2015-06-25 08:02:22]

(Reviving this old patch) Updated. PTAL

https://codereview.chromium.org/1178043002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1178043002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:37227: -<histogram
name="ServiceWorker.UnhandledEventRatio" units="percent">
On 2015/06/12 05:50:29, falken wrote:
> I don't think we're allowed to rm histograms, they are just marked obsolete.
But
> histogram OWNER would know for sure.

Oops, oh yeah. :(

[falken, 2015-06-25 09:31:39]

This looks good but... I'm wondering what's causing the difference between the
current histogram and the SWURLRequestJob histograms. The SWURLRequestJobs are
showing FALLBACK_RESPONSE occurs 20% for subresources and 1% for main resource,
but the UnhandledEventRatio is showing 99% handled. I'd expect these to be
correlated. I wonder if there's a bug somewhere with how we're counting.

https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=06-24-2015...

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

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:33: false /* case
sensitive */);
StartsWithASCII with the bool param is now deprecated:
https://codereview.chromium.org/1182453004/

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:39:
EVENT_HANDLED_STATUS_MAX,
nit: I slightly prefer NUM_TYPES over MAX. Some other enums use LAST, and then
pass "LAST + 1" to the histogram macro whereas we want to pass "MAX"
(CACHE_STORAGE_ERROR_LAST is an example). MAX and LAST seem easier to confuse
than NUM_TYPES.

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_metrics.h:71: EVENT_TYPE_MAX,
the histogram people encouraged us to use NUM_EVENT_TYPES instead of MAX

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:401: Metrics(const
GURL& scope) : scope_(scope) {}
nit: explicit

[kinuko, 2015-06-29 05:17:37]

On 2015/06/25 09:31:39, falken wrote:
> This looks good but... I'm wondering what's causing the difference between the
> current histogram and the SWURLRequestJob histograms. The SWURLRequestJobs are
> showing FALLBACK_RESPONSE occurs 20% for subresources and 1% for main
resource,
> but the UnhandledEventRatio is showing 99% handled. I'd expect these to be
> correlated. I wonder if there's a bug somewhere with how we're counting.
> 
>
https://uma.googleplex.com/p/chrome/histograms/?dayCount=1&endDate=06-24-2015...

This is counted per-version, if there's a single version that keeps falling back
the number could be a bit skewed.

Also per-user counter gives slightly different view:
https://uma.googleplex.com/p/chrome/histograms/?dayCount=7&endDate=06-27-2015...

- 2~3% of users seeing FALLBACK_RESPONSE (that add up 20% of loading)
- 12% of users seeing ServiceWorker that falls back for all or some fetch events


Anyways I started to feel that the per-version percentage may not be that useful
(after all the versions we want to optimize might be only a few, which could
affect large or not depending on which site it's coming from).

[kinuko, 2015-07-02 06:45:04]

Patchset #3 (id:40001) has been deleted

[kinuko, 2015-07-02 07:16:24]

Patchset #3 (id:60001) has been deleted

[kinuko, 2015-07-02 14:23:57]

Patchset #3 (id:50005) has been deleted

[kinuko, 2015-07-02 14:27:26]

Updated. After having some discussion not entirely sure if we want this UMA, if
not I could just drop the changes and obsolete the histogram.

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

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:33: false /* case
sensitive */);
On 2015/06/25 09:31:39, falken wrote:
> StartsWithASCII with the bool param is now deprecated:
> https://codereview.chromium.org/1182453004/

Done.

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:39:
EVENT_HANDLED_STATUS_MAX,
On 2015/06/25 09:31:39, falken wrote:
> nit: I slightly prefer NUM_TYPES over MAX. Some other enums use LAST, and then
> pass "LAST + 1" to the histogram macro whereas we want to pass "MAX"
> (CACHE_STORAGE_ERROR_LAST is an example). MAX and LAST seem easier to confuse
> than NUM_TYPES.
> 

Done.

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_metrics.h:71: EVENT_TYPE_MAX,
On 2015/06/25 09:31:39, falken wrote:
> the histogram people encouraged us to use NUM_EVENT_TYPES instead of MAX

Done.

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1178043002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:401: Metrics(const
GURL& scope) : scope_(scope) {}
On 2015/06/25 09:31:39, falken wrote:
> nit: explicit

Acknowledged.

[falken, 2015-07-03 04:45:00]

lgtm

I'm not totally sure of how to handle the NTP issue. Another approach is to
split the metrics so there is both ServiceWorker.XXX and ServiceWorker.XXX.NTP
metrics. For something like FetchEvent that's trying more to get an idea of how
widespread something is, discounting NTP completely seems fine. I think for
UrlRequestJob that's trying to get a heartbeat on health, adding the .NTP metric
would be more worthwhile.

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

https://codereview.chromium.org/1178043002/diff/90001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:26: // Exclude NTP
scope from UMA for now.
Comment should also say why, as the CL description does.

https://codereview.chromium.org/1178043002/diff/90001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/1178043002/diff/90001/content/browser/service...
content/browser/service_worker/service_worker_metrics.h:120: // Records how much
of dispatched events is handled while a Service
I think it's "are handled"

[kinuko, 2015-07-05 09:19:04]

Updated, thanks. I'm not fully sure how we should handle NTP values either yet..
as you say for some cases it might be worth recording UMA specifically for NTP.

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

https://codereview.chromium.org/1178043002/diff/90001/content/browser/service...
content/browser/service_worker/service_worker_metrics.cc:26: // Exclude NTP
scope from UMA for now.
On 2015/07/03 04:45:00, falken wrote:
> Comment should also say why, as the CL description does.

Done.

https://codereview.chromium.org/1178043002/diff/90001/content/browser/service...
File content/browser/service_worker/service_worker_metrics.h (right):

https://codereview.chromium.org/1178043002/diff/90001/content/browser/service...
content/browser/service_worker/service_worker_metrics.h:120: // Records how much
of dispatched events is handled while a Service
On 2015/07/03 04:45:00, falken wrote:
> I think it's "are handled"

Done.

[kinuko, 2015-07-05 09:21:31]

kinuko@chromium.org changed reviewers:
+ isherman@chromium.org

[kinuko, 2015-07-05 09:22:43]

isherman@ could you review for histogram change?

[Ilya Sherman, 2015-07-06 18:44:51]

LGTM % nits:

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

https://codereview.chromium.org/1178043002/diff/110001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:37516: +    Records how much of fetch
events are handled compared with the number of
nit: "how much of" -> "how many"

https://codereview.chromium.org/1178043002/diff/110001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:66355: +  <int value="2"
label="NO_EVENTS_HANDLED"/>
nit: I'd use shorter labels, like "All", "Some", and "None".  Or, if you prefer
the more explicit longer labels, I'd at least recommend writing them in a more
human-friendly way, e.g. "All events handled"

[kinuko, 2015-07-07 08:31:39]

Thanks!

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

https://codereview.chromium.org/1178043002/diff/110001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:37516: +    Records how much of fetch
events are handled compared with the number of
On 2015/07/06 18:44:50, Ilya Sherman (Away July 8-13) wrote:
> nit: "how much of" -> "how many"

Done.

https://codereview.chromium.org/1178043002/diff/110001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:66355: +  <int value="2"
label="NO_EVENTS_HANDLED"/>
On 2015/07/06 18:44:50, Ilya Sherman (Away July 8-13) wrote:
> nit: I'd use shorter labels, like "All", "Some", and "None".  Or, if you
prefer
> the more explicit longer labels, I'd at least recommend writing them in a more
> human-friendly way, e.g. "All events handled"

Done.

[kinuko, 2015-07-07 08:33:37]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2015-07-07 08:33:38]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1178043002/#ps130001
(title: " ")

[commit-bot: I haz the power, 2015-07-07 08:33:58]

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

[commit-bot: I haz the power, 2015-07-07 09:50:40]

Committed patchset #5 (id:130001)

[commit-bot: I haz the power, 2015-07-07 09:51:37]

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