1192359 - Add Telemetry for search suggestions being enabled

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P2)

Description

[Georg Fritzsche [:gfritzsche]]

We have opt-in search suggestions now; we need Telemetry on whether this is enabled.

Ideally this goes into the Telemetry environment in one of these places:
http://hg.mozilla.org/mozilla-central/annotate/3e51753a099f/toolkit/components/telemetry/TelemetryEnvironment.jsm#l82
http://hg.mozilla.org/mozilla-central/annotate/3e51753a099f/toolkit/components/telemetry/TelemetryEnvironment.jsm#l997

Documentation updates go here:
http://hg.mozilla.org/mozilla-central/annotate/3e51753a099f/toolkit/components/telemetry/docs/environment.rst

Comment 1

[Georg Fritzsche [:gfritzsche]]

[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.

Comment 2

[Drew Willcoxon :adw]

Attachment: 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.

Comment 3

[Georg Fritzsche [:gfritzsche]]

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

Comment 4

[Georg Fritzsche [:gfritzsche]]

(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

Comment 5

[Georg Fritzsche [:gfritzsche]]

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.

Comment 6

[Drew Willcoxon :adw]

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

:-/

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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.

Comment 8

[Georg Fritzsche [:gfritzsche]]

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.

Comment 9

[Drew Willcoxon :adw]

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?

Comment 10

[Drew Willcoxon :adw]

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.

Comment 11

[Drew Willcoxon :adw]

Attachment: 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.

Comment 12

[Drew Willcoxon :adw]

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?

Comment 13

[Drew Willcoxon :adw]

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

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 15

[Georg Fritzsche [:gfritzsche]]

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.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/45de359919d7

Comment 17

[Drew Willcoxon :adw]

Attachment: landed patch

Thanks, addressed comments and tested manually that it works.

https://hg.mozilla.org/integration/fx-team/rev/45de359919d7

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/45de359919d7

Comment 19

[Drew Willcoxon :adw]

Attachment: 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

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8654407 [details] [diff] [review]
Aurora/42 patch

Taking it as it will help to know how our users use fx.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3bb5c17c6856