The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3231c2fd53d4cba8ed20fa684e7ea64a340f0192

commit 3231c2fd53d4cba8ed20fa684e7ea64a340f0192
Author: Robert Kaplow <rkaplow@chromium.org>
Date: Wed Oct 03 23:35:37 2018

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/1258573
Reviewed-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}
[modify] https://crrev.com/3231c2fd53d4cba8ed20fa684e7ea64a340f0192/components/ukm/observers/sync_disable_observer.cc
[modify] https://crrev.com/3231c2fd53d4cba8ed20fa684e7ea64a340f0192/tools/metrics/histograms/histograms.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5ca4558ab43d3def43a03d0bf10fd94bb40dfcd6

commit 5ca4558ab43d3def43a03d0bf10fd94bb40dfcd6
Author: Mihai Sardarescu <msarda@chromium.org>
Date: Fri Oct 05 21:13:43 2018

Revert SyncDisableObserver to its pre-Unity version.

This CL reverts all code in the SyncDisableObserver to its state
before http://crrev.com/c/1152744. It also disables all UKMBrowserTests
for Unified Consent enabled feature as this not supported anymore.

This is an attempt to verify whether http://crrev.com/c/1152744 is
the culprit from the drop in UKM collected metrics seen in bug 891777.

For reference - here is the diff of file sync_disable_observer.cc with the version
before http://crrev.com/c/1152744:
$ git diff 8f1b6d0997a2e8277553be0475496955711a7956 --  components/ukm/observers/sync_disable_observer.cc
diff --git a/components/ukm/observers/sync_disable_observer.cc b/components/ukm/observers/sync_disable_observer.cc
index f6d72866cfdd..beb5dac78eb5 100644
--- a/components/ukm/observers/sync_disable_observer.cc
+++ b/components/ukm/observers/sync_disable_observer.cc
@@ -68,7 +68,8 @@ SyncDisableObserver::SyncState SyncDisableObserver::GetSyncState(
 }

 void SyncDisableObserver::ObserveServiceForSyncDisables(
-    syncer::SyncService* sync_service) {
+    syncer::SyncService* sync_service,
+    PrefService* pref_service) {
   previous_states_[sync_service] = GetSyncState(sync_service);
   sync_observer_.Add(sync_service);
   UpdateAllProfileEnabled(false);

$ git diff 8f1b6d0997a2e8277553be0475496955711a7956 --  components/ukm/observers/sync_disable_observer.h
diff --git a/components/ukm/observers/sync_disable_observer.h b/components/ukm/observers/sync_disable_observer.h
index 2de46181f0f5..2b89842addcb 100644
--- a/components/ukm/observers/sync_disable_observer.h
+++ b/components/ukm/observers/sync_disable_observer.h
@@ -11,6 +11,8 @@
 #include "components/sync/driver/sync_service.h"
 #include "components/sync/driver/sync_service_observer.h"

+class PrefService;
+
 namespace ukm {

 // Observes the state of a set of SyncServices for changes to history sync
@@ -23,7 +25,8 @@ class SyncDisableObserver : public syncer::SyncServiceObserver {
   ~SyncDisableObserver() override;

   // Starts observing a service for sync disables.
-  void ObserveServiceForSyncDisables(syncer::SyncService* sync_service);
+  void ObserveServiceForSyncDisables(syncer::SyncService* sync_service,
+                                     PrefService* pref_service);

   // Returns true iff sync is in a state that allows UKM to be enabled.
   // This means that for all profiles, sync is initialized, connected, has the

Bug: 891777

Change-Id: Ie55d4df1a66454918fa4931b3cc5000dade1d3e9
Reviewed-on: https://chromium-review.googlesource.com/c/1264781
Reviewed-by: Robert Kaplow (sloooow) <rkaplow@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597298}
[modify] https://crrev.com/5ca4558ab43d3def43a03d0bf10fd94bb40dfcd6/chrome/browser/metrics/ukm_browsertest.cc
[modify] https://crrev.com/5ca4558ab43d3def43a03d0bf10fd94bb40dfcd6/components/ukm/observers/sync_disable_observer.cc
[modify] https://crrev.com/5ca4558ab43d3def43a03d0bf10fd94bb40dfcd6/components/ukm/observers/sync_disable_observer.h
[modify] https://crrev.com/5ca4558ab43d3def43a03d0bf10fd94bb40dfcd6/components/ukm/observers/sync_disable_observer_unittest.cc

Marking this bug as started and raising priority as we're actively working on it.

Robert / Alexei: Should this be an RBS for M70? Or is it fine to only have it in M71?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/63e29b1c4edd3e029a748315381e96ad6e9c5ce5

commit 63e29b1c4edd3e029a748315381e96ad6e9c5ce5
Author: Mihai Sardarescu <msarda@chromium.org>
Date: Wed Oct 10 13:44:51 2018

Revert "Revert SyncDisableObserver to its pre-Unity version."

This reverts commit 5ca4558ab43d3def43a03d0bf10fd94bb40dfcd6.

Reason for revert: Based on the internal analysis on b/117162509, it seems that this CL is not the culprit for the drop in UKM collected metrics seen in bug http://crbug.com/891777.

Original change's description:
> Revert SyncDisableObserver to its pre-Unity version.
> 
> This CL reverts all code in the SyncDisableObserver to its state
> before http://crrev.com/c/1152744. It also disables all UKMBrowserTests
> for Unified Consent enabled feature as this not supported anymore.
> 
> This is an attempt to verify whether http://crrev.com/c/1152744 is
> the culprit from the drop in UKM collected metrics seen in bug 891777.
> 
> For reference - here is the diff of file sync_disable_observer.cc with the version
> before http://crrev.com/c/1152744:
> $ git diff 8f1b6d0997a2e8277553be0475496955711a7956 --  components/ukm/observers/sync_disable_observer.cc
> diff --git a/components/ukm/observers/sync_disable_observer.cc b/components/ukm/observers/sync_disable_observer.cc
> index f6d72866cfdd..beb5dac78eb5 100644
> --- a/components/ukm/observers/sync_disable_observer.cc
> +++ b/components/ukm/observers/sync_disable_observer.cc
> @@ -68,7 +68,8 @@ SyncDisableObserver::SyncState SyncDisableObserver::GetSyncState(
>  }
> 
>  void SyncDisableObserver::ObserveServiceForSyncDisables(
> -    syncer::SyncService* sync_service) {
> +    syncer::SyncService* sync_service,
> +    PrefService* pref_service) {
>    previous_states_[sync_service] = GetSyncState(sync_service);
>    sync_observer_.Add(sync_service);
>    UpdateAllProfileEnabled(false);
> 
> $ git diff 8f1b6d0997a2e8277553be0475496955711a7956 --  components/ukm/observers/sync_disable_observer.h
> diff --git a/components/ukm/observers/sync_disable_observer.h b/components/ukm/observers/sync_disable_observer.h
> index 2de46181f0f5..2b89842addcb 100644
> --- a/components/ukm/observers/sync_disable_observer.h
> +++ b/components/ukm/observers/sync_disable_observer.h
> @@ -11,6 +11,8 @@
>  #include "components/sync/driver/sync_service.h"
>  #include "components/sync/driver/sync_service_observer.h"
> 
> +class PrefService;
> +
>  namespace ukm {
> 
>  // Observes the state of a set of SyncServices for changes to history sync
> @@ -23,7 +25,8 @@ class SyncDisableObserver : public syncer::SyncServiceObserver {
>    ~SyncDisableObserver() override;
> 
>    // Starts observing a service for sync disables.
> -  void ObserveServiceForSyncDisables(syncer::SyncService* sync_service);
> +  void ObserveServiceForSyncDisables(syncer::SyncService* sync_service,
> +                                     PrefService* pref_service);
> 
>    // Returns true iff sync is in a state that allows UKM to be enabled.
>    // This means that for all profiles, sync is initialized, connected, has the
> 
> Bug: 891777
> 
> Change-Id: Ie55d4df1a66454918fa4931b3cc5000dade1d3e9
> Reviewed-on: https://chromium-review.googlesource.com/c/1264781
> Reviewed-by: Robert Kaplow (sloooow) <rkaplow@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: Brian White <bcwhite@chromium.org>
> Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#597298}

TBR=rkaplow@chromium.org,asvitkine@chromium.org,msarda@chromium.org,bcwhite@chromium.org,holte@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 891777
Change-Id: I03d7b579d9175a1c3897adaf6c2df93e1a339174
Reviewed-on: https://chromium-review.googlesource.com/c/1273036
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Robert Kaplow (sloooow) <rkaplow@chromium.org>
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598306}
[modify] https://crrev.com/63e29b1c4edd3e029a748315381e96ad6e9c5ce5/chrome/browser/metrics/ukm_browsertest.cc
[modify] https://crrev.com/63e29b1c4edd3e029a748315381e96ad6e9c5ce5/components/ukm/observers/sync_disable_observer.cc
[modify] https://crrev.com/63e29b1c4edd3e029a748315381e96ad6e9c5ce5/components/ukm/observers/sync_disable_observer.h
[modify] https://crrev.com/63e29b1c4edd3e029a748315381e96ad6e9c5ce5/components/ukm/observers/sync_disable_observer_unittest.cc

This issue is marked as a release blocker with no OS labels associated. Please add an appropriate OS label.

All release blocking issues should have OS labels associated to it, so that the issue can tracked and promptly verified, once it gets fixed.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Pls apply appropriate OSs label. Thank you.

Hi, I'd like to request a merge of 
Change-Id: I74596d4a92eb19de301461d2b6d18a6dca8741c4
Reviewed-on: https://chromium-review.googlesource.com/c/1258573

Which landed on Oct3 and has been on dev for a week.
This is fixing a regression to UKM recording - and this logic is reverting it back the previous logic. We have evidence that this change has brought us back to the previous state.

I'm happy to talk if there is any questions about this merge. Thanks

This bug requires manual review: Less than 0 days to go before AppStore submit on M70
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

M70 stable cut has passed. See schedule here: https://chromiumdash.appspot.com/schedule

This will have to wait for M71.

Ok, I didn't realize past the cut meant it was past the point that a merge could be requested.

I think waiting until 71 is acceptable here though.

Yes, stable cut is when we cut the build and do testing to make sure it's ready for release. Additionally for iOS, stable cut is when we send the build to Apple for review.

Rejecting merge to 70, as per c#10.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3749fd3935feb31170059d483082291de540ac9a

commit 3749fd3935feb31170059d483082291de540ac9a
Author: Robert Kaplow <rkaplow@chromium.org>
Date: Tue Oct 23 14:01:36 2018

Add additional diagnostic UMAs for UKM.

Adding new metrics related to when UKM client_id is reset, as well as when logs are not written because they are empty.

Bug: 891777
Change-Id: Iad3ad56c88fe97ad75feadc33d158adb90ee12a2
Reviewed-on: https://chromium-review.googlesource.com/c/1292432
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Commit-Queue: Robert Kaplow (sloooow) <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601933}
[modify] https://crrev.com/3749fd3935feb31170059d483082291de540ac9a/chrome/browser/metrics/chrome_metrics_service_client.cc
[modify] https://crrev.com/3749fd3935feb31170059d483082291de540ac9a/components/metrics_services_manager/metrics_services_manager.cc
[modify] https://crrev.com/3749fd3935feb31170059d483082291de540ac9a/components/ukm/ukm_service.cc
[modify] https://crrev.com/3749fd3935feb31170059d483082291de540ac9a/components/ukm/ukm_service.h
[modify] https://crrev.com/3749fd3935feb31170059d483082291de540ac9a/ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm
[modify] https://crrev.com/3749fd3935feb31170059d483082291de540ac9a/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/3749fd3935feb31170059d483082291de540ac9a/tools/metrics/histograms/histograms.xml

rkaplow@: did you want to merge c14 to M71?

Thanks, but no it is ok, it's just additional diagnostics and I don't think required for 71.

Can this be closed out now?

There's still some cleanup work associated, but the main problem is gone. I will remove RB label and leave it open for ongoing work here, is that good?