Add metrics to all items in the 'wrench' menu

Added histograms:
+ count frequency of use for each of the menu items.
+ log time taken to get to any menu item.
+ time taken to get to individual menu items.

BUG=432561
Committed: https://crrev.com/b8dde40da782604227baee3cd28c774920a13470
Cr-Commit-Position: refs/heads/master@{#304586}

[edwardjung, 2014-11-14 12:22:36]

edwardjung@chromium.org changed reviewers:
+ asvitkine@chromium.org, pkasting@chromium.org

[edwardjung, 2014-11-14 12:22:37]

Hi Peter, could you review these updates to the metrics gathering on the menu:
  c/b/ui/toolbar/
  c/b/ui/views/toolbar/

Alexei, the histograms file:
  tools/metrics/histograms/histograms.xml

Thanks.

[Peter Kasting, 2014-11-14 19:48:59]

LGTM with a lot of comment nits

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:172: base::ElapsedTimer
timer;
Nit: Give a more descriptive name and/or write a comment about what this is for.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:419: // Menu user metrics
Nit: This comment is meaningless, remove.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.h (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:210: // Log UMA metrics for each
command clicked on.
Nit: Grammar (and specificity).  How about: "Called when a command is selected. 
Logs UMA metrics about which command was chosen and how long the user took to
select the command."

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:213: // Time menu wasopened for.
Nit: was opened for before what?  Be much more specific, e.g. "Time menu has
been open.  Used by LogMenuMetrics() to record the time to action when the user
selects a menu item."

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:216: // Whether a UMA menu action
has been recorded.
Nit: Has been recorded since when?  Menu open?  Ever?  Why do we need to track
this?  Again, be more specific.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.h (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.h:189: // Timer for menu open
metrics.
Nit: This comment adds nothing to the member name.  Remove it or else make much
more detailed.

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

https://codereview.chromium.org/700673003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40427: +  <summary>Time the menu is
opened for before a menu item is selected.</summary>
Grammar.  How about "The time a user takes to select a menu item after opening
the menu."

[Alexei Svitkine (slow), 2014-11-15 00:00:16]

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:172: base::ElapsedTimer
timer;
On 2014/11/14 19:48:59, Peter Kasting wrote:
> Nit: Give a more descriptive name and/or write a comment about what this is
for.

Also, name should end with a _.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:664: MENU_ACTION_ZOOM_PLUS,
LIMIT_MENU_ACTION);
Please make a helper function for this macro. Each macro expands to a lot of
code, so having a helper function avoids some of the bloat.

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

https://codereview.chromium.org/700673003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40424: +<histogram
name="WrenchMenu.TimeToAction">
Since the actual histograms are things like
WrenchMenu.TimeToAction.BookmarkOpen, what you actually want to do is define a
histogram_suffixes block with all the suffixes and mark this base histogram as
affected-histogram. You can see instructions in the comment at the top of the
file, or find other examples throughout the file.

[Peter Kasting, 2014-11-15 00:03:10]

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:664: MENU_ACTION_ZOOM_PLUS,
LIMIT_MENU_ACTION);
On 2014/11/15 00:00:16, Alexei Svitkine wrote:
> Please make a helper function for this macro. Each macro expands to a lot of
> code, so having a helper function avoids some of the bloat.

I'm not sure that's feasible.  Don't we have to explicitly pass the raw strings
to our histogram functions so our source-crawling tools annotate things
correctly, rather than passing in variables of string type?

[Alexei Svitkine (slow), 2014-11-15 00:06:43]

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:664: MENU_ACTION_ZOOM_PLUS,
LIMIT_MENU_ACTION);
On 2014/11/15 00:03:10, Peter Kasting wrote:
> On 2014/11/15 00:00:16, Alexei Svitkine wrote:
> > Please make a helper function for this macro. Each macro expands to a lot of
> > code, so having a helper function avoids some of the bloat.
> 
> I'm not sure that's feasible.  Don't we have to explicitly pass the raw
strings
> to our histogram functions so our source-crawling tools annotate things
> correctly, rather than passing in variables of string type?

