for #12573, added telemetry for startup type

[sraturi]

Pull Request checklist

• Tests: This PR includes thorough tests or an explanation of why it does not
• Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

After merge

• Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

1. click on Show All Checks,
2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
3. click on the "Fenix - assemble" task, then click "Run Artifacts".
4. the APK links should be on the left side of the screen, named for each CPU architecture

[sraturi]

Tested scenarios

App starting fresh

1. Launch app, FenixApplication is created = COLD startup
2. Launch app on external activity = COLD
3. Launch app through icon ->press home button -> open external activity (creating a new activity) = WARM
4. Launch app through icon -> press home button -> open a link = HOT

App is already launched

1. press home button (Homeactivity stopped)-> launch app = HOT
2. press the back button (Homeactivity destroyed)-> launch app = WARM
3. press home button-> show all recent tasks -> pick fenix = HOT

[codecov-commenter]

Codecov Report

Merging #13494 into master will increase coverage by 0.07%.
The diff coverage is 78.57%.

@@             Coverage Diff              @@
##             master   #13494      +/-   ##
============================================
+ Coverage     29.65%   29.72%   +0.07%     
- Complexity     1147     1159      +12     
============================================
  Files           440      440              
  Lines         17792    17824      +32     
  Branches       2309     2315       +6     
============================================
+ Hits           5276     5298      +22     
- Misses        12132    12140       +8     
- Partials        384      386       +2     

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/org/mozilla/fenix/FenixApplication.kt	7.45% <0.00%> (-0.05%)	5.00 <0.00> (ø)
...pp/src/main/java/org/mozilla/fenix/HomeActivity.kt	5.80% <0.00%> (-0.07%)	11.00 <0.00> (ø)
...la/fenix/components/metrics/GleanMetricsService.kt	12.13% <0.00%> (ø)	4.00 <0.00> (ø)
...lla/fenix/customtabs/ExternalAppBrowserActivity.kt	38.46% <0.00%> (ø)	4.00 <0.00> (ø)
...java/org/mozilla/fenix/components/metrics/Event.kt	31.56% <83.33%> (+2.00%)	0.00 <0.00> (ø)
...la/fenix/components/metrics/AppStartupTelemetry.kt	97.05% <97.05%> (ø)	22.00 <22.00> (?)
...in/java/org/mozilla/fenix/components/Components.kt	30.88% <100.00%> (ø)	1.00 <0.00> (ø)
...in/java/org/mozilla/fenix/settings/SupportUtils.kt	70.00% <0.00%> (-4.00%)	11.00% <0.00%> (-2.00%)
...mponents/searchengine/FenixSearchEngineProvider.kt	62.16% <0.00%> (-1.81%)	14.00% <0.00%> (+1.00%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3713b5a...ba73c6e. Read the comment docs.

[mcomella]

The code looks good for what it's doing but I still have to test it. However, I feel like we should be merging this with the app_link, custom_tab, etc. data as it helps guarantee we're not getting wonky numbers (e.g. one probe reports more invocations than another probe). What do you think?

I wrote all of these comments (mostly nits) before I thought of ^ so try to reinterpret them in that lens and, if we do merge the probes, maybe avoid tackling them until we get there.

[sraturi]

@mcomella I made all the changes to merge together the two metrics (app startup source and type). Let me know if this is what you were looking for? If not I can revert back to the previous commit.

[mcomella]

The code looks like it'll work but I think it's hard to say given the current formulation – I'm wondering if we can make it clearer, especially the onRestart flow. Let's see what you can come up with (timebox it?) and if not feasible or it's not a good use of time, I can re-review this with nits and maybe land it as is.

[sraturi]

Request for data collection review form

All questions are mandatory. You must receive a 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?

What type of startup user uses most? For example, cold, warm, hot.

What type of startup source users uses most? For example, app_icon, link, custom_tab

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:

Creating an accurate understanding of real-world usage to prioritize performance improvements

3. What alternative methods did you consider to answer these questions? Why were they not sufficient?

There is no existing metrics that answer this question.

4. Can current instrumentation answer these questions?

No, we are modifying the existing probe to add additional startup data.

5. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories found on the Mozilla wiki.

data is Interaction data. What type of startup user use most.

Options

• cold

• Cold (with saved_instance state)

• warm

• warm, (with saved_instance state)

• hot.

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.

This is documented in the metrics.md file

7. How long will this data be collected?

Until 2021-02-01

8. What populations will you measure?

All release, beta, and nightly users with telemetry enabled.

9. If this data collection is default on, what is the opt-out mechanism for users?

default on, opt out by Settings ->Data collection

10. Please provide a general description of how you will analyze this data.

we can set up better tests and prioritize analyses to improve performance in the most impactful areas.

11. Where do you intend to share the results of your analysis?

Glean, Amplitude, and mobile teams.

12. Is there a third-party tool (i.e. not Telemetry) that you are proposing to use for this data collection? If so:

N/A

[mcomella]

Looks functional but I still think we can clean this up a bit more (sorry for all the nits). Also, I think we should restore the UNKNOWN case or similar.

[sraturi]

@mcomella

Seems like we might never get [CUSTOM_TAB, COLD, false/true] case (or maybe I'm missing something).

So whenever I open a specific email on Gmail (maybe it's a Gmail thing), our application seems to start right away (probably because its a default browser). So before even clicking a link to open the custom tab, the application is already started.

You might already know this? or maybe it's specific to certain API or apps?

[mcomella]

I believe the error case isn't handled quite right.

[mcomella]

I just noticed: since it's always false, can't we just omit this extra in the telemetry? (e.g. a nullable field in the metric class that, when null, does not add the extra)

[sraturi]

not sure what you mean by omit extra in telemetry?

[mcomella]

Instead of recording [HOT, APP_ICON, false], record [HOT, APP_ICON]

[mcomella]

Be sure to document the fact that it's optional

[liuche]

Looking at the metrics.yaml for the data review only here

It looks like this is updating an existing probe with more detail, and adding some more extras

Data Review Form (to be filled by Data Stewards)

1. Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, this is updating the app_opened_all_startup perf telemetry event

2. Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, this will also be controlled by the Fenix telemetry settings

3. If the request is for permanent data collection, is there someone who will monitor the data over time?

No, there is expiry for this probe

4. Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Type 2 entry point of startup and whether this is returning to a saved app instance

5. Is the data collection request for default-on or default-off?

Default on

6. 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

7. Is the data collection covered by the existing Firefox privacy notice?
   Yes

8. Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No)
   No

9. Does the data collection use a third-party collection tool?

No

[mcomella]

Be sure to document the fact that it's optional

[mcomella]

Initial thoughts: I'll keep looking through the tests.

[mcomella]

Tests are looking good – it looks like we're capturing all the major cases.

I think the major areas remaining are from the previous comment block:

• simplify mergeOnRestart...
• Try to omit savedInstanceState when it's not relevant (i.e. onCreate is not called)

[mcomella]

nit: You should avoid using else if possible: e.g. if we add a new case, someone might be less likely to notice they need to update this code. The underlying coding principle: it's frequently better to be explicit.

-> UNKNOWN, CUSTOM_TAB -> {

Also, being explicit makes you ask questions as a developer. For example, the CUSTOM_TAB intent isn't a MAIN action, right? So that means this setupIntentMock function isn't accurate for that case and that might cause people issues when adding future tests.

[mcomella]

What does answers = false do?

[sraturi]

I couldn't really find something official and clear but it's consistent so I went on to use it. basically, it is clearing up calls to metricController.
For example,

metricController.track(something)
clearMocks(metricController, answers = false) 
metricController.track(anotherThing)

verify(exactly = 1) { metricController.track(any()) } // this will be true

[mcomella]

Do you need answers = false or does clearMocks(metricController) work? I would prefer we do the latter if it does and are not sure what the arguments do.

[sraturi]

we need the answers part. otherwise, we need to re-init the mock every time.

[mcomella]

No action needed

Note: I notice you're writing a test for many combinations: 3 startup types foreground, 3 startup types background, etc.

I probably would have written all these tests 😅 but it probably is a perfect is the enemy of the good situation where it's probably not necessary: doing all the combinations in one type of test and hitting all the other types of tests is probably good enough.

That being said, junit supports a feature called parameterized tests which would make writing all these tests combinations much less code but I haven't ever been able to set it up.

[mcomella]

@sraturi btw, there is also a UI test failure now – I assume it's intermittent given what we're changing.

[mcomella]

lgtm, thanks for sticking in there through this lengthy PR!

I tested COLD, WARM, HOT across LINK, CUSTOM_TAB, APP_ICON, and the app switcher case and it appears to be working correctly. I wasn't able to replicate the hasSavedInstanceState case (maybe Don't Keep Activities?) but the code looks simple enough so I think it should be fine.

It looks like you'll need to rebase though.

[mcomella]

These AppAllStartup tests should technically be in the corresponding test file for where AppAllStartup is defined.

However, let's avoid another back and forth: let's land it as is and if you have time and you're feeling inspired afterwards, you can move it. :) Thanks for writing tests for these cases.

[sraturi]

The events have tons of data classes so would it make sense to create a single Events test file and put all the tests for all the classes there? or make a folder called event, and put all the individual files there?

I think with a single file, before/after might get confusing? unless we use Junit5? which has nested support.

[mcomella]

In a follow-up PR (so we can get this landed), can we document two additional things:

1. that custom tab never sees COLD start and briefly explain why?
2. that if you click "Open in Firefox Preview" from a custom tab, a second event is not recorded

[sraturi]

I think there are places where custom tab might have a cold start. One case would be me: launch gmail (fenix in background), close fenix from recent tasks, and launch a link from gmail.