Add a probe to log user's Urlbar/ Search Bar State (2 bars vs. unified) in Environment variables

RESOLVED FIXED in Firefox 57

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: javaun, Assigned: Dexter)

Tracking

57 Branch
mozilla57
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed, firefox58 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Starting in 57, new installs (new profiles) will see a Unified Awesome Bar/Search Bar in Firefox. 

We want a new probe added for environment variables so this will store in session fragments:
http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/toolkit/components/telemetry/TelemetryEnvironment.jsm#184

Our objective is to understand:
1.(Obviously) how many users are switching back and forth, and also 
2. make behavioral correlations, i.e. do users with one state search more, do certain kinds of users need Unified vs. 2 bars. 

The pref that we need to examine to understand the user's bar state is:

`browser.search.widget.inNavBar`

(Bool): False = unified, True = 2 bars. 

We need to land this ASAP and uplift to 57.
(Reporter)

Updated

2 years ago
Hardware: Unspecified → All
Target Milestone: --- → mozilla57
Version: unspecified → 57 Branch
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 years ago
(In reply to Javaun Moradi [:javaun] from comment #0)
> The pref that we need to examine to understand the user's bar state is:
> 
> `browser.search.widget.inNavBar`
> 
> (Bool): False = unified, True = 2 bars. 
> 
> We need to land this ASAP and uplift to 57.

I've added the pref to TelemetryEnvironment.jsm and gone through manual testing. Automated test coverage is already provided by test_TelemetryEnvironment.js (for the general pref collection logic).

This is how the collection behaves:

- if the user did not change the value of the pref (and so it has the default value), it won't be reported. So, missing pref (from the pings who should have it) means default value.
- if the user changed the pref, the pref value is reported (either True or False).
(Assignee)

Comment 3

2 years ago
Javaun, please flag Rebecca if comment 2 looks ok along with the details she requested for the data review :)
Flags: needinfo?(jmoradi)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8913324 [details]
Bug 1403978 - Add 'browser.search.widget.inNavBar' to TelemetryEnvironment.

https://reviewboard.mozilla.org/r/184688/#review189872
Attachment #8913324 - Flags: review?(chutten) → review+
(Reporter)

Comment 5

2 years ago
Data collection review form, NI to Rweiss
https://docs.google.com/document/d/1a8EkK0kimfy9MsRBGSiJh1JMQrBju23yDElnTD0kYRw/edit#
Flags: needinfo?(jmoradi) → needinfo?(rweiss)
(Reporter)

Comment 6

2 years ago
Thanks so much Alessio!

I believe we may need to change the logic to always report the pref value, even if the user did not change it. That's because existing Fx users (existing profiles) will keep 2 bars by default in 57. But new installs (new profiles) will get unified in 57. 

It all depends on whether we report that Bool value for the 57 users who get it by default. In that case, we'd see the following for that variable:
1. False, for users who do a new install of Fx57 and stay unified
2. True, for users who do a new install in Fx57 but manually switch back to separate bars
3. No value at all for old profiles who keep their separate bar and make no change
4. False for old profiles who manually switch to unified bars

We need to be sure that Case 1 actually happens for new installs who get unified. Otherwise both bucket #1 and bucket #3 will have no value at all. We can ask Panos/Paolo about how we're triggering (flipping the pref) for new installs in 57 (case 1)

If it's not to much trouble, we could also just always send it. 



> - if the user did not change the value of the pref (and so it has the
> default value), it won't be reported. So, missing pref (from the pings who
> should have it) means default value.
> - if the user changed the pref, the pref value is reported (either True or
> False).
Flags: needinfo?(alessio.placitelli)

Comment 7

2 years ago
r+

1) Is there documentation that describes the schema for the ultimate data set available publicly, complete and accurate? 
Yes, standard Telemetry documentation.

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism)  
Yes, users can opt out of Telemetry in the normal fashion.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?
Javaun Moradi is accountable. 

4) Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? 
This is type 2.

5) Is the data collection default-on or default-off? 
Default on everywhere.

6) Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)? 
No.

7) Is the data collection covered by the existing Firefox privacy notice? If unsure: escalate to legal.
Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) 
No
Flags: needinfo?(rweiss)
(Assignee)

Updated

2 years ago
Depends on: 1404238
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
(In reply to Javaun Moradi [:javaun] from comment #6)
> Thanks so much Alessio!

You're welcome! :)

> I believe we may need to change the logic to always report the pref value,
> even if the user did not change it. That's because existing Fx users
> (existing profiles) will keep 2 bars by default in 57. But new installs (new
> profiles) will get unified in 57. 
> ...
> If it's not to much trouble, we could also just always send it. 

You're right. The new users are being served a different UI because the default value changed in bug 1387416, so the first patch alone doesn't cover your needs.

I'm afraid we don't have a general way to both always send a user pref (even if it has the default value) and trigger an environment change. I filed bug 1404238 for that, but it may take some time to get it to land (if it will at all) and it would be too risky to uplift.

I decided to follow another path here, that some other data points in TelemetryEnvironment are following as well:

- Add the preference to the |DEFAULT_ENVIRONMENT_PREFS| in TelemetryEnvironment.jsm (first patch). This allows us to monitor the pref for changes and generate a new subsession/environment change when it does.
- Add the new boolean entry |environment.settings.searchInNavbar|, which always report the pref value, even if it's a default value. This exact same approach is being used for the "app.update.*" prefs which are mirrored in the |environment.settings.update.*| section of the environment.

The data review still stands, since it's the same data.

@Chris, any better idea?
Flags: needinfo?(alessio.placitelli)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8913579 [details]
Bug 1403978 - Add 'searchInNavbar' to the environment's settings section.

https://reviewboard.mozilla.org/r/184956/#review190096

::: toolkit/components/telemetry/TelemetryEnvironment.jsm:1388
(Diff revision 1)
>          enabled: Services.prefs.getBoolPref(PREF_UPDATE_ENABLED, true),
>          autoDownload: Services.prefs.getBoolPref(PREF_UPDATE_AUTODOWNLOAD, true),
>        },
>        userPrefs: this._getPrefData(),
>        sandbox: this._getSandboxData(),
> +      searchInNavbar: Services.prefs.getBoolPref(PREF_SEARCH_INNAVBAR, false),

