Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(61)

Issue 2653693004: UKM Sync Observer (Closed)

Created:
3 years, 10 months ago by Steven Holte
Modified:
3 years, 9 months ago
CC:
chromium-reviews, pkl (ping after 24h if needed), droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, noyau+watch_chromium.org, asvitkine+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

UKM Sync Observer Disables UKM when any there are any active profiles which do not have history sync enabled. Purges local UKM data + resets client ID when any profile disables history sync. Purges local UKM data + resets client ID when UMA is disabled. Depends on https://codereview.chromium.org/2657673004/ BUG=678682 Review-Url: https://codereview.chromium.org/2653693004 Cr-Commit-Position: refs/heads/master@{#449447} Committed: https://chromium.googlesource.com/chromium/src/+/1334c0aa8505dc48616233442f89e4b1fd1fb778

Patch Set 1 #

Patch Set 2 : Comments #

Patch Set 3 : Rebase #

Patch Set 4 : Split #

Total comments: 26

Patch Set 5 : Address comments #

Total comments: 14

Patch Set 6 : Reset client id #

Patch Set 7 : Unit test #

Patch Set 8 : Suppress recording and fix tests #

Total comments: 14

Patch Set 9 : Incorporate Feedback #

Patch Set 10 : Rebase #

Patch Set 11 : Update DEPS #

Patch Set 12 : Update BUILD deps #

Patch Set 13 : Fix tests & IOS deps #

Patch Set 14 : Fix PageLoadMetricsObserverTest #

Total comments: 5

Patch Set 15 : Complex state #

Total comments: 2

Patch Set 16 : Check for no logs to send #

Patch Set 17 : Rebase #

Patch Set 18 : MSVC struct initializer #

Unified diffs Side-by-side diffs Delta from patch set Stats (+631 lines, -48 lines) Patch
M chrome/browser/metrics/chrome_metrics_service_client.h View 1 2 3 4 5 6 7 8 9 4 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/metrics/chrome_metrics_service_client.cc View 1 2 3 4 5 6 7 8 9 5 chunks +20 lines, -3 lines 0 comments Download
M components/metrics/metrics_service_client.h View 1 2 3 4 5 6 7 8 3 chunks +18 lines, -2 lines 0 comments Download
M components/metrics/metrics_service_client.cc View 1 2 3 4 5 6 7 8 2 chunks +18 lines, -0 lines 0 comments Download
M components/metrics_services_manager/metrics_services_manager.h View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -1 line 0 comments Download
M components/metrics_services_manager/metrics_services_manager.cc View 1 2 3 4 5 6 7 8 3 chunks +41 lines, -17 lines 0 comments Download
M components/ukm/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +6 lines, -0 lines 0 comments Download
M components/ukm/observers/DEPS View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
A components/ukm/observers/sync_disable_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +78 lines, -0 lines 0 comments Download
A components/ukm/observers/sync_disable_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +81 lines, -0 lines 0 comments Download
A components/ukm/observers/sync_disable_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +216 lines, -0 lines 0 comments Download
M components/ukm/test_ukm_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M components/ukm/ukm_service.h View 1 2 3 4 5 6 7 8 9 3 chunks +10 lines, -0 lines 0 comments Download
M components/ukm/ukm_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 7 chunks +51 lines, -7 lines 0 comments Download
M components/ukm/ukm_service_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 7 chunks +41 lines, -6 lines 0 comments Download
M ios/chrome/browser/metrics/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M ios/chrome/browser/metrics/ios_chrome_metrics_service_client.h View 1 2 3 4 4 chunks +8 lines, -3 lines 0 comments Download
M ios/chrome/browser/metrics/ios_chrome_metrics_service_client.mm View 1 2 3 4 5 6 7 8 4 chunks +19 lines, -2 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +8 lines, -3 lines 0 comments Download

Messages

