Add UMA metrics on policies that are ignored by atomic groups

Bug: 962665
Change-Id: Icf34f4214189aed2f8cdae8b3d3028eb39337a4e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1615671
Commit-Queue: Yann Dago <ydago@chromium.org>
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665979}

components/policy/core/common/policy_map.cc

@@ -151,8 +151,7 @@ base::string16 PolicyMap::Entry::GetLocalizedWarnings(
 bool PolicyMap::Entry::IsBlockedOrIgnored() const {
   return error_message_ids_.find(IDS_POLICY_BLOCKED) !=
              error_message_ids_.end() ||
-         error_message_ids_.find(IDS_POLICY_IGNORED_BY_GROUP_MERGING) !=
-             error_message_ids_.end();
+         IsIgnoredByAtomicGroup();
 }
 
 void PolicyMap::Entry::SetBlocked() {
@@ -163,6 +162,11 @@ void PolicyMap::Entry::SetIgnoredByPolicyAtomicGroup() {
   error_message_ids_.insert(IDS_POLICY_IGNORED_BY_GROUP_MERGING);
 }
 
+bool PolicyMap::Entry::IsIgnoredByAtomicGroup() const {
+  return error_message_ids_.find(IDS_POLICY_IGNORED_BY_GROUP_MERGING) !=
+         error_message_ids_.end();
+}
+
 PolicyMap::PolicyMap() {}
 
 PolicyMap::~PolicyMap() {
@@ -232,6 +236,11 @@ void PolicyMap::AddError(const std::string& policy, int message_id) {
   map_[policy].AddError(message_id);
 }
 
+bool PolicyMap::IsPolicyIgnoredByAtomicGroup(const std::string& policy) const {
+  const auto& entry = map_.find(policy);
+  return entry != map_.end() && entry->second.IsIgnoredByAtomicGroup();
+}
+
 void PolicyMap::SetSourceForAll(PolicySource source) {
   for (auto& it : map_) {
     it.second.source = source;

components/policy/core/common/policy_map.h

@@ -88,6 +88,7 @@ class POLICY_EXPORT PolicyMap {
     // Marks the policy as ignored because it does not share the priority of
     // its policy atomic group.
     void SetIgnoredByPolicyAtomicGroup();
+    bool IsIgnoredByAtomicGroup() const;
 
     // Callback used to look up a localized string given its l10n message ID. It
     // should return a UTF-16 string.
@@ -146,6 +147,11 @@ class POLICY_EXPORT PolicyMap {
   // should only be called for policies that are already stored in the map.
   void AddError(const std::string& policy, int message_id);
 
+  // Return True if the policy is set but its value is ignored because it does
+  // not share the highest priority from its atomic group. Returns False if the
+  // policy is active or not set.
+  bool IsPolicyIgnoredByAtomicGroup(const std::string& policy) const;
+
   // For all policies, overwrite the PolicySource with |source|.
   void SetSourceForAll(PolicySource source);
 

components/policy/core/common/policy_statistics_collector.cc

@@ -62,6 +62,10 @@ void PolicyStatisticsCollector::RecordPolicyUse(int id) {
   base::UmaHistogramSparse("Enterprise.Policies", id);
 }
 
+void PolicyStatisticsCollector::RecordPolicyIgnoredByAtomicGroup(int id) {
+  base::UmaHistogramSparse("Enterprise.Policies.IgnoredByPolicyGroup", id);
+}
+
 void PolicyStatisticsCollector::CollectStatistics() {
   const PolicyMap& policies = policy_service_->GetPolicies(
       PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()));
@@ -76,6 +80,13 @@ void PolicyStatisticsCollector::CollectStatistics() {
       else
         NOTREACHED();
     }
+    if (policies.IsPolicyIgnoredByAtomicGroup(it.key())) {
+      const PolicyDetails* details = get_details_.Run(it.key());
+      if (details)
+        RecordPolicyIgnoredByAtomicGroup(details->id);
+      else
+        NOTREACHED();
+    }
   }
 
   // Take care of next update.

components/policy/core/common/policy_statistics_collector.h

@@ -48,6 +48,9 @@ class POLICY_EXPORT PolicyStatisticsCollector {
   // protected virtual for mocking.
   virtual void RecordPolicyUse(int id);
 
+  // protected virtual for mocking.
+  virtual void RecordPolicyIgnoredByAtomicGroup(int id);
+
  private:
   void CollectStatistics();
   void ScheduleUpdate(base::TimeDelta delay);

components/policy/core/common/policy_statistics_collector_unittest.cc

@@ -66,6 +66,7 @@ class TestPolicyStatisticsCollector : public PolicyStatisticsCollector {
                                   task_runner) {}
 
   MOCK_METHOD1(RecordPolicyUse, void(int));
+  MOCK_METHOD1(RecordPolicyIgnoredByAtomicGroup, void(int));
 };
 
 }  // namespace
@@ -112,6 +113,12 @@ class PolicyStatisticsCollectorTest : public testing::Test {
                     nullptr);
   }
 
+  void SetPolicyIgnoredByAtomicGroup(const std::string& name) {
+    SetPolicy(name);
+    auto* policy = policy_map_.GetMutable(name);
+    policy->SetIgnoredByPolicyAtomicGroup();
+  }
+
   base::TimeDelta GetFirstDelay() const {
     if (!task_runner_->HasPendingTask()) {
       ADD_FAILURE();
@@ -186,4 +193,17 @@ TEST_F(PolicyStatisticsCollectorTest, MultiplePolicies) {
   EXPECT_EQ(1u, task_runner_->NumPendingTasks());
 }
 
+TEST_F(PolicyStatisticsCollectorTest, PolicyIgnoredByAtomicGroup) {
+  SetPolicyIgnoredByAtomicGroup(kTestPolicy1);
+
+  prefs_.SetInt64(policy_prefs::kLastPolicyStatisticsUpdate,
+                  (base::Time::Now() - update_delay_).ToInternalValue());
+
+  EXPECT_CALL(*policy_statistics_collector_,
+              RecordPolicyIgnoredByAtomicGroup(kTestPolicy1Id));
+
+  policy_statistics_collector_->Initialize();
+  EXPECT_EQ(1u, task_runner_->NumPendingTasks());
+}
+
 }  // namespace policy

tools/metrics/histograms/histograms.xml

@@ -31829,6 +31829,17 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="Enterprise.Policies.IgnoredByPolicyGroup"
+    enum="EnterprisePolicies" expires_after="M78">
+  <owner>ydago@chromium.org</owner>
+  <summary>
+    A set of enterprise policy rules that are ignored because they do not share
+    the highest priority from their policy atomic group. This is recorded at
+    startup, then every 24 hours. If chrome is not running at the 24 hours mark,
+    this will be recorded at the next startup.
+  </summary>
+</histogram>
+
 <histogram name="Enterprise.Policy" enum="EnterprisePolicyType"
     expires_after="2019-03-15">
   <obsolete>