New Profile metric for Auth Errors.

New Profile metric for Auth Errors.

This will capture how many profiles are in an Auth Error state and which
auth error the profiles are in. This is comparable with
Profile.NumberOfProfiles and Profile.NumberOfSignedInProfiles.

BUG=434829
Committed: https://crrev.com/c29285e353a29b8433a8cfb699374729286ae168
Cr-Commit-Position: refs/heads/master@{#310848}

[Mike Lerman, 2014-12-19 16:52:29]

mlerman@chromium.org changed reviewers:
+ jcivelli@chromium.org, jwd@chromium.org, noms@chromium.org,
rogerta@chromium.org

[Mike Lerman, 2014-12-19 16:52:30]

Hi all,

Can you please review this CL? Not urgent before the holidays.

jcivelli: c/test
jwd: histograms.xml
noms: c/b/profiles
rogerta: c/b/signin

Thanks,
Mike

[Mike Lerman, 2014-12-19 16:57:57]

Patchset #5 (id:80001) has been deleted

[noms (inactive), 2014-12-19 19:33:01]

Some initial nits. :)

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:465: else
nit: You don't need the else.

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:761:
info->SetBoolean(kIsUsingDefaultAvatarKey, (int)value);
Hmm, I don't understand this change. You are setting a boolean, and value is
already a boolean. Why is this needed?

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache.h (right):

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.h:89: GoogleServiceAuthError::State
ProfileAuthErrorAtIndex(size_t index) const;
I think this should be GetProfileAuthErrorAtIndex (the Get is only dropped for
the Is type functions)

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

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:9: 
nit: drop the empty line

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:35: auth_errors() {}
nit: now that they don't fit in one line, please align all the initializers.

https://codereview.chromium.org/813133003/diff/120001/chrome/test/base/testin...
File chrome/test/base/testing_profile_manager.h (right):

https://codereview.chromium.org/813133003/diff/120001/chrome/test/base/testin...
chrome/test/base/testing_profile_manager.h:88: void UpdateLastUser(Profile*
last_active);
Bleh, this name doesn't really agree with the comment. How about something like
SetLastUsedProfile()?
(i know it's calling UpdateLastUser on the profile manager, but I don't think
that method is well named either)

[Roger Tawa OOO till Jul 10th, 2014-12-19 19:52:33]

Hi Mike,

After offline conversation, I think we can simplify.  I don't see a need to
track every type of error per profile.  I think we only need to track a bool:
has auth error or not, since the purpose is to figure out how many signed in
profiles are in this state.

Also, it would be interesting to know which types of errors happen.  For this,
we could just call UMA_HISTOGRAM_ENUMERATION directly in
SigninErrorController::AuthStatusChanged().  This number would be comparable to
itself.  That is, we could say things like: X% of auth errors are
INVALID_GAIA_CREDENTIALS, Y% of auth errors are USER_NOT_SIGNED_UP, and so on.

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

https://codereview.chromium.org/813133003/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.cc:192: 
Should we clear the counts after reporting?

https://codereview.chromium.org/813133003/diff/100001/chrome/browser/signin/s...
File chrome/browser/signin/signin_global_error.cc (right):

https://codereview.chromium.org/813133003/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error.cc:201: }
Why not just call UMA_HISTOGRAM_ENUMERATION right here?

Are you sure this is comparable to Profile.NumberOfProfiles and
Profile.NumberOfSignedInProfiles?  How often is OnErrorChagned() called?

I don't believe SigninGlobalError is used on all platforms.  May be better to
move this to SigninErrorController::AuthStatusChanged(), right before calling
OnErrorChanged().

[Mike Lerman, 2014-12-19 19:53:18]

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:465: else
On 2014/12/19 19:33:01, Monica Dinculescu wrote:
> nit: You don't need the else.

Done.

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:761:
info->SetBoolean(kIsUsingDefaultAvatarKey, (int)value);
On 2014/12/19 19:33:00, Monica Dinculescu wrote:
> Hmm, I don't understand this change. You are setting a boolean, and value is
> already a boolean. Why is this needed?

Sorry, bad things with copy pasting.done.

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache.h (right):

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.h:89: GoogleServiceAuthError::State
ProfileAuthErrorAtIndex(size_t index) const;
On 2014/12/19 19:33:01, Monica Dinculescu wrote:
> I think this should be GetProfileAuthErrorAtIndex (the Get is only dropped for
> the Is type functions)

