UMA for Notification action buttons

UMA for Notification action buttons

- Notifications.PersistentNotificationActionCount tracks number of
  action buttons per notification

- Notifications.Persistent.ClickedActionButton counts clicks on action
  buttons

- Notifications.Persistent.Clicked no longer includes clicks on action
  buttons

BUG=513671
Committed: https://crrev.com/f661988bc9a6ce398494a69988aae4871b8fdb51
Cr-Commit-Position: refs/heads/master@{#361328}

[johnme, 2015-10-28 16:16:51]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[Peter Beverloo, 2015-10-28 16:27:07]

https://codereview.chromium.org/1422193003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1422193003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:104: if
(action_index == -1) {
Please don't forget about https://codereview.chromium.org/1422673002/

https://codereview.chromium.org/1422193003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:305: 0, 10,
12 /* one extra bucket for > 10 */);
I don't understand, |notification_data.actions.size()| won't ever be >2 here
since Blink limits it in the renderer process, and SanitizeNotificationData() in
the browser process.

Did you instead intend to measure this in Blink *before* we apply the cap?

[johnme, 2015-10-28 18:43:48]

Addressed review comments - thanks :)

https://codereview.chromium.org/1422193003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1422193003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:104: if
(action_index == -1) {
On 2015/10/28 16:27:07, Peter Beverloo wrote:
> Please don't forget about https://codereview.chromium.org/1422673002/

Acknowledged. I already rebased my local 1422673002 branch onto this one to
replace this -1 :)

https://codereview.chromium.org/1422193003/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:305: 0, 10,
12 /* one extra bucket for > 10 */);
On 2015/10/28 16:27:07, Peter Beverloo wrote:
> I don't understand, |notification_data.actions.size()| won't ever be >2 here
> since Blink limits it in the renderer process, and SanitizeNotificationData()
in
> the browser process.
> 
> Did you instead intend to measure this in Blink *before* we apply the cap?

No, but that's a good idea. Done.

[johnme, 2015-10-28 18:43:49]

Addressed review comments - thanks :)

[johnme, 2015-10-28 18:46:08]

johnme@chromium.org changed reviewers:
+ asvitkine@chromium.org

[johnme, 2015-10-28 18:46:08]

asvitkine@chromium.org: Please review changes in tools/metrics - thanks :)

[Peter Beverloo, 2015-10-28 18:55:50]

lgtm

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

https://codereview.chromium.org/1422193003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29129: +    The number of action buttons
shown on each persistent Web Notification.
each -> a

[johnme, 2015-10-28 19:03:39]

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

https://codereview.chromium.org/1422193003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29129: +    The number of action buttons
shown on each persistent Web Notification.
On 2015/10/28 18:55:50, Peter Beverloo wrote:
> each -> a

Done.

[Alexei Svitkine (slow), 2015-10-28 19:09:09]

https://codereview.chromium.org/1422193003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1422193003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:64:
Platform::current()->histogramCustomCounts("Notifications.PersistentNotificationActionCount",
options.actions().size(), 0, 10, 12 /* one extra bucket for > 10 */);
If you want a single bucket for each distinct counts (i.e. each bucket has a
range of 1 value), suggest using  histogramEnumeration(),

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

https://codereview.chromium.org/1422193003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29129: +    The number of action buttons
shown on a persistent Web Notification.
Nit: Mention when it's logged.

[johnme, 2015-10-29 14:24:41]

Addressed review comments - thanks :)

https://codereview.chromium.org/1422193003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1422193003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:64:
Platform::current()->histogramCustomCounts("Notifications.PersistentNotificationActionCount",
options.actions().size(), 0, 10, 12 /* one extra bucket for > 10 */);
On 2015/10/28 19:09:09, Alexei Svitkine (slow) wrote:
> If you want a single bucket for each distinct counts (i.e. each bucket has a
> range of 1 value), suggest using  histogramEnumeration(),

We don't strictly care about having distinct buckets, especially for larger
numbers of buttons (in practice it'd be useful for 0, 1 and 2 to have distinct
buckets). Converting the count to an enum feels rather hacky. So keeping it as a
histogram seems reasonable?

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

https://codereview.chromium.org/1422193003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29129: +    The number of action buttons
shown on a persistent Web Notification.
On 2015/10/28 19:09:09, Alexei Svitkine (slow) wrote:
> Nit: Mention when it's logged.

Done.

[Alexei Svitkine (slow), 2015-10-29 19:24:05]

https://codereview.chromium.org/1422193003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1422193003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:64:
Platform::current()->histogramCustomCounts("Notifications.PersistentNotificationActionCount",
options.actions().size(), 0, 10, 12 /* one extra bucket for > 10 */);
On 2015/10/29 14:24:41, johnme wrote:
> On 2015/10/28 19:09:09, Alexei Svitkine (slow) wrote:
> > If you want a single bucket for each distinct counts (i.e. each bucket has a
> > range of 1 value), suggest using  histogramEnumeration(),
> 
> We don't strictly care about having distinct buckets, especially for larger
> numbers of buttons (in practice it'd be useful for 0, 1 and 2 to have distinct
> buckets). Converting the count to an enum feels rather hacky. So keeping it as
a
> histogram seems reasonable?

Well, the way you're using counts() right now is equivalent to the enumeration
function. (Other than a few errors you have such as specifying a min 0 instead
of 1, since the API already gives you an underflow bucket from 0 to min value).
So it would be simpler and less error-prone to call that function.

On the other hand, if you don't mind having broader ranges at the high end, you
can actually specify a higher max value than 10 and continue using Counts().

I am ok with either approach, but your current one can be simplified using the
other function.

[johnme, 2015-11-23 15:06:09]

Addressed Alexei's review comment - thanks!

https://codereview.chromium.org/1422193003/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1422193003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:64:
Platform::current()->histogramCustomCounts("Notifications.PersistentNotificationActionCount",
options.actions().size(), 0, 10, 12 /* one extra bucket for > 10 */);
On 2015/10/29 19:24:05, Alexei Svitkine (slow) wrote:
> On 2015/10/29 14:24:41, johnme wrote:
> > On 2015/10/28 19:09:09, Alexei Svitkine (slow) wrote:
> > > If you want a single bucket for each distinct counts (i.e. each bucket has
a
> > > range of 1 value), suggest using  histogramEnumeration(),
> > 
> > We don't strictly care about having distinct buckets, especially for larger
> > numbers of buttons (in practice it'd be useful for 0, 1 and 2 to have
distinct
> > buckets). Converting the count to an enum feels rather hacky. So keeping it
as
> a
> > histogram seems reasonable?
> 
> Well, the way you're using counts() right now is equivalent to the enumeration
> function. (Other than a few errors you have such as specifying a min 0 instead
> of 1, since the API already gives you an underflow bucket from 0 to min
value).
> So it would be simpler and less error-prone to call that function.
> 
> On the other hand, if you don't mind having broader ranges at the high end,
you
> can actually specify a higher max value than 10 and continue using Counts().
> 
> I am ok with either approach, but your current one can be simplified using the
> other function.

Gotcha. I've switched to histogramEnumeration/LinearHistogram as you suggested
since we care more about exact counts for small values than we care about the
order of magnitude for excessively large values provided (even though those are
possible, since this is developer-supplied input).

It feels a little dirty that we'll be deliberately making use of the underflow
bucket to pass in zeroes, but I guess that's normal?

[Alexei Svitkine (slow), 2015-11-23 16:19:19]

lgtm % nit

https://codereview.chromium.org/1422193003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1422193003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:63:
// Log number of actions developer provided, even if it's more than we currently
support.
Nit: Expand comment to mention you're intentionally using a linear histogram
here and where the magic number '17' came from.

[johnme, 2015-11-24 11:19:38]

The CQ bit was checked by johnme@chromium.org

[johnme, 2015-11-24 11:19:39]

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

[commit-bot: I haz the power, 2015-11-24 11:20:15]

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

[johnme, 2015-11-24 11:24:12]

Addressed review nit - thanks!

https://codereview.chromium.org/1422193003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp
(right):

https://codereview.chromium.org/1422193003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/notifications/ServiceWorkerRegistrationNotifications.cpp:63:
// Log number of actions developer provided, even if it's more than we currently
support.
On 2015/11/23 16:19:19, Alexei Svitkine (slow) wrote:
> Nit: Expand comment to mention you're intentionally using a linear histogram
> here and where the magic number '17' came from.

Done.

[commit-bot: I haz the power, 2015-11-24 13:13:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-24 13:14:04]

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