Add UMA for policy load result of enrolled Chrome OS devices.

Add UMA for policy load result on enrolled Chrome OS devices.

Log only one entry per session to reduce distortion due to potentially different usage patterns depending on the policy load result.  To facilitate correct accounting, the status of DeviceSettingsService is switched to STORE_VALIDATION_ERROR once retries for STORE_TEMP_VALIDATION_ERROR have been exhausted.

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

[Thiemo Nagel, 2014-05-23 19:58:56]

Hi Julian, Mattias,

could you please take a look at this CL which is intended to quickly provide us
with knowledge of the approximate size of Population 2?  In order to facilitate
back-merging, I've tried to keep it as simple as possible while still being
useful.

I'd appreciate your input on how to test this, maybe Julian can help me on
Monday to manually run the code on an intentionally corrupted Chromebook?

Thank you and have a nice weekend,
Thiemo

[Mattias Nissler (ping if slow), 2014-05-26 07:54:11]

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:132: if
(!install_attributes_->IsEnterpriseDevice()) {
It would make sense to change the reporting code to also cover the situation of
!IsEnterpriseDevice(), but we do have a DMToken (indicating that install attrs
gone south).

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:141: =
device_settings_service_->status();
nit: equals sign goes on previous line.

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:147:
!device_settings_service_->policy_data()->has_request_token())
You're dereferencing policy_data() here without checking whether it is valid,
relying on the (currently accurate assumption) that STORE_SUCCESS, implies the
pointer is good. Note that the code below is more cautious - either document
your assumption in the device_settings_service.h and change the code below, or
add a check here.

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:148:
uma_status = 7;  // Success but has no DM token.
This is dangerous, as whatever changes we make to CloudPolicyStore::Status in
the future will hose this UMA histogram. Why not just log a second boolean
histogram?

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.h
(right):

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.h:79: //
Ensure that UMA stats are only reported once per session.
This is not per session, but per Chrome restart. And you definitively will see
distortion due to different usage patterns the way you have it now. Other code
makes decisions based on time, i.e. reporting only if we haven't for the last 24
hours - maybe that's more appropriate here?

[Thiemo Nagel, 2014-05-26 10:53:54]

Hi Mattias,

thank you for your comments!  As discussed, I've broken down the change now to
the absolute minimum.

Could you please take another look?

Thank you!
Thiemo

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:132: if
(!install_attributes_->IsEnterpriseDevice()) {
That's going to be subject of a separate CL.

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:141: =
device_settings_service_->status();
On 2014/05/26 07:54:11, Mattias Nissler wrote:
> nit: equals sign goes on previous line.

Done.

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:147:
!device_settings_service_->policy_data()->has_request_token())
Thanks.  I've added a check.  (I'd like to keep the change as compact as
possible to facilitate back-merging.)

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:148:
uma_status = 7;  // Success but has no DM token.
I've simplified the CL so that this doesn't apply anymore.

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.h
(right):

https://codereview.chromium.org/295103013/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.h:79: //
Ensure that UMA stats are only reported once per session.
I'd like to keep it as simple as possible, thus I've updated the description
instead of adding a once-in-24-hours logic.

[Mattias Nissler (ping if slow), 2014-05-26 12:06:09]

LGTM

[Thiemo Nagel, 2014-05-26 12:42:11]

On 2014/05/26 12:06:09, Mattias Nissler wrote:
> LGTM

Thank you very much!

[Thiemo Nagel, 2014-05-26 12:51:12]

Hi Alexei,

may I kindly ask you to take a quick look at histograms.xml? I've added a
histogram there which is intended to help debugging an intricate enterprise
enrollment problem that has been observed for some time.  (The changes to the
other files are approved already.)

Thank you and best regards,
Thiemo

[Alexei Svitkine (slow), 2014-05-26 17:27:11]

https://codereview.chromium.org/295103013/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/295103013/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:146:
"Enterprise.EnrolledPolicyHasDMToken",
This looks like a boolean histogram, so there's no need for it to be sparse. Use
UMA_HISTOGRAM_BOOLEAN instead.

https://codereview.chromium.org/295103013/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:149:
!device_settings_service_->policy_data()->has_request_token());
Nit: Refactor this into a local bool and use that var in the macro.

[Thiemo Nagel, 2014-05-26 17:54:29]

Hi Alexei,

many thanks for your comments!  I've uploaded a patchset to address them.  Could
you please take another look?

Cheers,
Thiemo

https://codereview.chromium.org/295103013/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc
(right):

https://codereview.chromium.org/295103013/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:146:
"Enterprise.EnrolledPolicyHasDMToken",
Thanks for catching this!  I had started with a more complex histogram and
simplified it until there were only two bins left...

https://codereview.chromium.org/295103013/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc:149:
!device_settings_service_->policy_data()->has_request_token());
On 2014/05/26 17:27:11, Alexei Svitkine wrote:
> Nit: Refactor this into a local bool and use that var in the macro.

Done.

[Alexei Svitkine (slow), 2014-05-26 18:00:59]

LGTM

[Thiemo Nagel, 2014-05-27 09:28:42]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-05-27 09:29:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tnagel@chromium.org/295103013/130001

[commit-bot: I haz the power, 2014-05-27 10:36:03]

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_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-27 12:00:09]

Change committed as 272949