Done.

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

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:9: 
On 2014/12/19 19:33:01, Monica Dinculescu wrote:
> nit: drop the empty line

Done.

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:35: auth_errors() {}
On 2014/12/19 19:33:01, Monica Dinculescu wrote:
> nit: now that they don't fit in one line, please align all the initializers.

Like this?

https://codereview.chromium.org/813133003/diff/120001/chrome/test/base/testin...
File chrome/test/base/testing_profile_manager.h (right):

https://codereview.chromium.org/813133003/diff/120001/chrome/test/base/testin...
chrome/test/base/testing_profile_manager.h:88: void UpdateLastUser(Profile*
last_active);
On 2014/12/19 19:33:01, Monica Dinculescu wrote:
> Bleh, this name doesn't really agree with the comment. How about something
like
> SetLastUsedProfile()?
> (i know it's calling UpdateLastUser on the profile manager, but I don't think
> that method is well named either)

I really think they should have the same name, since they're doing the same
thing and this is just exposing the ProfileManager's private call. Giving them
different names would be confusing.

If I'll rename them, then I'll rename both. I don't like SetLastUsedProfile
since the method does more than just set the LastUsedProfile in the LocalState,
it also updates the active time of the ProfileInfo Cache. How about
MarkProfileActive()?

You're just getting me back for the signin_metrics thing aren't you ;)

[Mike Lerman, 2015-01-05 20:24:02]

Hi Roger,

I've moved everything out of signin_global_error into the
signin_error_controller or the chrome_signin_client.

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

https://codereview.chromium.org/813133003/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.cc:192: 
On 2014/12/19 19:52:33, Roger Tawa wrote:
> Should we clear the counts after reporting?

Clear them from the ProfileInfoCache, you mean? No. We want each call to
LogNumberOfProfiles to log all of the information, not just part of it.

If you're referring to the ProfileCounts object, it's very shortlived.

Can you clarify?

https://codereview.chromium.org/813133003/diff/100001/chrome/browser/signin/s...
File chrome/browser/signin/signin_global_error.cc (right):

https://codereview.chromium.org/813133003/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error.cc:201: }
On 2014/12/19 19:52:33, Roger Tawa wrote:
> Why not just call UMA_HISTOGRAM_ENUMERATION right here?
> 
> Are you sure this is comparable to Profile.NumberOfProfiles and
> Profile.NumberOfSignedInProfiles?  How often is OnErrorChagned() called?
> 
> I don't believe SigninGlobalError is used on all platforms.  May be better to
> move this to SigninErrorController::AuthStatusChanged(), right before calling
> OnErrorChanged().

The answer to the first two questions is the same: By making
ProfileMetrics::LogNumbersOfProfiles responsible for calling UMA_HISTOGRAMS we
always log the Profile.NumberOf* histograms at the same times in a comparable
way.

Moved into SigninErrorController. SigninGlobalError just sets the bit as to
whether an error has occured or not.

[Roger Tawa OOO till Jul 10th, 2015-01-06 22:32:42]

Looks good Mike.  See comment below in signin_client.h, sorry for the yak shave
request, but it seems like this is more than just a nice cleanup for later. 
What do you think?  What do you think?

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

https://codereview.chromium.org/813133003/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.cc:192: 
On 2015/01/05 20:24:02, Mike Lerman wrote:
> On 2014/12/19 19:52:33, Roger Tawa wrote:
> > Should we clear the counts after reporting?
> 
> Clear them from the ProfileInfoCache, you mean? No. We want each call to
> LogNumberOfProfiles to log all of the information, not just part of it.
> 
> If you're referring to the ProfileCounts object, it's very shortlived.
> 
> Can you clarify?

You're right, I missed that.  Looks good.

https://codereview.chromium.org/813133003/diff/220001/components/signin/core/...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/813133003/diff/220001/components/signin/core/...
components/signin/core/browser/signin_client.h:36: public
SigninErrorController::Observer {
ChromeSigninClient should implement this interface directly instead of
inheriting from SigninClient.  It should then add itself as an observer when
created, instead of being added in the SigninErrorController ctor.

However, I see that this will create a circular PKS dependency chain: PO2TS ->
ChromeSigninClient -> PO2TS.  But it seems we are creating this dependency
anyway; since ChromeSigninClient::OnErrorChanged() calls on PO2TS,
ChromeSigninClientFactory should be updated to depend on PO2TSFactory.

We discussed making SigninErrorController its own PKS, but thought that was a
cleanup for later.  Maybe it should be done here.  Both PO2TS and
ChromeSigninClientFactory can depend on SigninErrorController, and the latter
depends on nothing.

[noms (inactive), 2015-01-08 21:49:05]

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

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:35: auth_errors() {}
On 2014/12/19 19:53:18, Mike Lerman wrote:
> On 2014/12/19 19:33:01, Monica Dinculescu wrote:
> > nit: now that they don't fit in one line, please align all the initializers.
> 
> Like this?

Yup!

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:61: const char kAuthErrorKey[] =
"auth_error";
nit: now that it's a boolean, is_auth_error would match the other keys that
store booleans :)

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

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.cc:182: counts.gaia_icon);
nit: I'm not sure I like this name. At first glance I thought it meant "Number
of auth errors for profile". I don't have a better one though. Maybe
NumberOfProfilesWithAuthErrors?

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:242: if (index !=
std::string::npos) {
nit: change to 
if (index==std::string::npos) 
  return;
to cut down on the staircase.

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.h (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.h:69: void OnErrorChanged() override;
nit: change to "SigninErrorController::Observer implemetation." to match the
other 2 overrides in the file.

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/s...
File chrome/browser/signin/signin_global_error_unittest.cc (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error_unittest.cc:169:
histogram_tester.ExpectBucketCount("Signin.AuthError", i, 1);
nit: no squiggles {}

https://codereview.chromium.org/813133003/diff/220001/chrome/test/base/testin...
File chrome/test/base/testing_profile_manager.cc (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/test/base/testin...
chrome/test/base/testing_profile_manager.cc:182: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
Do we care about it running on ChromeOS? If not, maybe the comment in the .h
file should say "This is only useful on Desktop" or something.

https://codereview.chromium.org/813133003/diff/220001/components/signin/core/...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/813133003/diff/220001/components/signin/core/...
components/signin/core/browser/signin_metrics.h:169: // Track when the current
auth error changed.
nit: since it fits, use "authentication" in the comment :)

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

https://codereview.chromium.org/813133003/diff/220001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27607: +    Counts the type GAIA Auth
Errors on a user's machine when Chrome starts up.
nit: I don't think we need to capitalize error. And maybe use "Authentication"
instead? Here and below

[Mike Lerman, 2015-01-09 02:15:05]

Roger, I'm working to move the SigninErrorController into a PKS in another CL.

Monica, thanks for the comments. See below.

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

https://codereview.chromium.org/813133003/diff/120001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.h:35: auth_errors() {}
On 2015/01/08 21:49:04, Monica Dinculescu wrote:
> On 2014/12/19 19:53:18, Mike Lerman wrote:
> > On 2014/12/19 19:33:01, Monica Dinculescu wrote:
> > > nit: now that they don't fit in one line, please align all the
initializers.
> > 
> > Like this?
> 
> Yup!

Acknowledged.

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/profiles...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_info_cache.cc:61: const char kAuthErrorKey[] =
"auth_error";
On 2015/01/08 21:49:04, Monica Dinculescu wrote:
> nit: now that it's a boolean, is_auth_error would match the other keys that
> store booleans :)

Done.

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

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/profiles...
chrome/browser/profiles/profile_metrics.cc:182: counts.gaia_icon);
On 2015/01/08 21:49:04, Monica Dinculescu wrote:
> nit: I'm not sure I like this name. At first glance I thought it meant "Number
> of auth errors for profile". I don't have a better one though. Maybe
> NumberOfProfilesWithAuthErrors?

It almost reads like English! Might make it comprehensible!

Done.

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:242: if (index !=
std::string::npos) {
On 2015/01/08 21:49:04, Monica Dinculescu wrote:
> nit: change to 
> if (index==std::string::npos) 
>   return;
> to cut down on the staircase.

I don't really like it... but Done.

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.h (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.h:69: void OnErrorChanged() override;
On 2015/01/08 21:49:04, Monica Dinculescu wrote:
> nit: change to "SigninErrorController::Observer implemetation." to match the
> other 2 overrides in the file.

Done.

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/s...
File chrome/browser/signin/signin_global_error_unittest.cc (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error_unittest.cc:169:
histogram_tester.ExpectBucketCount("Signin.AuthError", i, 1);
On 2015/01/08 21:49:04, Monica Dinculescu wrote:
> nit: no squiggles {}

Done.

https://codereview.chromium.org/813133003/diff/220001/chrome/test/base/testin...
File chrome/test/base/testing_profile_manager.cc (right):

https://codereview.chromium.org/813133003/diff/220001/chrome/test/base/testin...
chrome/test/base/testing_profile_manager.cc:182: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
On 2015/01/08 21:49:04, Monica Dinculescu wrote:
> Do we care about it running on ChromeOS? If not, maybe the comment in the .h
> file should say "This is only useful on Desktop" or something.

This can and would run on ChromeOS.

https://codereview.chromium.org/813133003/diff/220001/components/signin/core/...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/813133003/diff/220001/components/signin/core/...
components/signin/core/browser/signin_metrics.h:169: // Track when the current
auth error changed.
On 2015/01/08 21:49:04, Monica Dinculescu wrote:
> nit: since it fits, use "authentication" in the comment :)

Done.

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

https://codereview.chromium.org/813133003/diff/220001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27607: +    Counts the type GAIA Auth
Errors on a user's machine when Chrome starts up.
On 2015/01/08 21:49:04, Monica Dinculescu wrote:
> nit: I don't think we need to capitalize error. And maybe use "Authentication"
> instead? Here and below

Done.

[Roger Tawa OOO till Jul 10th, 2015-01-09 16:59:39]

lgtm

[Mike Lerman, 2015-01-09 17:49:06]

Thanks, Roger.

Monica, further comments?

Jesse, Jay, and comments for c/test or histograms?

Thanks!

[noms (inactive), 2015-01-09 17:56:22]

lgtm % nits

https://codereview.chromium.org/813133003/diff/340001/chrome/browser/signin/s...
File chrome/browser/signin/signin_global_error_unittest.cc (right):

https://codereview.chromium.org/813133003/diff/340001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error_unittest.cc:53: "Person 1",
scoped_ptr<PrefServiceSyncable>(),
nit: wrong indent

https://codereview.chromium.org/813133003/diff/340001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error_unittest.cc:170:
"Profile.NumberOfProfilesWithAuthErrors", table[i].is_error, 1);
nit: wrong indent.

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

https://codereview.chromium.org/813133003/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32677: +    Log the type of GAIA Auth
Error that occur whenever the error state changes.
nit: same comment about "authentication errors" applies here too :)

[Jay Civelli, 2015-01-09 17:58:21]

LGTM for tests

[Mike Lerman, 2015-01-09 18:04:11]

https://codereview.chromium.org/813133003/diff/340001/chrome/browser/signin/s...
File chrome/browser/signin/signin_global_error_unittest.cc (right):

https://codereview.chromium.org/813133003/diff/340001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error_unittest.cc:53: "Person 1",
scoped_ptr<PrefServiceSyncable>(),
On 2015/01/09 17:56:22, Monica Dinculescu wrote:
> nit: wrong indent

Done.

https://codereview.chromium.org/813133003/diff/340001/chrome/browser/signin/s...
chrome/browser/signin/signin_global_error_unittest.cc:170:
"Profile.NumberOfProfilesWithAuthErrors", table[i].is_error, 1);
On 2015/01/09 17:56:22, Monica Dinculescu wrote:
> nit: wrong indent.

Done.

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

https://codereview.chromium.org/813133003/diff/340001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:32677: +    Log the type of GAIA Auth
Error that occur whenever the error state changes.
On 2015/01/09 17:56:22, Monica Dinculescu wrote:
> nit: same comment about "authentication errors" applies here too :)

Done.

[jwd, 2015-01-09 19:25:17]

lgtm

[Mike Lerman, 2015-01-09 19:46:37]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2015-01-09 19:47:53]

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

[commit-bot: I haz the power, 2015-01-09 20:37:26]

Committed patchset #19 (id:380001)

[commit-bot: I haz the power, 2015-01-09 20:41:57]

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