Add option to defer ping lifetime metric persistence #530

brizental · 2019-11-27T17:14:26Z

Opening this as WIP so I can get validation that I am on the right way and also to discuss the name of the defer_collection configuration option -- keeping in mind that we might change this config option to a interval right after this gets PR accepted (see comment #9 and comment #10).

I think the biggest part of this is done, that was adding the option and changing the way metrics are stored when the config option is set. I thought I had to change a little bit the PingMaker, but after looking at the code seems like my changes to Database were all that was necessary.

In my test I wasn't sure how to check that things were actually saved to the map and not rkv. I am not sure assert!(db.ping_lifetime_data.is_some()); is enough...

Finally, I hard-coded the value of defer_collection on the ffi, so I still need to change that.

Dexterp37

This looks reasonable to me from an high level look. Let's make sure @badboy also gives this a look.

glean-core/src/database/mod.rs

Dexterp37 · 2019-11-28T09:42:39Z

glean-core/src/database/mod.rs

@@ -443,6 +534,14 @@ impl Database {
            .write()
            .expect("Can't access app lifetime data as writable")
            .clear();
+
+        let ping_lifetime_data = &self.ping_lifetime_data;


When we do for lifetime in [Lifetime::User, Lifetime::Ping].iter() on line 522, if the deferred write is on, we don't need to also clear the RKV store. Maybe not a big deal, but could save us some I/O.

This is going to require some refoactoring of this function and I don't know if it makes sense to do it, because if I do that it will be removed in my next PR.

glean-core/src/database/mod.rs

Dexterp37 · 2019-11-28T09:51:16Z

glean-core/src/database/mod.rs

+            }
+        };
+
+        db.iter_store_from(Lifetime::Ping, test_storage, None, &mut snapshotter);


One thing you should probably do is manually check if data was written to rkv. You might want to do something like this.

I added code related to this (database/mod.rs 850-859), I am not sure it works 🤔

What makes you think this is not working?

codecov-io · 2019-11-28T10:02:33Z

Codecov Report

Attention: Patch coverage is 51.51515% with 48 lines in your changes missing coverage. Please review.

Project coverage is 75.17%. Comparing base (4915dbc) to head (02dfd1d).

Files with missing lines	Patch %	Lines
glean-core/src/database/mod.rs	50.00%	24 Missing and 21 partials ⚠️
glean-core/src/lib.rs	0.00%	1 Missing and 1 partial ⚠️
...va/mozilla/telemetry/glean/config/Configuration.kt	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #530      +/-   ##
============================================
- Coverage     75.61%   75.17%   -0.44%     
+ Complexity      334      333       -1     
============================================
  Files            98       98              
  Lines          5736     5820      +84     
  Branches        715      741      +26     
============================================
+ Hits           4337     4375      +38     
- Misses          879      905      +26     
- Partials        520      540      +20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

badboy

One major note here: we need to reload data on startup.

(Haven't looked super-closely at the tests yet, I will do that in a follow-up-review)

glean-core/src/lib.rs

glean-core/src/database/mod.rs

brizental · 2019-11-29T14:57:07Z

@Dexterp37 @badboy this is ready for review.

I still need to see what's going on with CI and the new cbindgen version, but that shouldn't get much in the way. For context: I just downloaded cbindgen in version 0.10.0 and that broke some things and changed the generated header files (because CI was using 0.9.10). I added a commit with the update + header file changes to this PR, but that is still breaking the build.

iOS integration test is also failing in the CI, but that doesn't look like it is related to these chages.

.circleci/config.yml

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

glean-core/src/database/mod.rs

docs/appendix/twig.md

glean-core/src/lib.rs

glean-core/src/database/mod.rs

badboy

Got one question left, but that's for @Dexterp37.
When that is answered we can land it I think.

glean-core/ffi/src/lib.rs

Dexterp37

This looks good to me with the nits below addressed.

glean-core/src/database/mod.rs

Dexterp37 · 2019-12-04T09:40:59Z

glean-core/src/database/mod.rs

+            }
+        };
+
+        db.iter_store_from(Lifetime::Ping, test_storage, None, &mut snapshotter);


What makes you think this is not working?

glean-core/src/lib.rs

glean-core/tests/timing_distribution.rs

Dexterp37 · 2019-12-04T10:14:27Z

glean-core/src/database/mod.rs

+        db.iter_store_from(Lifetime::Ping, test_storage, None, &mut snapshotter);
+        assert_eq!(1, found_metrics, "We only expect 1 Lifetime.Ping metric.");
+
+        let store: SingleStore = unwrap_or!(


We discussed over Slack and agreed to slightly change this to make sure that something is being written to rkv at all :)

Would the comment I added suffice here or do you mean for me to add the whole experiment I did to lower my confusion?

I personally don't think the experiment adds to the test and makes it a little confusing (since it is code that is not related), but I want to know your opinion :)

No, that's fine :)

Dexterp37 · 2019-12-04T13:25:26Z

glean-core/src/database/mod.rs

+        db.iter_store_from(Lifetime::Ping, test_storage, None, &mut snapshotter);
+        assert_eq!(1, found_metrics, "We only expect 1 Lifetime.Ping metric.");
+
+        let store: SingleStore = unwrap_or!(


No, that's fine :)

brizental requested review from badboy and Dexterp37 November 27, 2019 17:14

brizental requested review from mdboom and travis79 as code owners November 27, 2019 17:14

brizental force-pushed the defer-ping-collection branch 2 times, most recently from 1f91292 to 15bd43b Compare November 28, 2019 09:12

Dexterp37 reviewed Nov 28, 2019

View reviewed changes

brizental force-pushed the defer-ping-collection branch from 15bd43b to 10c6059 Compare November 28, 2019 09:51

badboy requested changes Nov 28, 2019

View reviewed changes

glean-core/src/lib.rs Outdated Show resolved Hide resolved

glean-core/src/database/mod.rs Show resolved Hide resolved

brizental force-pushed the defer-ping-collection branch 2 times, most recently from b8038c2 to b864069 Compare November 29, 2019 13:53

brizental changed the title ~~[WIP] Add option to defer ping lifetime metric persistence~~ Add option to defer ping lifetime metric persistence Nov 29, 2019

brizental force-pushed the defer-ping-collection branch 3 times, most recently from e888a33 to 3bd01c7 Compare November 29, 2019 14:45

badboy reviewed Nov 29, 2019

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

brizental force-pushed the defer-ping-collection branch from 3bd01c7 to 77b6d8b Compare December 2, 2019 09:31

Dexterp37 requested changes Dec 2, 2019

View reviewed changes

brizental force-pushed the defer-ping-collection branch 2 times, most recently from f371ae7 to b6a6153 Compare December 2, 2019 15:02

Dexterp37 reviewed Dec 2, 2019

View reviewed changes

docs/appendix/twig.md Outdated Show resolved Hide resolved

brizental force-pushed the defer-ping-collection branch 2 times, most recently from 66261e4 to 24129ce Compare December 2, 2019 16:38

travis79 removed their request for review December 2, 2019 20:27

Dexterp37 requested changes Dec 3, 2019

View reviewed changes

glean-core/src/lib.rs Outdated Show resolved Hide resolved

glean-core/src/database/mod.rs Outdated Show resolved Hide resolved

brizental force-pushed the defer-ping-collection branch from 24129ce to 1c4a1a1 Compare December 3, 2019 11:40

badboy approved these changes Dec 3, 2019

View reviewed changes

glean-core/ffi/src/lib.rs Show resolved Hide resolved

Dexterp37 approved these changes Dec 4, 2019

View reviewed changes

brizental force-pushed the defer-ping-collection branch from 1c4a1a1 to 759937d Compare December 4, 2019 09:59

Dexterp37 requested changes Dec 4, 2019

View reviewed changes

brizental force-pushed the defer-ping-collection branch from 759937d to 5ebc4bf Compare December 4, 2019 10:23

Add option to defer ping lifetime metric persistence

02dfd1d

brizental force-pushed the defer-ping-collection branch from 5ebc4bf to 02dfd1d Compare December 4, 2019 12:43

Dexterp37 approved these changes Dec 4, 2019

View reviewed changes

brizental merged commit eb984ca into mozilla:master Dec 4, 2019

brizental deleted the defer-ping-collection branch December 4, 2019 13:26

brizental mentioned this pull request Dec 11, 2019

Add method to persist ping lifetime data on demand #565

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to defer ping lifetime metric persistence #530

Add option to defer ping lifetime metric persistence #530

brizental commented Nov 27, 2019

Dexterp37 left a comment

Dexterp37 Nov 28, 2019

brizental Dec 2, 2019

Dexterp37 Nov 28, 2019

brizental Dec 2, 2019 •

edited

Loading

Dexterp37 Dec 4, 2019

codecov-io commented Nov 28, 2019 •

edited by codecov bot

Loading

badboy left a comment •

edited

Loading

brizental commented Nov 29, 2019

badboy left a comment

Dexterp37 left a comment

Dexterp37 Dec 4, 2019

Dexterp37 Dec 4, 2019

brizental Dec 4, 2019 •

edited

Loading

Dexterp37 Dec 4, 2019

Dexterp37 Dec 4, 2019

Add option to defer ping lifetime metric persistence #530

Add option to defer ping lifetime metric persistence #530

Conversation

brizental commented Nov 27, 2019

Dexterp37 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

brizental Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 28, 2019 • edited by codecov bot Loading

Codecov Report

badboy left a comment • edited Loading

Choose a reason for hiding this comment

brizental commented Nov 29, 2019

badboy left a comment

Choose a reason for hiding this comment

Dexterp37 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brizental Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brizental Dec 2, 2019 •

edited

Loading

codecov-io commented Nov 28, 2019 •

edited by codecov bot

Loading

badboy left a comment •

edited

Loading

brizental Dec 4, 2019 •

edited

Loading