UMA metrics - number of accounts per profile and account reconciler.

UMA metrics - number of accounts per profile and account reconciler.
Creation of a new signin_metrics class to track components/signin UMA.

BUG=357693
TEST=New histograms are called Profile.NumberOfAccountsPerProfile,
Signin.ReconcilerAddedToChrome and Signin.ReconcilerAddedToCookieJar.
There is also a UserCount called
AccountReconcilerDifferentPrimaryAccounts. These can be viewed in
chrome://histograms and chrome://user-actions respectively.
The Profile one can be viewed any time a profile is opened. The others
are best tested the first time you Sign In to Chrome, assuming you
already were signed in to google in the content area with 1 or more
accounts.

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

[Mike Lerman, 2014-05-07 13:51:24]

Roger can you look at the signin-specific code, and Alexei can you look at the
UMA config?
I had to modify the python script to take the components\ directory into
account.

Thanks!

[Roger Tawa OOO till Jul 10th, 2014-05-07 17:05:38]

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:666: are_primaries_equal,
first_execution_);
Spacing is off.  Line up all subsequent args under the 'v' in
|valid_chrome_accounts_|.

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.h (right):

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.h:226: bool first_execution_;
Please add a comment for this member.

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:28:
base::UserMetricsAction("AccountReconcilerDifferentPrimaryAccounts"));
Is there a reason to prefer a user action to a UMA_HISTOGRAM_BOOLEAN?

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:29: }
Indent whole function body two less.

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:19: static void
LogSigninAccountReconciliation(int total_number_accounts,
Should have a comment describing what this function does and what the arguments
mean.  In header files its probably better to list each arg on its own line when
they don't fit the 80 char limit.

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:22: };
New code should not use classes that to define a scope for static functions. 
Better to wrap the function in a namespace like signin_metrics.

[Mike Lerman, 2014-05-07 18:49:36]

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.cc (right):

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.cc:666: are_primaries_equal,
first_execution_);
On 2014/05/07 17:05:38, Roger Tawa wrote:
> Spacing is off.  Line up all subsequent args under the 'v' in
> |valid_chrome_accounts_|.

Done.

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
File components/signin/core/browser/account_reconcilor.h (right):

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/account_reconcilor.h:226: bool first_execution_;
On 2014/05/07 17:05:38, Roger Tawa wrote:
> Please add a comment for this member.

Done.

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:28:
base::UserMetricsAction("AccountReconcilerDifferentPrimaryAccounts"));
On 2014/05/07 17:05:38, Roger Tawa wrote:
> Is there a reason to prefer a user action to a UMA_HISTOGRAM_BOOLEAN?

No, in fact the histogram will be able to filter against the Finch experiment.
I'll swap it for a histogram.

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:29: }
On 2014/05/07 17:05:38, Roger Tawa wrote:
> Indent whole function body two less.

Done.

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:19: static void
LogSigninAccountReconciliation(int total_number_accounts,
On 2014/05/07 17:05:38, Roger Tawa wrote:
> Should have a comment describing what this function does and what the
arguments
> mean.  In header files its probably better to list each arg on its own line
when
> they don't fit the 80 char limit.

Done.

https://codereview.chromium.org/270673003/diff/40001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:22: };
On 2014/05/07 17:05:38, Roger Tawa wrote:
> New code should not use classes that to define a scope for static functions. 
> Better to wrap the function in a namespace like signin_metrics.

Done.

[Roger Tawa OOO till Jul 10th, 2014-05-08 00:48:20]

lgtm

Thanks Mike.

[Alexei Svitkine (slow), 2014-05-08 14:39:29]

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:23: count_added_to_token) {
It seems kind of weird to log a 0 in some cases and not log at all in others.
Can you convince me why this makes sense to do?

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:30:
UMA_HISTOGRAM_BOOLEAN("Signin.ReconcilerDifferentPrimaryAccounts", true);
Consider logging both true and false here, so that you have a baseline to
compare against. (i.e. a count just by itself is not super meaningful vs. if you
log both cases, you can see the percentage)

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:8: #include <stddef.h>
Nit: I don't think this is needed.

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:10: namespace signin_metrics {
Nit: Add an empty line after this.

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:23: void
LogSigninAccountReconciliation(int total_number_accounts,
Nit: Don't indent in a namespace.

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

https://codereview.chromium.org/270673003/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:20504: +    be counted multiple times
per profile.
Clarify description of when exactly this is recorded.

Also, if it's counted multiple times per profile, it may not be a very
meaningful stat. Is there a way to ensure that it's done only once?

https://codereview.chromium.org/270673003/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:23363: +    After execution of the
account reconciler, how many accounts where added to
Nit: where -> were

https://codereview.chromium.org/270673003/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:23371: +    After execution of the
account reconciler, how many accounts where added to
Nit: where -> were

https://codereview.chromium.org/270673003/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:23376: +<histogram
name="Signin.ReconcilerDifferentPrimaryAccounts">
Suggestion:

Consider renaming these three histograms to add a . after "Reconciler" to get
better grouping on the dashboard.

i.e.

Signin.Reconciler.DifferentPrimaryAccounts

[Mike Lerman, 2014-05-08 18:23:14]

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:23: count_added_to_token) {
On 2014/05/08 14:39:29, Alexei Svitkine wrote:
> It seems kind of weird to log a 0 in some cases and not log at all in others.
> Can you convince me why this makes sense to do?

My concern is that every execution of the account reconcilor will log data, but
the majority of "routine" executions will pass in zeroes, and I don't want to
over-log them. But I am interested in them on the first pass.

I think I'll split this into two sets of histograms - one that tracks the
"first" execution of the reconcilor, and one that tracks all others. That way I
can interpret the data as necessary.

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:30:
UMA_HISTOGRAM_BOOLEAN("Signin.ReconcilerDifferentPrimaryAccounts", true);
On 2014/05/08 14:39:29, Alexei Svitkine wrote:
> Consider logging both true and false here, so that you have a baseline to
> compare against. (i.e. a count just by itself is not super meaningful vs. if
you
> log both cases, you can see the percentage)

I was going to use the total number of count in
Profile.NumberOfAccountsPerProfile as a baseline, but I can just log both
scenarios here.

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:8: #include <stddef.h>
On 2014/05/08 14:39:29, Alexei Svitkine wrote:
> Nit: I don't think this is needed.

Done.

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:10: namespace signin_metrics {
On 2014/05/08 14:39:29, Alexei Svitkine wrote:
> Nit: Add an empty line after this.

Done.

https://codereview.chromium.org/270673003/diff/70001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:23: void
LogSigninAccountReconciliation(int total_number_accounts,
On 2014/05/08 14:39:29, Alexei Svitkine wrote:
> Nit: Don't indent in a namespace.

Done.

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

https://codereview.chromium.org/270673003/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:20504: +    be counted multiple times
per profile.
On 2014/05/08 14:39:29, Alexei Svitkine wrote:
> Clarify description of when exactly this is recorded.
> 
> Also, if it's counted multiple times per profile, it may not be a very
> meaningful stat. Is there a way to ensure that it's done only once?

Clarified in the summary - this is intended to be used with the Show User Counts
option.

https://codereview.chromium.org/270673003/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:23363: +    After execution of the
account reconciler, how many accounts where added to
On 2014/05/08 14:39:29, Alexei Svitkine wrote:
> Nit: where -> were

Done.

https://codereview.chromium.org/270673003/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:23371: +    After execution of the
account reconciler, how many accounts where added to
On 2014/05/08 14:39:29, Alexei Svitkine wrote:
> Nit: where -> were

Done.

https://codereview.chromium.org/270673003/diff/70001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:23376: +<histogram
name="Signin.ReconcilerDifferentPrimaryAccounts">
On 2014/05/08 14:39:29, Alexei Svitkine wrote:
> Suggestion:
> 
> Consider renaming these three histograms to add a . after "Reconciler" to get
> better grouping on the dashboard.
> 
> i.e.
> 
> Signin.Reconciler.DifferentPrimaryAccounts

Done.

[Alexei Svitkine (slow), 2014-05-09 14:16:16]

https://codereview.chromium.org/270673003/diff/90001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/270673003/diff/90001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:23:
UMA_HISTOGRAM_COUNTS_100("Signin.Reconciler.AddedToCookieJarFirstRun",
How about Signin.Reconciler.AddedToCookieJar.FirstRun and
Signin.Reconciler.AddedToCookieJar.SubsequentRun? and so on?

https://codereview.chromium.org/270673003/diff/90001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/270673003/diff/90001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:30: 
Nit: Remove extra blank line.

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

https://codereview.chromium.org/270673003/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:23403: +<histogram
name="Signin.Reconciler.DifferentPrimaryAccountsSubsequentRun">
If you adopt the scheme I suggest in my other comment, you can instead avoid
having two entries for each type of histogram and use a histogram_suffixes tag
to modulate them by FirstRun and SubsequentRun suffixes. Search this file for an
example of histogram_suffixes.

[Mike Lerman, 2014-05-09 20:13:27]

https://codereview.chromium.org/270673003/diff/90001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/270673003/diff/90001/components/signin/core/b...
components/signin/core/browser/signin_metrics.cc:23:
UMA_HISTOGRAM_COUNTS_100("Signin.Reconciler.AddedToCookieJarFirstRun",
On 2014/05/09 14:16:16, Alexei Svitkine wrote:
> How about Signin.Reconciler.AddedToCookieJar.FirstRun and
> Signin.Reconciler.AddedToCookieJar.SubsequentRun? and so on?

Done.

https://codereview.chromium.org/270673003/diff/90001/components/signin/core/b...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/270673003/diff/90001/components/signin/core/b...
components/signin/core/browser/signin_metrics.h:30: 
On 2014/05/09 14:16:16, Alexei Svitkine wrote:
> Nit: Remove extra blank line.

Done.

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

https://codereview.chromium.org/270673003/diff/90001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:23403: +<histogram
name="Signin.Reconciler.DifferentPrimaryAccountsSubsequentRun">
On 2014/05/09 14:16:16, Alexei Svitkine wrote:
> If you adopt the scheme I suggest in my other comment, you can instead avoid
> having two entries for each type of histogram and use a histogram_suffixes tag
> to modulate them by FirstRun and SubsequentRun suffixes. Search this file for
an
> example of histogram_suffixes.

Done.

[Alexei Svitkine (slow), 2014-05-09 20:18:20]

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

https://codereview.chromium.org/270673003/diff/110001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:23403: +<histogram
name="Signin.Reconciler.DifferentPrimaryAccountsSubsequentRun">
Simply have single entry which should be
Signin.Reconciler.DifferentPrimaryAccounts and only entry. The suffixes tag will
create two out of these. Same for the others.

https://codereview.chromium.org/270673003/diff/110001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:45489: +  <affected-histogram
name="Signin.Reconciler.DifferentPrimaryAccounts"/>
You're missing AddedToChrome here and duplicating this one.

[Mike Lerman, 2014-05-09 20:24:48]

Thanks Alexei!

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

https://codereview.chromium.org/270673003/diff/110001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:23403: +<histogram
name="Signin.Reconciler.DifferentPrimaryAccountsSubsequentRun">
On 2014/05/09 20:18:20, Alexei Svitkine wrote:
> Simply have single entry which should be
> Signin.Reconciler.DifferentPrimaryAccounts and only entry. The suffixes tag
will
> create two out of these. Same for the others.

Err... yes.

https://codereview.chromium.org/270673003/diff/110001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:45489: +  <affected-histogram
name="Signin.Reconciler.DifferentPrimaryAccounts"/>
On 2014/05/09 20:18:20, Alexei Svitkine wrote:
> You're missing AddedToChrome here and duplicating this one.

Thanks for catching the copy-paste.

[Alexei Svitkine (slow), 2014-05-09 20:40:33]

lgtm

[Mike Lerman, 2014-05-09 20:41:33]

On 2014/05/09 20:40:33, Alexei Svitkine wrote:
> lgtm

Thanks for the review and feedback, alexei!

[Mike Lerman, 2014-05-09 20:41:41]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-05-09 20:49:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/270673003/120001

[commit-bot: I haz the power, 2014-05-09 22:55:46]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-09 23:18:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-09 23:18:09]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)

[Mike Lerman, 2014-05-12 14:02:54]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-05-12 14:03:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/270673003/160001

[commit-bot: I haz the power, 2014-05-12 15:38:09]

Change committed as 269789