I'm suggesting specifically for "WrenchMenu.MenuAction" macro. I agree that the
other ones (*TimeToAction*) cannot be refactored this way.

[edwardjung, 2014-11-17 15:58:24]

Thanks for the comments. 

Alexei, could you have a check the suffix implementation and helper function are
okay.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:172: base::ElapsedTimer
timer;
On 2014/11/15 00:00:16, Alexei Svitkine wrote:
> On 2014/11/14 19:48:59, Peter Kasting wrote:
> > Nit: Give a more descriptive name and/or write a comment about what this is
> for.
> 
> Also, name should end with a _.

Done.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h:172: base::ElapsedTimer
timer;
On 2014/11/14 19:48:59, Peter Kasting wrote:
> Nit: Give a more descriptive name and/or write a comment about what this is
for.

Done.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:419: // Menu user metrics
On 2014/11/14 19:48:59, Peter Kasting wrote:
> Nit: This comment is meaningless, remove.

Done.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:664: MENU_ACTION_ZOOM_PLUS,
LIMIT_MENU_ACTION);
On 2014/11/15 00:06:43, Alexei Svitkine wrote:
> On 2014/11/15 00:03:10, Peter Kasting wrote:
> > On 2014/11/15 00:00:16, Alexei Svitkine wrote:
> > > Please make a helper function for this macro. Each macro expands to a lot
of
> > > code, so having a helper function avoids some of the bloat.
> > 
> > I'm not sure that's feasible.  Don't we have to explicitly pass the raw
> strings
> > to our histogram functions so our source-crawling tools annotate things
> > correctly, rather than passing in variables of string type?
> 
> I'm suggesting specifically for "WrenchMenu.MenuAction" macro. I agree that
the
> other ones (*TimeToAction*) cannot be refactored this way.

Done.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.h (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:210: // Log UMA metrics for each
command clicked on.
On 2014/11/14 19:48:59, Peter Kasting wrote:
> Nit: Grammar (and specificity).  How about: "Called when a command is
selected. 
> Logs UMA metrics about which command was chosen and how long the user took to
> select the command."

Done.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:213: // Time menu wasopened for.
On 2014/11/14 19:48:59, Peter Kasting wrote:
> Nit: was opened for before what?  Be much more specific, e.g. "Time menu has
> been open.  Used by LogMenuMetrics() to record the time to action when the
user
> selects a menu item."

Done.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:216: // Whether a UMA menu action
has been recorded.
On 2014/11/14 19:48:59, Peter Kasting wrote:
> Nit: Has been recorded since when?  Menu open?  Ever?  Why do we need to track
> this?  Again, be more specific.

Done.

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.h (right):

https://codereview.chromium.org/700673003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.h:189: // Timer for menu open
metrics.
On 2014/11/14 19:48:59, Peter Kasting wrote:
> Nit: This comment adds nothing to the member name.  Remove it or else make
much
> more detailed.

Removed.

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

https://codereview.chromium.org/700673003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40424: +<histogram
name="WrenchMenu.TimeToAction">
On 2014/11/15 00:00:16, Alexei Svitkine wrote:
> Since the actual histograms are things like
> WrenchMenu.TimeToAction.BookmarkOpen, what you actually want to do is define a
> histogram_suffixes block with all the suffixes and mark this base histogram as
> affected-histogram. You can see instructions in the comment at the top of the
> file, or find other examples throughout the file.

Makes sense, I hadn't spotted this. Thanks.

https://codereview.chromium.org/700673003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40427: +  <summary>Time the menu is
opened for before a menu item is selected.</summary>
On 2014/11/14 19:48:59, Peter Kasting wrote:
> Grammar.  How about "The time a user takes to select a menu item after opening
> the menu."

Done.

[Alexei Svitkine (slow), 2014-11-17 18:51:19]

Just a few more comments, thanks!

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:414: base::TimeDelta now =
timer_.Elapsed();
Nit: Use a different name - |now| implies it's a time, rather than a duration.
How about |duration| or |delta|?

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:428: if (!uma_action_recorded_)
Nit: {}'s because body is multiline. Same for others below.

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:682: LIMIT_MENU_ACTION);
Nit: Align.

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.h (right):

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.h:189: base::ElapsedTimer
menu_opened_timer_;
Add a comment.

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

