1475571 - Replace follow-on add-on with something in Firefox

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[Mike Kaply [:mkaply]]

We're removing the follow-on add-on and replacing it with code in Firefox telemetry.

It will be much easier to manager there and also avoid the overhead of framescripts/content sripts.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Bug 1475571 - Replace follow-on addon with in tree telemetry.

Comment 2

[Phabricator Automation]

Comment on attachment 8991961 [details]
Bug 1475571 - Replace follow-on addon with in tree telemetry.

Mike de Boer [:mikedeboer] has approved the revision.

https://phabricator.services.mozilla.com/D2125

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Data Collection Reqiest

Marshall has requested that we do another data review for this information.

Comment 4

[François Marier [:francois]]

Comment on attachment 8996857 [details]
Data Collection Reqiest

> 5) List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox [data collection categories](https://wiki.mozilla.org/Firefox/Data_Collection) on the Mozilla wiki.   
> 
> Proposed measurement is an additional to our sap category in telemetry that identifies follow-on searches as a count only.

Missing word: an additional key?

What does "sap" mean? Is that an acronym?

I assume that this is modifying what is being sent via the SEARCH_COUNTS histogram: https://searchfox.org/mozilla-central/rev/d0a0ae30d534dc55e712bd3eedc1b4553ba56323/toolkit/components/telemetry/Histograms.json#7793-7800

As far as I can see from the patch, the histogram keys are fixed and indicate what kind of Google search it is. Could you please update the probe description in Histograms.json so that it describes what's in the key? I will then point to Histograms.json as the place where this is documented.

Comment 5

[Mike Kaply [:mkaply]]

Attachment: Data Collection Request

Greatly clarified data description. I will update Histograms.json as part of my patch. Working on that now.

Comment 6

[François Marier [:francois]]

Comment on attachment 8996865 [details]
Data Collection Request

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Histograms.json.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, Ryan Harter.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 2 (Interaction data).

5) Is the data collection request for default-on or default-off?

Default ON.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, permanent.

Comment 7

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]: Needed for getting data on Google searches.

Comment 8

[Phabricator Automation]

Comment on attachment 8991961 [details]
Bug 1475571 - Replace follow-on addon with in tree telemetry.

Florian Quèze [:florian] has approved the revision.

https://phabricator.services.mozilla.com/D2125

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/502893089232
Replace follow-on addon with in tree telemetry. r=mikedeboer,florian

Comment 10

[Tiberius Oros[:tiberius_oros]]

Backed out changeset 502893089232 (bug 1475571) for browser_preferences_usage.js

Backout:
https://hg.mozilla.org/integration/autoland/rev/46cf1090208659eb072c8f7b33cf783171793a1e

Push with the failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=502893089232b37d57c44e2a3c045dca80fff266

Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=192391343&repo=autoland&lineNumber=1994

Comment 11

[Mike Kaply [:mkaply]]

Florian:

I'm really confused by this.

browser.search.log is queried constantly in a debug build for logging every search message. Why did we put a limit on how many times it could be called?

https://bugzilla.mozilla.org/show_bug.cgi?id=1425613

It appears that because search startup is happening earlier, we're getting all the absearch messages on load. If we're going to have a debug pref for logging, it's going to get called a lot.

As far as this goes:

Whitelist item extensions.getAddons.cache.enabled should be accessed at least 8 times. - 8 <= 7

I guess because I removed the followon search addon, I need to change this number? Why do we have such a sensitive number in our tests?

Mike

Comment 12

[Mike Kaply [:mkaply]]

On top of that, browser.search.log is debug only, so why does it matter?

Comment 13

[Mike Kaply [:mkaply]]

So the problem is that the function _buildParseSubmissionMap:

https://searchfox.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#4192

Puts a ton out to the console plus the new code adds two additional logs on each page load (for the ParseSubmissionResult).

Comment 14

[Mike Kaply [:mkaply]]

Attachment: Patch that removes logs

This would be the patch to remove this.

Removing all of these LOGs.

I think they are unnecessary anyway.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2db828f877c8
Replace follow-on addon with in tree telemetry. r=mikedeboer,florian

Comment 16

[Johann Hofmann [:johannh]]

FYI browser_preferences_usage.js can only run in debug builds because it records the number of times preferences have been set, which of course we couldn't do in opt, to avoid regressing performance. There's an option to whitelist your preference in the test or just cache the preference (which you should do in any case, even just for debug usage).

Comment 17

[Mike Kaply [:mkaply]]

