-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #21009 - New Recently closed tabs telemetry #22588
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.
|
app/metrics.yaml
Outdated
- interaction | ||
notification_emails: | ||
- android-probes@mozilla.com | ||
expires: "2022-02-01" |
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.
Are you sure you want to set it in a month and a half?
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.
Since "Recently closed tabs" is a section in History I went with using the same expiry date as for some history probes -
Lines 2495 to 2527 in b41542d
history: | |
opened: | |
type: event | |
description: | | |
A user opened the history screen | |
bugs: | |
- https://github.com/mozilla-mobile/fenix/issues/2362 | |
data_reviews: | |
- https://github.com/mozilla-mobile/fenix/pull/3940 | |
- https://github.com/mozilla-mobile/fenix/pull/15713#issuecomment-703972068 | |
- https://github.com/mozilla-mobile/fenix/pull/19924#issuecomment-861423789 | |
- https://github.com/mozilla-mobile/fenix/pull/20517#pullrequestreview-718069041 | |
data_sensitivity: | |
- interaction | |
notification_emails: | |
- android-probes@mozilla.com | |
expires: "2022-02-01" | |
removed: | |
type: event | |
description: | | |
A user removed a history item | |
bugs: | |
- https://github.com/mozilla-mobile/fenix/issues/2362 | |
data_reviews: | |
- https://github.com/mozilla-mobile/fenix/pull/3940 | |
- https://github.com/mozilla-mobile/fenix/pull/15713#issuecomment-703972068 | |
- https://github.com/mozilla-mobile/fenix/pull/19924#issuecomment-861423789 | |
- https://github.com/mozilla-mobile/fenix/pull/20517#pullrequestreview-718069041 | |
data_sensitivity: | |
- interaction | |
notification_emails: | |
- android-probes@mozilla.com | |
expires: "2022-02-01" |
Can change to maybe a year from now it it would fit better. Please advise.
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.
I see. I would suggest going for a year though. I think it's OK not to expire at the same time as other probes.
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. Updated the expiry date to 2023-01-31
.
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.
Sorry for the late review. Don't forget to update metrics.yml
with data-review URL. Thanks,
Data Review
Yes, through the metrics.yaml file and the Glean Dictionary
Yes, through the "Send Usage Data" preference in the application settings
N/A, collection set to end or be renewed by 2023-01-31
Category 2, Interaction data
default-on
No
Yes
No Resultdata-review+ |
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.
Looks good to me. Please let me know once you've update metrics.yml
with the data review URL.
This adds a new `recently_closed_tabs` category with then events for all user interactions on the screen. The already existent `events.recently_closed_tabs_opened` is still kept for a bit more time to still have this data available while the new telemetry ride the trains but can later be removed in favor of this newly added events.
Did that now. |
@@ -41,6 +43,8 @@ class RecentlyClosedFragment : LibraryPageFragment<RecoverableTab>(), UserIntera | |||
private lateinit var recentlyClosedInteractor: RecentlyClosedFragmentInteractor | |||
private lateinit var recentlyClosedController: RecentlyClosedController | |||
|
|||
private lateinit var metrics: MetricController |
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.
Do you still need this? Looks like you can just use requireComponents.analytics.metrics
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.
There are 3 places where this is used in the fragment (two places where we track events, one where we pass the instance). Seems cleaner to use a use a property instead of accessors chains in all places.
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.
Sure. Just thought that we can save an assignment with directly using it. I'm OK with either.
@@ -41,6 +43,8 @@ class RecentlyClosedFragment : LibraryPageFragment<RecoverableTab>(), UserIntera | |||
private lateinit var recentlyClosedInteractor: RecentlyClosedFragmentInteractor | |||
private lateinit var recentlyClosedController: RecentlyClosedController | |||
|
|||
private lateinit var metrics: MetricController |
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.
Sure. Just thought that we can save an assignment with directly using it. I'm OK with either.
lint was failing because of
Restarting CI. |
This adds a new
recently_closed_tabs
category with then events for all userinteractions on the screen.
The already existent
events.recently_closed_tabs_opened
is still kept for abit more time to still have this data available while the new telemetry ride
the trains but can later be removed in favor of this newly added events.
Pull Request checklist
To download an APK when reviewing a PR: