Update SEARCH_COUNTS telemetry for private browsing mode
Categories
(Firefox :: Search, defect, P1)
Tracking
()
People
(Reporter: adw, Assigned: adw)
References
Details
Attachments
(3 files, 1 obsolete file)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
12.87 KB,
patch
|
mkaply
:
review+
pascalc
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
17.01 KB,
patch
|
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Add a check here: https://searchfox.org/mozilla-central/rev/50ba1dd30cf013bddce1ae756f1b3c95b26f0628/browser/modules/BrowserUsageTelemetry.jsm#203 to not do the count when in private browsing mode or when a pref is set, similar to here: https://searchfox.org/mozilla-central/rev/50ba1dd30cf013bddce1ae756f1b3c95b26f0628/browser/modules/BrowserUsageTelemetry.jsm#160
Assignee | ||
Comment 1•6 years ago
|
||
I think this is what we want? I'm not totally sure. Let me know if the pref name is bad or the logic is wrong, etc. * If a search is performed in a private window and the new pref `browser.engagement.search_counts.pbm` is true, then do not record `SEARCH_COUNTS` telemetry. Note that the the pref must be true. If it's false or doesn't exist, then we record telemetry even in pbm like we normally do currently. (We record `SEARCH_COUNTS` telemetry in two places: (1) In BrowserUsageTelemetry.jsm, and (2) "in-content" telemetry directly in the search service. So skip both of those places.) * Also skip the other ancillary telemetry recorded by `BrowserUsageTelemetry._recordSearch`: a keyed scalar and a telemetry event. * I made some modifications to the search service to let me test the "in-content" telemetry keys. I exposed `wrappedJSObject` on the service and consolidated the search provider info and their regexps.
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa311c8939c608811841d8c86ba81de13b2c92f8
Assignee | ||
Comment 3•6 years ago
|
||
Peter, could you please verify that this patch does what we want? In summary, it changes current telemetry so that we do not record three search-related telemetry probes for searches that happen in private windows *and* when the browser.engagement.search_counts.pbm bool pref is true. If a search happens in a non-private window, or if browser.engagement.search_counts.pbm does not exist or is false, then there's no change from current telemetry. The three probes are: (1) The SEARCH_COUNTS keyed histogram (never expires) (2) The keyed scalar `browser.engagement.navigation.${source}` (never expires) (3) The navigation search telemetry event (expires in 65)
Comment 4•6 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #3) > Peter, could you please verify that this patch does what we want? > > In summary, it changes current telemetry so that we do not record three > search-related telemetry probes for searches that happen in private windows > *and* when the browser.engagement.search_counts.pbm bool pref is true. If a > search happens in a non-private window, or if > browser.engagement.search_counts.pbm does not exist or is false, then > there's no change from current telemetry. > > The three probes are: > > (1) The SEARCH_COUNTS keyed histogram (never expires) > (2) The keyed scalar `browser.engagement.navigation.${source}` (never > expires) > (3) The navigation search telemetry event (expires in 65) This is exactly right. Just NIing Mconnor to make sure he's okay with this approach as Search owner.
Comment 5•6 years ago
|
||
Works for me. I assume "navigation search" is 'organic' search? I really want to see that delta to PB.
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Mike Connor [:mconnor] from comment #5) > Works for me. I assume "navigation search" is 'organic' search? I really > want to see that delta to PB. The navigation search that I mentioned here: (In reply to Drew Willcoxon :adw from comment #3) > (3) The navigation search telemetry event (expires in 65) is a telemetry event that's recorded here: https://hg.mozilla.org/mozilla-central/annotate/c291143e24019097d087f9307e59b49facaf90cb/browser/modules/BrowserUsageTelemetry.jsm#l451 and defined here: https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/toolkit/components/telemetry/Events.yaml#148 So I think that's probably not what you mean by "organic"? You might mean the "organic" type substring that's added to the SEARCH_COUNTS keyed histogram? https://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#4575 e.g. we add keys to that histogram that look like: "provider.in-content:organic:code" If so, that's included in the "(1) The SEARCH_COUNTS keyed histogram" I mentioned. So yes, we will be able to see the delta. Just want to confirm with you before landing this.
Comment 7•6 years ago
|
||
When he says organic, he means my new code in the in-content search probe. So yes, this affects all of our search recording.
Pushed by dwillcoxon@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6beec7d503af Update SEARCH_COUNTS telemetry for private browsing mode r=mkaply
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6beec7d503af
Assignee | ||
Comment 11•6 years ago
|
||
[Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Not a regression, and not related to any other bug/feature User impact if declined: We want to uplift this patch ASAP in order to perform a study to better understand how users are using search in private browsing mode (in a way that still respects privacy) Is this code covered by automated tests?: Yes Has the fix been verified in Nightly?: No Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: Bug 1499193 (this patch is based on the one in that bug but is not strictly related) Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): All the patch does is skip telemetry while in private browsing mode and a pref is set. It has automated tests, and it's relatively early in the cycle String changes made/needed: None
Assignee | ||
Comment 12•6 years ago
|
||
Try push on release: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c75d220a235afe00c493b72b42a5dfe1f837d51 If this is OK, I'll ask for review on my release patch (which is different from the m-c and beta patch) and request uplift to release.
Assignee | ||
Comment 13•6 years ago
|
||
This could use a re-review because there are non-trivial changes between this patch for release/63 and the patch for beta/64 and nightly/65. The in-content telemetry is only for google on 63 (unlike 64/65), so this patch is different from the 64/65 patch because of that. Also, nsSearchService.recordSearchURLTelemetry() doesn't exist yet and instead the in-content telemetry is collected directly in BrowserUsageTelemetry.jsm. I'll request uplift to release once this is r+'ed. There's no guarantee release drivers will accept it though.
Comment 14•6 years ago
|
||
Comment on attachment 9024082 [details] [diff] [review] Beta/64 patch changes to search telemetry recording, approved for 64.0b9
Comment 15•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a106808198a6
Comment 16•6 years ago
|
||
Backed out from beta for failing bc at browser/modules/test/browser/browser_UsageTelemetry_urlbar.js Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=211229706&revision=a4175191b29b274d59ed117567ec03df9d6b8cf5 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=211229706&repo=mozilla-beta&lineNumber=7810 Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/6226ce7f4038d566fe750b1d13f107146d137463
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 17•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bf233fdfc2c9ee651167f06ab800a2c058452a6
Assignee | ||
Comment 18•6 years ago
|
||
This should fix the test failure on beta. Try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bf233fdfc2c9ee651167f06ab800a2c058452a6
Updated•6 years ago
|
Comment 19•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5336b29e9db0
Comment 20•6 years ago
|
||
bugherder uplift |
This changeset was backed out: https://hg.mozilla.org/releases/mozilla-beta/rev/c1babc4f5dfc82eaf9edf4377c87d029dd6564c1 and then relanded: https://hg.mozilla.org/releases/mozilla-beta/rev/f3c21b2ef242
Assignee | ||
Comment 21•6 years ago
|
||
Comment on attachment 9024137 [details] [diff] [review] Release/63 patch [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Not a regression, and not related to any other bug/feature User impact if declined: We want to uplift this patch ASAP in order to perform a study to better understand how users are using search in private browsing mode (in a way that still respects privacy) and how it impacts related business decisions Is this code covered by automated tests?: Yes Has the fix been verified in Nightly?: No Needs manual test from QE?: No If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): All the patch does is skip telemetry while in private browsing mode and a pref is set. It has automated tests. String changes made/needed: None
Assignee | ||
Comment 22•6 years ago
|
||
A try push to release is in comment 12
Assignee | ||
Updated•6 years ago
|
Comment 23•6 years ago
|
||
(In reply to Drew Willcoxon :adw from comment #21) > Comment on attachment 9024137 [details] [diff] [review] > Release/63 patch > > [Beta/Release Uplift Approval Request] > > Feature/Bug causing the regression: Not a regression, and not related to any > other bug/feature > > User impact if declined: We want to uplift this patch ASAP in order to > perform a study to better understand how users are using search in private > browsing mode (in a way that still respects privacy) and how it impacts > related business decisions > This bug is not blocking any shield study, can you give pointers to the study in question? Thanks
Comment 25•6 years ago
|
||
Updating flags according to the new info.
Comment 26•6 years ago
|
||
Justin, could you give us your assessment on the risk of taking this patch in 63.0.3 and if it must be fixed in .3 or not? Since this is a late request, we would like to understand better the risk/benefit ratio here. Thanks!
Comment 27•6 years ago
|
||
My understanding is that this looks safe and is indeed desired for 63.0.3. Followup with :mikedeboer with further questions.
Comment 28•6 years ago
|
||
Comment on attachment 9024137 [details] [diff] [review] Release/63 patch Uplift approved for 63.0.3, thanks.
Comment 29•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/056066d28f8a
Updated•6 years ago
|
Comment 30•5 years ago
|
||
Cornel, can this be QA'd so that we can ship the experiment? I'd like to ship on Tuesday, January 22nd.
Comment 31•5 years ago
|
||
Hi @pdol, a PI request should have been sent for this experiment. Could you please file one?
In the meantime, our team will start looking over the bug and the information provided in https://experimenter.services.mozilla.com/experiments/search-volume-in-private-browsing-observational-study/.
As soon as the PI request is sent, we'll reply with an owner and work towards sending a sign-off as soon as possible.
Comment 32•5 years ago
|
||
(In reply to Ciprian Muresan [:cmuresan], Experiments QA, :steve from comment #31)
Hi @pdol, a PI request should have been sent for this experiment. Could you please file one?
In the meantime, our team will start looking over the bug and the information provided in https://experimenter.services.mozilla.com/experiments/search-volume-in-private-browsing-observational-study/.
As soon as the PI request is sent, we'll reply with an owner and work towards sending a sign-off as soon as possible.
PI request sent. Thanks!
Comment 33•5 years ago
|
||
Search Volume in Private Browsing Observational Study, Release 64
Targeted: Firefox Release 64
We have finished testing the “Search Volume in Private Browsing Observational Study, Release 64" Shield Study experiment.
QA’s recommendation: GREEN - SHIP IT
Reasoning: We haven't found any issues during testing.
Testing Summary:
- Full Functional test suite: https://goo.gl/4xA9JB;
Tested Platforms:
- Windows 10 x64;
- Mac 10.13.3;
- Arch Linux 4.14.3 x64.
Tested Firefox versions:
- Firefox Release 64.0.2;
- Firefox Beta 65.0b11.
Tested Locales:
- en-Us;
- de;
- fr;
- ru;
- es-ES.
Description
•