[glean]Use WorkManager for uploading glean pings

[travis79]

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

[travis79]

This is still work in progress, mostly because I found a limitation on how I was passing data to the WorkManager worker that restricts the data to 10240 bytes because it relies on androidx.work.Data object. This probably isn't an issue for baseline pings but I have concerns with events and metrics pings exceeding this size. If that is the case, another method of serializing the ping payload that is accessible to the worker may be required.

Also, I need to add tests for the worker class, which I'm currently looking at the best way to do this.

[Dexterp37]

This is still work in progress, mostly because I found a limitation on how I was passing data to the WorkManager worker that restricts the data to 10240 bytes because it relies on androidx.work.Data object.

I think this requires a bit of discussion around the design: we probably want to follow a similar approach to how the legacy telemetry library was handling things. This would allow us to work around the size limitation by serializing pings to disk.

This probably isn't an issue for baseline pings but I have concerns with events and metrics pings exceeding this size. If that is the case, another method of serializing the ping payload that is accessible to the worker may be required.

Yeah, that depends on how the work about "extending the baseline ping goes". I doubt we'll ever go over 10kB with the baseline ping, but we will probably do for other pings.

[pocmo]

This is still work in progress, mostly because I found a limitation on how I was passing data to the WorkManager worker that restricts the data to 10240 bytes because it relies on androidx.work.Data object.

I think this requires a bit of discussion around the design: we probably want to follow a similar approach to how the legacy telemetry library was handling things. This would allow us to work around the size limitation by serializing pings to disk.

Yeah, the old library writes pings to disk and then schedules an upload job. There will always only be one job scheduled. Once the job runs it uploads all pings that are waiting on disk.

[codecov]

Codecov Report

Merging #1973 into master will decrease coverage by 0.24%.
The diff coverage is 77.08%.

@@             Coverage Diff              @@
##             master    #1973      +/-   ##
============================================
- Coverage     84.88%   84.63%   -0.25%     
- Complexity     2353     2379      +26     
============================================
  Files           279      281       +2     
  Lines          9624     9685      +61     
  Branches       1418     1425       +7     
============================================
+ Hits           8169     8197      +28     
- Misses          872      897      +25     
- Partials        583      591       +8

Impacted Files	Coverage Δ	Complexity Δ
...ents/service/glean/storages/EventsStorageEngine.kt	100% <100%> (ø)	12 <0> (ø)	⬇️
...onents/service/glean/scheduler/PingUploadWorker.kt	60% <60%> (ø)	2 <2> (?)
...onents/service/glean/storages/PingStorageEngine.kt	68.18% <68.18%> (ø)	13 <13> (?)
...ain/java/mozilla/components/service/glean/Glean.kt	75% <75%> (-7.5%)	1 <0> (ø)
...omponents/service/glean/scheduler/PingScheduler.kt	88.09% <88.09%> (ø)	12 <12> (?)
...lla/components/service/glean/DatetimeMetricType.kt	70.96% <0%> (-16.13%)	20% <0%> (-1%)
...a/components/service/glean/net/HttpPingUploader.kt	77.19% <0%> (-14.04%)	15% <0%> (-1%)
... 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 4418aad...cbad8cf. Read the comment docs.

[travis79]

Okay, I finally feel pretty good about the test coverage and functionality. I was able to pull most/all of the scheduling code out of Glean.kt and implement it into the PingScheduler class (which also assumed the role of the LifecycleObserver), as well as implement a PingStorageEngine for managing writing the pings to file. This change in things forced me to do a lot of changes to the way we test the pings and unfortunately its now much harder(impossible?) to test using a mock http server where we were using it so it now tests that the work gets scheduled and that the worker does what it's supposed to do working with the HttpUploader class.

I expect some criticism/feedback over the size/breadth of the refactor so please don't hold back. I think that overall this cleans up the things that needed cleaned up and puts things where they should be. I'll be leaving this a WIP until everyone weighs in.

[travis79]

Gah, this is so weird. All tests pass locally but I'm randomly getting different test in taskcluster that fail. I've done this twice now, first time 12 tests failed, this time 2 tests failed (all in service-glean). Going to try it again and see what happens...

Well, at least I got two different tests to fail this time :/

4th times a charm! (FINALLY lol)

[mdboom]

This was a lot of work, but it's looking good.

I feel like it's big enough that I may have missed things, so don't be offended if my comments are pointing out problems that don't exist.

[Dexterp37]

Hey Travis! I gave this a first look. In general, this looks ok. A few high level observations:

• can we remove from this PR everything that is not required for it? it would make review much easier;
• do we need to save pings in specific, separate directories for each type?

[Dexterp37]

I gave this a more in-depth look. I had to un-resolve some comments that were marked as resolved in the previous PR, but they were not :( Let's make sure to address all the comments before the next iteration - darn github.

[Dexterp37]

We're close to the finish line with this, I think. This final pass is for requesting bugs to be filed. My remaining major concern is about the Thread.sleep functions in tests. Let's not use them but rather investigate why this is failing.

[travis79]

@Dexterp37 ready for another look. I believe I have addressed all of the changes you requested.

[travis79]

FYI The merging in of the async await stuff for the test API functions helped the flakiness with passing of tests.

[Dexterp37]

!

[pocmo]

🚢

[pocmo]

nit: You can mark them as val inside the constructor above. (maybe internal too?)