1662553 - Search telemetry needs thinking about/rewriting (SEARCH_COUNTS/navigation.search.*)

Status: VERIFIED FIXED

(Firefox :: Search, task, P2)

Description

[Mark Banner (:standard8)]

There's currently various architectural issues with the search telemetry that we currently have. The biggest is that it is complex, and hard to understand exactly what is going on.

For example, if you click a one-off button in the URLBar, then you head into both of these calls:

https://searchfox.org/mozilla-central/rev/d54712b9644b49cec6cc90a9e0c325fdfab04e7c/browser/components/urlbar/UrlbarInput.jsm#1876,1889

They both end up in BrowserUsageTelemetry.recordSearch which takes different paths for each call. One ends up logging <engine>.urlbar in SEARCH_COUNTS and nothing more. The other path logs into navigation.search and browser.engagement.navigation via a confusing route.

The search bar is similar, and akes the two confusing paths again:

https://searchfox.org/mozilla-central/rev/d54712b9644b49cec6cc90a9e0c325fdfab04e7c/browser/components/search/content/searchbar.js#349,426

This all seems scary to touch, as there's lots of possible different routes, and no clear documentation on exactly what we're recording where (see also bug 1198848). The test suite is fairly extensive, so there is that, though it would probably be nicer if it was more structured towards listing up front the outcomes for each test (so you can check the matrix), than on a test-by-test basis.

This seems to have grown up organically, and I think it is time we reassessed it before we need to make any big changes to it.

In addition, I noticed the following:

• BrowserSearch.recordSearchInTelemetry is basically a forwarding function with a try/catch, we should just look at handling that in the appropriate functions and passing gBrowser where necessary.
• BrowserSearch.recordOneoffSearchInTelemetry is also basically a forwarding function, but adds the isOneOff flag - I think that could just be simplified to call direct.
• We could probably move the search specific functions from BrowserUsageTelemetry to something that's search and/or urlbar specific.

Comment 1

[Marco Bonardo [:mak]]

I must note that there is some documentation here: https://firefox-source-docs.mozilla.org/browser/urlbar/telemetry.html

It would probably be nice to keep search and address bar telemetry docs together.

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1662553 - Remove unused detail.type property for BrowserSearchTelemetry.recordSearch calls. r?mak!

Comment 3

[Mark Banner (:standard8)]

Attachment: Bug 1662553 - Remove unnecessary call to recordSearch in searchbar.js as nothing gets recorded in telemetry. r?mak!

'header' and 'paste' sources are unknown, so they don't get logged for search counts, and they just get skipped.
Additionally, the target check for 'paste' is wrong so that would not have been recorded in recent releases anyway.

Depends on D100050

Comment 4

[Mark Banner (:standard8)]

Attachment: Bug 1662553 - Reorder BrowserSearchTelemetry so the functions are in a slightly better order. r?mak!

Depends on D100051

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1662553 - Simplify recording search telemetry. r?mak!

This simplifies recording of the search telemetry by dropping the seperate logging of one-off button clicks, and combining the flows in recordSearch into one.

With the previous simplifications the calls in search bar and url bar, the remaining parts were two flows through recordSearch for the one-off buttons, and one for everything else. The two flows for one-off buttons each stopped at various times or skipped parts to continue to the rest.

This means we can combine those into one, have the flow governed by details.isOneOff, and change maybeRecordTelemetry into a function that simply determines if a one-off button was selected.

Depends on D100052

Comment 6

[Mark Banner (:standard8)]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e6a7d97695f4088309e43ff8f5751d6a86010f6

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/430b57e80c40
Remove unused detail.type property for BrowserSearchTelemetry.recordSearch calls. r=mak
https://hg.mozilla.org/integration/autoland/rev/0975dbb1f999
Remove unnecessary call to recordSearch in searchbar.js as nothing gets recorded in telemetry. r=mak
https://hg.mozilla.org/integration/autoland/rev/3fb447036750
Reorder BrowserSearchTelemetry so the functions are in a slightly better order. r=mak
https://hg.mozilla.org/integration/autoland/rev/5cc716760000
Simplify recording search telemetry. r=mak

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/430b57e80c40
https://hg.mozilla.org/mozilla-central/rev/0975dbb1f999
https://hg.mozilla.org/mozilla-central/rev/3fb447036750
https://hg.mozilla.org/mozilla-central/rev/5cc716760000

Comment 9

[Mark Banner (:standard8)]

Requesting qe-verify on this bug. What to test:

• Standard per-release search telemetry tests.
• Ensure the following search areas have been covered: address bar, search bar, context menu, about:home, webextension, command line.

In all cases, before & after should be the same.

Comment 10

[Petruta Horea [:phorea]]

We performed a telemetry full run across OS'es in order to verify this on Nightly 86.0a1. No new issues were found.