Add Chrome OS enrollment status UMA stat

Add Chrome OS enrollment status UMA stat

BUG=462770
Committed: https://crrev.com/9ec1251edacac344eca129c821ba5d0f0c3cc788
Cr-Commit-Position: refs/heads/master@{#320152}

[Vladislav Kaznacheev, 2015-03-04 19:53:51]

kaznacheev@chromium.org changed reviewers:
+ asvitkine@chromium.org, nkostylev@chromium.org

[Vladislav Kaznacheev, 2015-03-04 19:53:53]

Please take a look.

I did some manual testing. Not sure if unit/browser test is feasible here.

Best regards,

  Vlad

[asvitkine_google, 2015-03-04 20:18:42]

asvitkine@google.com changed reviewers:
+ asvitkine@google.com

[asvitkine_google, 2015-03-04 20:18:44]

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
chrome/browser/metrics/chromeos_metrics_provider.cc:107: // Get the enrollment
status.
Expand this comment to explain what "enrollment status" is.

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
chrome/browser/metrics/chromeos_metrics_provider.cc:114: if
(!connector->IsEnterpriseManaged())
Can this status ever change during the execution of the same Chrome process?

https://chromiumcodereview.appspot.com/974273004/diff/20001/tools/metrics/his...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/974273004/diff/20001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:40227: + 
<owner>asvitkine@chromium.org</owner>
Please add yourself as the owner.

https://chromiumcodereview.appspot.com/974273004/diff/20001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:40228: +  <summary>Logs the enrollment
status.</summary>
The description should be more descriptive than this. What is an "enrollment
status" - it doesn't sound meaningful to someone not familiar with this. Mention
that it's logged with every UMA upload.

[Vladislav Kaznacheev, 2015-03-05 00:24:26]

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
chrome/browser/metrics/chromeos_metrics_provider.cc:107: // Get the enrollment
status.
On 2015/03/04 20:18:43, asvitkine wrote:
> Expand this comment to explain what "enrollment status" is.

Done.

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
chrome/browser/metrics/chromeos_metrics_provider.cc:114: if
(!connector->IsEnterpriseManaged())
On 2015/03/04 20:18:43, asvitkine wrote:
> Can this status ever change during the execution of the same Chrome process?

It can change once at most, if the enrollment happens during the login. After
that it never changes util the device is wiped. Is there is a better way to
report such seldom-changing metrics?

https://chromiumcodereview.appspot.com/974273004/diff/20001/tools/metrics/his...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/974273004/diff/20001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:40227: + 
<owner>asvitkine@chromium.org</owner>
On 2015/03/04 20:18:43, asvitkine wrote:
> Please add yourself as the owner.

Done.

https://chromiumcodereview.appspot.com/974273004/diff/20001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:40228: +  <summary>Logs the enrollment
status.</summary>
On 2015/03/04 20:18:43, asvitkine wrote:
> The description should be more descriptive than this. What is an "enrollment
> status" - it doesn't sound meaningful to someone not familiar with this.
Mention
> that it's logged with every UMA upload.

Done.

[Alexei Svitkine (slow), 2015-03-05 16:38:08]

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
chrome/browser/metrics/chromeos_metrics_provider.cc:114: if
(!connector->IsEnterpriseManaged())
On 2015/03/05 00:24:26, Vladislav Kaznacheev wrote:
> On 2015/03/04 20:18:43, asvitkine wrote:
> > Can this status ever change during the execution of the same Chrome process?
> 
> It can change once at most, if the enrollment happens during the login. After
> that it never changes util the device is wiped. Is there is a better way to
> report such seldom-changing metrics?

I've thought about doing a log rotation at the time of logon on CrOS, but we
don't have this support right now.

For now, I suggest keeping track of this as a member - i.e. init an instance
variable with this at class ctor time - and at the time of re-porting query it
again and if its the same, log it as that - and if it changed, log a special
"MIXED" value.

[Nikita (slow), 2015-03-05 18:03:48]

https://codereview.chromium.org/974273004/diff/40001/chrome/browser/metrics/c...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://codereview.chromium.org/974273004/diff/40001/chrome/browser/metrics/c...
chrome/browser/metrics/chromeos_metrics_provider.cc:115: return NON_MANAGED;
Please talk to tls@ whether we want to report devices that are not yet consumer,
nor managed. User has gone through out-of-box but has neither enrolled device,
nor signed as a user.

If device is used in that state only for browsing in Guest mode, than such
device will be reported as non_managed, but it is not yet consumer device and
can later become managed. Yet consumer device will not convert to managed one
without Powerwash.

[Thiemo Nagel, 2015-03-06 06:03:29]

tnagel@chromium.org changed reviewers:
+ tnagel@chromium.org

[Thiemo Nagel, 2015-03-06 06:03:29]

https://chromiumcodereview.appspot.com/974273004/diff/40001/tools/metrics/his...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/974273004/diff/40001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:46822: +  <int value="2" label="Managed
non-EDU"/>
In the future, I plan to extend this for other platforms, eg. reporting whether
a Windows machine is joined to an Active Directory domain or whether the
MSI-specific brand code is being used.  As far as I understand, that would be
easier to accomplish if EnrollmentStatus was interpreted as a bitmask.  I'd
suggest to change to:

<int value="0" label="Install Attributes managed"/>
<int value="1" label="EDU Domain"/>

In that scenario, a machine enrolled to an EDU domain would have two histogram
entries and an unenrolled machine would have zero entries.

This could later be extended eg. by:
<int value="2" label="Active Directory managed"/>
<int value="3" label="MSI installer"/>
...

Alexei, would that look reasonable to you?

[Alexei Svitkine (slow), 2015-03-10 15:11:45]

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
chrome/browser/metrics/chromeos_metrics_provider.cc:114: if
(!connector->IsEnterpriseManaged())
On 2015/03/05 16:38:07, Alexei Svitkine wrote:
> On 2015/03/05 00:24:26, Vladislav Kaznacheev wrote:
> > On 2015/03/04 20:18:43, asvitkine wrote:
> > > Can this status ever change during the execution of the same Chrome
process?
> > 
> > It can change once at most, if the enrollment happens during the login.
After
> > that it never changes util the device is wiped. Is there is a better way to
> > report such seldom-changing metrics?
> 
> I've thought about doing a log rotation at the time of logon on CrOS, but we
> don't have this support right now.
> 
> For now, I suggest keeping track of this as a member - i.e. init an instance
> variable with this at class ctor time - and at the time of re-porting query it
> again and if its the same, log it as that - and if it changed, log a special
> "MIXED" value.

Sorry, turns out I misunderstood your original comment - as I thought this
change of state was much more common than what you described (i.e. that it would
be the case on every log on).

Now that I understand your comment better, I think it's OK to not have a mixed
state and just log it as you're doing now.

[Alexei Svitkine (slow), 2015-03-10 15:16:11]

https://chromiumcodereview.appspot.com/974273004/diff/40001/tools/metrics/his...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/974273004/diff/40001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:46822: +  <int value="2" label="Managed
non-EDU"/>
On 2015/03/06 06:03:29, Thiemo Nagel wrote:
> In the future, I plan to extend this for other platforms, eg. reporting
whether
> a Windows machine is joined to an Active Directory domain or whether the
> MSI-specific brand code is being used.  As far as I understand, that would be
> easier to accomplish if EnrollmentStatus was interpreted as a bitmask.  I'd
> suggest to change to:
> 
> <int value="0" label="Install Attributes managed"/>
> <int value="1" label="EDU Domain"/>
> 
> In that scenario, a machine enrolled to an EDU domain would have two histogram
> entries and an unenrolled machine would have zero entries.
> 
> This could later be extended eg. by:
> <int value="2" label="Active Directory managed"/>
> <int value="3" label="MSI installer"/>
> ...
> 
> Alexei, would that look reasonable to you?

I'm not sure I'm understanding your suggestion. In your proposal, how is
"unmanaged" being tracked? It seems none of the cases above say "unmanaged"
whereas as it is currently in this CL, it is tracked as value 0. What am I
missing?

Note, that in general, UMA dashboards don't currently have built-in support for
dealing with bit masks. e.g. if you have values "1, 3 and 7" that both have bit
1 set, there's no easy way to ask the dashboards to just show me values where
that bit is set - without manually group-selecting those cases together, which
is no different than not using bit masks. But maybe I'm missing some other
advantage of the solution you're proposing - which is likely true, since as I
mentioned above, I don't actually understand your proposal. Could you clarify?

[Thiemo Nagel, 2015-03-10 15:37:12]

Thank you for your comments, Alexei!  I've replied in-line.

https://chromiumcodereview.appspot.com/974273004/diff/40001/tools/metrics/his...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/974273004/diff/40001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:46822: +  <int value="2" label="Managed
non-EDU"/>
On 2015/03/10 15:16:10, Alexei Svitkine wrote:
> I'm not sure I'm understanding your suggestion. In your proposal, how is
> "unmanaged" being tracked? It seems none of the cases above say "unmanaged"
> whereas as it is currently in this CL, it is tracked as value 0. What am I
> missing?

My intent is to track "unmanaged" by not setting any value, but it seems that
this does not play nice with the UMA design?

> Note, that in general, UMA dashboards don't currently have built-in support
for
> dealing with bit masks. e.g. if you have values "1, 3 and 7" that both have
bit
> 1 set, there's no easy way to ask the dashboards to just show me values where
> that bit is set - without manually group-selecting those cases together, which
> is no different than not using bit masks. But maybe I'm missing some other
> advantage of the solution you're proposing - which is likely true, since as I
> mentioned above, I don't actually understand your proposal. Could you clarify?

While on Chrome OS the "enterprise" vs. "consumer" distinction is easy to make
by referencing install attributes, we don't have that luxury on other platforms
and thus need get by with somewhat ambiguous and potentially overlapping
indicators as described above.  In order to study the systematics of these
ambiguous indicators (let's call them i1, i2, i3), I'd like to be able to look
at the stats for different logical combinations (such as eg. i1 && !i2 || i3). 
To that end, I had intended to treat the histogram as a bitmask and fill it once
for every indicator that is observed, but this doesn't seem to work out.

From your perspective, what would be the best way to implement multiple,
overlapping indicators and their logical combinations?

[Vladislav Kaznacheev, 2015-03-10 22:06:09]

It looks like I have resolved concerns from Alexei and Nikita.

Is everyone OK with committing this, or do we want to figure out how this could
be extended to meet tnagel's requirements? I would very much like to commit this
before M43 branch.

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://chromiumcodereview.appspot.com/974273004/diff/20001/chrome/browser/me...
chrome/browser/metrics/chromeos_metrics_provider.cc:114: if
(!connector->IsEnterpriseManaged())
On 2015/03/10 15:11:45, Alexei Svitkine wrote:
> On 2015/03/05 16:38:07, Alexei Svitkine wrote:
> > On 2015/03/05 00:24:26, Vladislav Kaznacheev wrote:
> > > On 2015/03/04 20:18:43, asvitkine wrote:
> > > > Can this status ever change during the execution of the same Chrome
> process?
> > > 
> > > It can change once at most, if the enrollment happens during the login.
> After
> > > that it never changes util the device is wiped. Is there is a better way
to
> > > report such seldom-changing metrics?
> > 
> > I've thought about doing a log rotation at the time of logon on CrOS, but we
> > don't have this support right now.
> > 
> > For now, I suggest keeping track of this as a member - i.e. init an instance
> > variable with this at class ctor time - and at the time of re-porting query
it
> > again and if its the same, log it as that - and if it changed, log a special
> > "MIXED" value.
> 
> Sorry, turns out I misunderstood your original comment - as I thought this
> change of state was much more common than what you described (i.e. that it
would
> be the case on every log on).
> 
> Now that I understand your comment better, I think it's OK to not have a mixed
> state and just log it as you're doing now.

Acknowledged.

https://chromiumcodereview.appspot.com/974273004/diff/40001/chrome/browser/me...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://chromiumcodereview.appspot.com/974273004/diff/40001/chrome/browser/me...
chrome/browser/metrics/chromeos_metrics_provider.cc:115: return NON_MANAGED;
On 2015/03/05 18:03:48, Nikita wrote:
> Please talk to tls@ whether we want to report devices that are not yet
consumer,
> nor managed. User has gone through out-of-box but has neither enrolled device,
> nor signed as a user.
> 
> If device is used in that state only for browsing in Guest mode, than such
> device will be reported as non_managed, but it is not yet consumer device and
> can later become managed. Yet consumer device will not convert to managed one
> without Powerwash.

Talked to tls@. She is OK with this implementation.

[Thiemo Nagel, 2015-03-11 09:51:31]

> Is everyone OK with committing this, or do we want to figure out how this
could
> be extended to meet tnagel's requirements? I would very much like to commit
this
> before M43 branch.

I'd greatly appreciate if we could come to a solution that can later be extended
without unnecessary effort.  We still have some time before the branch, so let's
please wait for Alexei's recommendation concerning extensibility.

[asvitkine_google, 2015-03-11 14:36:03]

https://chromiumcodereview.appspot.com/974273004/diff/40001/tools/metrics/his...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/974273004/diff/40001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:46822: +  <int value="2" label="Managed
non-EDU"/>
On 2015/03/10 15:37:12, Thiemo Nagel wrote:
> On 2015/03/10 15:16:10, Alexei Svitkine wrote:
> > I'm not sure I'm understanding your suggestion. In your proposal, how is
> > "unmanaged" being tracked? It seems none of the cases above say "unmanaged"
> > whereas as it is currently in this CL, it is tracked as value 0. What am I
> > missing?
> 
> My intent is to track "unmanaged" by not setting any value, but it seems that
> this does not play nice with the UMA design?

Yes, this is problematic. Because if we see no value, we don't know if it means
"unmanaged" or simply from clients that don't report it - e.g. old versions,
platforms where it's not implemented, etc. It would be misleading to just say
"unmanaged" for these rather than "unknown".

However, I actually don't think that reporting an "unmanaged" value is
incomatible with the bitmask proposal. 0 can still mean unmanaged, whereas
non-zero values can be intrepreted as bit-masks.

> 
> > Note, that in general, UMA dashboards don't currently have built-in support
> for
> > dealing with bit masks. e.g. if you have values "1, 3 and 7" that both have
> bit
> > 1 set, there's no easy way to ask the dashboards to just show me values
where
> > that bit is set - without manually group-selecting those cases together,
which
> > is no different than not using bit masks. But maybe I'm missing some other
> > advantage of the solution you're proposing - which is likely true, since as
I
> > mentioned above, I don't actually understand your proposal. Could you
clarify?
> 
> While on Chrome OS the "enterprise" vs. "consumer" distinction is easy to make
> by referencing install attributes, we don't have that luxury on other
platforms
> and thus need get by with somewhat ambiguous and potentially overlapping
> indicators as described above.  In order to study the systematics of these
> ambiguous indicators (let's call them i1, i2, i3), I'd like to be able to look
> at the stats for different logical combinations (such as eg. i1 && !i2 || i3).

> To that end, I had intended to treat the histogram as a bitmask and fill it
once
> for every indicator that is observed, but this doesn't seem to work out.
> 
> From your perspective, what would be the best way to implement multiple,
> overlapping indicators and their logical combinations?

We don't have a good way to implement this. If it's just a regular histogram,
then it's fine to use bit-masks with the inconvenience that you have to have
enum entries for all possible bit combinations (which doesn't scale well but can
work ok for a small number of bits - e.g. 3). But actually we want to make this
a filter that can be used to slice other histograms. Which would require a lot
of logic specific to this histogram in our dashboards to get meaning out of the
values - which we prefer not to add (we don't want logic that's custom tailored
to specific histograms).

How about this:

Have two histograms. One that logs the overall summary stats (e.g. as it is
here) and a second histogram (that can be added later when we implement this for
Windows) that logs more details in the bit-mask approach you suggest. This way,
we can move forward with this approach now and add the more detailed cases once
we do the Windows implementation - which won't need to conflict with this
initial approach.

[Thiemo Nagel, 2015-03-11 17:44:53]

On 2015/03/11 14:36:03, asvitkine wrote:
> Have two histograms. One that logs the overall summary stats (e.g. as it is
> here) and a second histogram (that can be added later when we implement this
for
> Windows) that logs more details in the bit-mask approach you suggest. This
way,
> we can move forward with this approach now and add the more detailed cases
once
> we do the Windows implementation - which won't need to conflict with this
> initial approach.

Thanks a lot for your input!  You're certainly better qualified to reason about
UMA than me, thus I happily accept your suggestion.  :)

[Thiemo Nagel, 2015-03-11 17:45:02]

https://chromiumcodereview.appspot.com/974273004/diff/60001/tools/metrics/his...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/974273004/diff/60001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:46986: +  <int value="2" label="Managed
EDU"/>
Is there a specific reason for skipping value="1"?

[Vladislav Kaznacheev, 2015-03-11 17:56:38]

Fixed bucket numbering

[Vladislav Kaznacheev, 2015-03-11 18:04:11]

Rebase

[Vladislav Kaznacheev, 2015-03-11 18:05:32]

https://chromiumcodereview.appspot.com/974273004/diff/60001/tools/metrics/his...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/974273004/diff/60001/tools/metrics/his...
tools/metrics/histograms/histograms.xml:46986: +  <int value="2" label="Managed
EDU"/>
On 2015/03/11 17:45:02, Thiemo Nagel wrote:
> Is there a specific reason for skipping value="1"?

Thanks for catching this. This was me experimenting with more values (such as
MIXED).

Fixed.

[Alexei Svitkine (slow), 2015-03-11 18:53:50]

asvitkine@chromium.org changed reviewers:
- asvitkine@google.com

[Alexei Svitkine (slow), 2015-03-11 18:53:51]

lgtm % comment

thanks!

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

https://codereview.chromium.org/974273004/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46949: +  <int value="2" label="Managed
non-EDU"/>
Add the error case from your enum too.

[Vladislav Kaznacheev, 2015-03-11 18:57:42]

Addressed comment

[Vladislav Kaznacheev, 2015-03-11 18:58:21]

The CQ bit was checked by kaznacheev@chromium.org

[Vladislav Kaznacheev, 2015-03-11 18:58:56]

Thanks! Committing.

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

https://codereview.chromium.org/974273004/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:46949: +  <int value="2" label="Managed
non-EDU"/>
On 2015/03/11 18:53:51, Alexei Svitkine (slow) wrote:
> Add the error case from your enum too.

Done.

[commit-bot: I haz the power, 2015-03-11 19:38:34]

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

[commit-bot: I haz the power, 2015-03-11 21:49:55]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-11 21:50:39]

Patchset 7 (id:??) landed as
https://crrev.com/9ec1251edacac344eca129c821ba5d0f0c3cc788
Cr-Commit-Position: refs/heads/master@{#320152}