Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5287003:
Add user metrics for the login screen. (Closed)
|
Created:
10 years ago by satorux1 Modified:
9 years, 7 months ago CC:
chromium-reviews, pam+watch_chromium.org, nkostylev+cc_chromium.org, davemoore+watch_chromium.org, ben+cc_chromium.org Visibility:
Public. |
DescriptionAdd user metrics for the login screen.
Add user metrics for the following login actions:
1. login success
2. login failure
3. off the record login success (guest mode).
Update chromeactions.txt by:
% cd chrome/tools
% PYTHONPATH=../../tools/python python extract_actions.py --hash
TEST=login screen worked as before.
BUG=chromium-os:8382
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68981
Patch Set 1 #
Total comments: 2
Patch Set 2 : address comments #Patch Set 3 : rename actions #Patch Set 4 : update chromeactions.txt #
Total comments: 3
Patch Set 5 : use histograms #Patch Set 6 : update chromeactions.txt #
Total comments: 5
Patch Set 7 : address comments #
Total comments: 5
Messages
Total messages: 25 (0 generated)
LGTM though I'd like Nikita to review it as well (I've discussed this with him too).
Ouch, this is wrong file! login_screen.cc is only used in tests. Please, see login_performer.cc as to where you should insert these records.
On 2010/11/29 10:51:44, whywhat wrote: > Ouch, this is wrong file! > login_screen.cc is only used in tests. > Please, see login_performer.cc as to where you should insert these records. Yes, metrics should be tracked in LoginPerformer::OnLoginSuccess(...) It makes sense to track metric based on pending_requests. Logic here is simple: pending_requests is false - returns status based on offline & online auth pending_requests is true - returns status based on offline auth only, online auth is pending.
http://codereview.chromium.org/5287003/diff/1/chrome/browser/chromeos/login/l... File chrome/browser/chromeos/login/login_screen.cc (right): http://codereview.chromium.org/5287003/diff/1/chrome/browser/chromeos/login/l... chrome/browser/chromeos/login/login_screen.cc:80: UserMetrics::RecordAction(UserMetricsAction("LoginScreen_LoginFailure")); I think it's better to track error codes here as well. See histogram Net.ErrorCodesForMainFrame for instance.
I'm very glad to ask you guys a code review. I didn't know I was touching a wrong file. :) http://codereview.chromium.org/5287003/diff/1/chrome/browser/chromeos/login/l... File chrome/browser/chromeos/login/login_screen.cc (right): http://codereview.chromium.org/5287003/diff/1/chrome/browser/chromeos/login/l... chrome/browser/chromeos/login/login_screen.cc:80: UserMetrics::RecordAction(UserMetricsAction("LoginScreen_LoginFailure")); On 2010/11/29 10:59:06, Nikita Kostylev wrote: > I think it's better to track error codes here as well. > See histogram Net.ErrorCodesForMainFrame for instance. Good idea, done.
Yeah, I have an issue to get rid of that deprecated screen. http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:29: UserMetrics::RecordAction( I think what you need here instead is UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.ErrorCodesForMainFrame", request->status().os_error(), GetAllNetErrorCodes());
http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:29: UserMetrics::RecordAction( On 2010/12/02 10:45:12, Nikita Kostylev wrote: > I think what you need here instead is > > UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.ErrorCodesForMainFrame", > request->status().os_error(), > GetAllNetErrorCodes()); My understanding is that histogram is used for collecting times. Repurposing this for collecting error code doesn't sound right. Besides, histgorams are shown in a different dashboard page than the user metrics actions.
On 2010/12/02 10:52:17, satorux1 wrote: > http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/login_performer.cc (right): > > http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/login_performer.cc:29: UserMetrics::RecordAction( > On 2010/12/02 10:45:12, Nikita Kostylev wrote: > > I think what you need here instead is > > > > UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.ErrorCodesForMainFrame", > > request->status().os_error(), > > GetAllNetErrorCodes()); > > My understanding is that histogram is used for collecting times. Repurposing > this for collecting error code doesn't sound right. Besides, histgorams are > shown in a different dashboard page than the user metrics actions. Looking at use of UMA_HISTOGRAM_ENUMERATION, using histograms for collecting codes seems to be common. Maybe we could do something like below: UserMetricsAction("Login_Failure")); UMA_HISTOGRAM_ENUMERATION("Login.FailureType", failure.reason(), NUM_FAILURE_TYPES); However, this will be shown in a different dashboard page than user metrics. Having all information in the user metrics dashboard would be convenient. I'm fine with either way.
http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:29: UserMetrics::RecordAction( On 2010/12/02 10:52:18, satorux1 wrote: > On 2010/12/02 10:45:12, Nikita Kostylev wrote: > > I think what you need here instead is > > > > UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.ErrorCodesForMainFrame", > > request->status().os_error(), > > GetAllNetErrorCodes()); > > My understanding is that histogram is used for collecting times. Repurposing > this for collecting error code doesn't sound right. Besides, histgorams are > shown in a different dashboard page than the user metrics actions. Having Login status error/success codes reported as histogram gives us better understanding on error codes distribution, gives nice graph. We might track successful login as a user action too. Makes sense to me.
On 2010/12/02 11:38:22, Nikita Kostylev wrote: > http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/login_performer.cc (right): > > http://codereview.chromium.org/5287003/diff/12002/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/login_performer.cc:29: UserMetrics::RecordAction( > On 2010/12/02 10:52:18, satorux1 wrote: > > On 2010/12/02 10:45:12, Nikita Kostylev wrote: > > > I think what you need here instead is > > > > > > UMA_HISTOGRAM_CUSTOM_ENUMERATION("Net.ErrorCodesForMainFrame", > > > request->status().os_error(), > > > GetAllNetErrorCodes()); > > > > My understanding is that histogram is used for collecting times. Repurposing > > this for collecting error code doesn't sound right. Besides, histgorams are > > shown in a different dashboard page than the user metrics actions. > > Having Login status error/success codes reported as histogram gives us better > understanding on error codes distribution, gives nice graph. > > We might track successful login as a user action too. > Makes sense to me. Sounds good. Changed the code to use histograms. Please take another look.
LGTM http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:77: UserMetricsAction("Login_OffTheRecordLoginSuccess")); nit: Login_GuestLoginSuccess
http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:57: // 0 = login success offline and online. Makes sense. 0 - it does mean that it's a new user. * or it's an existing user and offline auth took longer than online auth. Chris, that's possible, right? 1 - it's an existing user login
http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:57: // 0 = login success offline and online. On 2010/12/02 12:27:50, Nikita Kostylev wrote: > Makes sense. > > 0 - it does mean that it's a new user. > * or it's an existing user and offline auth took longer than online auth. > Chris, that's possible, right? > > 1 - it's an existing user login Updated comments accordingly. http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:77: UserMetricsAction("Login_OffTheRecordLoginSuccess")); On 2010/12/02 12:26:35, Nikita Kostylev wrote: > nit: Login_GuestLoginSuccess Done. http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:77: UserMetricsAction("Login_OffTheRecordLoginSuccess")); On 2010/12/02 12:26:35, Nikita Kostylev wrote: > nit: Login_GuestLoginSuccess Done.
On Thu, Dec 2, 2010 at 4:27 AM, <nkostylev@chromium.org> wrote: > > > http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/login_performer.cc (right): > > > http://codereview.chromium.org/5287003/diff/24001/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/login_performer.cc:57: // 0 = login > success offline and online. > Makes sense. > > 0 - it does mean that it's a new user. > * or it's an existing user and offline auth took longer than online > auth. > Chris, that's possible, right? > > For an existing user, it is possible for offline login to take longer than online login, yes. > 1 - it's an existing user login > > > http://codereview.chromium.org/5287003/ >
Reviewing only the use of HISTOGRAM() stuff... I assume the calls are all on one thread (probably the UI thread) and with that proviso, LGTM (for histogram usage... not data etc.)
http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:60: UMA_HISTOGRAM_ENUMERATION("Login.SuccessReason", pending_requests, 2); I though about this histogram again and there's this issue with it: When parallel authentication process will be enabled by default (soon - M10), OnLoginSuccess will be called twice most of the times. So for most logins it will report Login.SuccessReason / pending_requests = true // at this point only offline auth is done Login.SuccessReason / pending_requests = false // at this point both auth are done
http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:60: UMA_HISTOGRAM_ENUMERATION("Login.SuccessReason", pending_requests, 2); On 2010/12/13 11:55:31, Nikita Kostylev wrote: > I though about this histogram again and there's this issue with it: > > When parallel authentication process will be enabled by default (soon - M10), > OnLoginSuccess will be called twice most of the times. So for most logins it > will report > > Login.SuccessReason / pending_requests = true // at this point only offline auth > is done > Login.SuccessReason / pending_requests = false // at this point both auth are > done If we can't do an online check, we'll see the latter behavior as well, right? If we report these metrics from down in ParallelAuthenticator, would it help us enumerate the cases we want to track better?
http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:60: UMA_HISTOGRAM_ENUMERATION("Login.SuccessReason", pending_requests, 2); On 2010/12/13 15:55:12, Chris Masone wrote: > If we report these metrics from down in ParallelAuthenticator, would it help us > enumerate the cases we want to track better? Yes, as what we basically want is on each successful sign in record whether it was offline or offline&online
LGTM for the use of the histogram calls, but I'm less familiar with the data flowing into it, and can't comment/approve that.
Sorry about the belated response. I was ooo for the last couple of days. http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:60: UMA_HISTOGRAM_ENUMERATION("Login.SuccessReason", pending_requests, 2); On 2010/12/13 16:12:43, Nikita Kostylev wrote: > On 2010/12/13 15:55:12, Chris Masone wrote: > > If we report these metrics from down in ParallelAuthenticator, would it help > us > > enumerate the cases we want to track better? > > Yes, as what we basically want is on each successful sign in record whether it > was offline or offline&online So you'll be adding code in ParallelAuthenticator to collect metrics there? Do we need to change the code here?
http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/login_performer.cc (right): http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... chrome/browser/chromeos/login/login_performer.cc:60: UMA_HISTOGRAM_ENUMERATION("Login.SuccessReason", pending_requests, 2); On 2010/12/17 05:58:09, satorux1 wrote: > On 2010/12/13 16:12:43, Nikita Kostylev wrote: > > On 2010/12/13 15:55:12, Chris Masone wrote: > > > If we report these metrics from down in ParallelAuthenticator, would it help > > us > > > enumerate the cases we want to track better? > > > > Yes, as what we basically want is on each successful sign in record whether it > > was offline or offline&online > > So you'll be adding code in ParallelAuthenticator to collect metrics there? Do > we need to change the code here? Chris, will you move this histogram to ParallelAuthenticator?
Yeah...when do we need this done? On Fri, Dec 17, 2010 at 2:30 AM, <nkostylev@chromium.org> wrote: > > > http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/login_performer.cc (right): > > > http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/login_performer.cc:60: > UMA_HISTOGRAM_ENUMERATION("Login.SuccessReason", pending_requests, 2); > On 2010/12/17 05:58:09, satorux1 wrote: > >> On 2010/12/13 16:12:43, Nikita Kostylev wrote: >> > On 2010/12/13 15:55:12, Chris Masone wrote: >> > > If we report these metrics from down in ParallelAuthenticator, >> > would it help > >> > us >> > > enumerate the cases we want to track better? >> > >> > Yes, as what we basically want is on each successful sign in record >> > whether it > >> > was offline or offline&online >> > > So you'll be adding code in ParallelAuthenticator to collect metrics >> > there? Do > >> we need to change the code here? >> > > Chris, will you move this histogram to ParallelAuthenticator? > > > http://codereview.chromium.org/5287003/ >
Is this something we want to do and get merged into 597? On Saturday, December 18, 2010, Chris Masone <cmasone@chromium.org> wrote: > Yeah...when do we need this done? > > On Fri, Dec 17, 2010 at 2:30 AM, <nkostylev@chromium.org> wrote: > > > http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... > File chrome/browser/chromeos/login/login_performer.cc (right): > > http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... > chrome/browser/chromeos/login/login_performer.cc:60: > UMA_HISTOGRAM_ENUMERATION("Login.SuccessReason", pending_requests, 2); > > On 2010/12/17 05:58:09, satorux1 wrote: > > On 2010/12/13 16:12:43, Nikita Kostylev wrote: >> On 2010/12/13 15:55:12, Chris Masone wrote: >> > If we report these metrics from down in ParallelAuthenticator, > > would it help > >> us >> > enumerate the cases we want to track better? >> >> Yes, as what we basically want is on each successful sign in record > > whether it > >> was offline or offline&online > > > > So you'll be adding code in ParallelAuthenticator to collect metrics > > there? Do > > we need to change the code here? > > > > Chris, will you move this histogram to ParallelAuthenticator? > > http://codereview.chromium.org/5287003/ > >
The more I look at this, the less I understand what needs to be done to successfully add a metric here. I could point out the places in parallel authenticator that should be instrumented, though, to someone who knows what to do. On Monday, January 3, 2011, Chris Masone <cmasone@chromium.org> wrote: > Is this something we want to do and get merged into 597? > > On Saturday, December 18, 2010, Chris Masone <cmasone@chromium.org> wrote: >> Yeah...when do we need this done? >> >> On Fri, Dec 17, 2010 at 2:30 AM, <nkostylev@chromium.org> wrote: >> >> >> http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... >> File chrome/browser/chromeos/login/login_performer.cc (right): >> >> http://codereview.chromium.org/5287003/diff/10002/chrome/browser/chromeos/log... >> chrome/browser/chromeos/login/login_performer.cc:60: >> UMA_HISTOGRAM_ENUMERATION("Login.SuccessReason", pending_requests, 2); >> >> On 2010/12/17 05:58:09, satorux1 wrote: >> >> On 2010/12/13 16:12:43, Nikita Kostylev wrote: >>> On 2010/12/13 15:55:12, Chris Masone wrote: >>> > If we report these metrics from down in ParallelAuthenticator, >> >> would it help >> >>> us >>> > enumerate the cases we want to track better? >>> >>> Yes, as what we basically want is on each successful sign in record >> >> whether it >> >>> was offline or offline&online >> >> >> >> So you'll be adding code in ParallelAuthenticator to collect metrics >> >> there? Do >> >> we need to change the code here? >> >> >> >> Chris, will you move this histogram to ParallelAuthenticator? >> >> http://codereview.chromium.org/5287003/ >> >> > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
