add metrics coroutine scope #6255
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6255 +/- ##
=======================================
Coverage 50.85% 50.85%
=======================================
Files 112 112
Lines 11811 11811
=======================================
Hits 6006 6006
Misses 5805 5805 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
One small code suggestion in-line. My biggest concern here is that this doesn't fundamentally change when the recording is happening by putting it into a serial coroutine scope. Glean is actually already doing this same thing with its internal dispatcher and I don't think that's going to prevent the need to load geckoview as soon as the coroutine scope executes the Job. You may be able to work around this with what you have by putting a blocking coroutine into your serial execution scope first, and have that coroutine wait on engine-gecko to load before it returns. Since you are using a serial executor, this should hold up any metric/glean interactions that are in the queue behind the "wait for gecko" Job until gecko (and Glean) are loaded.
| } | ||
| recordExperimentTelemetryEvents(events) | ||
| recordExperimentTelemetryEvents(events!!) |
There was a problem hiding this comment.
I'd avoid the bang bang (!!) here, on the off chance that events was null this would throw an exception.
If you can try using the safe unwrap operators and patterns, I think it would avoid this altogether.
| recordExperimentTelemetryEvents(events!!) | |
| events?.let { | |
| recordExperimentTelemetryEvents(it) | |
| } |
That was ultimately what I planned to do in the Fenix code, I'm just not familiar with the gecko parts of Fenix so I'm not sure what exactly I need to be blocking on. |
That's probably best asked of the Fenix folks, but I think you should probably target something at the end of the "visual completeness queue" around here: https://searchfox.org/mozilla-central/source/mobile/android/fenix/app/src/main/java/org/mozilla/fenix/FenixApplication.kt#459 |
Pull Request checklist
[ci full]to the PR title.Branch builds: add
[firefox-android: branch-name]to the PR title.