Add UMA histogram data representing the Synced Notifications feature.

Add UMA histogram data representing the Synced Notifications feature.

1. First, I added a new data point for muting and unmuting the notification
system.

2. Second, I added a new histogram for Synced Notifications that measures
common events that happen with them.

3. In a follow up chekin, I will also measure how often individual
synced notification services are turned off.


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

[Pete Williamson, 2013-10-10 23:19:39]

thakis@ Please review changes to chrome_browser.gypi (two files only)

asvitkine@ Please review changes to histogram.xml

dewittj@ Please review everything else

Questions for reviewers:
1. Do I need the "PerNotificationActions section of histograms.xml?  I copied it
from the NotificationAction section without completely understanding it.  I am
happy to read some instructions if you can send me a pointer to adding histogram
data.

2. Is there any way to test that my notifications are getting into the system
short of checking the code in and watching the UMA results website?

3. Will the existing "PerNotificationActions" section cover the new
Notification.Mute data point that I have added, or is there something else I
need to do (it seems like my code should need to register it somehow).

[Alexei Svitkine (slow), 2013-10-11 14:50:11]

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/message_center_stats_collector.cc (right):

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_stats_collector.cc:137: if
(in_quiet_mode)
Nit: This needs {}.

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_stats_collector.cc:139:
content::UserMetricsAction("Notification.Mute"));
Please run tools/metrics/extract_actions.py and add the modified file to the CL.

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/synced_notification_stats.cc
(right):

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification_stats.cc:18: for
(size_t i = 0; i < SYNCED_NOTIFICATION_ACTION_COUNT; i++) {
Nit: ++i and remove {}'s

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification_stats.cc:43:
SYNCED_NOTIFICATION_ACTION_COUNT);
This looks strange to me. I'm not familiar with this "PerNotification" thing
either. Can you explain what this is trying to measure? (Not necessarily this
specific code, but your CL in general - what questions do you want the metrics
to answer?)

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/synced_notification_stats.h
(right):

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification_stats.h:49: 
Nit: Remove extra blank line.

[Pete Williamson, 2013-10-14 03:46:06]

CR changes per ASvitkine, added changes to chromeactions.txt

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/message_center_stats_collector.cc (right):

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_stats_collector.cc:137: if
(in_quiet_mode)
On 2013/10/11 14:50:12, Alexei Svitkine wrote:
> Nit: This needs {}.

Done.

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/message_center_stats_collector.cc:139:
content::UserMetricsAction("Notification.Mute"));
On 2013/10/11 14:50:12, Alexei Svitkine wrote:
> Please run tools/metrics/extract_actions.py and add the modified file to the
CL.

Done, though the tool had some errors (I sent mail about it separately)

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/synced_notification_stats.cc
(right):

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification_stats.cc:18: for
(size_t i = 0; i < SYNCED_NOTIFICATION_ACTION_COUNT; i++) {
On 2013/10/11 14:50:12, Alexei Svitkine wrote:
> Nit: ++i and remove {}'s

Done.

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification_stats.cc:43:
SYNCED_NOTIFICATION_ACTION_COUNT);
On 2013/10/11 14:50:12, Alexei Svitkine wrote:
> This looks strange to me. I'm not familiar with this "PerNotification" thing
> either. Can you explain what this is trying to measure? (Not necessarily this
> specific code, but your CL in general - what questions do you want the metrics
> to answer?)

