Update SEARCH_COUNTS telemetry for private browsing mode

RESOLVED FIXED in Firefox 63

Status

()

defect
P1
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
Firefox 65
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox63+ fixed, firefox64 fixed, firefox65 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Comment 1

7 months 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 3

7 months 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)
Flags: needinfo?(pdolanjski)
(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.
Flags: needinfo?(pdolanjski) → needinfo?(mconnor)
Works for me.  I assume "navigation search" is 'organic' search?  I really want to see that delta to PB.
Flags: needinfo?(mconnor)
Assignee

Comment 6

7 months 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.
Flags: needinfo?(mconnor)
When he says organic, he means my new code in the in-content search probe.

So yes, this affects all of our search recording.
Assignee

Comment 8

7 months ago
OK, thanks Mike.
Flags: needinfo?(mconnor)

Comment 9

7 months ago
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6beec7d503af
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee

Comment 11

7 months ago
Posted patch Beta/64 patch (obsolete) — Splinter Review
[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
Attachment #9024082 - Flags: approval-mozilla-beta?
Assignee

Comment 12

7 months 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

7 months 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.
Attachment #9024137 - Flags: review?(mozilla)
Comment on attachment 9024082 [details] [diff] [review]
Beta/64 patch

changes to search telemetry recording, approved for 64.0b9
Attachment #9024082 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

7 months ago

Updated

7 months ago
Attachment #9024137 - Flags: review?(mozilla) → review+
Assignee

Comment 18

7 months ago
This should fix the test failure on beta.  Try push here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bf233fdfc2c9ee651167f06ab800a2c058452a6
Attachment #9024082 - Attachment is obsolete: true
Flags: needinfo?(adw) → needinfo?(rmaries)
Attachment #9024580 - Flags: approval-mozilla-beta+
Assignee

Comment 21

7 months 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
Attachment #9024137 - Flags: approval-mozilla-release?
Assignee

Comment 22

7 months ago
A try push to release is in comment 12
Assignee

Updated

7 months ago
Flags: needinfo?(rmaries)
(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
Flags: needinfo?(adw)
Assignee

Comment 24

6 months ago
I sent you an email with some info.
Flags: needinfo?(adw)
Updating flags according to the new info.
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!
Flags: needinfo?(dolske)
My understanding is that this looks safe and is indeed desired for 63.0.3. Followup with :mikedeboer with further questions.
Flags: needinfo?(dolske)
Comment on attachment 9024137 [details] [diff] [review]
Release/63 patch

Uplift approved for 63.0.3, thanks.
Attachment #9024137 - Flags: approval-mozilla-release? → approval-mozilla-release+
Assignee

Updated

5 months ago
Blocks: 1510570

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

Flags: needinfo?(cornel.ionce)
Depends on: 1518393

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.

Flags: needinfo?(cornel.ionce) → needinfo?(pdolanjski)

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

Flags: needinfo?(pdolanjski)

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:

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.
You need to log in before you can comment on or make changes to this bug.