Log UMA metrics for multiprofile actions.

Log UMA metrics for multiprofile actions.

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

[Tim Song, 2013-09-28 00:18:01]

+xiyuan@ for chrome/browser/chromeos/
+oshima@ for ash/
+isherman@ for histograms.xml

[Ilya Sherman, 2013-09-28 00:29:35]

https://codereview.chromium.org/25098009/diff/6001/ash/multi_profile_uma.cc
File ash/multi_profile_uma.cc (right):

https://codereview.chromium.org/25098009/diff/6001/ash/multi_profile_uma.cc#n...
ash/multi_profile_uma.cc:42: NUM_SWITCH_ACTIVE_USER_ACTIONS);
Of lines 40-42, lines 40 and 42 are identical to the ones on lines 33-35.  This
means that it's easy for the code to drift, so that you update one
implementation but forget to update the other.  I'd strongly recommend having
just one UMA_HISTOGRAM_ENUMERATION macro per histogram name.

https://codereview.chromium.org/25098009/diff/6001/ash/multi_profile_uma.h
File ash/multi_profile_uma.h (right):

https://codereview.chromium.org/25098009/diff/6001/ash/multi_profile_uma.h#ne...
ash/multi_profile_uma.h:21: static void RecordSigninUserFromBrowserFrame();
nit: Please document these methods.

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

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:6585: +<histogram
name="MultiProfile.SigninUser" enum="SigninUserAction">
nit: I'd suggest naming this histogram something more like
"MultiProfile.SignInAccessPoint".

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:6588: +    for each UI flow (eg. system
tray, browser frame).
nit: I'd rephrase this description to something a little clearer.  One
suggestion, that you should feel free to improve upon: "The frequency with which
each UI access point is used for signing into a multiprofile session on
ChromeOS.  Access points include the system tray and the user account switcher
on the browser frame."

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:6592: +<histogram
name="MultiProfile.SwitchActiveUser" enum="SwitchActiveUserAction">
nit: Since the enums are highly related, it would be nice if they shared a
common prefix.

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:6602: +    ChromeOS.
Please document when this will be recorded.

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:25533: +  <int value="1"
label="SIGNIN_USER_BY_BROWSER_FRAME"/>
These labels are human-readable strings, so it would be nice to format them to
be more human-friendly.  Suggestions: "System tray", "Browser frame"

[Tim Song, 2013-09-28 01:35:12]

https://codereview.chromium.org/25098009/diff/6001/ash/multi_profile_uma.cc
File ash/multi_profile_uma.cc (right):

https://codereview.chromium.org/25098009/diff/6001/ash/multi_profile_uma.cc#n...
ash/multi_profile_uma.cc:42: NUM_SWITCH_ACTIVE_USER_ACTIONS);
On 2013/09/28 00:29:35, Ilya Sherman wrote:
> Of lines 40-42, lines 40 and 42 are identical to the ones on lines 33-35. 
This
> means that it's easy for the code to drift, so that you update one
> implementation but forget to update the other.  I'd strongly recommend having
> just one UMA_HISTOGRAM_ENUMERATION macro per histogram name.

Done.

https://codereview.chromium.org/25098009/diff/6001/ash/multi_profile_uma.h
File ash/multi_profile_uma.h (right):

https://codereview.chromium.org/25098009/diff/6001/ash/multi_profile_uma.h#ne...
ash/multi_profile_uma.h:21: static void RecordSigninUserFromBrowserFrame();
On 2013/09/28 00:29:35, Ilya Sherman wrote:
> nit: Please document these methods.

Done.

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

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:6585: +<histogram
name="MultiProfile.SigninUser" enum="SigninUserAction">
On 2013/09/28 00:29:35, Ilya Sherman wrote:
> nit: I'd suggest naming this histogram something more like
> "MultiProfile.SignInAccessPoint".

Done. I think SigninUserUIPath should better describe what we're recording.

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:6588: +    for each UI flow (eg. system
tray, browser frame).
On 2013/09/28 00:29:35, Ilya Sherman wrote:
> nit: I'd rephrase this description to something a little clearer.  One
> suggestion, that you should feel free to improve upon: "The frequency with
which
> each UI access point is used for signing into a multiprofile session on
> ChromeOS.  Access points include the system tray and the user account switcher
> on the browser frame."

