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

chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/policy/device_cloud_policy_store_chromeos.h"
 
 #include "base/bind.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/sequenced_task_runner.h"
 #include "chrome/browser/chromeos/policy/device_policy_decoder_chromeos.h"
 #include "chrome/browser/chromeos/policy/enterprise_install_attributes.h"
 #include "chrome/browser/chromeos/policy/proto/chrome_device_policy.pb.h"
 #include "policy/proto/device_management_backend.pb.h"
 
 namespace em = enterprise_management;
 
 namespace policy {
 
 DeviceCloudPolicyStoreChromeOS::DeviceCloudPolicyStoreChromeOS(
     chromeos::DeviceSettingsService* device_settings_service,
     EnterpriseInstallAttributes* install_attributes,
     scoped_refptr<base::SequencedTaskRunner> background_task_runner)
     : device_settings_service_(device_settings_service),
       install_attributes_(install_attributes),
       background_task_runner_(background_task_runner),
+      uma_done_(false),
       weak_factory_(this) {
   device_settings_service_->AddObserver(this);
 }
 
 DeviceCloudPolicyStoreChromeOS::~DeviceCloudPolicyStoreChromeOS() {
   device_settings_service_->RemoveObserver(this);
 }
 
 void DeviceCloudPolicyStoreChromeOS::Store(
     const em::PolicyFetchResponse& policy) {
@@ skipping 85 matching lines @@
       validator->policy().Pass(),
       base::Bind(&DeviceCloudPolicyStoreChromeOS::OnPolicyStored,
                  weak_factory_.GetWeakPtr()));
 }
 
 void DeviceCloudPolicyStoreChromeOS::OnPolicyStored() {
   UpdateFromService();
 }
 
 void DeviceCloudPolicyStoreChromeOS::UpdateFromService() {
   if (!install_attributes_->IsEnterpriseDevice()) {

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

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).

[Thiemo Nagel, 2014/05/26 10:53:55]

That's going to be subject of a separate CL.

     status_ = STATUS_BAD_STATE;
     NotifyStoreError();
     return;
   }
 
-  switch (device_settings_service_->status()) {
+  // Fill UMA histogram once per session.  Skip temp validation error because it
+  // is not a definitive result (policy load will be retried).
+  const chromeos::DeviceSettingsService::Status status
+      = device_settings_service_->status();

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

nit: equals sign goes on previous line.

[Thiemo Nagel, 2014/05/26 10:53:55]

Done.

+  if (!uma_done_ &&
+      status != chromeos::DeviceSettingsService::STORE_TEMP_VALIDATION_ERROR) {
+    uma_done_ = true;
+    int uma_status = static_cast<int>(status);
+    if (status == chromeos::DeviceSettingsService::STORE_SUCCESS &&
+        !device_settings_service_->policy_data()->has_request_token())

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

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.

[Thiemo Nagel, 2014/05/26 10:53:55]

Thanks.  I've added a check.  (I'd like to keep the change as compact as
possible to facilitate back-merging.)

+      uma_status = 7;  // Success but has no DM token.

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

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?

[Thiemo Nagel, 2014/05/26 10:53:55]

I've simplified the CL so that this doesn't apply anymore.

+    UMA_HISTOGRAM_SPARSE_SLOWLY("Enterprise.EnrolledPolicyLoad", uma_status);
+  }
+
+  switch (status) {
     case chromeos::DeviceSettingsService::STORE_SUCCESS: {
       status_ = STATUS_OK;
       policy_.reset(new em::PolicyData());
       if (device_settings_service_->policy_data())
         policy_->MergeFrom(*device_settings_service_->policy_data());
 
       PolicyMap new_policy_map;
       if (is_managed()) {
         DecodeDevicePolicy(*device_settings_service_->device_settings(),
                            &new_policy_map, install_attributes_);
@@ skipping 15 matching lines @@
     case chromeos::DeviceSettingsService::STORE_VALIDATION_ERROR:
     case chromeos::DeviceSettingsService::STORE_TEMP_VALIDATION_ERROR:
       status_ = STATUS_LOAD_ERROR;
       break;
   }
 
   NotifyStoreError();
 }
 
 }  // namespace policy