New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 895488 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 3
Type: Task



Sign in to add a comment

UMA histograms Sync.CustomSync and (to a lesser degree) Sync.SyncEverything are misleading

Project Member Reported by treib@chromium.org, Oct 15

Issue description

tl;dr I think we should get rid of both of these histograms, and optionally replace them with something that can actually be interpreted.

The UMA histograms Sync.SyncEverything and Sync.CustomSync supposedly record whether users have the "sync everything" flag enabled, and which data types they chose. However, the recording logic (especially for the latter) is so weird that they're IMO not very useful, and probably more harmful than helpful. In particular:

Sync.SyncEverything is recorded every time the value changes, plus during the first setup (where the previous value isn't really defined). That's at least straightforward, but not all that useful:
- For existing users that don't change their settings, it's never recorded. So it doesn't actually tell us which fraction of users have selected custom types.
- It's recorded immediately when the checkbox is toggled, so *before* the change is actually applied. If a user can't make up their mind and toggles it back and forth 10 times, we'll record 10 samples.
- It's buggy: During initial Sync setup (even without going through the advanced settings flow), it's recorded twice, once with "false" and once with "true", because PSS::OnUserChoseDatatypes gets called twice for some reason.

Sync.CustomSync similarly tries to record only when something changes, so shares all the above problems. Additionally, it only records when a data type is turned *on* - we don't record anything when it's turned *off*.
Exercise: Go to chrome://settings/syncSetup, flip one of the data types on and off a few times, then go to chrome://histograms and try to make any sense of the recorded values. Try both during initial setup and later re-configuration and pay attention to the difference :)
 
Additional ugliness: Sync.CustomSync uses a special enum UserSelectableSyncType which exists only for this purpose, plus an array of its values in ProfileSyncService::UpdateSelectedTypesHistogram. These must be manually maintained and kept in sync with the real ModelType enum, in particular the ordering of that array matters. So it'd be very easy to accidentally break the histogram and record wrong values. (It took me quite a while to convince myself that it's actually correct right now.)
Owner: treib@chromium.org
Status: Started (was: Available)
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 22

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

commit e777bf902b6289e22966ba1d08c6e6f59232b04b
Author: Marc Treib <treib@chromium.org>
Date: Mon Oct 22 08:42:31 2018

Sync: Replace histograms Sync.SyncEverything and Sync.CustomSync

...by *2 histograms.
The old histograms had very weird semantics for when samples were taken.
The basic idea was to record only when the state *changed*, which means
that for users that just keep using Sync without changing settings, we'd
never record anything at all. There were also additional oddities around
recording, see bug. Taken together, this makes the old histograms very
hard to interpret.
The new histograms are recorded when Sync configures the data types,
which usually means during Sync startup. These will actually let us
answer questions like "how many users have selected custom data types".

Bug:  895488 
Change-Id: Ie67ddebd562c7007b576dcfc962a3bc058bb9452
Reviewed-on: https://chromium-review.googlesource.com/c/1286812
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601499}
[modify] https://crrev.com/e777bf902b6289e22966ba1d08c6e6f59232b04b/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/e777bf902b6289e22966ba1d08c6e6f59232b04b/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/e777bf902b6289e22966ba1d08c6e6f59232b04b/components/sync/BUILD.gn
[delete] https://crrev.com/178e7c533a7ee5cb58b99ca90752858ded8ab851/components/sync/driver/user_selectable_sync_type.h
[modify] https://crrev.com/e777bf902b6289e22966ba1d08c6e6f59232b04b/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Started)

Sign in to add a comment