Add multi_profile_user_count field to system_profile proto.

Add multi_profile_user_count field to system_profile proto.

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

[Tim Song, 2013-09-30 22:53:03]

Hi Ilya, This is a follow-up to the earlier multiprofile UMA patch adding a user
count field to system_profile.proto.

As per our discussion, you mentioned that it would be possible to do the same
thing with a dremel query. We can probably use the "MultiProfile.UserCount"
histogram and find the range of logs per client and session that have N logged
in users. This would essentially track the same thing as the field in this
patch.

However, we would need to add some additional logic on the dashboard for this
special case, so do we want to do this on the client or server?

[Ilya Sherman, 2013-09-30 23:42:52]

On 2013/09/30 22:53:03, Tim Song wrote:
> Hi Ilya, This is a follow-up to the earlier multiprofile UMA patch adding a
user
> count field to system_profile.proto.
> 
> As per our discussion, you mentioned that it would be possible to do the same
> thing with a dremel query. We can probably use the "MultiProfile.UserCount"
> histogram and find the range of logs per client and session that have N logged
> in users. This would essentially track the same thing as the field in this
> patch.
> 
> However, we would need to add some additional logic on the dashboard for this
> special case, so do we want to do this on the client or server?

Please start a thread (or revive your existing one) with uma-team@ to resolve
this question.  I'm no longer working on the dashboard side of things, so I'm
not sure whether it would make sense to just keep the histogram.  It does seem
inefficient to record this data in two different places -- both in SystemProfile
and as a histogram.  It would be nice to figure out a way to only record it in
one place, though that might not be practical with the current dashboard setup.

If you decide that adding this field to the protocol buffer is appropriate,
you'll need to first update the internal copy of the protocol buffer in the
google3 codebase.  I can point you in the right direction if you're not sure how
to do that.

https://codereview.chromium.org/25364002/diff/1/chrome/browser/metrics/metric...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/25364002/diff/1/chrome/browser/metrics/metric...
chrome/browser/metrics/metrics_log.cc:388: initial_multi_profile_user_count_ =
0;
nit: Please move this to the initializer list.

https://codereview.chromium.org/25364002/diff/1/chrome/browser/metrics/metric...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/25364002/diff/1/chrome/browser/metrics/metric...
chrome/browser/metrics/metrics_log.h:194: // when the log is first opened.
nit: Please wrap this to 80-col.

https://codereview.chromium.org/25364002/diff/1/chrome/browser/metrics/metric...
chrome/browser/metrics/metrics_log.h:195: int initial_multi_profile_user_count_;
nit: const?

https://codereview.chromium.org/25364002/diff/1/chrome/browser/metrics/metric...
chrome/browser/metrics/metrics_log.h:195: int initial_multi_profile_user_count_;
Why not store this directly in the protocol buffer, and then reset it if need
be?

https://codereview.chromium.org/25364002/diff/1/chrome/common/metrics/proto/s...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/25364002/diff/1/chrome/common/metrics/proto/s...
chrome/common/metrics/proto/system_profile.proto:444: optional int32
multi_profile_user_count = 17;
Can this be an unsigned field?  If not, please document what the semantics of
distinct non-positive values are.

[Tim Song, 2013-10-05 01:25:22]

If the proto field looks good, I'll start the process of updating it on the
server side. I also added some unit tests.

https://codereview.chromium.org/25364002/diff/1/chrome/browser/metrics/metric...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/25364002/diff/1/chrome/browser/metrics/metric...
chrome/browser/metrics/metrics_log.h:195: int initial_multi_profile_user_count_;
On 2013/09/30 23:42:52, Ilya Sherman wrote:
> Why not store this directly in the protocol buffer, and then reset it if need
> be?

This would probably be cleaner. I'll remove this member variable.

https://codereview.chromium.org/25364002/diff/1/chrome/common/metrics/proto/s...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/25364002/diff/1/chrome/common/metrics/proto/s...
chrome/common/metrics/proto/system_profile.proto:444: optional int32
multi_profile_user_count = 17;
On 2013/09/30 23:42:52, Ilya Sherman wrote:
> Can this be an unsigned field?  If not, please document what the semantics of
> distinct non-positive values are.

Done. I wanted to leave open the possibility of using negative values to
indicate the number of users when the log is invalidated, but I don't think this
would be really useful.

[Ilya Sherman, 2013-10-08 01:31:02]

Thanks.  Generally looks good; will approve once the google3 CL has landed.

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:978: user_count = 0;
nit: Please leave a blank line after this one, so that it's easier to tell when
scanning that the below line is not part of the if-stmt.

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.h:189: // If the number of users change while
the log is open, we invalidate
nit: "we invalidate" -> "the call invalidates" or some other phrasing without
"we" is preferred.

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log_unittest.cc (right):

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log_unittest.cc:190:
base::FieldTrialList::ACTIVATE_TRIALS);
Hmm, you need both a command-line switch and a field trial to enable
multi-profiles?

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log_unittest.cc:399: #endif
nit: #endif  // OS_CHROMEOS

[Tim Song, 2013-10-08 20:29:40]

Thank you. I'll get started on the log review process.

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:978: user_count = 0;
On 2013/10/08 01:31:03, Ilya Sherman wrote:
> nit: Please leave a blank line after this one, so that it's easier to tell
when
> scanning that the below line is not part of the if-stmt.

Done.

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.h:189: // If the number of users change while
the log is open, we invalidate
On 2013/10/08 01:31:03, Ilya Sherman wrote:
> nit: "we invalidate" -> "the call invalidates" or some other phrasing without
> "we" is preferred.

Done.

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log_unittest.cc (right):

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log_unittest.cc:190:
base::FieldTrialList::ACTIVATE_TRIALS);
On 2013/10/08 01:31:03, Ilya Sherman wrote:
> Hmm, you need both a command-line switch and a field trial to enable
> multi-profiles?

Yeah, it's a bit strict at the moment. Because this is such a new and unstable
feature, multi-profile support will only be available to Googlers (this is the
reason for the field trial) who manually enable the feature in chrome://flags.

https://codereview.chromium.org/25364002/diff/8001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log_unittest.cc:399: #endif
On 2013/10/08 01:31:03, Ilya Sherman wrote:
> nit: #endif  // OS_CHROMEOS

Done.

[Tim Song, 2013-10-10 23:13:31]

I landed the the corresponding CL in google3.

[Ilya Sherman, 2013-10-10 23:16:16]

LGTM, thanks.

[commit-bot: I haz the power, 2013-10-10 23:19:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/25364002/15001

[commit-bot: I haz the power, 2013-10-11 06:01:38]

Change committed as 228126