Closed Bug 1475571 Opened 2 years ago Closed 2 years ago
Replace follow-on add-on with something in Firefox
46 bytes, text/x-phabricator-request
|Details | Review|
1.93 KB, text/plain
7.57 KB, patch
|Details | Diff | Splinter Review|
16.48 KB, patch
|Details | Diff | Splinter Review|
15.73 KB, patch
|Details | Diff | Splinter Review|
47 bytes, text/x-phabricator-request
|Details | Review|
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
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
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
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/502893089232 Replace follow-on addon with in tree telemetry. r=mikedeboer,florian
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
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.
Pushed by firstname.lastname@example.org: 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.
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]:
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.
I'm working on a beta patch now.
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.
This caused a 65KB improvement in base content JS memory usage! \o/
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
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!
That's right. We had discussed communicating about the organic search measurement before it shipped.
> 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.
(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   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?
aswan: yep - bug 1481442
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.
Ryan, Mike, just noting that this change is in 61.0.2, but with the typo.
(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.
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 email@example.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.