cros: Add enterprise user session metrics

cros: Add enterprise user session metrics

- Add a "Enterprise.UserSession.Logins" enum UMA to track the
  following user session starts (aka login):
    - Regular user,
    - Manual public session,
    - Auto public session,
    - Manual kiosk,
    - Auto kiosk;
- Tracks user session length of regular user session and public
  seission in "Enterprise.RegularUserSEssion.SessionLength" and
  "Enterprise.PublicSession.SessionLength".

BUG=683306
Review-Url: https://codereview.chromium.org/2692163003
Cr-Commit-Position: refs/heads/master@{#451079}
Committed: https://chromium.googlesource.com/chromium/src/+/f59c187069079ab6422da75f73bbbdf376e62914

[xiyuan, 2017-02-14 20:39:14]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 20:40:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-02-14 20:40:10]

xiyuan@chromium.org changed reviewers:
+ achuith@chromium.org, isherman@chromium.org

[xiyuan, 2017-02-14 20:40:11]

achuith@, please review everything

isherman@, you can focus UMA related files if you want
  enterprise_user_session_metrics.* and
  histograms.xml

Thanks.

[commit-bot: I haz the power, 2017-02-14 21:48:46]

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

[commit-bot: I haz the power, 2017-02-14 21:48:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xiyuan, 2017-02-14 22:34:09]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 22:35:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 00:06:02]

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

[commit-bot: I haz the power, 2017-02-15 00:06:03]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-02-15 00:16:58]

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enterprise_user_session_metrics.cc (right):

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:93: metric_name
= "Enterprise.PublicSession.SessionLength";
Metric names used with histogram macros must be runtime constant.  Are these
runtime constants for the browser process?

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:101: //
minutes.
Note that histogram buckets are logarithmically spaced, so they won't actually
be every 10 minutes.  Perhaps you want to use a sparse histogram, capped at 24
hours?  You then don't need to worry about the runtime constant name issue
either.

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

https://codereview.chromium.org/2692163003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14153: +  <summary>Length of a public
session.</summary>
Please clarify exactly what is measured here -- what are the start and stop
events for a public session?  Note that this metric might be hard to interpret,
depending on how long a typical session length is.  For example, during a
30-minute span, I could either have six 5-minute sessions, or one 30-minute
session.

https://codereview.chromium.org/2692163003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14156: +<histogram
name="Enterprise.RegularUserSession.SessionLength" units="minutes">
In the C++ code, this is "RegularSession" rather than "RegularUserSession"

https://codereview.chromium.org/2692163003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14159: +  <summary>Length of a regular
user session on an enrolled device.</summary>
Ditto

[achuithb, 2017-02-15 10:54:57]

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enterprise_user_session_metrics.cc (right):

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:22: return
connector->IsEnterpriseManaged();
Any reason to not have this all be one line?
return
g_browser_process->platform_part()->browser_policy_connector_chromeos()->IsEnterpriseManaged();

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enterprise_user_session_metrics.h (right):

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.h:20: enum
SignInEventType {
enum class is preferred now I believe:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Q5WmkAImanc

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.h:41: // Records a
sign-in event by UserContext for an enrolled device.
Is is_auto_login well understood in this part of the codebase?

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.h:47: // called on
shutdown path and metrics are likely to lost. The stored value
to be lost.

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin_specifics.h (right):

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin_specifics.h:16: SigninSpecifics();
You should be able to drop this and delete the .cc, right?

[xiyuan, 2017-02-15 18:05:39]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 18:06:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-02-15 19:08:11]

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enterprise_user_session_metrics.cc (right):

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:22: return
connector->IsEnterpriseManaged();
On 2017/02/15 10:54:57, achuithb wrote:
> Any reason to not have this all be one line?
> return
>
g_browser_process->platform_part()->browser_policy_connector_chromeos()->IsEnterpriseManaged();

Done. Also updated the same util function in AppLaunchController.

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:93: metric_name
= "Enterprise.PublicSession.SessionLength";
On 2017/02/15 00:16:57, Ilya Sherman wrote:
> Metric names used with histogram macros must be runtime constant.  Are these
> runtime constants for the browser process?

