Revert the UKM Sync observer purge logic to pre crrev.com/c/1152744.

This reverts to the older purging logic behind a feature flag (for non-unified consent). We can see if this changes the metrics as expected. If unclear, we can do an A/B test since this is also behind a Feature.

Added a simple boolean histogram to count how often we are purging on sync changes.

Bug: 891777
Change-Id: I74596d4a92eb19de301461d2b6d18a6dca8741c4
Reviewed-on: https://chromium-review.googlesource.com/c/1258573Reviewed-by: Robert Kaplow (sloooow) <rkaplow@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Robert Kaplow (sloooow) <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596423}

components/ukm/observers/sync_disable_observer.cc

@@ -4,6 +4,9 @@
 
 #include "components/ukm/observers/sync_disable_observer.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
@@ -18,6 +21,8 @@ namespace ukm {
 
 const base::Feature kUkmCheckAuthErrorFeature{"UkmCheckAuthError",
                                               base::FEATURE_ENABLED_BY_DEFAULT};
+const base::Feature kUkmPurgingOnConnection{"UkmPurgingOnConnection",
+                                            base::FEATURE_DISABLED_BY_DEFAULT};
 
 namespace {
 
@@ -198,8 +203,35 @@ void SyncDisableObserver::UpdateSyncState(
              DataCollectionState::kIgnored ||
          consent_helper);
   SyncDisableObserver::SyncState state = GetSyncState(sync, consent_helper);
+
   // Trigger a purge if sync state no longer allows UKM.
-  bool must_purge = previous_state.AllowsUkm() && !state.AllowsUkm();
+  // TODO(rkaplow): Clean this up once crbug.com/891777 is resolved.
+  bool must_purge;
+
+  // If unified_consent is used, we keep the logic introduced in
+  // http://crrev.com/c/1152744. Otherwise, if the kUkmPurgingOnConnection
+  // feature is enabled, we still use that logic.
+  if (unified_consent::IsUnifiedConsentFeatureEnabled() ||
+      base::FeatureList::IsEnabled(kUkmPurgingOnConnection)) {
+    // Purge using AllowsUkm which includes connected status.
+    must_purge = previous_state.AllowsUkm() && !state.AllowsUkm();
+  } else {
+    // Use the previous logic to investigate crbug.com/891777.
+    must_purge =
+        // Trigger a purge if history sync was disabled.
+        (previous_state.history_enabled && !state.history_enabled) ||
+        // Trigger a purge if engine has become disabled.
+        (previous_state.initialized && !state.initialized) ||
+        // Trigger a purge if the user added a passphrase.  Since we can't
+        // detect the use of a passphrase while the engine is not initialized,
+        // we may miss the transition if the user adds a passphrase in this
+        // state.
+        (previous_state.initialized && state.initialized &&
+         !previous_state.passphrase_protected && state.passphrase_protected);
+  }
+
+  UMA_HISTOGRAM_BOOLEAN("UKM.SyncDisable.Purge", must_purge);
+
   previous_states_[sync] = state;
   UpdateAllProfileEnabled(must_purge);
 }

tools/metrics/histograms/histograms.xml

@@ -110003,6 +110003,16 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="UKM.SyncDisable.Purge" enum="Boolean"
+    expires_after="2019-05-01">
+  <owner>bcwhite@chromium.org</owner>
+  <owner>rkaplow@chromium.org</owner>
+  <summary>
+    Logged in the UpdateSyncState call from the UKM Sync observer. This records
+    if the Sync change will trigger a purge of the local UKM data.
+  </summary>
+</histogram>
+
 <histogram name="UKM.UnsentLogs.DroppedSize" units="bytes">
   <owner>holte@chromium.org</owner>
   <owner>rkaplow@chromium.org</owner>