Total messages: 67 (40 generated)
Steven Holte
3 years, 10 months ago (2017-01-31 21:25:15 UTC) #4
rkaplow
mostly lg. wondering if it makes sense to get someone from sync (skym?) to look ...
3 years, 10 months ago (2017-02-01 16:40:55 UTC) #5
Alexei Svitkine (slow)
Any way to unit test the logic? https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode897 chrome/browser/metrics/chrome_metrics_service_client.cc:897: browser_sync::ProfileSyncService* sync ...
3 years, 10 months ago (2017-02-01 19:31:08 UTC) #6
rkaplow
One other thing we need to verify - we can only do this if the ...
3 years, 10 months ago (2017-02-01 20:17:34 UTC) #7
Steven Holte
Addressed comments + added passphrase check. https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2653693004/diff/60001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode897 chrome/browser/metrics/chrome_metrics_service_client.cc:897: browser_sync::ProfileSyncService* sync = ...
3 years, 10 months ago (2017-02-03 00:04:34 UTC) #8
Steven Holte
+skym for sync logic +olivierrobin for ios/chrome/browser/metrics
3 years, 10 months ago (2017-02-03 00:06:27 UTC) #10
skym
Oumph, this is trickier than I thought it was going to be. Life is simpler ...
3 years, 10 months ago (2017-02-03 01:04:00 UTC) #11
Olivier
ios LGTM
3 years, 10 months ago (2017-02-03 08:26:11 UTC) #12
Steven Holte
Added some code to suppress recording Sources when UKM recording is disabled. https://codereview.chromium.org/2653693004/diff/80001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc ...
3 years, 10 months ago (2017-02-04 00:16:33 UTC) #13
Alexei Svitkine (slow)
https://codereview.chromium.org/2653693004/diff/140001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2653693004/diff/140001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode938 chrome/browser/metrics/chrome_metrics_service_client.cc:938: if (ukm_service_) { Nit: Early return instead. https://codereview.chromium.org/2653693004/diff/140001/components/metrics/metrics_service_client.h File ...
3 years, 10 months ago (2017-02-04 00:52:26 UTC) #14
Steven Holte
https://codereview.chromium.org/2653693004/diff/140001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2653693004/diff/140001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode938 chrome/browser/metrics/chrome_metrics_service_client.cc:938: if (ukm_service_) { On 2017/02/04 00:52:23, Alexei Svitkine (slow) ...
3 years, 10 months ago (2017-02-06 21:57:54 UTC) #15
Alexei Svitkine (slow)
lgtm but please specify a BUG=
3 years, 10 months ago (2017-02-06 22:02:14 UTC) #18
skym
sync lgtm https://codereview.chromium.org/2653693004/diff/80001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2653693004/diff/80001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode915 chrome/browser/metrics/chrome_metrics_service_client.cc:915: case chrome::NOTIFICATION_PROFILE_ADDED: On 2017/02/04 00:16:32, Steven Holte ...
3 years, 10 months ago (2017-02-07 16:45:45 UTC) #43
Steven Holte
+msramek for the Privacy perspective I think this CL adds the missing client side pieces ...
3 years, 10 months ago (2017-02-07 21:12:11 UTC) #45
msramek
I agree that MetricsServicesManager::UpdateUkmService() and SyncDisableObserver reflect our discussions in the privacy review. LGTM. I ...
3 years, 10 months ago (2017-02-08 14:30:10 UTC) #46
skym
https://codereview.chromium.org/2653693004/diff/260001/components/ukm/observers/sync_disable_observer.cc File components/ukm/observers/sync_disable_observer.cc (right): https://codereview.chromium.org/2653693004/diff/260001/components/ukm/observers/sync_disable_observer.cc#newcode71 components/ukm/observers/sync_disable_observer.cc:71: bool must_purge = was_enabled_map_[sync] && !is_enabled; On 2017/02/08 14:30:10, ...
3 years, 10 months ago (2017-02-08 17:51:17 UTC) #47
Steven Holte
I've updated the observer logic to track the bits of sync state separately. The new ...
3 years, 10 months ago (2017-02-08 20:44:02 UTC) #48
rkaplow
https://codereview.chromium.org/2653693004/diff/260001/components/ukm/ukm_service.cc File components/ukm/ukm_service.cc (right): https://codereview.chromium.org/2653693004/diff/260001/components/ukm/ukm_service.cc#newcode236 components/ukm/ukm_service.cc:236: StartScheduledUpload(); I was getting CHECK-fails in the persistant logs ...
3 years, 10 months ago (2017-02-08 21:21:12 UTC) #49
Steven Holte
https://codereview.chromium.org/2653693004/diff/260001/components/ukm/ukm_service.cc File components/ukm/ukm_service.cc (right): https://codereview.chromium.org/2653693004/diff/260001/components/ukm/ukm_service.cc#newcode236 components/ukm/ukm_service.cc:236: StartScheduledUpload(); On 2017/02/08 21:21:12, rkaplow wrote: > I was ...
3 years, 10 months ago (2017-02-08 21:51:54 UTC) #50
msramek
Thanks for the explanation, Sky! > I've updated the observer logic to track the bits ...
3 years, 10 months ago (2017-02-09 17:39:42 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2653693004/320001
3 years, 10 months ago (2017-02-09 20:20:21 UTC) #54
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/363647)
3 years, 10 months ago (2017-02-09 20:55:46 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2653693004/340001
3 years, 10 months ago (2017-02-09 21:48:52 UTC) #61
commit-bot: I haz the power
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/1334c0aa8505dc48616233442f89e4b1fd1fb778
3 years, 10 months ago (2017-02-09 22:53:33 UTC) #64
rkaplow
On 2017/02/09 22:53:33, commit-bot: I haz the power wrote: > Committed patchset #18 (id:340001) as ...
3 years, 9 months ago (2017-02-28 18:40:31 UTC) #65
Steven Holte
On 2017/02/28 18:40:31, rkaplow (slow) wrote: > On 2017/02/09 22:53:33, commit-bot: I haz the power ...
3 years, 9 months ago (2017-02-28 20:55:24 UTC) #66
rkaplow
3 years, 9 months ago (2017-02-28 21:05:41 UTC) #67
Message was sent while issue was closed.
On 2017/02/28 20:55:24, Steven Holte wrote:
> On 2017/02/28 18:40:31, rkaplow (slow) wrote:
> > On 2017/02/09 22:53:33, commit-bot: I haz the power wrote:
> > > Committed patchset #18 (id:340001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/1334c0aa8505dc48616233442f89...
> > 
> > Question about the sync behavior with this setup related to adding a new
> > profile. Say a user has a syncing profile and UKM is setup, and they add a
new
> > profile which they immediately setup. Once you add an new profile, the
profile
> > window opens and they get a screen to init Syncing. I assume at that point
> > OnStateChanged gets triggered and this will cause the UKM client ID to reset
> and
> > UKM to disable. After the user sets up Sync, UKM will be re-enabled (with a
> new
> > client id). Steve can can you confirm that behavior?
> 
> When a new profile is added that is not syncing, the behavior of the code is
> expected to be that it calls MSClient::OnSyncPrefsChanged(must_purge = false),
> and that AllSyncProfilesEnabled() == false.  This should result in UKM being
> disabled, and no new data being recorded, but clientID will not be reset, nor
> will any data be purged, since we won't have recorded anything related to the
> user that just signed in.

ok thanks for clarifying.

Powered by Google App Engine
This is Rietveld 408576698