Stats collector for message center.

Stats collector for message center.

This uses histograms to capture detailed data about how notifications
are being interacted with by the user.

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

[dewittj, 2013-09-20 22:14:09]

mukai: all
isherman: Please check out the new histograms.

[Jun Mukai, 2013-09-21 00:21:58]

lgtm with a nit.

https://codereview.chromium.org/23902057/diff/6001/chrome/browser/notificatio...
File chrome/browser/notifications/message_center_stats_collector.cc (right):

https://codereview.chromium.org/23902057/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/message_center_stats_collector.cc:75:
notification_stat.RecordAggregateStats();
Is it okay to compute the aggregation only when a notification is removed?

Should we do that at shutdown?

[dewittj, 2013-09-21 00:40:02]

On 2013/09/21 00:21:58, Jun Mukai wrote:
> lgtm with a nit.
> 
>
https://codereview.chromium.org/23902057/diff/6001/chrome/browser/notificatio...
> File chrome/browser/notifications/message_center_stats_collector.cc (right):
> 
>
https://codereview.chromium.org/23902057/diff/6001/chrome/browser/notificatio...
> chrome/browser/notifications/message_center_stats_collector.cc:75:
> notification_stat.RecordAggregateStats();
> Is it okay to compute the aggregation only when a notification is removed?
The reason I have the "raw" histogram is so that we can get a sense of what
proportion of notifications get removed.  I don't have any other event to work
off of if I want a histogram that 
> 
> Should we do that at shutdown?
I didn't want to slow shutdown by persisting a bunch of stuff to disk and trying
to send it on the network.  However, if it's OK to do these things at shutdown
that would be cool. isherman, any thoughts on that?

I think v1.1 would have a timer that can send stats after some unreasonable
amount of time passes.

[dewittj, 2013-09-21 00:58:04]

okay, to try to get this into m31 I will remove the histograms.xml to be added
in a separate commit.

[commit-bot: I haz the power, 2013-09-21 00:59:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/23902057/2001

[commit-bot: I haz the power, 2013-09-21 04:09:41]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Ilya Sherman, 2013-09-21 05:43:29]

Apologies for the slight delay.

https://codereview.chromium.org/23902057/diff/6001/chrome/browser/notificatio...
File chrome/browser/notifications/message_center_stats_collector.cc (right):

https://codereview.chromium.org/23902057/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/message_center_stats_collector.cc:37: for (size_t i
= 0; i < NOTIFICATION_ACTION_COUNT; i++) {
nit: ++i;

https://codereview.chromium.org/23902057/diff/6001/chrome/browser/notificatio...
chrome/browser/notifications/message_center_stats_collector.cc:41: i,
Should this be actions_[i]?

https://codereview.chromium.org/23902057/diff/6001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/23902057/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:10210: +</histogram>
Why measure this in two different ways?  That is, why is the RawActions
histogram -- which I would probably rename to just 'Actions' -- insufficient?

[dewittj, 2013-09-23 16:37:32]

isherman, I have the two metrics so that I can have a histogram that can tell me
"50% of notifications are clicked".  The raw info would only tell me
"Notifications are clicked 2x as often as they are created", which is slightly
different.  I would prefer to leave both but if in your experience it wouldn't
make a difference to us, I don't have a problem removing the complex stat.

[dewittj, 2013-09-23 17:03:54]

PTAL, this is more simplified, using only the raw aggregate data for histograms.

[Ilya Sherman, 2013-09-23 20:38:49]

histograms LGTM

On 2013/09/23 16:37:32, dewittj wrote:
> isherman, I have the two metrics so that I can have a histogram that can tell
me
> "50% of notifications are clicked".  The raw info would only tell me
> "Notifications are clicked 2x as often as they are created", which is slightly
> different.  I would prefer to leave both but if in your experience it wouldn't
> make a difference to us, I don't have a problem removing the complex stat.

Ah, I see, I'd misunderstood the meaning of this second histogram.  Now that I
understand that better, I think it's fine to include it if you think it might be
valuable.  If you do include it, though, I would recommend beefing up the
description a bit, so that it's clearer how that histogram differs from the
other one -- i.e. that each bucket will be incremented at most once per
notification, regardless of how many times that action occurred for the
notification.

[dewittj, 2013-09-23 21:34:15]

OK, added the other histogram back, and renamed it to Notifications.Actions
(other is Notifications.PerNotificationActions)

[commit-bot: I haz the power, 2013-09-23 21:40:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/23902057/53001

[Ilya Sherman, 2013-09-23 21:48:01]

(Still LGTM.)

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

https://codereview.chromium.org/23902057/diff/53001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:10241: +    is closed.  This differs
from the RawActions type in that multiple events of
nit: "RawActions type" -> "Actions histogram"

[commit-bot: I haz the power, 2013-09-23 21:51:46]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-09-23 21:51:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/23902057/53002

[dewittj, 2013-09-23 21:51:55]

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

https://codereview.chromium.org/23902057/diff/53001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:10241: +    is closed.  This differs
from the RawActions type in that multiple events of
On 2013/09/23 21:48:01, Ilya Sherman wrote:
> nit: "RawActions type" -> "Actions histogram"

Done.

[commit-bot: I haz the power, 2013-09-24 03:31:24]

Change committed as 224880