Closes #2727: Rust SyncManager integration #4480

grigoryk · 2019-09-20T11:56:39Z

Fixes #2727.
This has most of the pieces in the right places.
I think all of the necessary public API changes are done.

It mostly works, except that I've hit some bugs in the rust library:

RustSyncManager: does not generate an engine record in meta/global after enabling an engine mozilla/application-services#1831 (blocker)
RustSyncManager crashes with persistedState=null mozilla/application-services#1829 (minor)

Fenix PR that enables "choose what to sync" on top of this: mozilla-mobile/fenix#5450

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

...ts/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/SyncManager.kt

...nts/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt

...r/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt

components/service/firefox-accounts/build.gradle

codecov · 2019-09-23T22:46:36Z

Codecov Report

Merging #4480 into master will decrease coverage by 2.59%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master    #4480     +/-   ##
===========================================
- Coverage     86.64%   84.05%   -2.6%     
+ Complexity      911      171    -740     
===========================================
  Files           102       16     -86     
  Lines          3527      928   -2599     
  Branches        499      159    -340     
===========================================
- Hits           3056      780   -2276     
+ Misses          232      100    -132     
+ Partials        239       48    -191

Impacted Files	Coverage Δ	Complexity Δ
...nents/feature/prompts/widget/MonthAndYearPicker.kt
...illa/components/service/experiments/Experiments.kt
...oard/storage/flatfile/FlatFileExperimentStorage.kt
.../components/feature/prompts/AlertDialogFragment.kt
...lla/components/feature/toolbar/ToolbarPresenter.kt
...ozilla/components/feature/prompts/ChoiceAdapter.kt
...la/components/feature/toolbar/ToolbarInteractor.kt
...la/components/browser/search/suggestions/Parser.kt
...ents/feature/toolbar/ToolbarAutocompleteFeature.kt
.../mozilla/components/service/fretboard/Fretboard.kt
... and 108 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bd8ede...d9abf6e. Read the comment docs.

grigoryk · 2019-09-25T07:55:55Z

Reminder to self: flag @Dexterp37 and @linacambridge to review Glean changes.

grigoryk · 2019-09-26T03:15:45Z

Today's version of a-s - 0.39.4 - appears to work correctly, so we can start landing this now.

Dexterp37

This looks good to me from a Glean point of view. Please note that the following things are required before merging this:

test that sync data is still collected properly after your change. You can do so by using the Glean ping debug view either in a sample app or in any product using these components (Fenix?)
add support-sync-telemetry to the list of probe-scraper dependencies and update the entries for the components depending on it; you can coordinate with @fbertsch and @mdboom about this.

Amejia481 · 2019-09-27T01:43:16Z

We have a failing task on tc support-sync-telemetry assembleAndTestAndLintReleaseAll

grigoryk · 2019-09-27T01:49:46Z

