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

Issue 895899 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 17
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 870624



Sign in to add a comment

USS migrator inflates UMA metric Sync.ModelTypeEntityChange

Project Member Reported by mastiz@chromium.org, Oct 16

Issue description

The USS migrator does a one-off dump of directory sync entities to the USS architecture, when USS is enabled for a datatype and user.

In UMA, counting these as remote updates distorts reality, and makes it difficult to evaluate a USS rollout.

Instead, Sync.ModelTypeEntityChange should stick to interactions with the sync server, which is also what the histogram summary promises.
 
Blocking: 870624
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 16

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

commit 4845c684da763895b4e0b0a65798939c5149d8ac
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Oct 16 19:28:07 2018

Add more UMA metrics for USS migrator

Sync.USSMigrationEntityCount.<datatype> metrics are introduced to
count the number of entities that go through a USS migration
procedure.

In particular, it's useful to know how many entities were migrated
from directory to USS in order to explain differences in metric
Sync.ModelTypeEntityChange during rollout, because the USS migrator
doesn't really involve fetching updates from the server.

NOPRESUBMIT=true

Bug:  895899 
Change-Id: I5c3b731187fadab30dc499426d0121d6f97769ec
Reviewed-on: https://chromium-review.googlesource.com/c/1280583
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600080}
[modify] https://crrev.com/4845c684da763895b4e0b0a65798939c5149d8ac/components/sync/engine_impl/model_type_registry.cc
[modify] https://crrev.com/4845c684da763895b4e0b0a65798939c5149d8ac/components/sync/engine_impl/model_type_registry_unittest.cc
[modify] https://crrev.com/4845c684da763895b4e0b0a65798939c5149d8ac/components/sync/engine_impl/uss_migrator.cc
[modify] https://crrev.com/4845c684da763895b4e0b0a65798939c5149d8ac/components/sync/engine_impl/uss_migrator.h
[modify] https://crrev.com/4845c684da763895b4e0b0a65798939c5149d8ac/components/sync/engine_impl/uss_migrator_unittest.cc
[modify] https://crrev.com/4845c684da763895b4e0b0a65798939c5149d8ac/tools/metrics/histograms/histograms.xml

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 17

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

commit 862a4f0d4ef3a65e491847398548fe256dc71d0c
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Oct 17 07:56:28 2018

Fix Sync.ModelTypeEntityChange bucket for remote updates

Entities fed to the worker by the USS migrator should not contribute to
a UMA histogram that is about interactions with the server. Otherwise,
it is difficult to evaluate the impact of a USS rollout and do A/B
testing.

Implementation-wise, the code avoids abstractions and injects a boolean
to distinguish the USS migrator as a special caller, which seems to be
the simplest way and the easiest one to be cleaned up after the code
for the USS migrator itself is deleted.

Bug:  895899 
Change-Id: Ie3f4aed51da37780945f1e75ffd2be05063526fb
Reviewed-on: https://chromium-review.googlesource.com/c/1283041
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600311}
[modify] https://crrev.com/862a4f0d4ef3a65e491847398548fe256dc71d0c/components/sync/engine_impl/model_type_worker.cc
[modify] https://crrev.com/862a4f0d4ef3a65e491847398548fe256dc71d0c/components/sync/engine_impl/model_type_worker.h
[modify] https://crrev.com/862a4f0d4ef3a65e491847398548fe256dc71d0c/components/sync/engine_impl/uss_migrator.cc

Status: Fixed (was: Started)

Sign in to add a comment