-
Notifications
You must be signed in to change notification settings - Fork 1.3k
MR2 Telemetry: Opening screen preference #22333
MR2 Telemetry: Opening screen preference #22333
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.
|
settings.openHomepageAfterFourHoursOfInactivity -> "homepage after four hours" | ||
else -> "" | ||
} | ||
) | ||
components.analytics.experiments.register(object : NimbusInterface.Observer { |
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 was unsure what's going on here with the Nimbus code, so I chose to put this particular metric outside of it. Is this location alright, or should it be moved inside the experiment run block?
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.
This observer waits for Nimbus (our experimentation framework) to finish it's async startup before we send back telemetry results which may be based an experiment group the user is in.
The questions worth asking in that case are: do these telemetry probes you're collection dependant on an experiment, are the options visible/invisible, etc.
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.
If the answer is, they don't depend on an experiment. They should be fine there, although I'd leave that as a comment in the code because it would definitely look odd to the next person coming by and wondering the same thing.
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.
Yeah, that's where my uncertainly is mainly coming from. I'm primarily unsure whether the experiment code here shows/hides stuff (and maybe is the experiment even still running), in which case I'd need to put my new probe with the existing ones.
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.
(and maybe is the experiment even still running)
It's harder to determine future state but we can check the current state - so let's do that!
I've elaborated my investigation below if that helps you in the future on how to find the answers you're looking for. 🙂
Let's look at all the references to alwaysOpenTheHomepageWhenOpeningTheApp
, alwaysOpenTheLastTabWhenOpeningTheApp
, openHomepageAfterFourHoursOfInactivity
: they all seem to come from Settings.shouldStartOnHome
and that is only used on the HomeActivity
.
In that block itself we see a feature flag, FeatureFlags.showStartOnHomeSettings
which is defaulted to on. Looking at other references to the HomeActivity method there, I see none that suspect they are related to feature flags or nimbus.
So let's look at the FeatureFlags.showStartOnHomeSettings
: with a git blame on the file, we can see that we flipped the flag true as part of our 94 release. With 94 now in release, we typically remove the feature flag since we rarely have a need to keep it around for much longer.
This tells me that we should be fine with collecting the preference where you currently have, or even after Nimbus is done loading because the value isn't dependant on it.
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.
This makes sense. Thank you so much for taking the time to explain your thought process of your exploration and for the additional context about the retirement of feature flags.
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.
eng r+
(please see the comment below though!)
settings.openHomepageAfterFourHoursOfInactivity -> "homepage after four hours" | ||
else -> "" | ||
} | ||
) | ||
components.analytics.experiments.register(object : NimbusInterface.Observer { |
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.
(and maybe is the experiment even still running)
It's harder to determine future state but we can check the current state - so let's do that!
I've elaborated my investigation below if that helps you in the future on how to find the answers you're looking for. 🙂
Let's look at all the references to alwaysOpenTheHomepageWhenOpeningTheApp
, alwaysOpenTheLastTabWhenOpeningTheApp
, openHomepageAfterFourHoursOfInactivity
: they all seem to come from Settings.shouldStartOnHome
and that is only used on the HomeActivity
.
In that block itself we see a feature flag, FeatureFlags.showStartOnHomeSettings
which is defaulted to on. Looking at other references to the HomeActivity method there, I see none that suspect they are related to feature flags or nimbus.
So let's look at the FeatureFlags.showStartOnHomeSettings
: with a git blame on the file, we can see that we flipped the flag true as part of our 94 release. With 94 now in release, we typically remove the feature flag since we rarely have a need to keep it around for much longer.
This tells me that we should be fine with collecting the preference where you currently have, or even after Nimbus is done loading because the value isn't dependant on it.
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 2022-11-01
Category 2, Interaction data
default-on
No
Yes
No Resultdata-review+ |
* For mozilla-mobile#22145 - Added telemetry to the opening screen preference. * For mozilla-mobile#22145 - Added PR number to metric Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
#22145
Pull Request checklist
To download an APK when reviewing a PR: