1504686 - In content probe is double counting DuckDuckGo

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Mike Kaply [:mkaply]]

DuckDuck go does a redirect on the search so we are double counting.

Need to either get DuckDuckGo to fix or update our engine to include the param they are adding.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Bug 1504686 - Don't count search for same document changes.

Comment 2

[Mike Kaply [:mkaply]]

BrowserUsageTelemetry is counting all URLs including same document.

We don't want that, so we have to explicitly exclude it.

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Bug 1504686 - Don't count search for same document changes.

Comment 4

[Mike Kaply [:mkaply]]

adw:

Any ideas on how to write a test for this?

You did some pretty clever tests for the private browsing stuff.

Comment 5

[Drew Willcoxon :adw]

I would probably start by trying to write a small web page that mimics whatever DDG does to trigger this bug, and then the test would just load it and make sure we didn't double count.  Or if it's a server-level thing then you could write an sjs script and load that instead.

DDG is getting captured here due to the SEARCH_PROVIDER_INFO stuff that I recently modified, right?  So you could use the same observe() technique to add your test URL so that it's captured during the test.

I'd also run the test without the fix to make sure it does trigger the double count of course.

Comment 6

[Mike Kaply [:mkaply]]

I'm going to take this as is and file a followup for the test. We can't risk not having this with the other changes.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b39b4f4ba16b
Don't count search for same document changes. r=Standard8

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b39b4f4ba16b

Comment 9

[Mike Kaply [:mkaply]]

To test this, do a duckduckgo search via one off searches.

Go to about:Telemetry, search on SEARCH_COUNTS

notice that there were two recorded searches before this patch.

Comment 10

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on the affected Nightly 65.0a1 (2018-11-05) on Windows 7/10, Mac OS 10.13 and Ubuntu 16.04.

The issue can no longer be reproduced on the latest Nightly 65.0a1 (2018-11-15) as per Comment 9 now there is only 1 recorded search in about:telemetry, thus closing it as Verified - Fixed on all the above mentioned OS.

Comment 11

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]: We need this change in 64, but I think there was an issue with a checkin from Drew that needs to be resolved first.

Comment 12

[Julien Cristau [:jcristau] (back August 18)]

OK, please request uplift when you get a chance.  AFAICT this grafts cleanly on beta.  (I guess you're talking about 1499193?)

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 9023791 [details]
Bug 1504686 - Don't count search for same document changes.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1482158

User impact if declined: Incorrect telemetry on DDG

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: Steps in bug.

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just adds an extra check to the search record.

String changes made/needed:

Comment 14

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 9023791 [details]
Bug 1504686 - Don't count search for same document changes.

search telemetry fix, approved for 64.0b11

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ceba8351e044

Comment 16

[Timea Cernea [:tbabos][inactive]]

Verified - fixed on latest Firefox 64.0b11 (Build ID: 20181119162153) on Windows 7/10, Mac OS 10.14 and Ubuntu 16.04.
Marking it as verified for fx 64 and removing qe+ flag.