Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Searches initiated by clicking the action row in the locationbar don't increment the "search" / "urlbar" telemetry countable

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Firefox
Location Bar
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: adw)

Tracking

unspecified
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43 wontfix, firefox44+ fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

(Whiteboard: [unifiedcomplete][fxsearch][suggestions])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
I noticed this while working on bug 1219505.

when a search is initiated by clicking on the Search With: entry it should increment the "search" / "urlbar" UITelemetry probe. Only typing+enter increments the countable now, probably through the "keyword-search" observer in nsBrowserGlue.

We must pay attention to avoid double counting the same search in both keyword-search and _handleURLBarTelemetry.

That will give us more reliable data for the search volume from the location bar.
(Assignee)

Updated

2 years ago
Assignee: nobody → adw
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
Note however that we do correctly add to the FX_URLBAR_SELECTED_RESULT_INDEX probe (because _handleURLBarTelemetry is called), so we're not totally losing this info.
(Assignee)

Comment 2

2 years ago
Created attachment 8693780 [details] [diff] [review]
patch without test

This seems to work.  As we talked about on IRC, it records all selected searchengine results, not only the "Search with:" heuristic result.  I was able to remove a couple of duplicated chunks too.  I'll work on a test next.
Attachment #8693780 - Flags: feedback?(mak77)
(Reporter)

Comment 3

2 years ago
This patch also fixes bug 1226192, right?
(Reporter)

Comment 4

2 years ago
Comment on attachment 8693780 [details] [diff] [review]
patch without test

Review of attachment 8693780 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks ok and it seems to be working fine (also for search suggestions)

the only thing we are not counting yet is keywords/aliases, but I think that's fine, they are rarely used and unlikely to change numbers in any significant way.
Attachment #8693780 - Flags: feedback?(mak77) → feedback+
(Reporter)

Updated

2 years ago
Blocks: 1226192
(Assignee)

Comment 5

2 years ago
Comment on attachment 8693780 [details] [diff] [review]
patch without test

I'll change this to r? because I haven't changed it and I'll post a second patch for the test.
Attachment #8693780 - Flags: review?(mak77)
(Assignee)

Comment 6

2 years ago
Created attachment 8694457 [details] [diff] [review]
test patch

This test has a couple of add_task's (other than the first prepare one).  The first one tests the heuristic result, the second tests a search suggestion result.  This patch also factors out getNumberOfSearches FHR helper from the about:home test to head.js so that the new test can use it.
Attachment #8694457 - Flags: review?(mak77)
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=059dcd721ab9
(Assignee)

Comment 8