Acknowledged. Changed use UMA_HISTOGRAM_SPARSE_SLOWLY below.

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:101: //
minutes.
On 2017/02/15 00:16:57, Ilya Sherman wrote:
> Note that histogram buckets are logarithmically spaced, so they won't actually
> be every 10 minutes.  Perhaps you want to use a sparse histogram, capped at 24
> hours?  You then don't need to worry about the runtime constant name issue
> either.

Always has something to learn. :)
Changed to UMA_HISTOGRAM_SPARSE_SLOWLY with session length capped to 24 hours
and converted to a 10-minute bucket number to report.

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enterprise_user_session_metrics.h (right):

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.h:20: enum
SignInEventType {
On 2017/02/15 10:54:57, achuithb wrote:
> enum class is preferred now I believe:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Q5WmkAImanc
> 

Done.

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.h:41: // Records a
sign-in event by UserContext for an enrolled device.
On 2017/02/15 10:54:57, achuithb wrote:
> Is is_auto_login well understood in this part of the codebase?

Added comment to document what |is_auto_login| represents.

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.h:47: // called on
shutdown path and metrics are likely to lost. The stored value
On 2017/02/15 10:54:57, achuithb wrote:
> to be lost.

Done.

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/signin_specifics.h (right):

https://codereview.chromium.org/2692163003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/signin_specifics.h:16: SigninSpecifics();
On 2017/02/15 10:54:57, achuithb wrote:
> You should be able to drop this and delete the .cc, right?

Done.

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

https://codereview.chromium.org/2692163003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14153: +  <summary>Length of a public
session.</summary>
On 2017/02/15 00:16:57, Ilya Sherman wrote:
> Please clarify exactly what is measured here -- what are the start and stop
> events for a public session?  Note that this metric might be hard to
interpret,
> depending on how long a typical session length is.  For example, during a
> 30-minute span, I could either have six 5-minute sessions, or one 30-minute
> session.

Done. Let me know if it looks good now.

https://codereview.chromium.org/2692163003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14156: +<histogram
name="Enterprise.RegularUserSession.SessionLength" units="minutes">
On 2017/02/15 00:16:57, Ilya Sherman wrote:
> In the C++ code, this is "RegularSession" rather than "RegularUserSession"

Good catch. Fixed C++ code to use RegularUserSession.

https://codereview.chromium.org/2692163003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14159: +  <summary>Length of a regular
user session on an enrolled device.</summary>
On 2017/02/15 00:16:57, Ilya Sherman wrote:
> Ditto

Done.

[commit-bot: I haz the power, 2017-02-15 19:34:59]

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

[commit-bot: I haz the power, 2017-02-15 19:35:00]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ilya Sherman, 2017-02-15 23:42:44]

https://codereview.chromium.org/2692163003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enterprise_user_session_metrics.cc (right):

https://codereview.chromium.org/2692163003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:114: 10;
If you wanted to, you could actually then re-multiply this number by 10 (or
otherwise round to the nearest ten minutes), so that the units for the buckets
are still "minutes"

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

https://codereview.chromium.org/2692163003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14155: +    public account signs in. It
stops when the user signs out. Session length is
nit: "signs in" is repeated twice in this first sentence -- I think one of them
is a typo?

https://codereview.chromium.org/2692163003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14156: +    the time duration between
the start and stop events. It is recorded during
nit: s/time duration/duration

https://codereview.chromium.org/2692163003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14172: +    24 hours.
Do you expect that users often sign out within 24 hours?

[xiyuan, 2017-02-16 00:21:38]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[xiyuan, 2017-02-16 00:21:47]

https://codereview.chromium.org/2692163003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enterprise_user_session_metrics.cc (right):

https://codereview.chromium.org/2692163003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:114: 10;
On 2017/02/15 23:42:44, Ilya Sherman wrote:
> If you wanted to, you could actually then re-multiply this number by 10 (or
> otherwise round to the nearest ten minutes), so that the units for the buckets
> are still "minutes"

Done.

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

https://codereview.chromium.org/2692163003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14155: +    public account signs in. It
stops when the user signs out. Session length is
On 2017/02/15 23:42:44, Ilya Sherman wrote:
> nit: "signs in" is repeated twice in this first sentence -- I think one of
them
> is a typo?

Apparently I don't read what I typed. :) Done.

https://codereview.chromium.org/2692163003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14156: +    the time duration between
the start and stop events. It is recorded during
On 2017/02/15 23:42:44, Ilya Sherman wrote:
> nit: s/time duration/duration

Done.

https://codereview.chromium.org/2692163003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:14172: +    24 hours.
On 2017/02/15 23:42:44, Ilya Sherman wrote:
> Do you expect that users often sign out within 24 hours?

Nope.

The current hypothesis is that public session usually ends within 24 hours but
regular user session would last more than that (IMO, user probably never signs
out, only lock their devices). The UMA is to compare the behavior of the two
types of user sessions and the first 24 hours should be enough to see the
different pattern.

[commit-bot: I haz the power, 2017-02-16 00:22:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-02-16 00:29:08]

Metrics LGTM, thanks.

https://codereview.chromium.org/2692163003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enterprise_user_session_metrics.cc (right):

https://codereview.chromium.org/2692163003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:111: // (i.e.
every 10 minute).
By the way, it occurs to me that someone might later want to come along and make
this metric more fine-grained, and also remove that pesky 24-hours constraint. 
Let's add a brief comment documenting that it's important to limit the number of
buckets for this histogram to something reasonable, so future updates shouldn't
change it too much.

[commit-bot: I haz the power, 2017-02-16 01:46:34]

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

[commit-bot: I haz the power, 2017-02-16 01:46:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[achuithb, 2017-02-16 08:11:20]

lgtm

[xiyuan, 2017-02-16 16:25:53]

https://codereview.chromium.org/2692163003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enterprise_user_session_metrics.cc (right):

https://codereview.chromium.org/2692163003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enterprise_user_session_metrics.cc:111: // (i.e.
every 10 minute).
On 2017/02/16 00:29:08, Ilya Sherman wrote:
> By the way, it occurs to me that someone might later want to come along and
make
> this metric more fine-grained, and also remove that pesky 24-hours constraint.

> Let's add a brief comment documenting that it's important to limit the number
of
> buckets for this histogram to something reasonable, so future updates
shouldn't
> change it too much.

Done. Update comment to say:

  Note that sparse histogram is used here. It is important to limit the number
of buckets to something reasonable.

[xiyuan, 2017-02-16 16:43:05]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-02-16 16:43:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2692163003/#ps80001
(title: "update sparse histogram comment")

[commit-bot: I haz the power, 2017-02-16 16:43:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 20:18:57]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487263385064070,
"parent_rev": "d8d55fefdb0bcf5b7d25b7a4fb673731ac6a8b9a", "commit_rev":
"f59c187069079ab6422da75f73bbbdf376e62914"}

[commit-bot: I haz the power, 2017-02-16 20:19:33]

Description was changed from

==========
cros: Add enterprise user session metrics

- Add a "Enterprise.UserSession.Logins" enum UMA to track the
  following user session starts (aka login):
    - Regular user,
    - Manual public session,
    - Auto public session,
    - Manual kiosk,
    - Auto kiosk;
- Tracks user session length of regular user session and public
  seission in "Enterprise.RegularUserSEssion.SessionLength" and
  "Enterprise.PublicSession.SessionLength".

BUG=683306
==========

to

==========
cros: Add enterprise user session metrics

- Add a "Enterprise.UserSession.Logins" enum UMA to track the
  following user session starts (aka login):
    - Regular user,
    - Manual public session,
    - Auto public session,
    - Manual kiosk,
    - Auto kiosk;
- Tracks user session length of regular user session and public
  seission in "Enterprise.RegularUserSEssion.SessionLength" and
  "Enterprise.PublicSession.SessionLength".

BUG=683306

Review-Url: https://codereview.chromium.org/2692163003
Cr-Commit-Position: refs/heads/master@{#451079}
Committed:
https://chromium.googlesource.com/chromium/src/+/f59c187069079ab6422da75f73bb...
==========

[commit-bot: I haz the power, 2017-02-16 20:19:34]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f59c187069079ab6422da75f73bb...