Closed Bug 1475571 Opened 2 years ago Closed 2 years ago

Replace follow-on add-on with something in Firefox

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox61 + verified
firefox62 + verified
firefox63 + verified

People

(Reporter: mkaply, Assigned: mkaply)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:65k])

Attachments

(6 files, 1 obsolete file)

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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Priority: -- → P1
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
Attachment #8991961 - Flags: review+
Blocks: 1449052
Attached file Data Collection Reqiest (obsolete) —
Marshall has requested that we do another data review for this information.
Attachment #8996857 - Flags: review?(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.
Attachment #8996857 - Flags: review?(francois) → review-
Greatly clarified data description. I will update Histograms.json as part of my patch. Working on that now.
Attachment #8996865 - Flags: review?(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.
Attachment #8996865 - Flags: review?(francois) → review+
Attachment #8996857 - Attachment is obsolete: true
[Tracking Requested - why for this release]: Needed for getting data on Google searches.
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
Attachment #8991961 - Flags: review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/502893089232
Replace follow-on addon with in tree telemetry. r=mikedeboer,florian
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
Flags: needinfo?(mozilla) → needinfo?(florian)
On top of that, browser.search.log is debug only, so why does it matter?
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).
This would be the patch to remove this.

Removing all of these LOGs.

I think they are unnecessary anyway.
Attachment #8998078 - Flags: feedback?(florian)
Attachment #8998078 - Attachment is patch: true
Attachment #8998078 - Attachment mime type: text/x-log → text/plain
Flags: needinfo?(florian)
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/2db828f877c8
Replace follow-on addon with in tree telemetry. r=mikedeboer,florian
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).
(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 on attachment 8998078 [details] [diff] [review]
Patch that removes logs

The best solution would be a lazy pref getter.
Attachment #8998078 - Flags: feedback?(florian)
https://hg.mozilla.org/mozilla-central/rev/2db828f877c8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Attached patch Release patchSplinter Review
HG export against release.
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]:
Attachment #8991961 - Flags: approval-mozilla-release?
Attachment #8991961 - Flags: approval-mozilla-beta?
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.
Attachment #8991961 - Flags: approval-mozilla-release?
Attachment #8991961 - Flags: approval-mozilla-release+
Attachment #8991961 - Flags: approval-mozilla-beta?
Attachment #8991961 - Flags: approval-mozilla-beta+
I'm working on a beta patch now.
Flags: qe-verify+
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.
Attached patch Beta patchSplinter Review
Duplicate of this bug: 1473621
This caused a 65KB improvement in base content JS memory usage! \o/
Whiteboard: [overhead:65k]
Duplicate of this bug: 1473622
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
Flags: needinfo?(merwin)
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!
Flags: needinfo?(mozilla)
That's right.  We had discussed communicating about the organic search measurement before it shipped.
Flags: needinfo?(merwin)
> 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.
Flags: needinfo?(mozilla)
(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
> We'll want the patch to use colons instead of dots.

Fixing this in:

https://bugzilla.mozilla.org/show_bug.cgi?id=1481869
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?
Flags: needinfo?(mozilla)
See Also: → 1482158
aswan:

yep - bug 1481442
Flags: needinfo?(mozilla)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Ryan, Mike, just noting that this change is in 61.0.2, but with the typo.
Flags: needinfo?(rharter)
Flags: needinfo?(mozilla)
(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.
Flags: needinfo?(rharter)
Flags: needinfo?(mozilla)
See Also: → 1483422
Attachment #8996865 - Attachment mime type: text/markdown → text/plain
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.
Pushed by tismith@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1fa85da4127c
Update SEARCH_COUNT bug_numbers r=mkaply
You need to log in before you can comment on or make changes to this bug.