Yeah, it passes locally though :(

grigoryk · 2019-09-27T01:55:36Z

TODO before landing:

file remaining follow-up issues
changelog
documentation
test that sync data is still collected properly after your change. You can do so by using the Glean ping debug view either in a sample app or in any product using these components (Fenix?)

csadilek

👍

We will need to record sync telemetry from the sync manager. We already have telemetry related code, but at the DB Connection level. This patch refactors this code into a singleton object that may be used from within any context in which there's a ping object available that needs to be processed.

This allows us to use the same Glean definitions and processing logic from multiple components. In our case, it'll be browser-storage-sync (for non-AccountManager syncing) and services-firefox-accounts (SyncManager integration)

Co-authored-by: Arturo Mejia <arturomejiamarmol@gmail.com>

grigoryk · 2019-09-27T19:40:13Z

bors r=csadilek

4480: Closes #2727: Rust SyncManager integration r=csadilek a=grigoryk Fixes #2727. This has most of the pieces in the right places. I think all of the necessary public API changes are done. It mostly works, except that I've hit some bugs in the rust library: - mozilla/application-services#1831 (blocker) - mozilla/application-services#1829 (minor) Fenix PR that enables "choose what to sync" on top of this: mozilla-mobile/fenix#5450 Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>

bors · 2019-09-27T20:11:45Z

Build succeeded

Taskcluster (push)

grigoryk changed the title ~~WIP Rust SyncManager integration first pass~~ WIP Rust SyncManager integration Sep 20, 2019

grigoryk mentioned this pull request Sep 20, 2019

WIP Rust SyncManager integration #4317

Closed

6 tasks

grigoryk changed the title ~~WIP Rust SyncManager integration~~ Rust SyncManager integration Sep 20, 2019

grigoryk added the work in progress Not ready to land yet. Work in progress (WIP). label Sep 20, 2019

grigoryk commented Sep 20, 2019

View reviewed changes

...ts/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/sync/SyncManager.kt Outdated Show resolved Hide resolved

grigoryk mentioned this pull request Sep 20, 2019

(Fixing breaking API changes) "Choose what to sync" integration work mozilla-mobile/fenix#5450

Merged

5 tasks

grigoryk changed the title ~~Rust SyncManager integration~~ Closes #2727: Rust SyncManager integration Sep 20, 2019

Amejia481 reviewed Sep 23, 2019

View reviewed changes

...nts/browser/storage-sync/src/main/java/mozilla/components/browser/storage/sync/Connection.kt Outdated Show resolved Hide resolved

Amejia481 reviewed Sep 23, 2019

View reviewed changes

...r/storage-sync/src/main/java/mozilla/components/browser/storage/sync/PlacesHistoryStorage.kt Outdated Show resolved Hide resolved

Amejia481 reviewed Sep 23, 2019

View reviewed changes

components/service/firefox-accounts/build.gradle Outdated Show resolved Hide resolved

grigoryk force-pushed the syncManager branch 8 times, most recently from bc06b00 to 67f7389 Compare September 25, 2019 07:54

grigoryk force-pushed the syncManager branch 3 times, most recently from fa0344c to 4ac03ab Compare September 26, 2019 03:01

grigoryk marked this pull request as ready for review September 26, 2019 03:01

grigoryk requested a review from a team as a code owner September 26, 2019 03:01

grigoryk removed the work in progress Not ready to land yet. Work in progress (WIP). label Sep 26, 2019

grigoryk requested review from Dexterp37 and csadilek September 26, 2019 03:01

Dexterp37 approved these changes Sep 26, 2019

View reviewed changes

grigoryk force-pushed the syncManager branch from 4ac03ab to 0ed3c43 Compare September 27, 2019 01:08

grigoryk force-pushed the syncManager branch from 0ed3c43 to a69a81b Compare September 27, 2019 01:51

This was referenced Sep 27, 2019

Sync telemetry processing for the passwords engine #4556

Closed

Adding new types of SyncEngines #4557

Closed

grigoryk force-pushed the syncManager branch 7 times, most recently from f88002e to 934e7a2 Compare September 27, 2019 07:19

csadilek approved these changes Sep 27, 2019

View reviewed changes

grigoryk force-pushed the syncManager branch 2 times, most recently from 7fc14d2 to 4034a61 Compare September 27, 2019 19:25

Grisha Kruglov and others added 6 commits September 27, 2019 12:39

Pre: bump A-S to 0.40.0

b01f663

Part 3: Rust SyncManager integration

b3dfb22

Co-authored-by: Arturo Mejia <arturomejiamarmol@gmail.com>

Post: hide internal functions, documentation

84f342e

Post: make sure total sync failure telemetry is only reported once

d9abf6e

grigoryk force-pushed the syncManager branch from 4034a61 to d9abf6e Compare September 27, 2019 19:39

bors bot merged commit d9abf6e into mozilla-mobile:master Sep 27, 2019

mdboom mentioned this pull request Sep 27, 2019

1584182: Move browser-storage-sync to support-sync-telemetry mozilla/probe-scraper#122

Merged

grigoryk deleted the syncManager branch September 28, 2019 02:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #2727: Rust SyncManager integration #4480

Closes #2727: Rust SyncManager integration #4480

grigoryk commented Sep 20, 2019 •

edited

Loading

codecov bot commented Sep 23, 2019 •

edited

Loading

grigoryk commented Sep 25, 2019

grigoryk commented Sep 26, 2019

Dexterp37 left a comment

Amejia481 commented Sep 27, 2019

grigoryk commented Sep 27, 2019

grigoryk commented Sep 27, 2019 •

edited

Loading

csadilek left a comment

grigoryk commented Sep 27, 2019

bors bot commented Sep 27, 2019

Closes #2727: Rust SyncManager integration #4480

Closes #2727: Rust SyncManager integration #4480

Conversation

grigoryk commented Sep 20, 2019 • edited Loading

Pull Request checklist

After merge

codecov bot commented Sep 23, 2019 • edited Loading

Codecov Report

grigoryk commented Sep 25, 2019

grigoryk commented Sep 26, 2019

Dexterp37 left a comment

Choose a reason for hiding this comment

Amejia481 commented Sep 27, 2019

grigoryk commented Sep 27, 2019

grigoryk commented Sep 27, 2019 • edited Loading

csadilek left a comment

Choose a reason for hiding this comment

grigoryk commented Sep 27, 2019

bors bot commented Sep 27, 2019

Build succeeded

grigoryk commented Sep 20, 2019 •

edited

Loading

codecov bot commented Sep 23, 2019 •

edited

Loading

grigoryk commented Sep 27, 2019 •

edited

Loading