Separate UMA histograms for user and device policy invalidation

This CL separates the UMA histograms used for user and device policy
invalidation.

BUG=358699
TEST=Extended unit tests

Review URL: https://codereview.chromium.org/465433002

Cr-Commit-Position: refs/heads/master@{#289065}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@289065 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 171 insertions, 64 deletions

diff --git a/chrome/browser/policy/cloud/cloud_policy_invalidator.cc b/chrome/browser/policy/cloud/cloud_policy_invalidator.cc
index 6ba5047..ee83bbe 100644
--- a/chrome/browser/policy/cloud/cloud_policy_invalidator.cc
+++ b/chrome/browser/policy/cloud/cloud_policy_invalidator.cc
@@ -31,10 +31,12 @@ const int CloudPolicyInvalidator::kUnknownVersionIgnorePeriod = 30;
 const int CloudPolicyInvalidator::kMaxInvalidationTimeDelta = 300;
 
 CloudPolicyInvalidator::CloudPolicyInvalidator(
+    enterprise_management::DeviceRegisterRequest::Type type,
     CloudPolicyCore* core,
     const scoped_refptr<base::SequencedTaskRunner>& task_runner,
     scoped_ptr<base::Clock> clock)
     : state_(UNINITIALIZED),
+      type_(type),
       core_(core),
       task_runner_(task_runner),
       clock_(clock.Pass()),
@@ -141,11 +143,16 @@ void CloudPolicyInvalidator::OnStoreLoaded(CloudPolicyStore* store) {
   bool policy_changed = IsPolicyChanged(store->policy());
 
   if (is_registered_) {
-    // Update the kMetricPolicyRefresh histogram.
-    UMA_HISTOGRAM_ENUMERATION(
-        kMetricPolicyRefresh,
-        GetPolicyRefreshMetric(policy_changed),
-        METRIC_POLICY_REFRESH_SIZE);
+    // Update the kMetricDevicePolicyRefresh/kMetricUserPolicyRefresh histogram.
+    if (type_ == enterprise_management::DeviceRegisterRequest::DEVICE) {
+      UMA_HISTOGRAM_ENUMERATION(kMetricDevicePolicyRefresh,
+                                GetPolicyRefreshMetric(policy_changed),
+                                METRIC_POLICY_REFRESH_SIZE);
+    } else {
+      UMA_HISTOGRAM_ENUMERATION(kMetricUserPolicyRefresh,
+                                GetPolicyRefreshMetric(policy_changed),
+                                METRIC_POLICY_REFRESH_SIZE);
+    }
 
     // If the policy was invalid and the version stored matches the latest
     // invalidation version, acknowledge the latest invalidation.
@@ -189,10 +196,18 @@ void CloudPolicyInvalidator::HandleInvalidation(
 
   // Ignore the invalidation if it is expired.
   bool is_expired = IsInvalidationExpired(version);
-  UMA_HISTOGRAM_ENUMERATION(
-      kMetricPolicyInvalidations,
-      GetInvalidationMetric(payload.empty(), is_expired),
-      POLICY_INVALIDATION_TYPE_SIZE);
+
+  if (type_ == enterprise_management::DeviceRegisterRequest::DEVICE) {
+    UMA_HISTOGRAM_ENUMERATION(
+        kMetricDevicePolicyInvalidations,
+        GetInvalidationMetric(payload.empty(), is_expired),
+        POLICY_INVALIDATION_TYPE_SIZE);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION(
+        kMetricUserPolicyInvalidations,
+        GetInvalidationMetric(payload.empty(), is_expired),
+        POLICY_INVALIDATION_TYPE_SIZE);
+  }
   if (is_expired) {
     invalidation.Acknowledge();
     return;
diff --git a/chrome/browser/policy/cloud/cloud_policy_invalidator.h b/chrome/browser/policy/cloud/cloud_policy_invalidator.h
index ef51880..6653a5d 100644
--- a/chrome/browser/policy/cloud/cloud_policy_invalidator.h
+++ b/chrome/browser/policy/cloud/cloud_policy_invalidator.h
@@ -9,6 +9,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
@@ -18,6 +19,7 @@
 #include "components/policy/core/common/cloud/cloud_policy_core.h"
 #include "components/policy/core/common/cloud/cloud_policy_store.h"
 #include "google/cacheinvalidation/include/types.h"
+#include "policy/proto/device_management_backend.pb.h"
 
 namespace base {
 class Clock;
@@ -56,12 +58,14 @@ class CloudPolicyInvalidator : public syncer::InvalidationHandler,
   // invalidation timestamps when determining if an invalidation is expired.
   static const int kMaxInvalidationTimeDelta;
 
+  // |type| indicates the policy type that this invalidator is responsible for.
   // |core| is the cloud policy core which connects the various policy objects.
   // It must remain valid until Shutdown is called.
   // |task_runner| is used for scheduling delayed tasks. It must post tasks to
   // the main policy thread.
   // |clock| is used to get the current time.
   CloudPolicyInvalidator(
+      enterprise_management::DeviceRegisterRequest::Type type,
       CloudPolicyCore* core,
       const scoped_refptr<base::SequencedTaskRunner>& task_runner,
       scoped_ptr<base::Clock> clock);
@@ -156,6 +160,9 @@ class CloudPolicyInvalidator : public syncer::InvalidationHandler,
   };
   State state_;
 
+  // The policy type this invalidator is responsible for.
+  const enterprise_management::DeviceRegisterRequest::Type type_;
+
   // The cloud policy core.
   CloudPolicyCore* core_;
 
diff --git a/chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc b/chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc
index 75f7ec4..3e37658 100644
--- a/chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc
+++ b/chrome/browser/policy/cloud/cloud_policy_invalidator_unittest.cc
@@ -20,6 +20,7 @@
 #include "base/values.h"
 #include "chrome/browser/invalidation/fake_invalidation_service.h"
 #include "chrome/browser/policy/cloud/cloud_policy_invalidator.h"
+#include "chrome/browser/policy/cloud/user_cloud_policy_invalidator.h"
 #include "components/invalidation/invalidation_util.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/core/common/cloud/cloud_policy_core.h"
@@ -29,10 +30,11 @@
 #include "components/policy/core/common/cloud/mock_cloud_policy_store.h"
 #include "components/policy/core/common/policy_types.h"
 #include "policy/policy_constants.h"
-#include "policy/proto/device_management_backend.pb.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace em = enterprise_management;
+
 namespace policy {
 
 class CloudPolicyInvalidatorTest : public testing::Test {
@@ -46,8 +48,6 @@ class CloudPolicyInvalidatorTest : public testing::Test {
 
   CloudPolicyInvalidatorTest();
 
-  virtual void SetUp() OVERRIDE;
-
   virtual void TearDown() OVERRIDE;
 
   // Starts the invalidator which will be tested.
@@ -147,10 +147,6 @@ class CloudPolicyInvalidatorTest : public testing::Test {
   // invalidation service.
   bool IsInvalidatorRegistered();
 
-  // Get the current count for the given metric.
-  base::HistogramBase::Count GetCount(MetricPolicyRefresh metric);
-  base::HistogramBase::Count GetInvalidationCount(PolicyInvalidationType type);
-
   // Advance the test clock.
   void AdvanceClock(base::TimeDelta delta);
 
@@ -164,6 +160,9 @@ class CloudPolicyInvalidatorTest : public testing::Test {
   // Get an invalidation version for the given time.
   int64 GetVersion(base::Time time);
 
+  // Get the policy type that the |invalidator_| is responsible for.
+  virtual em::DeviceRegisterRequest::Type GetPolicyType() const;
+
  private:
   // Checks that the policy was refreshed due to an invalidation with the given
   // base delay.
@@ -175,10 +174,6 @@ class CloudPolicyInvalidatorTest : public testing::Test {
   // Returns the object id of the given policy object.
   const invalidation::ObjectId& GetPolicyObjectId(PolicyObject object) const;
 
-  // Get histogram samples for the given histogram.
-  scoped_ptr<base::HistogramSamples> GetHistogramSamples(
-      const std::string& name) const;
-
   base::MessageLoop loop_;
 
   // Objects the invalidator depends on.
@@ -203,12 +198,6 @@ class CloudPolicyInvalidatorTest : public testing::Test {
 
   // The currently used policy value.
   const char* policy_value_cur_;
-
-  // Stores starting histogram counts for kMetricPolicyRefresh.
-  scoped_ptr<base::HistogramSamples> refresh_samples_;
-
-  // Stores starting histogram counts for kMetricPolicyInvalidations.
-  scoped_ptr<base::HistogramSamples> invalidations_samples_;
 };
 
 CloudPolicyInvalidatorTest::CloudPolicyInvalidatorTest()
@@ -228,12 +217,6 @@ CloudPolicyInvalidatorTest::CloudPolicyInvalidatorTest()
       base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(987654321));
 }
 
-void CloudPolicyInvalidatorTest::SetUp() {
-  base::StatisticsRecorder::Initialize();
-  refresh_samples_ = GetHistogramSamples(kMetricPolicyRefresh);
-  invalidations_samples_ = GetHistogramSamples(kMetricPolicyInvalidations);
-}
-
 void CloudPolicyInvalidatorTest::TearDown() {
   if (invalidator_)
     invalidator_->Shutdown();
@@ -244,6 +227,7 @@ void CloudPolicyInvalidatorTest::StartInvalidator(
     bool initialize,
     bool start_refresh_scheduler) {
   invalidator_.reset(new CloudPolicyInvalidator(
+      GetPolicyType(),
       &core_,
       task_runner_,
       scoped_ptr<base::Clock>(clock_)));
@@ -287,8 +271,7 @@ void CloudPolicyInvalidatorTest::StorePolicy(
     int64 invalidation_version,
     bool policy_changed,
     const base::Time& time) {
-  enterprise_management::PolicyData* data =
-      new enterprise_management::PolicyData();
+  em::PolicyData* data = new em::PolicyData();
   if (object != POLICY_OBJECT_NONE) {
     data->set_invalidation_source(GetPolicyObjectId(object).source());
     data->set_invalidation_name(GetPolicyObjectId(object).name());
@@ -388,18 +371,6 @@ bool CloudPolicyInvalidatorTest::IsInvalidatorRegistered() {
       .GetRegisteredIds(invalidator_.get()).empty();
 }
 
-base::HistogramBase::Count CloudPolicyInvalidatorTest::GetCount(
-    MetricPolicyRefresh metric) {
-  return GetHistogramSamples(kMetricPolicyRefresh)->GetCount(metric) -
-      refresh_samples_->GetCount(metric);
-}
-
-base::HistogramBase::Count CloudPolicyInvalidatorTest::GetInvalidationCount(
-    PolicyInvalidationType type) {
-  return GetHistogramSamples(kMetricPolicyInvalidations)->GetCount(type) -
-      invalidations_samples_->GetCount(type);
-}
-
 void CloudPolicyInvalidatorTest::AdvanceClock(base::TimeDelta delta) {
   clock_->Advance(delta);
 }
@@ -416,6 +387,11 @@ int64 CloudPolicyInvalidatorTest::GetVersion(base::Time time) {
   return (time - base::Time::UnixEpoch()).InMicroseconds();
 }
 
+em::DeviceRegisterRequest::Type
+CloudPolicyInvalidatorTest::GetPolicyType() const {
+  return UserCloudPolicyInvalidator::GetPolicyType();
+}
+
 bool CloudPolicyInvalidatorTest::CheckPolicyRefreshed(base::TimeDelta delay) {
   base::TimeDelta max_delay = delay + base::TimeDelta::FromMilliseconds(
       CloudPolicyInvalidator::kMaxFetchDelayMin);
@@ -453,16 +429,6 @@ const invalidation::ObjectId& CloudPolicyInvalidatorTest::GetPolicyObjectId(
   return object == POLICY_OBJECT_A ? object_id_a_ : object_id_b_;
 }
 
-scoped_ptr<base::HistogramSamples>
-    CloudPolicyInvalidatorTest::GetHistogramSamples(
-        const std::string& name) const {
-  base::HistogramBase* histogram =
-      base::StatisticsRecorder::FindHistogram(name);
-  if (!histogram)
-    return scoped_ptr<base::HistogramSamples>(new base::SampleMap());
-  return histogram->SnapshotSamples();
-}
-
 TEST_F(CloudPolicyInvalidatorTest, Uninitialized) {
   // No invalidations should be processed if the invalidator is not initialized.
   StartInvalidator(false /* initialize */, true /* start_refresh_scheduler */);
@@ -795,7 +761,86 @@ TEST_F(CloudPolicyInvalidatorTest, Disconnect) {
   EXPECT_FALSE(InvalidationsEnabled());
 }
 
-TEST_F(CloudPolicyInvalidatorTest, RefreshMetricsUnregistered) {
+class CloudPolicyInvalidatorUserTypedTest
+    : public CloudPolicyInvalidatorTest,
+      public testing::WithParamInterface<em::DeviceRegisterRequest::Type>  {
+ protected:
+  CloudPolicyInvalidatorUserTypedTest();
+  virtual ~CloudPolicyInvalidatorUserTypedTest();
+
+  // CloudPolicyInvalidatorTest:
+  virtual void SetUp() OVERRIDE;
+
+  // Get the current count for the given metric.
+  base::HistogramBase::Count GetCount(MetricPolicyRefresh metric);
+  base::HistogramBase::Count GetInvalidationCount(PolicyInvalidationType type);
+
+ private:
+  // CloudPolicyInvalidatorTest:
+  virtual em::DeviceRegisterRequest::Type GetPolicyType() const OVERRIDE;
+
+  // Get histogram samples for the given histogram.
+  scoped_ptr<base::HistogramSamples> GetHistogramSamples(
+      const std::string& name) const;
+
+  // Stores starting histogram counts for kMetricPolicyRefresh.
+  scoped_ptr<base::HistogramSamples> refresh_samples_;
+
+  // Stores starting histogram counts for kMetricPolicyInvalidations.
+  scoped_ptr<base::HistogramSamples> invalidations_samples_;
+
+  DISALLOW_COPY_AND_ASSIGN(CloudPolicyInvalidatorUserTypedTest);
+};
+
+CloudPolicyInvalidatorUserTypedTest::CloudPolicyInvalidatorUserTypedTest() {
+}
+
+CloudPolicyInvalidatorUserTypedTest::~CloudPolicyInvalidatorUserTypedTest() {
+}
+
+void CloudPolicyInvalidatorUserTypedTest::SetUp() {
+  base::StatisticsRecorder::Initialize();
+  refresh_samples_ = GetHistogramSamples(
+      GetPolicyType() == em::DeviceRegisterRequest::DEVICE ?
+          kMetricDevicePolicyRefresh : kMetricUserPolicyRefresh);
+  invalidations_samples_ = GetHistogramSamples(
+      GetPolicyType() == em::DeviceRegisterRequest::DEVICE ?
+          kMetricDevicePolicyInvalidations : kMetricUserPolicyInvalidations);
+}
+
+base::HistogramBase::Count CloudPolicyInvalidatorUserTypedTest::GetCount(
+    MetricPolicyRefresh metric) {
+  return GetHistogramSamples(
+      GetPolicyType() == em::DeviceRegisterRequest::DEVICE ?
+          kMetricDevicePolicyRefresh : kMetricUserPolicyRefresh)->
+              GetCount(metric) - refresh_samples_->GetCount(metric);
+}
+
+base::HistogramBase::Count
+CloudPolicyInvalidatorUserTypedTest::GetInvalidationCount(
+    PolicyInvalidationType type) {
+  return GetHistogramSamples(
+      GetPolicyType() == em::DeviceRegisterRequest::DEVICE ?
+          kMetricDevicePolicyInvalidations : kMetricUserPolicyInvalidations)->
+          GetCount(type) - invalidations_samples_->GetCount(type);
+}
+
+em::DeviceRegisterRequest::Type
+CloudPolicyInvalidatorUserTypedTest::GetPolicyType() const {
+  return GetParam();
+}
+
+scoped_ptr<base::HistogramSamples>
+CloudPolicyInvalidatorUserTypedTest::GetHistogramSamples(
+    const std::string& name) const {
+  base::HistogramBase* histogram =
+      base::StatisticsRecorder::FindHistogram(name);
+  if (!histogram)
+    return scoped_ptr<base::HistogramSamples>(new base::SampleMap());
+  return histogram->SnapshotSamples();
+}
+
+TEST_P(CloudPolicyInvalidatorUserTypedTest, RefreshMetricsUnregistered) {
   // Store loads occurring before invalidation registration are not counted.
   StartInvalidator();
   StorePolicy(POLICY_OBJECT_NONE, 0, false /* policy_changed */);
@@ -807,7 +852,7 @@ TEST_F(CloudPolicyInvalidatorTest, RefreshMetricsUnregistered) {
   EXPECT_EQ(0, GetCount(METRIC_POLICY_REFRESH_INVALIDATED_UNCHANGED));
 }
 
-TEST_F(CloudPolicyInvalidatorTest, RefreshMetricsNoInvalidations) {
+TEST_P(CloudPolicyInvalidatorUserTypedTest, RefreshMetricsNoInvalidations) {
   // Store loads occurring while registered should be differentiated depending
   // on whether the invalidation service was enabled or not.
   StorePolicy(POLICY_OBJECT_A);
@@ -858,7 +903,7 @@ TEST_F(CloudPolicyInvalidatorTest, RefreshMetricsNoInvalidations) {
   EXPECT_EQ(0, GetCount(METRIC_POLICY_REFRESH_INVALIDATED_UNCHANGED));
 }
 
-TEST_F(CloudPolicyInvalidatorTest, RefreshMetricsInvalidation) {
+TEST_P(CloudPolicyInvalidatorUserTypedTest, RefreshMetricsInvalidation) {
   // Store loads after an invalidation are counted as invalidated, even if
   // the loads do not result in the invalidation being acknowledged.
   StartInvalidator();
@@ -887,7 +932,7 @@ TEST_F(CloudPolicyInvalidatorTest, RefreshMetricsInvalidation) {
   EXPECT_EQ(1, GetCount(METRIC_POLICY_REFRESH_INVALIDATED_UNCHANGED));
 }
 
-TEST_F(CloudPolicyInvalidatorTest, ExpiredInvalidations) {
+TEST_P(CloudPolicyInvalidatorUserTypedTest, ExpiredInvalidations) {
   StorePolicy(POLICY_OBJECT_A, 0, false, Now());
   StartInvalidator();
 
@@ -948,4 +993,27 @@ TEST_F(CloudPolicyInvalidatorTest, ExpiredInvalidations) {
   EXPECT_EQ(2, GetInvalidationCount(POLICY_INVALIDATION_TYPE_EXPIRED));
 }
 
+#if defined(OS_CHROMEOS)
+INSTANTIATE_TEST_CASE_P(
+    CloudPolicyInvalidatorUserTypedTestInstance,
+    CloudPolicyInvalidatorUserTypedTest,
+    testing::Values(em::DeviceRegisterRequest::USER,
+                    em::DeviceRegisterRequest::DEVICE));
+#elif defined(OS_ANDROID)
+INSTANTIATE_TEST_CASE_P(
+    CloudPolicyInvalidatorUserTypedTestInstance,
+    CloudPolicyInvalidatorUserTypedTest,
+    testing::Values(em::DeviceRegisterRequest::ANDROID_BROWSER));
+#elif defined(OS_IOS)
+INSTANTIATE_TEST_CASE_P(
+    CloudPolicyInvalidatorUserTypedTestInstance,
+    CloudPolicyInvalidatorUserTypedTest,
+    testing::Values(em::DeviceRegisterRequest::IOS_BROWSER));
+#else
+INSTANTIATE_TEST_CASE_P(
+    CloudPolicyInvalidatorUserTypedTestInstance,
+    CloudPolicyInvalidatorUserTypedTest,
+    testing::Values(em::DeviceRegisterRequest::BROWSER));
+#endif
+
 }  // namespace policy
diff --git a/chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc b/chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc
index 504abafc..7770616 100644
--- a/chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc
+++ b/chrome/browser/policy/cloud/user_cloud_policy_invalidator.cc
@@ -19,10 +19,10 @@ namespace policy {
 UserCloudPolicyInvalidator::UserCloudPolicyInvalidator(
     Profile* profile,
     CloudPolicyManager* policy_manager)
-    : CloudPolicyInvalidator(
-          policy_manager->core(),
-          base::MessageLoopProxy::current(),
-          scoped_ptr<base::Clock>(new base::DefaultClock())),
+    : CloudPolicyInvalidator(GetPolicyType(),
+                             policy_manager->core(),
+                             base::MessageLoopProxy::current(),
+                             scoped_ptr<base::Clock>(new base::DefaultClock())),
       profile_(profile) {
   DCHECK(profile);
 
@@ -38,6 +38,20 @@ UserCloudPolicyInvalidator::UserCloudPolicyInvalidator(
                  content::Source<Profile>(profile));
 }
 
+// static
+enterprise_management::DeviceRegisterRequest::Type
+UserCloudPolicyInvalidator::GetPolicyType() {
+#if defined(OS_CHROMEOS)
+  return enterprise_management::DeviceRegisterRequest::USER;
+#elif defined(OS_ANDROID)
+  return enterprise_management::DeviceRegisterRequest::ANDROID_BROWSER;
+#elif defined(OS_IOS)
+  return enterprise_management::DeviceRegisterRequest::IOS_BROWSER;
+#else
+  return enterprise_management::DeviceRegisterRequest::BROWSER;
+#endif
+}
+
 void UserCloudPolicyInvalidator::Shutdown() {
   CloudPolicyInvalidator::Shutdown();
 }
diff --git a/chrome/browser/policy/cloud/user_cloud_policy_invalidator.h b/chrome/browser/policy/cloud/user_cloud_policy_invalidator.h
index a5e7247..f9bea6c 100644
--- a/chrome/browser/policy/cloud/user_cloud_policy_invalidator.h
+++ b/chrome/browser/policy/cloud/user_cloud_policy_invalidator.h
@@ -9,6 +9,7 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "policy/proto/device_management_backend.pb.h"
 
 class Profile;
 
@@ -31,6 +32,8 @@ class UserCloudPolicyInvalidator : public CloudPolicyInvalidator,
       Profile* profile,
       CloudPolicyManager* policy_manager);
 
+  static enterprise_management::DeviceRegisterRequest::Type GetPolicyType();
+
   // KeyedService:
   virtual void Shutdown() OVERRIDE;