3869 fix default search engine #6197

severinrudie · 2019-10-22T21:30:52Z

NOTE: this does not close #3869, which is still blocked on getting an API key. This completes the prework so we will be able to just swap the key in and close.

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
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
No user facing changes
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features
No user facing changes

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:

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

codecov-io · 2019-10-23T17:28:19Z

Codecov Report

Merging #6197 into master will decrease coverage by 0.07%.
The diff coverage is 38.09%.

@@             Coverage Diff             @@
##             master   #6197      +/-   ##
===========================================
- Coverage     16.48%   16.4%   -0.08%     
+ Complexity      353     352       -1     
===========================================
  Files           276     276              
  Lines         11146   11206      +60     
  Branches       1602    1608       +6     
===========================================
+ Hits           1837    1838       +1     
- Misses         9155    9214      +59     
  Partials        154     154

Impacted Files	Coverage Δ	Complexity Δ
.../mozilla/fenix/search/awesomebar/AwesomeBarView.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...in/java/org/mozilla/fenix/search/SearchFragment.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...enix/settings/search/SearchEngineListPreference.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...la/fenix/components/metrics/GleanMetricsService.kt	`8.12% <50%> (+0.32%)`	`3 <0> (ø)`	⬇️
.../java/org/mozilla/fenix/search/SearchController.kt	`73.46% <54.54%> (-19.87%)`	`0 <0> (ø)`
...c/main/java/org/mozilla/fenix/components/Search.kt	`92% <93.33%> (+1.09%)`	`1 <0> (ø)`	⬇️
...mozilla/fenix/components/metrics/ActivationPing.kt	`45.9% <0%> (-11.48%)`	`6% <0%> (-2%)`
...pp/src/main/java/org/mozilla/fenix/HomeActivity.kt	`7.69% <0%> (-0.8%)`	`7% <0%> (+1%)`
...va/org/mozilla/fenix/components/metrics/Metrics.kt	`26.04% <0%> (-0.47%)`	`0% <0%> (ø)`

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 8f50e2a...d20cfd7. Read the comment docs.

severinrudie · 2019-10-23T17:36:56Z

app/src/main/java/org/mozilla/fenix/components/metrics/GleanMetricsService.kt

-        }
+                code.set(defaultEngine.identifier)
+                name.set(defaultEngine.name)
+                submissionUrl.set(defaultEngine.buildSearchUrl(""))


As far as I can tell, these properties are only used to send this ping, so making this async shouldn't change any behavior.

severinrudie · 2019-10-25T21:27:20Z

app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt

+     *
+     * Cache hits/misses are reported to telemetry.
+     */
+    private fun getCurrentSearchEngine(context: Context): SearchEngine {


See commit message for an explanation as to why this approach was taken.

severinrudie · 2019-10-28T16:23:45Z

app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt

+            .getDefaultSearchEngine(context)
+
+        val cacheWasHit = selectedSearchEngine != null
+        Events.searchFragmentCacheAccess.set(cacheWasHit)


This is the first time I've added a new ping, would you please double check that I'm doing it properly?

severinrudie · 2019-10-28T16:29:32Z

@boek data review

Request for data collection review form

What questions will you answer with this data?

How frequently do users reach the SearchFragment without their location already having been cached?

Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?

If cache misses are frequent, we will need to find a new solution to the problem. If not, this is probably fine.

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

This is the only way to determine this information.

Can current instrumentation answer these questions?

No telemetry currently exists to answer these questions.

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

Measurement Description	Data Collection Category	Tracking Bug #
When `SearchFragment` has been opened, whether or not our search engine lookup hits a warm cache	1	[Issue 3869](#3869

How long will this data be collected?

Fenix engineering will monitor this data until 2020-03-01

What populations will you measure?

All users with telemetry enabled, on release channel.

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

There is an option in settings to stop sending usage data.

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

The data will be analyzed through Redash queries and dashboards.

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

On Redash and with mobile teams.

SearchEngineManager now provides the default search engine when no name param is passed

See mozilla-mobile#3869 (comment) for an explanation of why

Alternatives to this approach that I tried: Approach: make searchEngineSource & defaultEngineSource nullable, launch an async call in onCreateView, and update them when possible. Problems: a lot of code relies on these states always being available. In particular, I was concerned that this could introduce problems into some of our telemetry. Approach: make EngineSource an argument to SearchFragment. Retrieve it before launching SearchFragment Problems: SearchFragment can be started via intent. Taking this approach would hurt our cold startup time in that instance, which is a critical performance area. This also spread complex coroutine code across a lot of the codebase.

boek · 2019-11-08T21:39:14Z

app/metrics.yaml

@@ -154,6 +154,16 @@ events:
    notification_emails:
      - fenix-core@mozilla.com
    expires: "2020-03-01"
+  search_fragment_cache_access:


Could you fill out the data request form for this? :)

@boek already did!

I'm a dummy, don't know how I missed it!

severinrudie · 2019-11-11T17:12:50Z

Closing, as these changes will be made redundant by #5577. We expect that story to fix #3869

severinrudie force-pushed the 3869-fix-default-search-engine branch from d553ab8 to 0bd9262 Compare October 23, 2019 16:58

severinrudie commented Oct 23, 2019

View reviewed changes

severinrudie force-pushed the 3869-fix-default-search-engine branch from 9cf35b1 to 719fb2c Compare October 23, 2019 17:53

severinrudie commented Oct 25, 2019

View reviewed changes

severinrudie force-pushed the 3869-fix-default-search-engine branch 2 times, most recently from 89df622 to 16708f3 Compare October 25, 2019 23:18

severinrudie commented Oct 28, 2019

View reviewed changes

severinrudie marked this pull request as ready for review October 28, 2019 16:29

severinrudie force-pushed the 3869-fix-default-search-engine branch from fe17b33 to b217723 Compare November 4, 2019 22:04

severinrudie added 7 commits November 6, 2019 13:12

For mozilla-mobile#3869: remove search engine hack

c71fb7b

SearchEngineManager now provides the default search engine when no name param is passed

For 3869: updated most SearchEngineManager usages to be async

d5cba99

See mozilla-mobile#3869 (comment) for an explanation of why

For 3869: fix tests broken by async SearchEngineManager

34bda4f

For 3869: GleanMetricsService now uses async SearchEngineManager

35bbd17

For 3869: add telemetry for SearchFragment cache hit/miss

7be5b77

For 3869: retrieve / use MLS key

d20cfd7

severinrudie force-pushed the 3869-fix-default-search-engine branch from b217723 to d20cfd7 Compare November 6, 2019 21:28

boek reviewed Nov 8, 2019

View reviewed changes

severinrudie requested a review from boek November 8, 2019 23:42

boek added the needs:data-review PR is awaiting a data review label Nov 8, 2019

severinrudie closed this Nov 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3869 fix default search engine #6197

3869 fix default search engine #6197

severinrudie commented Oct 22, 2019 •

edited

Loading

codecov-io commented Oct 23, 2019 •

edited

Loading

severinrudie Oct 23, 2019

severinrudie Oct 25, 2019

severinrudie Oct 28, 2019

severinrudie commented Oct 28, 2019

boek Nov 8, 2019 •

edited

Loading

severinrudie Nov 8, 2019

boek Nov 8, 2019

severinrudie commented Nov 11, 2019

3869 fix default search engine #6197

3869 fix default search engine #6197

Conversation

severinrudie commented Oct 22, 2019 • edited Loading

Pull Request checklist

After merge

To download an APK when reviewing a PR:

codecov-io commented Oct 23, 2019 • edited Loading

Codecov Report

severinrudie Oct 23, 2019

Choose a reason for hiding this comment

severinrudie Oct 25, 2019

Choose a reason for hiding this comment

severinrudie Oct 28, 2019

Choose a reason for hiding this comment

severinrudie commented Oct 28, 2019

Request for data collection review form

boek Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

severinrudie Nov 8, 2019

Choose a reason for hiding this comment

boek Nov 8, 2019

Choose a reason for hiding this comment

severinrudie commented Nov 11, 2019

severinrudie commented Oct 22, 2019 •

edited

Loading

codecov-io commented Oct 23, 2019 •

edited

Loading

boek Nov 8, 2019 •

edited

Loading