I was hoping to ask you if I needed it or not.  I copied this code from the
notification center changelist (https://codereview.chromium.org/23902057),
thinnking it was required to get 
    content::RecordAction(
                 content::UserMetricsAction("Notifications.ShowMessageCenter")
to work properly.  I'll be happy to remove it since I'm not quite sure what it
does.

Now, what I am trying to measure.  The idea of the synced notifications feature
is that now servers can send notifications directly to chrome clients.
We haven't done this before, and we want to measure the reaction of users to
this feature.  We want to know if users are turning it off.  We want to know if
users are dismissing the notifications while they are being shown as a "toast"
or waiting until later when they go into the notification center.  We want to
know if users are clicking on our notifications.  We want to know if users are
muting all notifications and leaving them turned off, or only turning off
notifications while they are presenting.  Each individual service which sends a
notification can be enabled or disabled separately, and my next changelist will
try to measure which sending services get turned off in practice.

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/synced_notification_stats.h
(right):

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification_stats.h:49: 
On 2013/10/11 14:50:12, Alexei Svitkine wrote:
> Nit: Remove extra blank line.

Done.

[Alexei Svitkine (slow), 2013-10-15 17:12:02]

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/synced_notification_stats.cc
(right):

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification_stats.cc:43:
SYNCED_NOTIFICATION_ACTION_COUNT);
On 2013/10/14 03:46:07, Pete Williamson wrote:
> On 2013/10/11 14:50:12, Alexei Svitkine wrote:
> > This looks strange to me. I'm not familiar with this "PerNotification" thing
> > either. Can you explain what this is trying to measure? (Not necessarily
this
> > specific code, but your CL in general - what questions do you want the
metrics
> > to answer?)
> 
> I was hoping to ask you if I needed it or not.  I copied this code from the
> notification center changelist (https://codereview.chromium.org/23902057),
> thinnking it was required to get 
>     content::RecordAction(
>                  content::UserMetricsAction("Notifications.ShowMessageCenter")
> to work properly.  I'll be happy to remove it since I'm not quite sure what it
> does.

The other stuff should work on its own. Unless you actually need this info, I
suggest not adding this (which should allow you to simplify your code a lot -
e.g. possibly even removing this class and inlining its impl into the caller).

> 
> Now, what I am trying to measure.  The idea of the synced notifications
feature
> is that now servers can send notifications directly to chrome clients.
> We haven't done this before, and we want to measure the reaction of users to
> this feature.  We want to know if users are turning it off.  We want to know
if
> users are dismissing the notifications while they are being shown as a "toast"
> or waiting until later when they go into the notification center.  We want to
> know if users are clicking on our notifications.  We want to know if users are
> muting all notifications and leaving them turned off, or only turning off
> notifications while they are presenting.  Each individual service which sends
a
> notification can be enabled or disabled separately, and my next changelist
will
> try to measure which sending services get turned off in practice.

Got it.

It sounds like just having the Notifications.Actions enum histogram should be
enough. I don't see the purpose of Notifications.PerNotificationActions, so
please remove it (or else find out its purpose from the author of the CL you're
copying and clarify why it's needed.)

[Pete Williamson, 2013-10-16 02:24:21]

CR fixes per ASvitkine

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
File chrome/browser/notifications/sync_notifier/synced_notification_stats.cc
(right):

https://codereview.chromium.org/26943002/diff/1/chrome/browser/notifications/...
chrome/browser/notifications/sync_notifier/synced_notification_stats.cc:43:
SYNCED_NOTIFICATION_ACTION_COUNT);
On 2013/10/15 17:12:03, Alexei Svitkine wrote:
> On 2013/10/14 03:46:07, Pete Williamson wrote:
> > On 2013/10/11 14:50:12, Alexei Svitkine wrote:
> > > This looks strange to me. I'm not familiar with this "PerNotification"
thing
> > > either. Can you explain what this is trying to measure? (Not necessarily
> this
> > > specific code, but your CL in general - what questions do you want the
> metrics
> > > to answer?)
> > 
> > I was hoping to ask you if I needed it or not.  I copied this code from the
> > notification center changelist (https://codereview.chromium.org/23902057),
> > thinnking it was required to get 
> >     content::RecordAction(
> >                 
content::UserMetricsAction("Notifications.ShowMessageCenter")
> > to work properly.  I'll be happy to remove it since I'm not quite sure what
it
> > does.
> 
> The other stuff should work on its own. Unless you actually need this info, I
> suggest not adding this (which should allow you to simplify your code a lot -
> e.g. possibly even removing this class and inlining its impl into the caller).
> 
> > 
> > Now, what I am trying to measure.  The idea of the synced notifications
> feature
> > is that now servers can send notifications directly to chrome clients.
> > We haven't done this before, and we want to measure the reaction of users to
> > this feature.  We want to know if users are turning it off.  We want to know
> if
> > users are dismissing the notifications while they are being shown as a
"toast"
> > or waiting until later when they go into the notification center.  We want
to
> > know if users are clicking on our notifications.  We want to know if users
are
> > muting all notifications and leaving them turned off, or only turning off
> > notifications while they are presenting.  Each individual service which
sends
> a
> > notification can be enabled or disabled separately, and my next changelist
> will
> > try to measure which sending services get turned off in practice.
> 
> Got it.
> 
> It sounds like just having the Notifications.Actions enum histogram should be
> enough. I don't see the purpose of Notifications.PerNotificationActions, so
> please remove it (or else find out its purpose from the author of the CL
you're
> copying and clarify why it's needed.)
> 

OK, after a closer reading of the code, the PerNotificationActions seems to be
for keeping track on a notification by notification basis about what happens to
the notification during its lifetime.  I think in my scenario we are find
without it, so I removed it.

[Alexei Svitkine (slow), 2013-10-16 13:51:01]

https://codereview.chromium.org/26943002/diff/21001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/synced_notification_stats.cc
(right):

https://codereview.chromium.org/26943002/diff/21001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification_stats.cc:19:
actions_[i] = false;
Now that you're no longer using |actions_|, can you just remove it?

[Pete Williamson, 2013-10-17 05:26:41]

1. Got rid of the actions[] array per CR feedback from ASvitkine
2. I was forgetting to add the stats object to my collection on the first
incoming notification (we don't catch notification added), so I added it.

Adding dewittj@ as a reviewer.  Please review all notification center changes.

https://codereview.chromium.org/26943002/diff/21001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/synced_notification_stats.cc
(right):

https://codereview.chromium.org/26943002/diff/21001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/synced_notification_stats.cc:19:
actions_[i] = false;
On 2013/10/16 13:51:01, Alexei Svitkine wrote:
> Now that you're no longer using |actions_|, can you just remove it?

Done.

[Alexei Svitkine (slow), 2013-10-17 16:38:00]

https://codereview.chromium.org/26943002/diff/27001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_delegate.cc
(right):

https://codereview.chromium.org/26943002/diff/27001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_delegate.cc:74:
notification_stat.CollectAction(SYNCED_NOTIFICATION_ACTION_BUTTON_CLICK);
I don't think you need the new class and a map of collectors anymore.

Simply make CollectAction() a free-standing function at the top of the file in
an anon namespace along with the enum declaration.

Them, just call it on its own from all the places you need to, without the logic
to find/insert the StatsCollection object and remove that class completely.

[Pete Williamson, 2013-10-18 04:32:46]

CR fixes per ASvitkine:  Removed the stats class, and put the UMA calls inline
into the delegate class.

[Alexei Svitkine (slow), 2013-10-18 13:09:08]

LGTM % comments

https://codereview.chromium.org/26943002/diff/38001/chrome/browser/notificati...
File chrome/browser/notifications/sync_notifier/chrome_notifier_delegate.cc
(right):

https://codereview.chromium.org/26943002/diff/38001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_delegate.cc:7: 
Nit: Remove extra blank line?

https://codereview.chromium.org/26943002/diff/38001/chrome/browser/notificati...
chrome/browser/notifications/sync_notifier/chrome_notifier_delegate.cc:67:
CollectAction(SYNCED_NOTIFICATION_ACTION_BUTTON_CLICK);
Move this into the if above (to be consistent with the function above that does
an early return when the notification isn't found. (Alternatively, change
Click() to match this code structure instead - either way make them consistent.)

[dewittj, 2013-10-18 16:49:55]

lgtm % Alexei's comments

[commit-bot: I haz the power, 2013-10-20 22:30:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/26943002/38001

[commit-bot: I haz the power, 2013-10-20 22:50:18]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-10-20 23:04:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/26943002/158001

[commit-bot: I haz the power, 2013-10-21 01:20:40]

Change committed as 229720