-
Notifications
You must be signed in to change notification settings - Fork 711
For #4967 - Record search / ads providers appearing in search results #4968
Conversation
Request for data collection review formAll questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.
|
Setting |
@@ -79,7 +80,9 @@ class GleanMetricsService(context: Context) : MetricsService { | |||
private fun collectPrefMetrics( | |||
components: Components, | |||
settings: Settings | |||
) = CoroutineScope(IO).async { | |||
) = CoroutineScope(Main).async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise I'd get a java.lang.IllegalThreadStateException: Must have a Handler
the same as was seen in mozilla-mobile/fenix#17045
A couple of quick requests, neither of which block me being able to perform the review at this time: Can you please edit your comment to list out the metrics being added here? The review form asks for this in tabular form but just a simple list that includes the "measurement description", the "data collection category" should be sufficent (and any links to other issues or bugs). From a stewarding standpoint, I happened to know how to find this in the metrics.yaml file but it is preferred that the newly added metrics are listed explicitly in the review request. Also, I would say that this is not Category 3 data since they are only the counts being recorded and not the specific ad or url. I think this should be Category 2, User Interaction data. And finally: Data Review
Yes, through the metrics.yaml file and the Glean Dictionary
Yes, through the Send Usage Data preference in the app settings.
N/A, collection set to expire 2022-07-01
Category 2, User Interaction or lower
Default-on
No
Yes
No Resultdata-review+ |
app/src/main/java/org/mozilla/focus/telemetry/TelemetryWrapper.kt
Outdated
Show resolved
Hide resolved
) = CoroutineScope(IO).async { | ||
) = CoroutineScope(Main).async { | ||
// Doing this on Main because this might be the first time BrowserStore is accessed and initialized | ||
// and this needs to be done on the Main thread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, the comment is a bit misleading, the Store
can be used on any thread, but the new install()
calls can't.
The whole pattern here is confusing to me, we launch a separate coroutine with the same dispatcher (and an unbound scope) and then await it immediately. Why? 🤔
I wonder if we could move the install()
calls into initialize()
here, before launching the coroutine, making this more explicit here? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Will look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked to be a great solution! Thank you!
@@ -47,6 +48,7 @@ open class FocusApplication : LocaleAwareApplication(), CoroutineScope { | |||
|
|||
TelemetryWrapper.init(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be nice to only initialize all these only if isTelemetryEnabled()
and then observe changes and if the users enable telemetry do the initialization or immediately stop the functionality?
Currently the first two will do their own isTelemetryEnabled()
check and abort the initialization but the FactsProcessor
won't.
Understood from Amedyne that this will have to also be uplifted to 91. (after testing) |
UI test failure; restarting in case it's intermittent. |
This brings the ads/search telemetry up to par with Fenix.
Rebased on the current |
@Mergifyio backport releases_v91.0 |
Command
|
Oops, this was supposed to be 92. |
@Mergifyio backport releases_v92.0 |
Command
|
Command
|
This brings the ads/search telemetry up to par with Fenix.