https://codereview.chromium.org/700673003/diff/140001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:59812: +<histogram_suffixes
name="WrenchMenuActionTimings">
I think using a dot as a separator is actually better here. To do that, specify
separator="." here and change the code back to .'s.

[edwardjung, 2014-11-17 19:18:56]

Addressed your comments. Please have another look.

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:414: base::TimeDelta now =
timer_.Elapsed();
On 2014/11/17 18:51:18, Alexei Svitkine wrote:
> Nit: Use a different name - |now| implies it's a time, rather than a duration.
> How about |duration| or |delta|?

Done.

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:428: if (!uma_action_recorded_)
On 2014/11/17 18:51:18, Alexei Svitkine wrote:
> Nit: {}'s because body is multiline. Same for others below.

This is actually single line. We still want to record the menu action for usage
levels, just not the timing. Should I add a line break for readability?

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:682: LIMIT_MENU_ACTION);
On 2014/11/17 18:51:18, Alexei Svitkine wrote:
> Nit: Align.

Done.

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.h (right):

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.h:189: base::ElapsedTimer
menu_opened_timer_;
On 2014/11/17 18:51:18, Alexei Svitkine wrote:
> Add a comment.

Done.

[edwardjung, 2014-11-17 19:18:57]

Addressed your comments. Please have another look.

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:414: base::TimeDelta now =
timer_.Elapsed();
On 2014/11/17 18:51:18, Alexei Svitkine wrote:
> Nit: Use a different name - |now| implies it's a time, rather than a duration.
> How about |duration| or |delta|?

Done.

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:428: if (!uma_action_recorded_)
On 2014/11/17 18:51:18, Alexei Svitkine wrote:
> Nit: {}'s because body is multiline. Same for others below.

This is actually single line. We still want to record the menu action for usage
levels, just not the timing. Should I add a line break for readability?

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:682: LIMIT_MENU_ACTION);
On 2014/11/17 18:51:18, Alexei Svitkine wrote:
> Nit: Align.

Done.

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.h (right):

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.h:189: base::ElapsedTimer
menu_opened_timer_;
On 2014/11/17 18:51:18, Alexei Svitkine wrote:
> Add a comment.

Done.

[Alexei Svitkine (slow), 2014-11-17 19:57:04]

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:428: if (!uma_action_recorded_)
On 2014/11/17 19:18:56, edwardjung wrote:
> On 2014/11/17 18:51:18, Alexei Svitkine wrote:
> > Nit: {}'s because body is multiline. Same for others below.
> 
> This is actually single line. We still want to record the menu action for
usage
> levels, just not the timing. Should I add a line break for readability?

The style guide is not about number of statements, but number of lines (i.e.
including wrapping). So since the UMA statement spans multiple lines due to
wrapping, the if should have {}'s.

[edwardjung, 2014-11-17 23:45:26]

Apologies Alexei, corrected the styling.

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/700673003/diff/140001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.cc:428: if (!uma_action_recorded_)
My bad, still getting used to the C++ style guide.

Corrected here and elsewhere.

[Alexei Svitkine (slow), 2014-11-18 00:49:50]

lgtm

[edwardjung, 2014-11-18 10:17:45]

The CQ bit was checked by edwardjung@chromium.org

[commit-bot: I haz the power, 2014-11-18 10:18:53]

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

[commit-bot: I haz the power, 2014-11-18 10:21:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-18 10:21:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[edwardjung, 2014-11-18 10:47:53]

The CQ bit was checked by edwardjung@chromium.org

[commit-bot: I haz the power, 2014-11-18 10:48:18]

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

[commit-bot: I haz the power, 2014-11-18 11:19:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-18 11:19:25]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[edwardjung, 2014-11-18 11:27:29]

The CQ bit was checked by edwardjung@chromium.org

[commit-bot: I haz the power, 2014-11-18 11:27:54]

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

[commit-bot: I haz the power, 2014-11-18 11:47:22]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-11-18 11:48:16]

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