2 years ago
(In reply to Marco Bonardo [::mak] from comment #3)
> This patch also fixes bug 1226192, right?

Yeah.
(Reporter)

Comment 9

2 years ago
Comment on attachment 8693780 [details] [diff] [review]
patch without test

Review of attachment 8693780 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks ok and it seems to be working fine (also for search suggestions)

the only thing we are not counting yet is keywords/aliases, but I think that's fine, they are rarely used and unlikely to change numbers in any significant way.
Attachment #8693780 - Flags: review?(mak77)
Attachment #8693780 - Flags: review+
Attachment #8693780 - Flags: feedback+
(Reporter)

Comment 10

2 years ago
Comment on attachment 8694457 [details] [diff] [review]
test patch

Review of attachment 8694457 [details] [diff] [review]:
-----------------------------------------------------------------

there's something wrong in the Try run, I don't see test runs.

::: browser/base/content/test/general/browser_urlbarSearchTelemetry.js
@@ +10,5 @@
> +  Services.prefs.setBoolPref(SUGGEST_URLBAR_PREF, true);
> +  let engine = yield promiseNewSearchEngine(TEST_ENGINE_BASENAME);
> +  let oldCurrentEngine = Services.search.currentEngine;
> +  Services.search.currentEngine = engine;
> +  registerCleanupFunction(function () {

function*

@@ +199,5 @@
> + */
> +function promiseFirstSuggestionIndex() {
> +  return new Promise(resolve => {
> +    let index = -1;
> +    waitForCondition(() => {

out of curiosity, do we really need to poll (both here and for the action)?
promiseAutocompleteResultPopup is waiting for searchComplete, that should wait for suggestions to be added, iirc.
I'd be interested in understanding if that's not the case, cause I think we have some intermittent failures around that look strange.
Attachment #8694457 - Flags: review?(mak77) → review+
(Assignee)

Comment 11

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=480258d44f39

Here's another try run with the same try syntax.  (Don't know why the previous one didn't run any tests.)  Unlike the previous run it doesn't poll.  I copied this test from browser_urlbarSearchSuggestions.js, and I added polling to that test because there were intermittent failures.  I can't reproduce failures with the new test locally, but I'll see if I can on try.
(Assignee)

Comment 12

2 years ago
A couple of more that specifically choose mochitest-bc instead of relying on `-u all` in try syntax, in case that suddenly matters.

no polling: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f1f82bf301f
   polling: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af810f77863b
(Reporter)

Comment 13

2 years ago
[Tracking Requested - why for this release]: We need this telemetry to work properly from Firefox 44 on to be able to run an experiment on it.
status-firefox42: --- → wontfix
status-firefox43: --- → wontfix
status-firefox44: --- → affected
tracking-firefox44: --- → ?

Comment 14

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/a78f8444400c
(Assignee)

Comment 15

2 years ago
The "no polling" try in comment 12 looks OK so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f1f82bf301f

So I landed with no polling.  If it turns out to intermittently fail anyway, we can just land the polling part then.
https://hg.mozilla.org/mozilla-central/rev/a78f8444400c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(Reporter)

Comment 17

2 years ago
Comment on attachment 8693780 [details] [diff] [review]
patch without test

Approval Request Comment
[Feature/regressing bug #]: never worked properly
[User impact if declined]: Searches from the locationbar are miscounted in telemetry. The most important reason we need this is we plan to start an experiment in January and we need proper counting (see bug 1219505, intent-to-ship is incoming)
[Describe test coverage new/current, TreeHerder]: automated test
[Risks and why]: low risk, just storing data in Telemetry reusing existing code
[String/UUID change made/needed]: none
Attachment #8693780 - Flags: approval-mozilla-aurora?

Updated

2 years ago
tracking-firefox44: ? → +
Comment on attachment 8693780 [details] [diff] [review]
patch without test

Making telemetry data more reliable and accurate, Aurora44+
(Assignee)

Comment 19

2 years ago
Ritu, did you mean to + the a? flag on that patch?
Flags: needinfo?(rkothari)
Comment on attachment 8693780 [details] [diff] [review]
patch without test

Missed the "+", Duh!
Flags: needinfo?(rkothari)
Attachment #8693780 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This failed to apply:

renamed 1226629 -> searchengine-telemetry-1226629
applying searchengine-telemetry-1226629
patching file browser/base/content/urlbarBindings.xml
Hunk #1 FAILED at 355
1 out of 3 hunks FAILED -- saving rejects to file browser/base/content/urlbarBindings.xml.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh searchengine-telemetry-1226629

coud you take a look, thanks!
Flags: needinfo?(adw)
(Assignee)

Comment 22

2 years ago
Created attachment 8697061 [details] [diff] [review]
--Aurora-- Beta/44 patch with test

Approval Request Comment

Please see comment 17 and 21 -- the m-c patch (without tests) was a+'ed but didn't apply cleanly to Aurora.

This patch applies cleanly and unlike comment 17 also includes the test patch, which should land together with the code changes.
Flags: needinfo?(adw)
Attachment #8697061 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

2 years ago
Comment on attachment 8697061 [details] [diff] [review]
--Aurora-- Beta/44 patch with test

Approval Request Comment

Please see comment 22.
Attachment #8697061 - Attachment description: Aurora/44 patch with test → --Aurora-- Beta/44 patch with test
Attachment #8697061 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8697061 [details] [diff] [review]
--Aurora-- Beta/44 patch with test

This patch has been on Nightly for 2 weeks already, seems safe to uplift to Beta44.
Attachment #8697061 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 25

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/acb2f0d2d7a0
status-firefox44: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/acb2f0d2d7a0
status-b2g-v2.5: --- → fixed

Updated

2 years ago
Depends on: 1239015
You need to log in before you can comment on or make changes to this bug.