1504370 - Update SEARCH_COUNTS telemetry for private browsing mode

Status: RESOLVED FIXED

(Firefox :: Search, defect, P1)

Description

[Drew Willcoxon :adw]

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

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1504370 - Update SEARCH_COUNTS telemetry for private browsing mode

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.

Comment 2

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa311c8939c608811841d8c86ba81de13b2c92f8

Comment 3

[Drew Willcoxon :adw]

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

[Peter Dolanjski [:pdol]]

(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

[Mike Connor [:mconnor]]

Works for me.  I assume "navigation search" is 'organic' search?  I really want to see that delta to PB.

Comment 6

[Drew Willcoxon :adw]

(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

[Mike Kaply [:mkaply]]

When he says organic, he means my new code in the in-content search probe.

So yes, this affects all of our search recording.

Comment 8

[Drew Willcoxon :adw]

OK, thanks Mike.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6beec7d503af
Update SEARCH_COUNTS telemetry for private browsing mode r=mkaply

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6beec7d503af

Comment 11

[Drew Willcoxon :adw]

Attachment: Beta/64 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)

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

Comment 12

[Drew Willcoxon :adw]

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.

Comment 13

[Drew Willcoxon :adw]

Attachment: Release/63 patch

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

[Julien Cristau [:jcristau]]

Comment on attachment 9024082 [details] [diff] [review]
Beta/64 patch

changes to search telemetry recording, approved for 64.0b9

Comment 15

[Razvan Maries]

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

Comment 16

[Razvan Maries]

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

Comment 17

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bf233fdfc2c9ee651167f06ab800a2c058452a6

Comment 18

[Drew Willcoxon :adw]

Attachment: Beta/64 patch v2

This should fix the test failure on beta.  Try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bf233fdfc2c9ee651167f06ab800a2c058452a6

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5336b29e9db0

Comment 20

[Cristian Brindusan [:cbrindusan]]

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

Comment 21

[Drew Willcoxon :adw]

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

Comment 22

[Drew Willcoxon :adw]

A try push to release is in comment 12

Comment 23

[Pascal Chevrel (relman team) -> :pascalc]

(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 24

[Drew Willcoxon :adw]

I sent you an email with some info.

Comment 25

[Pascal Chevrel (relman team) -> :pascalc]

Updating flags according to the new info.

Comment 26

[Pascal Chevrel (relman team) -> :pascalc]

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

[Justin Dolske [:Dolske]]

My understanding is that this looks safe and is indeed desired for 63.0.3. Followup with :mikedeboer with further questions.

Comment 28

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9024137 [details] [diff] [review]
Release/63 patch

Uplift approved for 63.0.3, thanks.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/056066d28f8a

Comment 30

[Peter Dolanjski [:pdol]]

Cornel, can this be QA'd so that we can ship the experiment? I'd like to ship on Tuesday, January 22nd.

Comment 31

[Ciprian Muresan [:cmuresan], Formerly of Ecosystem QA - please needinfo @mcoman for things regarding the team]

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

[Peter Dolanjski [:pdol]]

(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

[Marius Coman [:mcoman], Ecosystem QA]

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.