Add UMA metrics for profile switching.

Add UMA metrics for profile switching.

This changes adds four histograms to UMA. Profile.NumberofSwitches
collects the number of profile switches happening over a session.

The others are:

- Profile.ProfileSwitch.ToOpenedProfile counts the number of switches to
  opened profiles and buckets depending on the UI flow used to switch.
- Profile.ProfileSwitch.ToUnopenedProfile does the same but for switches
  to unopened profiles
- Profile.ProfileSwitch aggregates the data from the previous two

BUG=448982
Committed: https://crrev.com/34f3304e9f3c0ad31cfc546be7f77f1dbf26a619
Cr-Commit-Position: refs/heads/master@{#313951}

[anthonyvd, 2015-01-21 19:28:21]

anthonyvd@chromium.org changed reviewers:
+ jwd@chromium.org, mlerman@chromium.org, sky@chromium.org

[anthonyvd, 2015-01-21 19:28:21]

Hi,

This changelist adds some UMA metrics related to profile switching.

jwd@chromium.org: histograms.xml
mlerman@chromium.org: c/b/profiles/
sky@chromium.org: c/b/ui/ and c/b/browser_shutdown.cc

Thanks a lot everyone!

[Mike Lerman, 2015-01-21 20:58:03]

Great start on this, Anthony!

We may also want to think about tracking how often the profile-directory flag is
used to open a profile. Doesn't have to be in this CL, but if you think it fits
you can add it here.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/browser_shutd...
File chrome/browser/browser_shutdown.cc (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/browser_shutd...
chrome/browser/browser_shutdown.cc:157: //Log the amount of times the user
switched profiles during this session
nit: space after // and before Log
nit: end sentence with a period.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:25: size_t number_of_switches_ = 0;
call this "number_of_profile_switches_" please.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:169: ++number_of_switches_;
I don't think this needs to be a separate method; please inline within
LogProfileSwitch().

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:173:
UMA_HISTOGRAM_COUNTS_100("Profile.NumberofSwitches", number_of_switches_);
nit: NumberOfSwitches (capital O)

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:332:
UMA_HISTOGRAM_ENUMERATION("Profile.ProfileSwitch",
We really try not to rename histograms - doing so loses all the history
associated with them. One needs a really compelling reason to do the switch,
which I don't think is present here. Please still call it Profile.OpenMethod.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:210: static void
IncrementNumberOfSwitches();
IncrementNumberOfSwitches is only ever called locally - doesn't need to be in
the header file.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:211: static void
LogNumberOfSwitches();
Please specify - either call it LogNumberOfProfileSwitches or
LogProfileNumberOfSwitches.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_window.cc:274:
g_browser_process->profile_manager()->GetProfileByPath(path) != NULL;
Once a profile has been opened by the ProfileManager, the object stays in memory
for the lifetime of the Chrome Process, even if all the browser windows are
closed. However, we're interested in the user workflows - so if a Profile's been
loaded once but all browsers are currently closed, that's interesting and
distinct from the scenario where a Profile's been loaded and there's at least
one browser open. I'd track each state separately: Profile's not yet loaded,
Profile's been loaded and there are no browsers open, Profile's been loaded and
there are browsers open.

For a hint on counting browsers:  BrowserList::CloseAllBrowsersWithProfile()

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_window.cc:290: base::FilePath path =
ProfileManager::GetGuestProfilePath();
const base::FilePath&

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_window.cc:292:
g_browser_process->profile_manager()->GetProfileByPath(path) != NULL;
(Same as above)

https://codereview.chromium.org/844193005/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (left):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:389:
ProfileMetrics::LogProfileSwitchUser(ProfileMetrics::SWITCH_PROFILE_GUEST);
Why did you remove this? It's definitely interesting to know how often the Guest
Profile was opened.

https://codereview.chromium.org/844193005/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/844193005/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:27863: +    Counts the number of times
profiles where switched in a browser session.
nit: s/where/were

Also, mention when this was logged (e.g. during shutdown).

[anthonyvd, 2015-01-21 21:12:19]

Thanks Michael!  

I clarified a couple points, let me know if it makes sense.  I'll get right on
the other stuff.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:332:
UMA_HISTOGRAM_ENUMERATION("Profile.ProfileSwitch",
On 2015/01/21 20:58:02, Mike Lerman wrote:
> We really try not to rename histograms - doing so loses all the history
> associated with them. One needs a really compelling reason to do the switch,
> which I don't think is present here. Please still call it Profile.OpenMethod.

So I preserved the Profile.OpenMethod histogram and its behavior.  However, it
logged switch methods as well as users opening profile related UI elements
(which we're not interested in in this particular case).  This method logs the
actual switch in Profile.ProfileSwitch then calls LogProfileOpenMethod (line
339) to preserve the other histogram's behavior.  Does that make sense?

https://codereview.chromium.org/844193005/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (left):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:389:
ProfileMetrics::LogProfileSwitchUser(ProfileMetrics::SWITCH_PROFILE_GUEST);
On 2015/01/21 20:58:03, Mike Lerman wrote:
> Why did you remove this? It's definitely interesting to know how often the
Guest
> Profile was opened.

We ended up logging it twice.  Once here and once in
profiles::SwitchToGuestProfile so I removed it here and all code paths that go
through SwitchToGuestProfile log it.

[Mike Lerman, 2015-01-21 21:24:18]

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:332:
UMA_HISTOGRAM_ENUMERATION("Profile.ProfileSwitch",
On 2015/01/21 21:12:18, anthonyvd wrote:
> On 2015/01/21 20:58:02, Mike Lerman wrote:
> > We really try not to rename histograms - doing so loses all the history
> > associated with them. One needs a really compelling reason to do the switch,
> > which I don't think is present here. Please still call it
Profile.OpenMethod.
> 
> So I preserved the Profile.OpenMethod histogram and its behavior.  However, it
> logged switch methods as well as users opening profile related UI elements
> (which we're not interested in in this particular case).  This method logs the
> actual switch in Profile.ProfileSwitch then calls LogProfileOpenMethod (line
> 339) to preserve the other histogram's behavior.  Does that make sense?

Ah, I understand.
I'm leery of duplicating the data needlessly. I feel like we can keep the data
and still get the information we need from the dashboards, and just ignoring the
fields we don't need.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:338: //as well as opening of profile
related UI elements
nit: Add spaces after the //'s, and end it all with a period. Also, don't wrap
text to the next line until you have to. I think some of the words from the
second line fit on the first.

https://codereview.chromium.org/844193005/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/844193005/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:62124: +      label="The profile being
switched to wasn't already opened"/>
Do you need to explicitly declare a no-suffix histogram? I'm not sure if one
will be created otherwise.

[anthonyvd, 2015-01-21 23:31:19]

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

https://codereview.chromium.org/844193005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:62109: +  <affected-histogram
name="Profile.OpenMethod"/>
jwd@ can you confirm if suffixing an existing histogram is OK w.r.t preserving
its existing history?  Thanks!

[jwd, 2015-01-22 21:11:33]

lgtm

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

https://codereview.chromium.org/844193005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:62109: +  <affected-histogram
name="Profile.OpenMethod"/>
On 2015/01/21 23:31:19, anthonyvd wrote:
> jwd@ can you confirm if suffixing an existing histogram is OK w.r.t preserving
> its existing history?  Thanks! 

Yes, that should be fine.

[anthonyvd, 2015-01-23 16:01:31]

https://codereview.chromium.org/844193005/diff/1/chrome/browser/browser_shutd...
File chrome/browser/browser_shutdown.cc (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/browser_shutd...
chrome/browser/browser_shutdown.cc:157: //Log the amount of times the user
switched profiles during this session
On 2015/01/21 20:58:02, Mike Lerman wrote:
> nit: space after // and before Log
> nit: end sentence with a period.

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:25: size_t number_of_switches_ = 0;
On 2015/01/21 20:58:02, Mike Lerman wrote:
> call this "number_of_profile_switches_" please.

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:169: ++number_of_switches_;
On 2015/01/21 20:58:03, Mike Lerman wrote:
> I don't think this needs to be a separate method; please inline within
> LogProfileSwitch().

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:173:
UMA_HISTOGRAM_COUNTS_100("Profile.NumberofSwitches", number_of_switches_);
On 2015/01/21 20:58:03, Mike Lerman wrote:
> nit: NumberOfSwitches (capital O)

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:332:
UMA_HISTOGRAM_ENUMERATION("Profile.ProfileSwitch",
On 2015/01/21 21:24:18, Mike Lerman wrote:
> On 2015/01/21 21:12:18, anthonyvd wrote:
> > On 2015/01/21 20:58:02, Mike Lerman wrote:
> > > We really try not to rename histograms - doing so loses all the history
> > > associated with them. One needs a really compelling reason to do the
switch,
> > > which I don't think is present here. Please still call it
> Profile.OpenMethod.
> > 
> > So I preserved the Profile.OpenMethod histogram and its behavior.  However,
it
> > logged switch methods as well as users opening profile related UI elements
> > (which we're not interested in in this particular case).  This method logs
the
> > actual switch in Profile.ProfileSwitch then calls LogProfileOpenMethod (line
> > 339) to preserve the other histogram's behavior.  Does that make sense?
> 
> Ah, I understand.
> I'm leery of duplicating the data needlessly. I feel like we can keep the data
> and still get the information we need from the dashboards, and just ignoring
the
> fields we don't need.

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:338: //as well as opening of profile
related UI elements
On 2015/01/21 21:24:18, Mike Lerman wrote:
> nit: Add spaces after the //'s, and end it all with a period. Also, don't wrap
> text to the next line until you have to. I think some of the words from the
> second line fit on the first.

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:210: static void
IncrementNumberOfSwitches();
On 2015/01/21 20:58:03, Mike Lerman wrote:
> IncrementNumberOfSwitches is only ever called locally - doesn't need to be in
> the header file.

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:211: static void
LogNumberOfSwitches();
On 2015/01/21 20:58:03, Mike Lerman wrote:
> Please specify - either call it LogNumberOfProfileSwitches or
> LogProfileNumberOfSwitches.

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_window.cc:274:
g_browser_process->profile_manager()->GetProfileByPath(path) != NULL;
On 2015/01/21 20:58:03, Mike Lerman wrote:
> Once a profile has been opened by the ProfileManager, the object stays in
memory
> for the lifetime of the Chrome Process, even if all the browser windows are
> closed. However, we're interested in the user workflows - so if a Profile's
been
> loaded once but all browsers are currently closed, that's interesting and
> distinct from the scenario where a Profile's been loaded and there's at least
> one browser open. I'd track each state separately: Profile's not yet loaded,
> Profile's been loaded and there are no browsers open, Profile's been loaded
and
> there are browsers open.
> 
> For a hint on counting browsers:  BrowserList::CloseAllBrowsersWithProfile()
> 

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_window.cc:290: base::FilePath path =
ProfileManager::GetGuestProfilePath();
On 2015/01/21 20:58:03, Mike Lerman wrote:
> const base::FilePath&

Done.

https://codereview.chromium.org/844193005/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_window.cc:292:
g_browser_process->profile_manager()->GetProfileByPath(path) != NULL;
On 2015/01/21 20:58:03, Mike Lerman wrote:
> (Same as above)

Done.

[Mike Lerman, 2015-01-23 19:46:14]

Coming along nicely!

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:165: profile_open_state =
PROFILE_OPENED;
Perhaps you can just return PROFILE_OPENED; no need to create a local
ProfileOpenState instance?

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:170: return profile_open_state;
Flip your condition around, and return early. So what I'd so is:

Profile* switched_to = manager->GetProfileByPath()
// If the ProfileManager doesn't know the Profile, it's never been opened.
if (null) return PROFILE_UNOPENED;

<rest of function>
...

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.h:219: static ProfileOpenState
GetProfileOpenState(ProfileManager* manager,
GetProfileOpenState isn't called outside of profile_metrics and is defined in
the anonymous namespace. You can remove it from the header file.

I think the ProfileOpenState enum can also be removed from the header file, and
declared in the .cc's anonymous namespace.

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

https://codereview.chromium.org/844193005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:62103: +      label="The profile being
switched to was already opened"/>
I'd explicitly say "The profile being switched to was already loaded and had at
least one open browser".

https://codereview.chromium.org/844193005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:62105: +      label="The profile being
switched to wasn't already opened"/>
A lot of people confuse what a profile being open and what a browser being open
are. I'd write:
"The profile being switched to had not yet been loaded this Chrome session".

[anthonyvd, 2015-01-26 16:07:48]

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:165: profile_open_state =
PROFILE_OPENED;
On 2015/01/23 19:46:14, Mike Lerman wrote:
> Perhaps you can just return PROFILE_OPENED; no need to create a local
> ProfileOpenState instance?

Done.

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:170: return profile_open_state;
On 2015/01/23 19:46:14, Mike Lerman wrote:
> Flip your condition around, and return early. So what I'd so is:
> 
> Profile* switched_to = manager->GetProfileByPath()
> // If the ProfileManager doesn't know the Profile, it's never been opened.
> if (null) return PROFILE_UNOPENED;
> 
> <rest of function>
> ...
> 

Done.

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.h (right):

https://codereview.chromium.org/844193005/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.h:219: static ProfileOpenState
GetProfileOpenState(ProfileManager* manager,
On 2015/01/23 19:46:14, Mike Lerman wrote:
> GetProfileOpenState isn't called outside of profile_metrics and is defined in
> the anonymous namespace. You can remove it from the header file.
> 
> I think the ProfileOpenState enum can also be removed from the header file,
and
> declared in the .cc's anonymous namespace.

Done.

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

https://codereview.chromium.org/844193005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:62103: +      label="The profile being
switched to was already opened"/>
On 2015/01/23 19:46:14, Mike Lerman wrote:
> I'd explicitly say "The profile being switched to was already loaded and had
at
> least one open browser".

Done.

https://codereview.chromium.org/844193005/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:62105: +      label="The profile being
switched to wasn't already opened"/>
On 2015/01/23 19:46:14, Mike Lerman wrote:
> A lot of people confuse what a profile being open and what a browser being
open
> are. I'd write:
> "The profile being switched to had not yet been loaded this Chrome session".

Done.

[Mike Lerman, 2015-01-27 15:28:26]

lgtm. Good work!

https://codereview.chromium.org/844193005/diff/40001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_metrics.cc (right):

https://codereview.chromium.org/844193005/diff/40001/chrome/browser/profiles/...
chrome/browser/profiles/profile_metrics.cc:15: #include
"chrome/browser/ui/browser.h"
You can remove browser.h.

[sky, 2015-01-29 17:59:15]

LGTM

[anthonyvd, 2015-01-30 18:30:18]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2015-01-30 18:31:16]

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

[commit-bot: I haz the power, 2015-01-30 18:34:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-30 18:35:58]

Patchset 5 (id:??) landed as
https://crrev.com/34f3304e9f3c0ad31cfc546be7f77f1dbf26a619
Cr-Commit-Position: refs/heads/master@{#313951}