All users were logged out of Bugzilla on October 13th, 2018

Add Telemetry for search suggestions being enabled

RESOLVED FIXED in Firefox 42

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gfritzsche, Assigned: adw)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox41 unaffected, firefox42+ fixed, firefox43 fixed)

Details

(Whiteboard: [fxsearch][searchsuggestions])

Attachments

(2 attachments, 2 obsolete attachments)

Flags: firefox-backlog+
Priority: -- → P2
Whiteboard: [fxsearch][searchsuggestions]
(Reporter)

Updated

3 years ago
Depends on: 959567

Updated

3 years ago
Blocks: 958204
(Reporter)

Updated

3 years ago
status-firefox41: --- → unaffected
(Reporter)

Comment 1

3 years ago
[Tracking Requested - why for this release]:
Search is an important feature, search metrics are among our core metrics.
Without that we can't track search suggestion optin and not tell search usage changes apart.
tracking-firefox42: --- → ?

Updated

3 years ago
Rank: 25
(Assignee)

Comment 2

3 years ago
Created attachment 8652644 [details] [diff] [review]
patch

Is this all that needs to happen?  This is the first time I've seen TelemetryEnvironment.

There are three relevant prefs, and I would think we want all of them:

(1) browser.search.suggest.enabled: The "master switch" for search suggestions everywhere in Firefox (search bar, urlbar, etc.).  Defaults to true.

(2) browser.urlbar.suggest.searches: Whether search suggestions are enabled in the urlbar.  Defaults to false.

So search suggestions in the urlbar will be shown if browser.search.suggest.enabled && browser.urlbar.suggest.searches.

(3) browser.urlbar.userMadeSearchSuggestionsChoice: Whether the user has clicked Yes or No in the urlbar's opt-in notification.  Defaults to false of course.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Attachment #8652644 - Flags: review?(gfritzsche)
(Reporter)

Comment 3

3 years ago
Comment on attachment 8652644 [details] [diff] [review]
patch

Review of attachment 8652644 [details] [diff] [review]:
-----------------------------------------------------------------

That sounds like sufficient data for analysis to me - Ilana, is that ok / sufficient from your end?

I think we need to start documenting the prefs in the documentation too and your comment had good info on that.
Could you start a userPref section in the documentation here and list these prefs + explanation there?
https://dxr.mozilla.org/mozilla-central/rev/f61c3cc0eb8b7533818e7379ccc063b611015d9d/toolkit/components/telemetry/docs/environment.rst#239
Attachment #8652644 - Flags: review?(gfritzsche) → feedback?(isegall)
(Reporter)

Comment 4

3 years ago
(In reply to Drew Willcoxon :adw from comment #2)
> Is this all that needs to happen?  This is the first time I've seen
> TelemetryEnvironment.

For a quick background:
The TelemetryEnvironment contains important data - the "important charactics of the environment a browser session runs in" if you will.
We can submit that environment with different types of Telemetry pings, the most important ones being the "main" pings (contains the histograms, session information, ...).
https://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/telemetry/pings.html
https://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/telemetry/environment.html
(Reporter)

Comment 5

3 years ago
Comment on attachment 8652644 [details] [diff] [review]
patch

Review of attachment 8652644 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +100,5 @@
>    ["browser.newtabpage.enabled", TelemetryEnvironment.RECORD_PREF_VALUE],
>    ["browser.newtabpage.enhanced", TelemetryEnvironment.RECORD_PREF_VALUE],
>    ["browser.polaris.enabled", TelemetryEnvironment.RECORD_PREF_VALUE],
>    ["browser.shell.checkDefaultBrowser", TelemetryEnvironment.RECORD_PREF_VALUE],
> +  ["browser.search.suggest.enabled", TelemetryEnvironment.RECORD_PREF_VALUE],

Do any of the prefs require a restart to take effect after change?
If yes, you should add |requiresRestart: true| after rebasing to current m-c.
(Assignee)

Comment 6

3 years ago
Oh, browser.urlbar.unifiedcomplete should be included too since it also affects whether urlbar suggestions are shown.  (Suggestions are implemented on top of UnifiedComplete, which we want to pref on by default in 42.)  So in summary suggestions are shown if:

browser.urlbar.unifiedcomplete && browser.search.suggest.enabled && browser.urlbar.suggest.searches

:-/
Do we actually care about suggestions being shown, or only about whether the user opted-in?
cause if it's the latter browser.urlbar.suggest.searches would be enough imo.

Indeed the user won't be asked to enable search suggestions if browser.search.suggest.enabled as well as he won't see the option in preferences. The same way if unified is disabled we don't show the opt-in or the pref. I don't think we care much about about:config changes?

So, the actual data we need changes things a lot.
(Reporter)

Comment 8

3 years ago
If the logic becomes more complicated here, maybe we should just add one or two values here under environment.settings.search.* that contain exactly what we want to know.
(Assignee)

Comment 9

3 years ago
We probably want to know whether the user has made a choice at all, so I think we do want browser.urlbar.userMadeSearchSuggestionsChoice.  It's easy to imagine a non-negligible portion of users just never making a choice.

Given that, the choice is only offered if browser.search.suggest.enabled && browser.urlbar.unifiedcomplete, so we would need those too to be able to exclude users who were never offered the choice.

Does that make sense?
(Assignee)

Comment 10

3 years ago
In other words I think there are two arguments for collecting all the prefs: comment 9, and also to be able to tell with accuracy whether the user is actually seeing suggestions.
(Assignee)

Comment 11

3 years ago
Created attachment 8653141 [details] [diff] [review]
patch with docs

This adds browser.urlbar.unifiedcomplete and documentation.

(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> For a quick background:

Thanks!

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> Do any of the prefs require a restart to take effect after change?

The only one that kind of does is browser.urlbar.unifiedcomplete, but even that takes effect for new windows as you open them.  But that pref's purpose is to hide the in-progress UnifiedComplete implementation, not to allow the user to change anything, so I don't think we need to care about restarts for it.
Attachment #8652644 - Attachment is obsolete: true
Attachment #8652644 - Flags: feedback?(isegall)
Attachment #8653141 - Flags: review?(gfritzsche)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8653141 [details] [diff] [review]
patch with docs

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> Comment on attachment 8652644 [details] [diff] [review]
> patch
> 
> Review of attachment 8652644 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That sounds like sufficient data for analysis to me - Ilana, is that ok /
> sufficient from your end?
Attachment #8653141 - Flags: feedback?(isegall)
(Assignee)

Comment 13

3 years ago
Comment on attachment 8653141 [details] [diff] [review]
patch with docs

Review of attachment 8653141 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/docs/environment.rst
@@ +277,5 @@
>  
>  If the user has been enrolled into a search default change experiment, this contains the string identifying the experiment the user is taking part in. Most user profiles will never be part of any search default change experiment, and will not send this value.
> +
> +userPrefs
> +---------

Oops, these should be tildes, not dashes.  I need new glasses...
(In reply to Drew Willcoxon :adw from comment #11)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> > Do any of the prefs require a restart to take effect after change?
> 
> The only one that kind of does is browser.urlbar.unifiedcomplete, but even
> that takes effect for new windows as you open them.

I think it works also for the current window... Or at least, that was the intent. Regardless, as soon as we ship it in a release we should remove the pref and stop building the old autocomplete in Firefox.
(Reporter)

Comment 15

3 years ago
Comment on attachment 8653141 [details] [diff] [review]
patch with docs

Review of attachment 8653141 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +106,5 @@
>    ["browser.startup.homepage", {what: RECORD_PREF_STATE}],
>    ["browser.startup.page", {what: RECORD_PREF_VALUE}],
> +  ["browser.urlbar.suggest.searches", TelemetryEnvironment.RECORD_PREF_VALUE],
> +  ["browser.urlbar.unifiedcomplete", TelemetryEnvironment.RECORD_PREF_VALUE],
> +  ["browser.urlbar.userMadeSearchSuggestionsChoice", TelemetryEnvironment.RECORD_PREF_VALUE],

Note that the format changed here - you have to adopt to the {what: RECORD_PREF_VALUE} style.
Please give this a quick manual check via about:telemetry to confirm it is working.

::: toolkit/components/telemetry/docs/environment.rst
@@ +281,5 @@
> +---------
> +
> +This object contains user preferences.
> +
> +Each key in the object is the name of a preference. A key's value depends on the policy with which the preference was collected. There are two such policies, "value" and "state". For preferences collected under the "value" policy, the value will be the preference's value. For preferences collected under the "state" policy, the value will be an opaque marker signifying only that the preference has a user value. The "state" policy is therefore used when user privacy is a concern.

Thanks for adding a writeup for this too.
Attachment #8653141 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 17

3 years ago
Created attachment 8653805 [details] [diff] [review]
landed patch

Thanks, addressed comments and tested manually that it works.

https://hg.mozilla.org/integration/fx-team/rev/45de359919d7
Attachment #8653141 - Attachment is obsolete: true
Attachment #8653141 - Flags: feedback?(isegall)
https://hg.mozilla.org/mozilla-central/rev/45de359919d7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(Assignee)

Comment 19

3 years ago
Created attachment 8654407 [details] [diff] [review]
Aurora/42 patch

Approval Request Comment
[Feature/regressing bug #]:
Search suggestions in the urlbar (which landed in 42: bug 959567, bug 959594, bug 959595, others), and search suggestions in general

[User impact if declined]:
None, this patch adds telemetry.  But without it, we won't know how many people are opting in to search suggestions in the urlbar in the release where we are trying to introduce it, 42.  (It's currently preffed off on 42.)

[Describe test coverage new/current, TreeHerder]:
TelemetryEnvironment.jsm has an existing unit test.  I also tested this manually on m-a.

[Risks and why]:
Low risk, only adding some telemetry.

[String/UUID change made/needed]:
None
Attachment #8654407 - Flags: approval-mozilla-aurora?
Comment on attachment 8654407 [details] [diff] [review]
Aurora/42 patch

Taking it as it will help to know how our users use fx.
Attachment #8654407 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox42: ? → +
You need to log in before you can comment on or make changes to this bug.