ash:Shelf - Added UMA stats for ShelfAlignment

Adding 2 simple UMA stats to track shelf alignment usage, firstly track when a shelf alignment is changed (rare), and during usage of the shelf launcher (buttons on the shelf) track the current alignment (more common) to provide some data on how frequent usage is of the various alignments.

BUG=298174
TBR=askvitkine@chromium.org

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

[Harry McCleave, 2013-10-16 23:03:46]

ptal

[James Cook, 2013-10-16 23:35:27]

https://codereview.chromium.org/26373009/diff/6001/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

https://codereview.chromium.org/26373009/diff/6001/ash/launcher/launcher_view...
ash/launcher/launcher_view.cc:1563:
UMA_HISTOGRAM_ENUMERATION("Ash.ShelfAlignmentUsage",
This feels like the wrong way to do this. It will skew the results toward people
who use the shelf heavily. We need some equivalent of "usage minutes" or "page
loads".

One way might be to look in metrics_service.cc at how it collects memory
information periodically.  Another might be to add an ash "periodic stats"
function that gets called periodically on a timer via PostDelayedTask.

[Harry McCleave, 2013-10-30 00:02:42]

On 2013/10/16 23:35:27, James Cook wrote:
>
https://codereview.chromium.org/26373009/diff/6001/ash/launcher/launcher_view.cc
> File ash/launcher/launcher_view.cc (right):
> 
>
https://codereview.chromium.org/26373009/diff/6001/ash/launcher/launcher_view...
> ash/launcher/launcher_view.cc:1563:
> UMA_HISTOGRAM_ENUMERATION("Ash.ShelfAlignmentUsage",
> This feels like the wrong way to do this. It will skew the results toward
people
> who use the shelf heavily. We need some equivalent of "usage minutes" or "page
> loads".
> 
> One way might be to look in metrics_service.cc at how it collects memory
> information periodically.  Another might be to add an ash "periodic stats"
> function that gets called periodically on a timer via PostDelayedTask.

Added an over time track too, I left in the alignment during action as I think
that may also be a useful indication for a subset of users behavior.

[James Cook, 2013-10-30 16:49:12]

https://codereview.chromium.org/26373009/diff/52001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/26373009/diff/52001/ash/shelf/shelf_view.cc#n...
ash/shelf/shelf_view.cc:1895: UMA_HISTOGRAM_ENUMERATION(histogram_name,
This won't work. UMA_HISTOGRAM_ENUMERATION does some magic with the current
file/line name to create a cached value - it needs a literal string at the call
site.  This is why ChromeShellDelegate::RecordUserMetricsAction() has a huge
switch.  It might be better to pull out the shelf alignment into the sites that
are recording it and put the strings there.

If you really need to build histograms out of a string see
RecordLinearHistogram() in oom_priority_manager.cc for an example.

https://codereview.chromium.org/26373009/diff/52001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/26373009/diff/52001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1160:
ash::Launcher::ForPrimaryDisplay()->RecordCurrentAlignmentForUMAHistogram(
Now that I think about it more, this really needs to be on a repeating task in
Ash.  The metrics service shouldn't be responsible for querying all the modules
that need periodic information to record.  Sorry to give you bad advice before.

See chrome/browser/chromeos/memory/oom_priority_manager.cc
RecordRecentTabDiscard() for how to use a repeating timer for this.

I would introduce some kind of "RecordPeriodicStatistics" function or class in
Ash and stick the code there.  It should have a timer that is explicitly started
and stopped when ash::Shell is initialized and torn down.

[Harry McCleave, 2013-11-01 00:17:32]

On 2013/10/30 16:49:12, James Cook wrote:
> https://codereview.chromium.org/26373009/diff/52001/ash/shelf/shelf_view.cc
> File ash/shelf/shelf_view.cc (right):
> 
>
https://codereview.chromium.org/26373009/diff/52001/ash/shelf/shelf_view.cc#n...
> ash/shelf/shelf_view.cc:1895: UMA_HISTOGRAM_ENUMERATION(histogram_name,
> This won't work. UMA_HISTOGRAM_ENUMERATION does some magic with the current
> file/line name to create a cached value - it needs a literal string at the
call
> site.  This is why ChromeShellDelegate::RecordUserMetricsAction() has a huge
> switch.  It might be better to pull out the shelf alignment into the sites
that
> are recording it and put the strings there.
> 
> If you really need to build histograms out of a string see
> RecordLinearHistogram() in oom_priority_manager.cc for an example.
> 
>
https://codereview.chromium.org/26373009/diff/52001/chrome/browser/metrics/me...
> File chrome/browser/metrics/metrics_service.cc (right):
> 
>
https://codereview.chromium.org/26373009/diff/52001/chrome/browser/metrics/me...
> chrome/browser/metrics/metrics_service.cc:1160:
> ash::Launcher::ForPrimaryDisplay()->RecordCurrentAlignmentForUMAHistogram(
> Now that I think about it more, this really needs to be on a repeating task in
> Ash.  The metrics service shouldn't be responsible for querying all the
modules
> that need periodic information to record.  Sorry to give you bad advice
before.
> 
> See chrome/browser/chromeos/memory/oom_priority_manager.cc
> RecordRecentTabDiscard() for how to use a repeating timer for this.
> 
> I would introduce some kind of "RecordPeriodicStatistics" function or class in
> Ash and stick the code there.  It should have a timer that is explicitly
started
> and stopped when ash::Shell is initialized and torn down.

How does this look, odly enough the previous version (which the macros should
have mangled the strings on) populated the about://histograms as intended.

[James Cook, 2013-11-01 00:28:50]

Mostly nits.

https://codereview.chromium.org/26373009/diff/122001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/26373009/diff/122001/ash/ash.gyp#newcode75
ash/ash.gyp:75: 'ash_periodic_metrics_recorder.cc',
This file doesn't need to have ash_ in the name, since it lives in ash.

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
File ash/ash_periodic_metrics_recorder.cc (right):

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
ash/ash_periodic_metrics_recorder.cc:21: const int
kAshPeridicMetricsTimeInSeconds = 1 * 60;
Peridic -> Periodic

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
ash/ash_periodic_metrics_recorder.cc:35: internal::ShelfLayoutManager *manager =
nit: * on left

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
File ash/ash_periodic_metrics_recorder.h (right):

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
ash/ash_periodic_metrics_recorder.h:5: #ifndef
ASH_ASH_PERIODIC_METRICS_RECORDER_H
nit: end with trailing _

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
ash/ash_periodic_metrics_recorder.h:23: }  // ash
nit: "namespace ash"

https://codereview.chromium.org/26373009/diff/122001/tools/metrics/actions/ch...
File tools/metrics/actions/chromeactions.txt (right):

https://codereview.chromium.org/26373009/diff/122001/tools/metrics/actions/ch...
tools/metrics/actions/chromeactions.txt:1555:
0x3c863a67e6807fe9	Shelf_AlignmentChanged
Are you still using this one?  Maybe I missed it.

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

https://codereview.chromium.org/26373009/diff/122001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:236: +    regular basis (30 minutes),
this is used instead of log in or shelf usage to
Isn't it recorded once per minute, so this is user-minutes spent in each
configuration?

[James Cook, 2013-11-01 00:29:55]

Oh, and I like this much better.  :-)

[Harry McCleave, 2013-11-01 01:00:37]

https://codereview.chromium.org/26373009/diff/122001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/26373009/diff/122001/ash/ash.gyp#newcode75
ash/ash.gyp:75: 'ash_periodic_metrics_recorder.cc',
On 2013/11/01 00:28:51, James Cook wrote:
> This file doesn't need to have ash_ in the name, since it lives in ash.

Done.

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
File ash/ash_periodic_metrics_recorder.cc (right):

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
ash/ash_periodic_metrics_recorder.cc:21: const int
kAshPeridicMetricsTimeInSeconds = 1 * 60;
On 2013/11/01 00:28:51, James Cook wrote:
> Peridic -> Periodic

Done.

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
ash/ash_periodic_metrics_recorder.cc:35: internal::ShelfLayoutManager *manager =
On 2013/11/01 00:28:51, James Cook wrote:
> nit: * on left

Done.

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
File ash/ash_periodic_metrics_recorder.h (right):

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
ash/ash_periodic_metrics_recorder.h:5: #ifndef
ASH_ASH_PERIODIC_METRICS_RECORDER_H
On 2013/11/01 00:28:51, James Cook wrote:
> nit: end with trailing _

Done.

https://codereview.chromium.org/26373009/diff/122001/ash/ash_periodic_metrics...
ash/ash_periodic_metrics_recorder.h:23: }  // ash
On 2013/11/01 00:28:51, James Cook wrote:
> nit: "namespace ash"

Done.

https://codereview.chromium.org/26373009/diff/122001/tools/metrics/actions/ch...
File tools/metrics/actions/chromeactions.txt (right):

https://codereview.chromium.org/26373009/diff/122001/tools/metrics/actions/ch...
tools/metrics/actions/chromeactions.txt:1555:
0x3c863a67e6807fe9	Shelf_AlignmentChanged
On 2013/11/01 00:28:51, James Cook wrote:
> Are you still using this one?  Maybe I missed

Nope, but extract actions keeps creating it not sure if i should manually delete
it from the .txt?

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

https://codereview.chromium.org/26373009/diff/122001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:236: +    regular basis (30 minutes),
this is used instead of log in or shelf usage to
On 2013/11/01 00:28:51, James Cook wrote:
> Isn't it recorded once per minute, so this is user-minutes spent in each
> configuration?
 it's 30 minutes i changed it to 1 minute for testing purpose, thank you for
catching always write to file before uploading.

[James Cook, 2013-11-01 05:06:27]

LGTM with a couple suggestions

https://codereview.chromium.org/26373009/diff/202001/ash/periodic_metrics_rec...
File ash/periodic_metrics_recorder.h (right):

https://codereview.chromium.org/26373009/diff/202001/ash/periodic_metrics_rec...
ash/periodic_metrics_recorder.h:12: class AshPeriodicMetricsRecorder {
Just call it PeriodicMetricsRecorder since it is already in namespace ash. 
Also, it needs a class comment - why does this class exist?  What should the
next programmer add here?

https://codereview.chromium.org/26373009/diff/202001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/26373009/diff/202001/ash/shelf/shelf_view.cc#...
ash/shelf/shelf_view.cc:62: const int SHELF_ALIGNMENT_UMA_ENUM_VALUE_BOTTOM = 0;
Could these be shared with the other copy somehow?

https://codereview.chromium.org/26373009/diff/202001/tools/metrics/actions/ch...
File tools/metrics/actions/chromeactions.txt (right):

https://codereview.chromium.org/26373009/diff/202001/tools/metrics/actions/ch...
tools/metrics/actions/chromeactions.txt:1555:
0x3c863a67e6807fe9	Shelf_AlignmentChanged
I don't think it hurts to have an extra value here, but you might want to do a
"git grep" and see if there's somewhere in the source that it's hiding.

[James Cook, 2013-11-05 17:55:55]

btw, varkha might have a use for periodic metrics collection in
https://codereview.chromium.org/45343003/

[commit-bot: I haz the power, 2013-11-18 18:46:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/26373009/302001

[commit-bot: I haz the power, 2013-11-18 23:34:33]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-19 02:24:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/harrym@chromium.org/26373009/302001

[commit-bot: I haz the power, 2013-11-19 03:58:58]

Change committed as 235904