(In reply to Johann Hofmann [:johannh] - slow to respond, digging out of PTO backlog from comment #16)
> FYI browser_preferences_usage.js can only run in debug builds because it
> records the number of times preferences have been set, which of course we
> couldn't do in opt, to avoid regressing performance. There's an option to
> whitelist your preference in the test or just cache the preference (which
> you should do in any case, even just for debug usage).

You're right. We should switch to using the lazy preference getter. I'll open a bug.

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8998078 [details] [diff] [review]
Patch that removes logs

The best solution would be a lazy pref getter.

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/2db828f877c8

Comment 20

[Mike Kaply [:mkaply]]

Attachment: Release patch

HG export against release.

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 8991961 [details]
Bug 1475571 - Replace follow-on addon with in tree telemetry.

Approval Request Comment
[Feature/Bug causing the regression]: Add counting for organic searches
[User impact if declined]: None
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: Yes
Go to www.google.com. Execute a search.
Go to about:telemetry. Search on SEARCH_COUNT
Verify an entry that says "google.in-content:organic:none.
Do a search via the URL bar.
Go to about:telemetry. Search on SEARCH_COUNT
Verify an entry that says "google.in-content:sap:firefox-b-ab (or firefox-b-1-ab).
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Low
[Why is the change risky/not risky?]: Not user facing.
[String changes made/needed]:

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8991961 [details]
Bug 1475571 - Replace follow-on addon with in tree telemetry.

Adds additional Telemetry around search submissions. Approved for 62.0b16 and 61.0.2.

Comment 23

[Mike Kaply [:mkaply]]

I'm working on a beta patch now.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/975058795980

Comment 25

[Mike Kaply [:mkaply]]

Beta is going to be in an hour or so. DXR is out of sync and lost me some time. I'll have it as soon as I can.

Comment 26

[Mike Kaply [:mkaply]]

Attachment: Beta patch

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/053a2a490c33

Comment 29

[Kris Maglione [:kmag]]

This caused a 65KB improvement in base content JS memory usage! \o/

Comment 31

[Ryan Harter [:harter]]

We were asked to announce the organic search telemetry in a blog post before starting data collection. We have not announced this telemetry yet. Adding a ni? for Marshal

Comment 32

[Anca Soncutean, Desktop QA]

The entries mentioned in comment 21 (google.in-content.organic:none and google.in-content.sap:firefox-b-ab) are successfully display in about:telemetry on Firefox 62.0b16 (20180808015758) and Firefox 61.0.2 (20180807170231) under Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x32. Note thought, that after the “content“ word is [.] instead of [:]  as per comment 21. From what I see from the patch this should be the correct way. 
Mike can you please confirm if this punctuation marks are the expected ones? Thank you!

Comment 33

[Merwin]

That's right.  We had discussed communicating about the organic search measurement before it shipped.

Comment 34

[Mike Kaply [:mkaply]]

> Mike can you please confirm if this punctuation marks are the expected ones? Thank you!

Yes, that is correct per the code. I'll open a separate patch to either make the Histogram match or the code match.

We're getting the correct data, so we should be fine.

Thank you for testing this.

Comment 35

[Ryan Harter [:harter]]

(In reply to Mike Kaply [:mkaply] from comment #34)

> Yes, that is correct per the code. I'll open a separate patch to either make
> the Histogram match or the code match.

We'll want the patch to use colons instead of dots. I.e. <provider>.in-content:[sap|sap-follow-on|organic]:[code|none]. This will provide a consistent delimiter for the `source` portion of the key. main_summary also relies on there being exactly one dot in the key [1]

[1] https://github.com/mozilla/telemetry-batch-view/blob/master/src/main/scala/com/mozilla/telemetry/utils/MainPing.scala#L161

Comment 36

[Mike Kaply [:mkaply]]

> We'll want the patch to use colons instead of dots.

Fixing this in:

https://bugzilla.mozilla.org/show_bug.cgi?id=1481869

Comment 37

[Andrew Swan [:aswan]]

This just unlinked browser/extensions/followonsearch/ from the build but left all the old extension code there.  Is it still referenced somewhere or can it be removed now?

Comment 38

[Mike Kaply [:mkaply]]

aswan:

yep - bug 1481442

Comment 39

[Anca Soncutean, Desktop QA]

Based on comments 32 and 34 (the entries are displayed and no data is lost), I will mark this issue as verified fixed.

I will keep an eye on bug 1481869 and retest this after a fix is landed.

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Ryan, Mike, just noting that this change is in 61.0.2, but with the typo.

Comment 41

[Mike Kaply [:mkaply]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #40)
> Ryan, Mike, just noting that this change is in 61.0.2, but with the typo.

Thanks. Everyone is good with that.

Comment 42

[Anca Soncutean, Desktop QA]

Since target was set on 63, I will mark this issue verified also on Fx 63. The desired  entries are successfully display in about:telemetry on Beta 63.0b6 (20180913141435) across all platforms, with the correction made in bug 1481869.

Comment 43

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

Attachment: Bug 1475571 - Update SEARCH_COUNT bug_numbers r?mkaply

Comment 44

[Tim Smith (inactive) 👨‍🔬 [:tdsmith]]

The blog post Harter and merwin discussed is https://blog.mozilla.org/data/2018/08/20/effectively-measuring-search-in-firefox/.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1fa85da4127c
Update SEARCH_COUNT bug_numbers r=mkaply

Comment 46

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1fa85da4127c