Changing the UsersPerSession UMA metric to only count possible multi profile sessions and do this u…

Changing the UsersPerSession UMA metric to count relative user amount changes for sessions which can do multi profile

The old absolute UMA counter was stored upon shutdown. Unfortunately only one in 33 users is shutting down the system properly. As such only a fraction of sessions were accounted for => the values were useless. Furthermore we have decided to only count sessions which could be multi profile sessions.

The new values are recording state changes like e.g. going from one user to two in a single session. As such you might get a resulting stat counter list of:

1: 320 => 320 - 15 - 4 - 1 = 300 single user sessions
2: 15  =>       15 - 4 - 1 = 10  two user sessions
3: 4   =>            4 - 1 = 3   three user sessions
4: 1   =>                    1   four user session

Note that it is not possible for a single user to leave a multi profile session - so the counter will always count up.

BUG=349055
TEST=none

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

[Mr4D (OOO till 08-26), 2014-03-06 19:29:47]

Please have a look!

[James Cook, 2014-03-06 19:45:35]

Code LGTM assuming you fix histograms.xml

I'm not sure how I would interpret this stat -- are you assuming that once a
user adds a second profile they keep it forever?  So after you subtract things
out the number of "added second profile" events approximates a user count?  Or
do you just want a relative number of 2-profiles vs 3-profiles vs 4-profiles ...
?

You could also record the amount of time spent in the 1 user, 2 user, 3 user,
etc. states.  Harry built a thing in ash/metrics/user_metrics_recorder.h that
allows you to count the amount of time spent in a state, which he used for the
side launcher.

But anyhow, if you're OK with what you're measuring the code LGTM.

https://codereview.chromium.org/180243025/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/180243025/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:8120: +<histogram
name="MultiProfile.UsersPerSessionIncremental">
You can't rename a stat in histograms.xml. You need to add a new one and mark
the old one as obsolete.

[Mark P, 2014-03-06 19:45:59]

In addition to the comment I put in the file, I have two problems with the
changelist description:

> The old UMA counter was stored upon shutdown.
This is in direct contradiction to the description in histograms.xm.  If the
description was wrong, please revise it.  If it was correct, please revise the
changelist description.

> Unfortunately only ever 33th user is shutting down properly
I have no idea what this means.

--mark

[Mr4D (OOO till 08-26), 2014-03-06 20:15:19]

Updated the patch to reflect the deprecated stat counter.

I also updated the issue description - I hope that makes it clearer?

The problem is that storing the user value upon shutdown is too late since only
a fraction of users shut down properly.  Instead we need to post incremental
changes as they happen and do the math afterwards.

https://codereview.chromium.org/180243025/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/180243025/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:8120: +<histogram
name="MultiProfile.UsersPerSessionIncremental">
Ahh. I didn't know! Done!

[James Cook, 2014-03-06 20:23:13]

Still LGTM.  Amazing that only 1/30th of our users shut down properly.  Maybe
they have a clean shutdown but just don't write stats for some reason?

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

https://codereview.chromium.org/180243025/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8122: +    Deprecated 3/2014, renamed.
Often we list the name of the stat it got renamed to.

[Mark P, 2014-03-06 21:36:32]

https://codereview.chromium.org/180243025/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/180243025/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:8076: +   
'MultiProfile.UsersPerSessionIncremental'.
This was an earlier comment I forgot to publish.  Now publishing for posterity,
though it's somewhat wrong given new information in the changelist description.
----
This instruction about how to normalize these counts isn't right.

It's not even right if you do the math suggested in the description of
MultiProfile.UsersPerSessionIncremental.

For instance, suppose all the world does the following: log in with one user,
log in with another user, log in with another user.  Log out one user.  Now you
have two users.  Then browse the web.

If the world is 100 people, this means that the
MultiProfile.UsersPerSessionIncremental histogram will see 100 single user, 100
two-user, and 100 three-user.  The math will say 0 single-user sessions, 0
two-user sessions, 100 (= 100%) three-user sessions.

Now suppose some of the browsing causes discarded tabs.  These will all get put
in the two-user bucket for MultiProfile.DiscardedTabsPerUser.  There is no way
to normalize this based on the 0% of user we infer are in the two-user state.


This is no way to correct this mathematically.  It's a consequence of when
MultiProfile.UsersPerSessionIncremental.

Are you have a use for the MultiProfile.DiscardedTabsPerUser histogram without
this normalization instructions?  If not, then I advise you to remove it.  If
so, then keep it, but at least get rid of the incorrect normalization
instructions.

https://codereview.chromium.org/180243025/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:8120: +<histogram
name="MultiProfile.UsersPerSessionIncremental">
old unpublished comment:
Please mark old histograms as <obsolete> (with its original description), then
create a new entry for the new histogrma.

https://codereview.chromium.org/180243025/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/180243025/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8123: Chrome OS. This is recorded
whenever a new user logs in.
If this was actually recorded on shutdown, please correct this description.

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

https://codereview.chromium.org/180243025/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8076: +   
'MultiProfile.UsersPerSessionIncremental'.
The math still doesn't work.  Take my earlier example (did I forget to publish
it?) and suppose tons of tabs are evicted before the third user logs in.  You'll
still be dividing by 0 because you'll infer that no users were in two-user mode.

[Mr4D (OOO till 08-26), 2014-03-06 23:01:25]

Please have a look at my replies!

https://codereview.chromium.org/180243025/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/180243025/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:8076: +   
'MultiProfile.UsersPerSessionIncremental'.
As stated in the comment of my CL: there is no way to log out a single person
without closing the session. When one leaves the session, the session ends.

"If the world is 100 people, ..." correct that is exactly what that would mean,
all sessions would be 3 people sessions.

But... see my comment in the newer post.

https://codereview.chromium.org/180243025/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/180243025/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8123: Chrome OS. This is recorded
whenever a new user logs in.
On 2014/03/06 21:36:33, Mark P wrote:
> If this was actually recorded on shutdown, please correct this description.

Done.

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

https://codereview.chromium.org/180243025/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8076: +   
'MultiProfile.UsersPerSessionIncremental'.
Right. That is a possibility (but it unfortunately it won't happen as numbers
show ;) ).

Let's come back to what we want to do here: We want to show that with the
increasing amount of users the discarded tabs grow - and much more as linear
with a certain size. The question however is how many user it takes to get into
the "unstable area". There are power users which have 30+ tabs open and they
will get probably discarded tabs often - correct. But the most people will not.

The question which we try to answer is a "rough" number of users which make
sense to have (before running out of memory and the session gets ugly). There
are many factors (e.g. how much memory does the system have, how many tabs had
each user has open, what was in tabs (e.g. playing video)) but we only want to
look at it roughly statistically.

So now coming back to your comment:

You could logically assume that a session with 5 users consists of 5 separate
sessions (since no user can bail out without ending the session): one one user
session which ends when the second user logs in. That session ends when the
third user logs in and so forth.

So for all intents and purposes the percentage which is given under
"MultiProfile.UsersPerSessionIncremental" is *exactly* the value you are
interested in when doing the math as you indicated!

However ... If you go to the other extreme - each computer start would load all
users immediately before doing anything it would be incorrect and you should use
the "normalized" user counts instead.

So as usual the truth is somewhere in between the two extremes and you would
probably need to know the entire session length of *that* "segment" to be more
accurate, but in that case the memory configuration is much more important.
Where to stop? It's a rough estimate. :)

https://codereview.chromium.org/180243025/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8122: +    Deprecated 3/2014, renamed.
On 2014/03/06 20:23:14, James Cook wrote:
> Often we list the name of the stat it got renamed to.

Done. (when adding this I glanced over the <obsolete> entries and the first 4 or
so I looked at had exactly this comment).

[Mark P, 2014-03-06 23:31:21]

our good friend the chunk mismatch has struck again.  please try uploading
again



On Thu, Mar 6, 2014 at 3:01 PM, <skuhne@chromium.org> wrote:

> Please have a look at my replies!
>
>
>
> https://codereview.chromium.org/180243025/diff/1/tools/
> metrics/histograms/histograms.xml
> File tools/metrics/histograms/histograms.xml (right):
>
> https://codereview.chromium.org/180243025/diff/1/tools/
> metrics/histograms/histograms.xml#newcode8076
> tools/metrics/histograms/histograms.xml:8076: +
> 'MultiProfile.UsersPerSessionIncremental'.
> As stated in the comment of my CL: there is no way to log out a single
> person without closing the session. When one leaves the session, the
> session ends.
>
> "If the world is 100 people, ..." correct that is exactly what that
> would mean, all sessions would be 3 people sessions.
>
> But... see my comment in the newer post.
>
>
> https://codereview.chromium.org/180243025/diff/20001/
> tools/metrics/histograms/histograms.xml
> File tools/metrics/histograms/histograms.xml (left):
>
> https://codereview.chromium.org/180243025/diff/20001/
> tools/metrics/histograms/histograms.xml#oldcode8123
> tools/metrics/histograms/histograms.xml:8123: Chrome OS. This is
> recorded whenever a new user logs in.
> On 2014/03/06 21:36:33, Mark P wrote:
>
>> If this was actually recorded on shutdown, please correct this
>>
> description.
>
> Done.
>
>
> https://codereview.chromium.org/180243025/diff/20001/
> tools/metrics/histograms/histograms.xml
> File tools/metrics/histograms/histograms.xml (right):
>
> https://codereview.chromium.org/180243025/diff/20001/
> tools/metrics/histograms/histograms.xml#newcode8076
> tools/metrics/histograms/histograms.xml:8076: +
> 'MultiProfile.UsersPerSessionIncremental'.
> Right. That is a possibility (but it unfortunately it won't happen as
> numbers show ;) ).
>
> Let's come back to what we want to do here: We want to show that with
> the increasing amount of users the discarded tabs grow - and much more
> as linear with a certain size. The question however is how many user it
> takes to get into the "unstable area". There are power users which have
> 30+ tabs open and they will get probably discarded tabs often - correct.
> But the most people will not.
>
> The question which we try to answer is a "rough" number of users which
> make sense to have (before running out of memory and the session gets
> ugly). There are many factors (e.g. how much memory does the system
> have, how many tabs had each user has open, what was in tabs (e.g.
> playing video)) but we only want to look at it roughly statistically.
>
> So now coming back to your comment:
>
> You could logically assume that a session with 5 users consists of 5
> separate sessions (since no user can bail out without ending the
> session): one one user session which ends when the second user logs in.
> That session ends when the third user logs in and so forth.
>
> So for all intents and purposes the percentage which is given under
> "MultiProfile.UsersPerSessionIncremental" is *exactly* the value you are
> interested in when doing the math as you indicated!
>
> However ... If you go to the other extreme - each computer start would
> load all users immediately before doing anything it would be incorrect
> and you should use the "normalized" user counts instead.
>
> So as usual the truth is somewhere in between the two extremes and you
> would probably need to know the entire session length of *that*
> "segment" to be more accurate, but in that case the memory configuration
> is much more important. Where to stop? It's a rough estimate. :)
>
>
> https://codereview.chromium.org/180243025/diff/20001/
> tools/metrics/histograms/histograms.xml#newcode8122
> tools/metrics/histograms/histograms.xml:8122: +    Deprecated 3/2014,
> renamed.
> On 2014/03/06 20:23:14, James Cook wrote:
>
>> Often we list the name of the stat it got renamed to.
>>
>
> Done. (when adding this I glanced over the <obsolete> entries and the
> first 4 or so I looked at had exactly this comment).
>
> https://codereview.chromium.org/180243025/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-03-07 00:03:50]

