SEARCH_COUNTS is not counting one-off searches

RESOLVED FIXED in Firefox 51

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

({regression})

Trunk
Firefox 53
regression
Points:
2

Firefox Tracking Flags

(firefox50 unaffected, firefox51+ fixed, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [measurement:client][fxsearch])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
This is a regression fro bug 1303333, that was found during the data validation phase n bug 1308705.

It looks like we're not counting one-off searches coming from the searchbar (and the urlbar, but that's Nightly only) anymore.
(Assignee)

Updated

2 years ago
status-firefox52: --- → affected
Priority: -- → P1
Whiteboard: [measurement:client]
Blocks: 1303333
status-firefox50: --- → unaffected
status-firefox51: --- → affected
Keywords: regression
we should uplift a fix up to FF51.
[Tracking Requested - why for this release]:
This is search data & we care about search data.
tracking-firefox51: --- → ?
Does this also mean we don't count any direct search from the searchbar in it, since IIRC they are considered like one-off searches with an unknown type?
Whiteboard: [measurement:client] → [measurement:client][fxsearch]
(Assignee)

Comment 4

2 years ago
(In reply to Marco Bonardo [::mak] from comment #3)
> Does this also mean we don't count any direct search from the searchbar in
> it, since IIRC they are considered like one-off searches with an unknown
> type?

If by "direct search" you mean typing and pressing enter, that's counted.
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
Points: --- → 2
(In reply to Alessio Placitelli [:Dexter] from comment #4)
> If by "direct search" you mean typing and pressing enter, that's counted.

Yes, meant that. That's good, one-off buttons have never been widely used, so we are only missing a tiny amount of data.
(Assignee)

Comment 6

2 years ago
(In reply to Marco Bonardo [::mak] from comment #5)
> (In reply to Alessio Placitelli [:Dexter] from comment #4)
> > If by "direct search" you mean typing and pressing enter, that's counted.
> 
> Yes, meant that. That's good, one-off buttons have never been widely used,
> so we are only missing a tiny amount of data.

Yeah, we're talking about 0.03% of data missing from the main pings.
(Assignee)

Comment 7

2 years ago
Created attachment 8811813 [details] [diff] [review]
bug1318333.patch

This patch re-enables counting one-off searches in SEARCH_COUNTS.

It also adds test coverage for the SEARCH_COUNTS to make sure it matches up with what we expect (and with the search engagement measurements).
Attachment #8811813 - Flags: review?(mak77)
Comment on attachment 8811813 [details] [diff] [review]
bug1318333.patch

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

Grazie!
Attachment #8811813 - Flags: review?(mak77) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8812740 [details] [diff] [review]
bug1318333.patch

This changes the tests to enable extended telemetry, as some tests where failing on try (needed to get the correct engine name in SEARCH_COUNTS consistently).
Attachment #8811813 - Attachment is obsolete: true
Attachment #8812740 - Flags: review+
(Assignee)

Comment 12

2 years ago
Tests look good!
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 13

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6750f9bd4422
Fix SEARCH_COUNTS not counting one-off searches. r=mak
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6750f9bd4422
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Track 51+ for search data.

Hi :Dexter,
Since this bug also affects 51/52, do you think it's worth uplifting to Beta51 and Aurora52 if this patch is not too risky?
tracking-firefox51: ? → +
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 16

2 years ago
(In reply to Gerry Chang [:gchang] from comment #15)
> Track 51+ for search data.
> 
> Hi :Dexter,
> Since this bug also affects 51/52, do you think it's worth uplifting to
> Beta51 and Aurora52 if this patch is not too risky?

Hi Gerry!

Yes, it's definitely worth uplifting. I'm just holding the requests back for a day or two in order to have some data coming in from Nightly so that I double check that it's actually fixing the problem for good (I've also done manual QA, but I'd love to be 100% sure since this is search data).

How much time do I have left to request uplifting for Beta51?
Flags: needinfo?(alessio.placitelli) → needinfo?(gchang)
51 beta 3 GTB is Thursday(11/24) and Beta 4 gtb is 11/28. I can accept either one of these 2 days.
Flags: needinfo?(gchang)
(Assignee)

Comment 18

2 years ago
So, I've double checked this patch by running again the notebook from bug 1303333 on yesterday's Nightly data (31561 pings, 300 of which had one-off searches) and things are looking good:

> ok - Ratio 0.999
> system is not being record by scalars. - Ratio 0.000
> searchbar is not being record by scalars. - Ratio 0.000

There's no mismatch due to one-off searches anymore, hence the one-off searches are being counted again in SEARCH_COUNTS.
(Assignee)

Comment 19

2 years ago
Comment on attachment 8812740 [details] [diff] [review]
bug1318333.patch

Approval Request Comment
[Feature/regressing bug #]: 1303333
[User impact if declined]: None, but search counts will be under counted due to one-off searches not being considered. And this is bad.
[Describe test coverage new/current, TreeHerder]: This patch also introduces a series of consistency checks for SEARCH_COUNTS in the tests, which were missing before. So that we don't ever regress again.
[Risks and why]: Low risk, this patch as gone under manual QA and the investigation on one day worth of Nightly data suggested that the fix is indeed working.
[String/UUID change made/needed]: None
Attachment #8812740 - Flags: approval-mozilla-beta?
Attachment #8812740 - Flags: approval-mozilla-aurora?
Comment on attachment 8812740 [details] [diff] [review]
bug1318333.patch

Fix a regression related to search. Beta51+ and Aurora 52+. Should be in 51 beta 3.
Attachment #8812740 - Flags: approval-mozilla-beta?
Attachment #8812740 - Flags: approval-mozilla-beta+
Attachment #8812740 - Flags: approval-mozilla-aurora?
Attachment #8812740 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/233a0890b3b35d778b2a50f1d7b064a8bcb82fb1

but has problems to apply to beta

merging browser/modules/BrowserUsageTelemetry.jsm
merging browser/modules/test/browser_UsageTelemetry_content.js
merging browser/modules/test/browser_UsageTelemetry_searchbar.js
merging browser/modules/test/browser_UsageTelemetry_urlbar.js
merging browser/modules/test/head.js
warning: conflicts while merging browser/modules/test/browser_UsageTelemetry_content.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
status-firefox52: affected → fixed
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 22

2 years ago
Created attachment 8814130 [details] [diff] [review]
[BETA ONLY] Rebased patch for beta.
Flags: needinfo?(alessio.placitelli)
You need to log in before you can comment on or make changes to this bug.