Is the default value different for new vs old profiles? That `, false)` worries me, if so.

::: toolkit/components/telemetry/docs/data/environment.rst:75
(Diff revision 1)
>          },
>          sandbox: {
>            effectiveContentProcessLevel: <integer>,
> -        }
> -      },
> +        },
> +        searchInNavbar: <bool> // false if the unified searchbar is used, false if 2 bars

Shouldn't this be "true if the unified searchbar is used"?
Attachment #8913579 - Flags: review?(chutten) → review-
(Assignee)

Comment 11

2 years ago
mozreview-review-reply
Comment on attachment 8913579 [details]
Bug 1403978 - Add 'searchInNavbar' to the environment's settings section.

https://reviewboard.mozilla.org/r/184956/#review190096

> Is the default value different for new vs old profiles? That `, false)` worries me, if so.

That should only kick in if the pref was deleted (see [here](http://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/modules/libpref/prefapi.cpp#514)). In that case we have no way to detect the proper behaviour, so I'd say we stick to the default we ship, [which is false](http://searchfox.org/mozilla-central/source/browser/app/profile/firefox.js#409).

> Shouldn't this be "true if the unified searchbar is used"?

No, "false" means unified searchbar is used (see comment 0). However I agree with you, it's a bit confusing.

Updated

2 years ago
Depends on: 1404343
(Assignee)

Comment 12

2 years ago
Hah, forget about comment 9! There's a better way to do this: turns out this policy was already implemented in bug 1343943 :-D
Attachment #8913324 - Attachment is obsolete: true
Attachment #8913579 - Attachment is obsolete: true
Attachment #8913697 - Flags: review?(chutten)

Updated

2 years ago
Attachment #8913697 - Flags: review?(chutten) → review+
(Assignee)

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/973e6db25fc4a164177da6a723994efc93e61563
Bug 1403978 - Add 'browser.search.widget.inNavBar' to TelemetryEnvironment. r=chutten,data-review=rweiss

Comment 14

2 years ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/973e6db25fc4
Add 'browser.search.widget.inNavBar' to TelemetryEnvironment. r=chutten,data-review=rweiss
Just to confirm, this will record the pref value no matter how it gets set?

The searchbar position can be changed by:

- manually editing about:config
- dragging it in the Customize view
- toggling the setting in the Preferences page
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 16

2 years ago
(In reply to Dave Zeber [:dzeber] from comment #15)
> Just to confirm, this will record the pref value no matter how it gets set?

According to comment 0, all the cases should trace back to the preference change.
 
> The searchbar position can be changed by:
> 
> - manually editing about:config

This directly changes the pref, so we're safe here.

> - dragging it in the Customize view

This should be covered by http://searchfox.org/mozilla-central/source/browser/components/customizableui/SearchWidgetTracker.jsm

> - toggling the setting in the Preferences page

Should be covered by http://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/browser/components/preferences/in-content/search.xul#35

Panos, can you confirm?
Flags: needinfo?(alessio.placitelli) → needinfo?(past)
Yes, that's right.
Flags: needinfo?(past)
Alessio, are we ready to request uplift to 57 on this probe? Is there anything I can do to help?

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/973e6db25fc4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 20

2 years ago
Comment on attachment 8913697 [details] [diff] [review]
Add 'browser.search.widget.inNavBar' to TelemetryEnvironment.

Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]: We will have no data regarding how many users use "search in awesomebar" vs "use a searchbar and the awesomebar" in 57. And we're switching to awesomebar-only for new users in 57.
[Is this code covered by automated tests?]: test_TelemetryEnvironment.js contains test coverage for the collection system.
[Has the fix been verified in Nightly?]: Verified through manual QA
[Needs manual test from QE? If yes, steps to reproduce]: None
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It adds a new data point to the Telemetry environment using a well tested system (which has test coverage)
[String changes made/needed]: None
Attachment #8913697 - Flags: approval-mozilla-beta?
(Assignee)

Comment 21

2 years ago
(In reply to shgupta@mozilla.com from comment #18)
> Alessio, are we ready to request uplift to 57 on this probe? Is there
> anything I can do to help?

Yup! I was waiting for this to hit Nightly before asking for uplifts :) It's on today's Nightly \o/
I don't think there's anything else to do here.
status-firefox57: --- → affected
Comment on attachment 8913697 [details] [diff] [review]
Add 'browser.search.widget.inNavBar' to TelemetryEnvironment.

More telemetry, taking it.
Should be in 57b5
Attachment #8913697 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/cd59a9732166
status-firefox57: affected → fixed
You need to log in before you can comment on or make changes to this bug.