lgtm, baring one change requested below

https://codereview.chromium.org/180243025/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/180243025/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8123: 
You forgot to update this description.

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

https://codereview.chromium.org/180243025/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:8076: 
I'm okay with this comment because you understand the intricacies here (as show
by your last explanation).  Sorry for the long discussion; I frequently see
people try to make inferences that don't make that much sense or don't realize
the problems within.

[Mr4D (OOO till 08-26), 2014-03-07 01:10:47]

Thanks!

I updated the comment once more. Somehow the previous upload got hosed somehow.

[Mr4D (OOO till 08-26), 2014-03-07 01:11:10]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-07 01:46:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/180243025/70001

[commit-bot: I haz the power, 2014-03-07 02:18:54]

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

[commit-bot: I haz the power, 2014-03-07 02:18:55]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Mr4D (OOO till 08-26), 2014-03-07 04:32:17]

+dpolukhin for owner review.

Please have a look!

[Dmitry Polukhin, 2014-03-07 04:39:09]

lgtm

https://codereview.chromium.org/180243025/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/180243025/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/user_manager_impl.cc:2043: int users =
GetLoggedInUsers().size();
Nit, I would make it size_t for consistency.

[Mr4D (OOO till 08-26), 2014-03-07 16:06:21]

Darin, can you please do the last owners review for application_lifetime.cc?

Thanks!

[Mr4D (OOO till 08-26), 2014-03-07 19:14:57]

+Thakis - can you please do an owners review for life_time.cc

[Nico, 2014-03-07 19:16:58]

lifetime lgtm


"Unfortunately only one in 33 users is shutting down the system properly." O_O

[Mr4D (OOO till 08-26), 2014-03-07 19:20:38]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-07 19:22:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/180243025/80001

[commit-bot: I haz the power, 2014-03-07 20:26:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/180243025/80001

[commit-bot: I haz the power, 2014-03-07 20:36:01]

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

[commit-bot: I haz the power, 2014-03-07 20:36:02]

Try jobs failed on following builders: mac_chromium_compile_dbg

[Mr4D (OOO till 08-26), 2014-03-07 21:21:11]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-07 21:21:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/180243025/80001

[commit-bot: I haz the power, 2014-03-08 00:09:23]

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

[commit-bot: I haz the power, 2014-03-08 00:09:24]

Retried try job too often on win_x64_rel for step(s) base_unittests,
chrome_elf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[Mr4D (OOO till 08-26), 2014-03-08 01:41:45]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-08 10:58:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/180243025/100001

[commit-bot: I haz the power, 2014-03-08 12:34:56]

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

[commit-bot: I haz the power, 2014-03-08 12:34:56]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Mr4D (OOO till 08-26), 2014-03-08 16:32:06]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-08 16:32:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/180243025/100001

[commit-bot: I haz the power, 2014-03-08 17:16:19]

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

[commit-bot: I haz the power, 2014-03-08 17:16:20]

Retried try job too often on mac_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Mr4D (OOO till 08-26), 2014-03-08 19:54:42]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-08 19:55:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/180243025/100001

[commit-bot: I haz the power, 2014-03-08 20:38:02]

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

[commit-bot: I haz the power, 2014-03-08 20:38:03]

Retried try job too often on mac_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Mr4D (OOO till 08-26), 2014-03-09 17:43:36]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-03-09 17:44:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/180243025/100001

[commit-bot: I haz the power, 2014-03-09 18:00:10]

Change committed as 255865