Done.

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:6592: +<histogram
name="MultiProfile.SwitchActiveUser" enum="SwitchActiveUserAction">
On 2013/09/28 00:29:35, Ilya Sherman wrote:
> nit: Since the enums are highly related, it would be nice if they shared a
> common prefix.

Done.

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:6602: +    ChromeOS.
On 2013/09/28 00:29:35, Ilya Sherman wrote:
> Please document when this will be recorded.

Done.

https://codereview.chromium.org/25098009/diff/6001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:25533: +  <int value="1"
label="SIGNIN_USER_BY_BROWSER_FRAME"/>
On 2013/09/28 00:29:35, Ilya Sherman wrote:
> These labels are human-readable strings, so it would be nice to format them to
> be more human-friendly.  Suggestions: "System tray", "Browser frame"

Done.

[Ilya Sherman, 2013-09-28 03:16:37]

histograms lgtm, thanks.

[xiyuan, 2013-09-28 03:24:59]

c/b/chromeos/* LGTM

[oshima, 2013-09-28 11:45:19]

https://codereview.chromium.org/25098009/diff/15001/ash/multi_profile_uma.cc
File ash/multi_profile_uma.cc (right):

https://codereview.chromium.org/25098009/diff/15001/ash/multi_profile_uma.cc#...
ash/multi_profile_uma.cc:12: }  // namespace
remove empty namespace block?

https://codereview.chromium.org/25098009/diff/15001/ash/multi_profile_uma.h
File ash/multi_profile_uma.h (right):

https://codereview.chromium.org/25098009/diff/15001/ash/multi_profile_uma.h#n...
ash/multi_profile_uma.h:41: DISALLOW_COPY_AND_ASSIGN(MultiProfileUMA);
DISALLOW_IMPLICIT_CONSTRUCTORS

https://codereview.chromium.org/25098009/diff/15001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/25098009/diff/15001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:415:
GetUsersAdmittedForMultiProfile().size());
shouldn't this be in MultiProfileUMA too?

[xiyuan, 2013-09-28 14:50:25]

https://codereview.chromium.org/25098009/diff/15001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/25098009/diff/15001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:415:
GetUsersAdmittedForMultiProfile().size());
GetUsersAdmittedForMultiProfile() returns the users that are allowed to sign
into the current session. Just checked the histograms.xml, it seems
MultiProfile.UserCount wants the # of users in the current session. In this
case, we should use GetLoggedInUsers() here.

[Tim Song, 2013-09-30 17:19:39]

https://codereview.chromium.org/25098009/diff/15001/ash/multi_profile_uma.cc
File ash/multi_profile_uma.cc (right):

https://codereview.chromium.org/25098009/diff/15001/ash/multi_profile_uma.cc#...
ash/multi_profile_uma.cc:12: }  // namespace
On 2013/09/28 11:45:19, oshima wrote:
> remove empty namespace block?

Done.

https://codereview.chromium.org/25098009/diff/15001/ash/multi_profile_uma.h
File ash/multi_profile_uma.h (right):

https://codereview.chromium.org/25098009/diff/15001/ash/multi_profile_uma.h#n...
ash/multi_profile_uma.h:41: DISALLOW_COPY_AND_ASSIGN(MultiProfileUMA);
On 2013/09/28 11:45:19, oshima wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS

Done.

https://codereview.chromium.org/25098009/diff/15001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/25098009/diff/15001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:415:
GetUsersAdmittedForMultiProfile().size());
Very nice catch Xiyuan. I misunderstood what GetUsersAdmittedForMultiProfile()
did, and GetLoggedInUsers() is what we want.

It also seems odd to put this in an ash/ file as this isn't really a UI metric.

[oshima, 2013-09-30 22:36:47]

lgtm

[commit-bot: I haz the power, 2013-10-01 22:34:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/25098009/33001

[commit-bot: I haz the power, 2013-10-01 23:39:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-02 00:05:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/25098009/33001

[commit-bot: I haz the power, 2013-10-02 00:38:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-02 02:34:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/25098009/33001

[commit-bot: I haz the power, 2013-10-02 03:40:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-02 18:13:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/25098009/33001

[commit-bot: I haz the power, 2013-10-02 19:41:47]

Change committed as 226527