Migrate sequence numbers #309

Dexterp37 · 2019-09-19T17:22:31Z

This PR enables the cross-platform Glean SDK to migrate sequence numbers from glean-ac.

This additionally adds initial test coverage for the migration process.

Dexterp37 · 2019-09-20T17:14:58Z

@travis79 @badboy @mdboom thoughts about the overall design of this?

glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt

travis79

Overall the approach seems like a good one!

glean-core/android/src/main/java/mozilla/telemetry/glean/acmigration/GleanACDataMigrator.kt

badboy

(not approving/rejecting for now, I'll do that on the final PR).

Overall I think this is a good approach. Migration is encapsulated and mostly self-contained.
I'd like it to be properly and clearly documented why the migration code exist and it'd be good to not affect other platforms (e.g. iOS).

glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt

badboy · 2019-09-23T12:01:47Z

glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt

+            cfg,
+            newSequenceNums.first,
+            newSequenceNums.second,
+            if (newSequenceNums.first != null) newSequenceNums.first!!.size().toInt() else 0


Can we pull this out into a (then self-documenting) variable with a good name?

badboy · 2019-09-23T12:04:07Z

glean-core/ffi/src/lib.rs

+            seq_num_len
+        );
+
+        let glean = if (&seq_nums).as_ref().ok().is_none() {


That's a mouthful.
Can we swap around the if?

if let Ok(seq_nums) = seq_nums { Glean::with_sequence_numbers(glean_cfg, seq_nums) } else { Glean::new(...) }

glean-core/src/ac_migration/mod.rs

badboy · 2019-09-23T12:06:30Z

glean-core/src/ac_migration/mod.rs

+) {
+    let ping_maker = PingMaker::new();
+
+    for (store_name_with_suffix, next_seq) in seq_numbers.iter() {


We own seq_numbers, we can iterate and consume it with .into_iter() and save us the &0 below.

badboy · 2019-09-23T12:11:25Z

glean-core/src/ac_migration/mod.rs

+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+//! A module containing glean-core code for supporting data migration
+//! from glean-ac.


IMO we should document what it migrates and also call out that this is definitely temporary while we transition Glean AC and planned to be removed in 2020

badboy · 2019-09-23T12:13:47Z

glean-core/src/ping/mod.rs

+        seq.add(glean, next_seq);
+    }
+
+    /// This is pub(super) exclusively for enabling the migration tests.


IMO the first line should still document the method.

I'd also change the string to "This is crate-internal exclusively for enabling the migration tests." or something along the lines.

badboy · 2019-09-23T12:15:09Z

glean-core/android/src/main/java/mozilla/telemetry/glean/acmigration/GleanACDataMigrator.kt

+
+/**
+ * A class encapsulating the code used for migrating data from glean-ac
+ * to glean-core. This class, along all the migration code, should be removed


Suggested change

* to glean-core. This class, along all the migration code, should be removed

* to glean-core. This class, along with all the migration code, should be removed

glean-core/android/src/main/java/mozilla/telemetry/glean/acmigration/GleanACDataMigrator.kt

badboy · 2019-09-23T12:21:32Z

glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt

@@ -67,7 +67,12 @@ internal interface LibGleanFFI : Library {

    // Glean top-level API

-    fun glean_initialize(cfg: FfiConfiguration): Long
+    fun glean_initialize(


This will affect iOS. It will need to explicitly pass nil, nil, 0.
Would it make sense to have a duplicate of this method for use by Kotlin instead?

Could we have an FFI with this explicitly for migration (e.g. glean_initialize_migration), so it would be clear that we can remove it and revert back to the single argument version for Kotlin once we don't need migration code anymore?

Mh, Good point. Yes, it does.

mdboom · 2019-09-23T12:49:51Z

glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt

@@ -67,7 +67,12 @@ internal interface LibGleanFFI : Library {

    // Glean top-level API

-    fun glean_initialize(cfg: FfiConfiguration): Long
+    fun glean_initialize(


Could we have an FFI with this explicitly for migration (e.g. glean_initialize_migration), so it would be clear that we can remove it and revert back to the single argument version for Kotlin once we don't need migration code anymore?

mdboom · 2019-09-23T12:51:24Z

glean-core/ffi/src/from_raw.rs

+    values: RawIntArray,
+    len: i32,
+) -> glean_core::Result<Option<HashMap<String, i32>>> {
+    unsafe {


Can we put the unsafe around just the minimal parts that we know require unsafe? For example, I think the first clause here is actually safe, etc.

I think we touched this topic with @badboy a while ago and we agreed on wrapping the whole function there (I've copy pasted it, basically).

Yeah, we opted for wrapping it all.

unsafe doesn't disable Rust's safety features, it's just us telling the compiler "trust us, there's some things you can't proof, but devs can"

The null check is essential to the safety of what follows.

The Result collection is how we guarantee to catch problems that could lead to unsafety.

glean-core/src/ac_migration/mod.rs

mdboom · 2019-09-23T12:57:01Z

glean-core/src/database/mod.rs

@@ -298,7 +298,7 @@ mod test {

    #[test]
    fn test_panicks_if_fails_dir_creation() {
-        assert!(Database::new("").is_err());
+        assert!(Database::new("!#\"'@#°ç").is_err());


Do we know these characters are invalid on all major platforms? I think ext4 on Linux (Android) probably allows all of these. Maybe include a /? That's sure to fail everywhere, I think.

mdboom · 2019-09-23T12:57:50Z

glean-core/src/lib.rs

@@ -142,6 +145,32 @@ impl Glean {
        Ok(glean)
    }

+    /// Create and initialize a new Glean object.
+    ///
+    /// This will create the necessary directories and files in `data_path`.


And it clears any existing database, right? That seems potentially surprising and should probably be called out.

This class holds all the functions to perform data migration from glean-ac to glean-core, on the Kotlin side. The initial implementation reads the ac sequence numbers and allows to mark the client as migrated. Note: before the migration, glean-ac will need to land some code to set 'wasMigrated' to false, always, in case we need to roll back.

rkv currently writes data to `app_dir/glean`. To make it easier to roll back partial migrations, we should make it write to `app_dir/glean/db` so that we can delete that directory before starting a migration again.

We don't want this function to be used outside of the Kotlin FFI, as it's only useful in the glean-ac to glean-core migration.

codecov-io · 2019-09-24T09:56:54Z

Codecov Report

Merging #309 into master will increase coverage by 0.29%.
The diff coverage is 81.25%.

@@             Coverage Diff              @@
##             master     #309      +/-   ##
============================================
+ Coverage     70.58%   70.87%   +0.29%     
- Complexity      229      244      +15     
============================================
  Files            62       64       +2     
  Lines          3682     3815     +133     
  Branches        559      586      +27     
============================================
+ Hits           2599     2704     +105     
- Misses          667      676       +9     
- Partials        416      435      +19

Impacted Files	Coverage Δ	Complexity Δ
...n/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt	`66.66% <ø> (ø)`	`0 <0> (ø)`	⬇️
glean-core/src/database/mod.rs	`78.5% <33.33%> (ø)`	`0 <0> (ø)`	⬇️
glean-core/src/lib.rs	`61.45% <44.44%> (-0.84%)`	`0 <0> (ø)`
...oid/src/main/java/mozilla/telemetry/glean/Glean.kt	`84.39% <77.77%> (-0.46%)`	`1 <0> (ø)`
glean-core/src/ac_migration/mod.rs	`82.22% <82.22%> (ø)`	`0 <0> (?)`
glean-core/src/ping/mod.rs	`67.27% <85%> (+0.82%)`	`0 <0> (ø)`	⬇️
...telemetry/glean/acmigration/GleanACDataMigrator.kt	`86.95% <86.95%> (ø)`	`14 <14> (?)`
glean-core/src/lib_unit_tests.rs	`75.43% <91.66%> (+1.22%)`	`0 <0> (ø)`	⬇️
glean-core/src/error.rs	`18.57% <0%> (-5.72%)`	`0% <0%> (ø)`
... and 2 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 a39589b...2a73980. Read the comment docs.

badboy

Couple of nits left, but I think this is now a solid approach to the migration.

glean-core/android/src/main/java/mozilla/telemetry/glean/acmigration/GleanACDataMigrator.kt

glean-core/ffi/cbindgen.toml

badboy · 2019-09-24T12:46:54Z

glean-core/ffi/src/from_raw.rs

+    values: RawIntArray,
+    len: i32,
+) -> glean_core::Result<Option<HashMap<String, i32>>> {
+    unsafe {


Yeah, we opted for wrapping it all.

unsafe doesn't disable Rust's safety features, it's just us telling the compiler "trust us, there's some things you can't proof, but devs can"

The null check is essential to the safety of what follows.

The Result collection is how we guarantee to catch problems that could lead to unsafety.

glean-core/src/ac_migration/mod.rs

glean-core/src/database/mod.rs

Bonus: add license header to GleanDebugActivityTest.kt

c7d0021

Dexterp37 self-assigned this Sep 19, 2019

Bonus: remove duplicated new_glean code in ping/mod.rs

7583cc5

Dexterp37 force-pushed the migrate_sequence_nums branch from 0046395 to 4d8f7d2 Compare September 20, 2019 17:13

Dexterp37 requested review from badboy, mdboom and travis79 September 20, 2019 17:14

Dexterp37 commented Sep 20, 2019

View reviewed changes

glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt Outdated Show resolved Hide resolved

travis79 approved these changes Sep 20, 2019

View reviewed changes

glean-core/android/src/main/java/mozilla/telemetry/glean/acmigration/GleanACDataMigrator.kt Outdated Show resolved Hide resolved

Dexterp37 force-pushed the migrate_sequence_nums branch from 4d8f7d2 to 5e31979 Compare September 23, 2019 11:50

badboy reviewed Sep 23, 2019

View reviewed changes

mdboom reviewed Sep 23, 2019

View reviewed changes

Dexterp37 force-pushed the migrate_sequence_nums branch from 5e31979 to 21f9871 Compare September 23, 2019 16:56

Dexterp37 added 9 commits September 24, 2019 10:36

Record database data to the 'db' directory

15199a6

rkv currently writes data to `app_dir/glean`. To make it easier to roll back partial migrations, we should make it write to `app_dir/glean/db` so that we can delete that directory before starting a migration again.

Allow to override sequence numbers in Rust

530a021

Add the new glean initialization method to the FFI

be4a11c

Initiate the migration from Glean.initialize in Kotlin

77c3c54

Add from_raw_string_array_and_int_array to from_raw.rs

24d19f8

Add a Kotlin integration test for the Glean data migration

173c8c7

Add a test for the new Glean init API

2f993f1

Add bug TODOs

755be5a

Dexterp37 force-pushed the migrate_sequence_nums branch from 21f9871 to 755be5a Compare September 24, 2019 09:34

Dexterp37 added 2 commits September 24, 2019 11:41

Fix clippy

e8146ab

Exclude glean_initialize_migration from cbindgen

86ebfef

We don't want this function to be used outside of the Kotlin FFI, as it's only useful in the glean-ac to glean-core migration.

Dexterp37 marked this pull request as ready for review September 24, 2019 09:47

Dexterp37 requested review from badboy, travis79 and mdboom September 24, 2019 09:48

Dexterp37 mentioned this pull request Sep 24, 2019

Always set 'wasMigrated' to false in the Glean SDK mozilla-mobile/android-components#4511

Merged

6 tasks

mdboom approved these changes Sep 24, 2019

View reviewed changes

badboy approved these changes Sep 24, 2019

View reviewed changes

travis79 approved these changes Sep 24, 2019

View reviewed changes

Address review feedback

2a73980

Dexterp37 merged commit c43de1b into mozilla:master Sep 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate sequence numbers #309

Migrate sequence numbers #309

Dexterp37 commented Sep 19, 2019 •

edited

Loading

Dexterp37 commented Sep 20, 2019

travis79 left a comment

badboy left a comment

badboy Sep 23, 2019

badboy Sep 23, 2019

badboy Sep 23, 2019

badboy Sep 23, 2019

badboy Sep 23, 2019

badboy Sep 23, 2019

badboy Sep 23, 2019

mdboom Sep 23, 2019

Dexterp37 Sep 23, 2019

mdboom Sep 23, 2019

mdboom Sep 23, 2019

Dexterp37 Sep 23, 2019

badboy Sep 24, 2019

mdboom Sep 23, 2019

mdboom Sep 23, 2019

codecov-io commented Sep 24, 2019 •

edited

Loading

badboy left a comment

badboy Sep 24, 2019

	* to glean-core. This class, along all the migration code, should be removed
	* to glean-core. This class, along with all the migration code, should be removed

Migrate sequence numbers #309

Migrate sequence numbers #309

Conversation

Dexterp37 commented Sep 19, 2019 • edited Loading

Dexterp37 commented Sep 20, 2019

travis79 left a comment

Choose a reason for hiding this comment

badboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 24, 2019 • edited Loading

Codecov Report

badboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dexterp37 commented Sep 19, 2019 •

edited

Loading

codecov-io commented Sep 24, 2019 •

edited

Loading