|
|
Created:
9 years ago by rpetterson Modified:
9 years ago CC:
chromium-reviews, jar (doing other things) Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdding metrics to track browser launches per primary/secondary profile. Adding metrics to track number of profiles per machine.
BUG=160771, 106774
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114088
Patch Set 1 #Patch Set 2 : '' #
Total comments: 8
Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 12
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 1
Patch Set 7 : '' #
Total comments: 4
Messages
Total messages: 20 (0 generated)
isherman - metrics miranda - profiles sky - browser_init Thanks!
http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:390: ProfileMetrics::LogNumberOfProfiles(false); nit: Rather than using a bool, I think the style guide recommends creating an enumerate constant with descriptive values, so that callsites are easier to understand. http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_metrics.cc (right): http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.cc:225: number_of_profiles = kMaxNumberOfProfilesTracked; There shouldn't be any need to do this -- the last histogram bucket will also serve as an overflow bucket. http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.cc:229: NUM_PROFILE_TYPE_METRICS); I think you want UMA_HISTOGRAM_COUNTS_100 (or something like it), rather than UMA_HISTOGRAM_ENUMERATION here. http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.cc:231: UMA_HISTOGRAM_ENUMERATION("Profile.NumberOfProfilesOnAddDelete", ditto
http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:390: ProfileMetrics::LogNumberOfProfiles(false); On 2011/12/09 00:53:41, Ilya Sherman wrote: > nit: Rather than using a bool, I think the style guide recommends creating an > enumerate constant with descriptive values, so that callsites are easier to > understand. Done. http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_metrics.cc (right): http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.cc:225: number_of_profiles = kMaxNumberOfProfilesTracked; On 2011/12/09 00:53:41, Ilya Sherman wrote: > There shouldn't be any need to do this -- the last histogram bucket will also > serve as an overflow bucket. Cool. Done. http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.cc:229: NUM_PROFILE_TYPE_METRICS); On 2011/12/09 00:53:41, Ilya Sherman wrote: > I think you want UMA_HISTOGRAM_COUNTS_100 (or something like it), rather than > UMA_HISTOGRAM_ENUMERATION here. Done. http://codereview.chromium.org/8890054/diff/2002/chrome/browser/profiles/prof... chrome/browser/profiles/profile_metrics.cc:231: UMA_HISTOGRAM_ENUMERATION("Profile.NumberOfProfilesOnAddDelete", On 2011/12/09 00:53:41, Ilya Sherman wrote: > ditto Done.
Metrics LGTM (with one last comment addressed) http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_metrics.cc (right): http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.cc:218: ProfileManager* manager = g_browser_process->profile_manager(); I think accessing g_browser_process directly is discouraged, as there are various subtleties, like needing to be sure to only access it from the main thread. I think there's a wrapper accessor somewhere that's recommended to be used instead -- don't quite remember where that lives, though, atm.
http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_metrics.cc (right): http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.cc:225: UMA_HISTOGRAM_COUNTS_100("Profile.NumberOfProfilesOnAddDelete", Perhaps a better name here would be: NumberOfProfilesBeforeAddOrDelete http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_metrics.h (right): http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.h:64: enum ProfileTime { I'm confused by the name here. This has nothing to do with "Time" but yet your are naming it that way. Can you motivate the use of the word, or suggest a better name? Maybe the enum should be called ProfileEvents, or ProfileHandlingEvents. Maybe the enums should be something like: STARTUP_PROFILE_EVENT ADD_PROFILE_EVENT DELETE_PROFILE_EVENT
profiles LGTM (w/naming stuff addressed), thanks! http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_metrics.cc (right): http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.cc:218: ProfileManager* manager = g_browser_process->profile_manager(); On 2011/12/09 01:50:47, Ilya Sherman wrote: > I think accessing g_browser_process directly is discouraged, as there are > various subtleties, like needing to be sure to only access it from the main > thread. I think there's a wrapper accessor somewhere that's recommended to be > used instead -- don't quite remember where that lives, though, atm. Using g_b_p is definitely to be avoided (for threading reasons, and because it's often a thin veneer over a global variable pattern), but it's the standard way of grabbing the profile_manager through a static method -- I think it's fine here. You can DCHECK for the ui thread if there are threading concerns. http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_metrics.h (right): http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.h:64: enum ProfileTime { On 2011/12/09 04:57:13, jar wrote: > I'm confused by the name here. This has nothing to do with "Time" but yet your > are naming it that way. > > Can you motivate the use of the word, or suggest a better name? > > Maybe the enum should be called ProfileEvents, or ProfileHandlingEvents. > > Maybe the enums should be something like: > STARTUP_PROFILE_EVENT > ADD_PROFILE_EVENT > DELETE_PROFILE_EVENT Ditto -- I vote for "ProfileEvents", and the enum names jar listed here. http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.h:80: static void LogNumberOfProfiles(ProfileTime startup); Maybe this list should be in alpha-order so they're easy to find quickly, and because I have OCD? ;-)
http://codereview.chromium.org/8890054/diff/6/chrome/browser/ui/browser_init.cc File chrome/browser/ui/browser_init.cc (right): http://codereview.chromium.org/8890054/diff/6/chrome/browser/ui/browser_init.... chrome/browser/ui/browser_init.cc:585: ProfileMetrics::LogProfileLaunch(profile_path); Would this make more sense some where in the profilemanager when a new profile is created?
http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_metrics.cc (right): http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.cc:218: ProfileManager* manager = g_browser_process->profile_manager(); On 2011/12/09 14:01:02, Miranda Callahan wrote: > On 2011/12/09 01:50:47, Ilya Sherman wrote: > > I think accessing g_browser_process directly is discouraged, as there are > > various subtleties, like needing to be sure to only access it from the main > > thread. I think there's a wrapper accessor somewhere that's recommended to be > > used instead -- don't quite remember where that lives, though, atm. > > Using g_b_p is definitely to be avoided (for threading reasons, and because it's > often a thin veneer over a global variable pattern), but it's the standard way > of grabbing the profile_manager through a static method -- I think it's fine > here. You can DCHECK for the ui thread if there are threading concerns. Since this function was only called from the ProfileManager, I've fixed it to pass in the ProfileManager. GetProfileType is a bit harder to fix, but I've verified that all of th functions which use GetProfileType are only called on the UI thread and added a DCHECK to the function. I've also added a comment indicating such. http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.cc:225: UMA_HISTOGRAM_COUNTS_100("Profile.NumberOfProfilesOnAddDelete", On 2011/12/09 04:57:13, jar wrote: > Perhaps a better name here would be: > NumberOfProfilesBeforeAddOrDelete Changed to NumberOfProfilesAfterAddOrDelete (since the metric is logged at the end of the Add and Delete functions). http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... File chrome/browser/profiles/profile_metrics.h (right): http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.h:64: enum ProfileTime { On 2011/12/09 14:01:02, Miranda Callahan wrote: > On 2011/12/09 04:57:13, jar wrote: > > I'm confused by the name here. This has nothing to do with "Time" but yet > your > > are naming it that way. > > > > Can you motivate the use of the word, or suggest a better name? > > > > Maybe the enum should be called ProfileEvents, or ProfileHandlingEvents. > > > > Maybe the enums should be something like: > > STARTUP_PROFILE_EVENT > > ADD_PROFILE_EVENT > > DELETE_PROFILE_EVENT > > Ditto -- I vote for "ProfileEvents", and the enum names jar listed here. Yes, thanks, jar. These make a lot more sense are potentially more useful in the future as well. Fixed. http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... chrome/browser/profiles/profile_metrics.h:80: static void LogNumberOfProfiles(ProfileTime startup); On 2011/12/09 14:01:02, Miranda Callahan wrote: > Maybe this list should be in alpha-order so they're easy to find quickly, and > because I have OCD? ;-) :) All set. And rearranged to match in the .cc file so the definitions are easy to find as well. Except for the three FilePath functions which are pulled out for their comment. http://codereview.chromium.org/8890054/diff/6/chrome/browser/ui/browser_init.cc File chrome/browser/ui/browser_init.cc (right): http://codereview.chromium.org/8890054/diff/6/chrome/browser/ui/browser_init.... chrome/browser/ui/browser_init.cc:585: ProfileMetrics::LogProfileLaunch(profile_path); On 2011/12/09 17:46:38, sky wrote: > Would this make more sense some where in the profilemanager when a new profile > is created? In that case it will only get called once per browser session. We'd like this to be called each time a window is opened with a profile and to log which profile that window was opened with to have a better understanding of the activity per profile type.
On Fri, Dec 9, 2011 at 1:22 PM, <rlp@chromium.org> wrote: > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > File chrome/browser/profiles/profile_metrics.cc (right): > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > chrome/browser/profiles/profile_metrics.cc:218: ProfileManager* manager > = g_browser_process->profile_manager(); > On 2011/12/09 14:01:02, Miranda Callahan wrote: >> >> On 2011/12/09 01:50:47, Ilya Sherman wrote: >> > I think accessing g_browser_process directly is discouraged, as > > there are >> >> > various subtleties, like needing to be sure to only access it from > > the main >> >> > thread. I think there's a wrapper accessor somewhere that's > > recommended to be >> >> > used instead -- don't quite remember where that lives, though, atm. > > >> Using g_b_p is definitely to be avoided (for threading reasons, and > > because it's >> >> often a thin veneer over a global variable pattern), but it's the > > standard way >> >> of grabbing the profile_manager through a static method -- I think > > it's fine >> >> here. You can DCHECK for the ui thread if there are threading > > concerns. > > Since this function was only called from the ProfileManager, I've fixed > it to pass in the ProfileManager. GetProfileType is a bit harder to > fix, but I've verified that all of th functions which use GetProfileType > are only called on the UI thread and added a DCHECK to the function. > I've also added a comment indicating such. > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > chrome/browser/profiles/profile_metrics.cc:225: > UMA_HISTOGRAM_COUNTS_100("Profile.NumberOfProfilesOnAddDelete", > On 2011/12/09 04:57:13, jar wrote: >> >> Perhaps a better name here would be: >> NumberOfProfilesBeforeAddOrDelete > > > Changed to NumberOfProfilesAfterAddOrDelete (since the metric is logged > at the end of the Add and Delete functions). > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > File chrome/browser/profiles/profile_metrics.h (right): > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > chrome/browser/profiles/profile_metrics.h:64: enum ProfileTime { > On 2011/12/09 14:01:02, Miranda Callahan wrote: >> >> On 2011/12/09 04:57:13, jar wrote: >> > I'm confused by the name here. This has nothing to do with "Time" > > but yet >> >> your >> > are naming it that way. >> > >> > Can you motivate the use of the word, or suggest a better name? >> > >> > Maybe the enum should be called ProfileEvents, or > > ProfileHandlingEvents. >> >> > >> > Maybe the enums should be something like: >> > STARTUP_PROFILE_EVENT >> > ADD_PROFILE_EVENT >> > DELETE_PROFILE_EVENT > > >> Ditto -- I vote for "ProfileEvents", and the enum names jar listed > > here. > > Yes, thanks, jar. These make a lot more sense are potentially more > useful in the future as well. Fixed. > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > chrome/browser/profiles/profile_metrics.h:80: static void > LogNumberOfProfiles(ProfileTime startup); > On 2011/12/09 14:01:02, Miranda Callahan wrote: >> >> Maybe this list should be in alpha-order so they're easy to find > > quickly, and >> >> because I have OCD? ;-) > > > :) All set. And rearranged to match in the .cc file so the definitions > are easy to find as well. Except for the three FilePath functions which > are pulled out for their comment. > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/ui/browser_init.cc > File chrome/browser/ui/browser_init.cc (right): > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/ui/browser_init.... > chrome/browser/ui/browser_init.cc:585: > ProfileMetrics::LogProfileLaunch(profile_path); > On 2011/12/09 17:46:38, sky wrote: >> >> Would this make more sense some where in the profilemanager when a new > > profile >> >> is created? > > > In that case it will only get called once per browser session. We'd like > this to be called each time a window is opened with a profile and to log > which profile that window was opened with to have a better understanding > of the activity per profile type. > > http://codereview.chromium.org/8890054/ If that's the case, I think you want this code close to when a new browser is created. -Scott
On 2011/12/09 22:11:40, sky wrote: > On Fri, Dec 9, 2011 at 1:22 PM, <mailto:rlp@chromium.org> wrote: > > > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > > File chrome/browser/profiles/profile_metrics.cc (right): > > > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > > chrome/browser/profiles/profile_metrics.cc:218: ProfileManager* manager > > = g_browser_process->profile_manager(); > > On 2011/12/09 14:01:02, Miranda Callahan wrote: > >> > >> On 2011/12/09 01:50:47, Ilya Sherman wrote: > >> > I think accessing g_browser_process directly is discouraged, as > > > > there are > >> > >> > various subtleties, like needing to be sure to only access it from > > > > the main > >> > >> > thread. I think there's a wrapper accessor somewhere that's > > > > recommended to be > >> > >> > used instead -- don't quite remember where that lives, though, atm. > > > > > >> Using g_b_p is definitely to be avoided (for threading reasons, and > > > > because it's > >> > >> often a thin veneer over a global variable pattern), but it's the > > > > standard way > >> > >> of grabbing the profile_manager through a static method -- I think > > > > it's fine > >> > >> here. You can DCHECK for the ui thread if there are threading > > > > concerns. > > > > Since this function was only called from the ProfileManager, I've fixed > > it to pass in the ProfileManager. GetProfileType is a bit harder to > > fix, but I've verified that all of th functions which use GetProfileType > > are only called on the UI thread and added a DCHECK to the function. > > I've also added a comment indicating such. > > > > > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > > chrome/browser/profiles/profile_metrics.cc:225: > > UMA_HISTOGRAM_COUNTS_100("Profile.NumberOfProfilesOnAddDelete", > > On 2011/12/09 04:57:13, jar wrote: > >> > >> Perhaps a better name here would be: > >> NumberOfProfilesBeforeAddOrDelete > > > > > > Changed to NumberOfProfilesAfterAddOrDelete (since the metric is logged > > at the end of the Add and Delete functions). > > > > > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > > File chrome/browser/profiles/profile_metrics.h (right): > > > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > > chrome/browser/profiles/profile_metrics.h:64: enum ProfileTime { > > On 2011/12/09 14:01:02, Miranda Callahan wrote: > >> > >> On 2011/12/09 04:57:13, jar wrote: > >> > I'm confused by the name here. This has nothing to do with "Time" > > > > but yet > >> > >> your > >> > are naming it that way. > >> > > >> > Can you motivate the use of the word, or suggest a better name? > >> > > >> > Maybe the enum should be called ProfileEvents, or > > > > ProfileHandlingEvents. > >> > >> > > >> > Maybe the enums should be something like: > >> > STARTUP_PROFILE_EVENT > >> > ADD_PROFILE_EVENT > >> > DELETE_PROFILE_EVENT > > > > > >> Ditto -- I vote for "ProfileEvents", and the enum names jar listed > > > > here. > > > > Yes, thanks, jar. These make a lot more sense are potentially more > > useful in the future as well. Fixed. > > > > > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/profiles/profile... > > chrome/browser/profiles/profile_metrics.h:80: static void > > LogNumberOfProfiles(ProfileTime startup); > > On 2011/12/09 14:01:02, Miranda Callahan wrote: > >> > >> Maybe this list should be in alpha-order so they're easy to find > > > > quickly, and > >> > >> because I have OCD? ;-) > > > > > > :) All set. And rearranged to match in the .cc file so the definitions > > are easy to find as well. Except for the three FilePath functions which > > are pulled out for their comment. > > > > > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/ui/browser_init.cc > > File chrome/browser/ui/browser_init.cc (right): > > > > > http://codereview.chromium.org/8890054/diff/6/chrome/browser/ui/browser_init.... > > chrome/browser/ui/browser_init.cc:585: > > ProfileMetrics::LogProfileLaunch(profile_path); > > On 2011/12/09 17:46:38, sky wrote: > >> > >> Would this make more sense some where in the profilemanager when a new > > > > profile > >> > >> is created? > > > > > > In that case it will only get called once per browser session. We'd like > > this to be called each time a window is opened with a profile and to log > > which profile that window was opened with to have a better understanding > > of the activity per profile type. > > > > http://codereview.chromium.org/8890054/ > > If that's the case, I think you want this code close to when a new > browser is created. > > -Scott You're right. I think what I want is ProfileManager::NewWindowWithProfile. Thanks!
http://codereview.chromium.org/8890054/diff/9004/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/8890054/diff/9004/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:424: ProfileMetrics::LogProfileLaunch(profile_path); I believe NewWindowWithProfile is not invoked for every browser created. If you want to track creation of every browser you want it in browser's constructor.
Fair enough. That will certainly catch everything.
LGTM, but I'm not sure if you only care about type_ == TYPE_TABBED
On 2011/12/09 23:53:41, sky wrote: > LGTM, but I'm not sure if you only care about type_ == TYPE_TABBED For now, we care about everything as a measure of activity.
Ping to jar@.
http://codereview.chromium.org/8890054/diff/10007/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metrics.cc (right): http://codereview.chromium.org/8890054/diff/10007/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metrics.cc:76: UMA_HISTOGRAM_COUNTS_100("Profile.NumberOfProfilesAfterAddOrDelete", I'm curious, do you really expect the distribution to be much different at startup than "after add or delete?" In fact, I'd mostly expect the startup count would be the only number you'd care much about. It looks a bit like you're getting numerous histograms, and I wonder if you've thought through which ones you want, and intend to look at? Histograms are light weight... but nothing in life is free. http://codereview.chromium.org/8890054/diff/10007/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metrics.h (right): http://codereview.chromium.org/8890054/diff/10007/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metrics.h:82: // into g_browser_process through a helper function. If it was worth commenting... it is probably worth adding a DCHECK() in the method as well.
http://codereview.chromium.org/8890054/diff/10007/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metrics.cc (right): http://codereview.chromium.org/8890054/diff/10007/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metrics.cc:76: UMA_HISTOGRAM_COUNTS_100("Profile.NumberOfProfilesAfterAddOrDelete", On 2011/12/10 22:07:44, jar wrote: > I'm curious, do you really expect the distribution to be much different at > startup than "after add or delete?" > > In fact, I'd mostly expect the startup count would be the only number you'd care > much about. > > It looks a bit like you're getting numerous histograms, and I wonder if you've > thought through which ones you want, and intend to look at? > > Histograms are light weight... but nothing in life is free. > Because this is a stat which changes during usage, Ilya recommended doing two histograms at different times to sanity check one another. If you think this is unnecessary, I'm content going down to just startup and adding another later if it seems necessary. http://codereview.chromium.org/8890054/diff/10007/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_metrics.h (right): http://codereview.chromium.org/8890054/diff/10007/chrome/browser/profiles/pro... chrome/browser/profiles/profile_metrics.h:82: // into g_browser_process through a helper function. On 2011/12/10 22:07:44, jar wrote: > If it was worth commenting... it is probably worth adding a DCHECK() in the > method as well. There is already one there. See line 20 of profile_metrics.cc.
The almost redundant histogram is a little much, but at least borderline understandable. I suspect that soon, we'll need to make a pass though the browser as a "fixit" to prune histograms... and one of these might be dropped. For now... LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rlp@chromium.org/8890054/10007
Change committed as 114088 |