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

Issue 897658 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Split a bucket for Sync.ModelTypeEntityChange.*

Project Member Reported by jkrcal@chromium.org, Oct 22

Issue description

We should split the "remote update" bucket into update for initial sync and any later update (and rename the histogram).
 
Labels: butter-hotlist
Summary: Split a bucket for Sync.ModelTypeEntityChange.* (was: Split a bucket for Sync.ModelTypeEntityChange.WALLET_METADATA)
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 23

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

commit dd93b24ff9678a002f308228b2fa297084b07d90
Author: Jan Krcal <jkrcal@chromium.org>
Date: Tue Oct 23 12:25:46 2018

[Sync UMA] Split reporting of initial and incremental updates

This CL improves reporting of remote entity changes in the sync engine.
It splits remote updates into two buckets: one for the initial sync and
another one for later incremental updates. This is useful for ephemeral
storage where the initial sync is a common scenario.

Bug:  897658 
Change-Id: Ibab27b0e6a94e3917c1794c48065a8ae34a495eb
Reviewed-on: https://chromium-review.googlesource.com/c/1294031
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601908}
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine/cycle/update_counters.cc
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine/cycle/update_counters.h
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine_impl/cycle/data_type_debug_info_emitter.cc
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine_impl/cycle/data_type_debug_info_emitter_unittest.cc
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine_impl/directory_update_handler.cc
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine_impl/directory_update_handler.h
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine_impl/directory_update_handler_unittest.cc
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine_impl/model_type_worker.cc
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine_impl/model_type_worker_unittest.cc
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine_impl/process_updates_util.cc
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/components/sync/engine_impl/process_updates_util.h
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/dd93b24ff9678a002f308228b2fa297084b07d90/tools/metrics/histograms/histograms.xml

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 30

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

commit d5398c5cd3256cb51ea09936d897aa882d349ed5
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Oct 30 06:29:13 2018

Fix Sync.ModelTypeEntityChange2 undercounting initial updates

If an initial download is big enough to cause pagination on the server
(i.e. the number of updates exceed the maximum batch size), the UMA
metric incorrectly considered all batches except the first as
non-initial. This leads to undercounting for one of the two buckets
(REMOTE_INITIAL_UPDATE) and overcounting of another
(REMOTE_NON_INITIAL_UPDATE).

The bug affects the directory codepath only, preventing a proper
comparison between directory and USS implementations. For this reason,
and considering ongoing experiments, the metric is renamed.

Interestingly, there was no integration test coverage whatsoever
exercising pagination, and hence LoopbackServer had to be extended
with support for batch size truncation.

Bug:  897658 
Change-Id: Ie0feda2f917a5b0e283e68b3f703f015a977764e
Reviewed-on: https://chromium-review.googlesource.com/c/1303360
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603805}
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/chrome/browser/sync/test/integration/single_client_preferences_sync_test.cc
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/components/sync/engine_impl/cycle/data_type_debug_info_emitter.cc
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/components/sync/engine_impl/cycle/data_type_debug_info_emitter_unittest.cc
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/components/sync/engine_impl/directory_update_handler.cc
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/components/sync/engine_impl/loopback_server/loopback_server.cc
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/components/sync/engine_impl/loopback_server/loopback_server.h
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/components/sync/test/fake_server/fake_server.cc
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/components/sync/test/fake_server/fake_server.h
[modify] https://crrev.com/d5398c5cd3256cb51ea09936d897aa882d349ed5/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Assigned)

Sign in to add a comment