-
Notifications
You must be signed in to change notification settings - Fork 1.3k
for #11830 added new metric for collecting startup method from all startup phases #11940
Conversation
Test Flows
|
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'm not sure this is 100% accurate to what we're looking for.
@@ -229,6 +234,16 @@ open class HomeActivity : LocaleAwareAppCompatActivity() { | |||
val intentHandled = intentProcessors.any { it.process(intent, navHost.navController, this.intent) } | |||
browsingModeManager.mode = getModeFromIntentOrLastKnown(intent) | |||
|
|||
if (settings().isTelemetryEnabled) { | |||
/* If there is a warm or hot startup, onNewIntent method is always called first. |
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.
One issue I see with using onNewIntent
directly is that it will trigger if the app receives an intent while it's open. I assume this can happen when clicking notifications in the notification tray.
I wonder if we can take this new intent, set it in a variable, and use it in onStart
(I don't know what the lifecycle is though). This may not be worth the effort though (I tend to be too much of a perfectionist – call me out for it!). If making this work gets too complicated, we may want to land something like this sooner and document the known caveats given it probably doesn't happen that often so the data can still be useful.
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 added a new class AllSourceStartupTelemetry
to handle the logic of triggering recording our startup telemetry.
return when { | ||
intent.isLauncherIntent -> Event.OpenedAppAllStart.Source.APP_ICON | ||
intent.action == Intent.ACTION_VIEW -> Event.OpenedAppAllStart.Source.LINK | ||
else -> null |
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 feel like we should record "UNKNOWN" in the else case.
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.
Will update 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.
@mcomella if we record unknown, should we at least store the value of the "UNKNOWN" so that we have some insight on what those different starts were in order to adapt the code afterwards?
app/src/main/java/org/mozilla/fenix/components/metrics/Metrics.kt
Outdated
Show resolved
Hide resolved
@@ -308,6 +323,15 @@ open class HomeActivity : LocaleAwareAppCompatActivity() { | |||
} | |||
} | |||
|
|||
@VisibleForTesting(otherwise = PROTECTED) |
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.
nit: I don't see this being used for testing: shall we make this method protected
and skip the annotation?
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.
Ah yeah, most of the functions was copied from the existing code for "app_opened" metric. I was wondering if unit testing needs to be part of this PR?
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. 1. What questions will you answer with this data? 2. Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements? Some example responses: 3. What alternative methods did you consider to answer these questions? Why were they not sufficient? 4. Can current instrumentation answer these questions?
6. Please provide a link to the documentation for this data collection which describes the ultimate data set in a public, complete, and accurate way. 7. How long will this data be collected? 8. What populations will you measure? 9. If this data collection is default on, what is the opt-out mechanism for users? 10. Please provide a general description of how you will analyze this data. 11. Where do you intend to share the results of your analysis? 12. Is there a third-party tool (i.e. not Telemetry) that you are proposing to use for this data collection? If so: |
a75705e
to
2a88a87
Compare
ce96afa
to
557f996
Compare
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.
data-review+ only, nit on clearer documentation
Data Review Form (to be filled by Data Stewards)
- Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?
Yes, in metrics.md
- Is there a control mechanism that allows the user to turn the data collection on and off?
Yes, through telemetry controls in Fenix
-
If the request is for permanent data collection, is there someone who will monitor the data over time?
Expires in 12/2020 -
Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?
Yes, type 2, when home activity gets opened by user
- Is the data collection request for default-on or default-off?
default on
- Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?
no
- Is the data collection covered by the existing Firefox privacy notice?
yes
- Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**
has expiry
- Does the data collection use a third-party collection tool? If yes, escalate to legal.
no
app/metrics.yaml
Outdated
bugs: | ||
- https://github.com/mozilla-mobile/fenix/issues/11830 | ||
data_reviews: | ||
- NA |
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.
Link here to the PR where you filed the data review request (which will be this issue).
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.
done
app/metrics.yaml
Outdated
type: event | ||
description: | | ||
A user opened the app from any one of the following flows: | ||
hot, warm or cold start to the HomeActivity. |
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.
Could you give an example of what each of these mean in this description?
This description is used by anyone who might be using this telemetry, which could include data science, and they may not have a clear idea of what the difference is between each item (fwiw I'm not really clear if hot startup means just switching to HomeActivity from any other activity, or if it means something else!)
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.
done
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.
Code looks okay assuming the assumptions are correct (onStart is before onNewIntent): here are the nits while I test it out.
app/metrics.yaml
Outdated
source: | ||
description: | | ||
The method used to open Fenix. Possible values are `app_icon`, | ||
`custom_tab` or `link` or `unknown` |
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.
nit: custom_tab
, link
, or unknown
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.
done
app/metrics.yaml
Outdated
type: event | ||
description: | | ||
A user opened the app from any one of the following flows: | ||
hot, warm or cold start to the HomeActivity. |
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.
We may want to elaborate what "HomeActivity" represents – as liuche mentions, this can be used be anyone. I'd say something like, "to the HomeActivity. The HomeActivity encompasses the home screen and browser screen but may include other screens."
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.
done
|
||
override fun onStart() { | ||
super.onStart() | ||
allSourceStartupTelemetry.onStart() |
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.
nit: -> ...onStartHomeActivity
or similar. We want to disambiguate what this call is onStart
of.
* For example, Start app through APP_ICON -> run adb intent command to open a link. | ||
* We want to ignore these adb commands since app is already on foreground. | ||
*/ | ||
open class AllSourceStartupTelemetry(private var currentIntent: Intent?, val 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.
nit: don't include open, we're not overriding this anywhere
* For example, Start app through APP_ICON -> run adb intent command to open a link. | ||
* We want to ignore these adb commands since app is already on foreground. | ||
*/ | ||
open class AllSourceStartupTelemetry(private var currentIntent: Intent?, val 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.
nit: private val metrics
@@ -188,6 +190,16 @@ open class HomeActivity : LocaleAwareAppCompatActivity() { | |||
intent.removeExtra(START_IN_RECENTS_SCREEN) | |||
moveTaskToBack(true) | |||
} | |||
allSourceStartupTelemetry = |
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.
nit: newline above
@@ -245,6 +257,7 @@ open class HomeActivity : LocaleAwareAppCompatActivity() { | |||
?.let { it as? TabTrayDialogFragment } | |||
?.also { it.dismissAllowingStateLoss() } | |||
} | |||
allSourceStartupTelemetry.onNewIntent(intent) |
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.
nit: newline above
currentIntent = intent | ||
} | ||
|
||
open fun getIntentAllStartSource(intent: SafeIntent): Event.OpenedAppAllStart.Source? { |
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.
nit: not open, private
app/src/main/java/org/mozilla/fenix/components/metrics/AllSourceStartupTelemetry.kt
Outdated
Show resolved
Hide resolved
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.
Testing locally on my P2, onNewIntent
gets called after onStart
so I don't think the current code will work
557f996
to
fb00580
Compare
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.
Code looks good - submitting nits while I test.
app/metrics.yaml
Outdated
app_opened_all_startup: | ||
type: event | ||
description: | | ||
A user opened the app to the HomeActivity. The HomeActivity |
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.
"The system received an Intent for the HomeActivity. An intent is received an external entity wants to the app to display content. Intents can be received when the app is closed – at which point the app will be opened – or when the app is already opened – at which point the already open app will make changes such as loading a url. This can be used loosely as a heuristic for when the user requested to open the app. The HomeActivity encompasses..."
Or something like that. We need to make it clear this is not "user opened the app" anymore.
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.
Honestly, I would put all those details below what was initially written. Let's get the simple version first so it's easy for someone to understand the high-level, and then move all the details afterwards.
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.
@sraturi Can you address this in the follow-up? It should be okay to change the description
We should get another data review but let's not block the PR on that. |
for mozilla-mobile#11830 collecting startup metric inaside onNewIntent in HomeActivity clean up, added comments nitpicks cleanup move all source startup telemetry into its own logic and added an UNKOWN state nit cleanup minor changes added comment metric description nit cleanup
nit changes
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.
The intuitive behaviors I tried seem to be working correctly:
- Open app
- Press home, open app
- App link when app is closed (adb & gmail)
- app link when app is opened (adb)
- custom tab when app is closed (fastmail)
- force-stop app and try some of ^
If I wasn't so tired I'd probably try harder to break this but this lgtm for now after the nits are addressed (particular the event name and the docs).
fb00580
to
3ac67e9
Compare
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.
lgtm, let's land as is and follow-up for the nits.
Pull Request checklist
After merge
To download an APK when reviewing a PR: