1408339 - [dt-onboarding] Prepare for a shield study about the devtools onboarding flow on beta

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

We need to assess that the devtools onboarding page doesn't have a negative impact on users.

To do so we will run a study in beta, where we enable the devtools onboarding flow for part of the userbase.

In this bug we need to:
- add probes to measure the click-through rate for the onboarding page
- add higher order preferences to drive the study
- decide if the study should maximize the number of people who see the flow and bypass Bug 1408337

Comment 1

[Julian Descottes [:jdescottes]]

Some thoughts about how the higher order preference should work here.

Preference name : devtools.onboarding.enabled
Possible values : [false, true, experiment]

- if devtools.onboarding.enabled = false, 
  then devtools.enabled is forced to true whatever the profile (-> no onboarding)

- if devtools.onboarding.enabled = true, 
  then devtools.enabled should be calculated depending on the profile's info.

- if devtools.onboarding.enabled = experiment, 
  then devtools.enabled is not updated based on the profile info.

The default group of users should have devtools.onboarding.enabled = false.
The test group should have:
- devtools.onboarding.enabled = experiment
- devtools.enabled = false 

If something goes wrong we should rollback to devtools.onboarding.enabled = false.
`true` would only be used once the experiment is succesful and we want to push the real version of the feature to users, but we can also say that this pref is just a temporary thing for the study, and have only two values.

Comment 2

[Julian Descottes [:jdescottes]]

Added as blockers all the bugs that need to be fixed for the study.

@Harald: you mentioned you wanted to do a UX review of the enabling screen. If there are issues you would like to see fixed before the experiment and which are not marked as blocker, can you let me know?

For this particular bug I need to know:
- is my proposal for the preference OK? (see comment 1)
- what do we want to measure for the experiment: openings of enabling screen? clicks on enable button? 
- what else do we need to do to prepare for this? 

Reading https://wiki.mozilla.org/Firefox/Shield/Shield_Studies , it looks like we need to create an addon to drive the study.
In our case I think the role of the addon would be only to update preferences?
- on install: 
  - devtools.onboarding.enabled=experiment
  - devtools.enabled=false
- on uninstall
  - devtools.onboarding.enabled=false
  - devtools.enabled=true

We could also use it to log the data we need from the onboarding page. In this case I just have to make sure the page fires events for opening, closing, enabling, and we can wait until we create the addon to decide exactly what we want to measure.

Comment 3

[Julian Descottes [:jdescottes]]

Reading some more documents, it looks like shield studies can also be deployed simply by flipping a preference. I guess that's what you had in mind for this one. In this case, we need to embed the data collection in the onboarding page.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1408339 - add a preference to drive the devtools onboarding experiment;

Review commit: https://reviewboard.mozilla.org/r/196600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196600/

Comment 5

[Julian Descottes [:jdescottes]]

As discussed last Wednesday, the "experiment" value for the preference no longer makes sense as we would like to run the experiment by using our logic to detect devtools users. 

I switched to using a devtools.onboarding.experiment pref:
- if false: devtools.enabled is forced to true on startup
- if true:  devtools.enabled is computed from the user's profile (using devtools.selfxss.count, see Bug 1408337)

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1408339 - measure onboarding devtools user detection with telemetry;, datareview=francois

Review commit: https://reviewboard.mozilla.org/r/197706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197706/

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8925521 [details]
Bug 1408339 - add a preference to drive the devtools onboarding experiment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196600/diff/1-2/

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: request-bug1408339.md

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8925521 [details]
Bug 1408339 - add a preference to drive the devtools onboarding experiment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196600/diff/2-3/

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8926456 [details]
Bug 1408339 - measure onboarding devtools user detection with telemetry;, datareview=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197706/diff/1-2/

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8925521 [details]
Bug 1408339 - add a preference to drive the devtools onboarding experiment;

https://reviewboard.mozilla.org/r/196600/#review204100

::: devtools/shim/devtools-startup-prefs.js:24
(Diff revision 3)
>  #endif
>  
>  // Should the devtools toolbar be opened on startup
>  pref("devtools.toolbar.visible", false);
> +
> +// Flag to drive the devtools onboarding flow experiment.

You may say here that it forces devtools.enable to true when false.

::: devtools/shim/devtools-startup.js:465
(Diff revision 3)
>  
> +    if (!Services.prefs.getBoolPref("devtools.onboarding.experiment")) {
> +      // Force devtools.enabled to true for users that are not part of the experiment.
> +      Services.prefs.setBoolPref(DEVTOOLS_ENABLED_PREF, true);
> +      return;
> +    }

You could inline the existing code that already forces the pref to true:
  if (hasDevToolsFlag || hasToolbarPref || this.isDevToolsUser() || !isOnboardingExperiment) {
(and keep your comment before isOnboardingExperiment definition)

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8926456 [details]
Bug 1408339 - measure onboarding devtools user detection with telemetry;, datareview=francois

https://reviewboard.mozilla.org/r/197706/#review204104

::: devtools/shim/devtools-startup.js:264
(Diff revision 2)
> +    if (!this._firstWindowReadyReceived) {
> +      this.onFirstWindowReady(window);
> +      this._firstWindowReadyReceived = true;
> +    }
> +
> +    JsonView.initialize();

May be all but hookWindow could be moved to onFirstWindowReady?

::: devtools/shim/devtools-startup.js:274
(Diff revision 2)
> -      this.devtoolsFlag = false;
>      }
>  
> -    JsonView.initialize();
> +    // Wait until we get a window before sending a ping to telemetry to avoid slowing down
> +    // the startup phase.
> +    this.pingOnboardingExperimentTelemetry();

I imagine that's the plan right? This onboarding pref is going to be set to true by some remotely installed addon?
So It should be late enough for the pref to be toggle by it, right?

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8926456 [details]
Bug 1408339 - measure onboarding devtools user detection with telemetry;, datareview=francois

https://reviewboard.mozilla.org/r/197706/#review204104

Thanks for the review!

> May be all but hookWindow could be moved to onFirstWindowReady?

Yes, I will do it in a followup just in case. Will also remove the "initialized" flag from the JSONView at the same time.

> I imagine that's the plan right? This onboarding pref is going to be set to true by some remotely installed addon?
> So It should be late enough for the pref to be toggle by it, right?

Yes the onboarding preference will be switched remotely to enable the experiment. I am not sure when the flipping occurs though. 
Is it randomly during runtime? After a restart? Does it prompt for a restart? Haven't really thought about that.

When the user restarts the browser, the probe will be called. But we can't be sure that they will restart during the experiment.
Maybe we should add an observer on the onboarding experiment pref, just to make sure it gets called at least once for all the experiment users. 

I should gather a bit more information about the pref flipping mechanism though.

Comment 14

[François Marier [:francois]]

Comment on attachment 8927403 [details]
request-bug1408339.md

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate? (see [here](https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/data_dictionary.md), [here](https://github.com/mozilla-mobile/focus/wiki/Install-and-event-tracking-with-the-Adjust-SDK), and [here](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/index.html) for examples).  Refer to the appendix for "documentation" if more detail about documentation standards is needed.

Yes, the scalar description.

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) Provide details as to the control mechanism available.

Yes, telemetry pref.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Not permanent.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 2.

5) Is the data collection request for default-on or default-off?

default-on

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?

No.

8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**

No, telemetry alerts are fine.

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: request-bug1408339-v2.md

Sorry Francois, I just got an update about the requirements for this probe. It should target all users rather than only the devtools-onboarding experiment users. 

Most of the the content remains valid but I assume it needs to be reviewed again.

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8925521 [details]
Bug 1408339 - add a preference to drive the devtools onboarding experiment;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196600/diff/3-4/

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8926456 [details]
Bug 1408339 - measure onboarding devtools user detection with telemetry;, datareview=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197706/diff/2-3/

Comment 18

[François Marier [:francois]]

Comment on attachment 8928188 [details]
request-bug1408339-v2.md

(In reply to Julian Descottes [:jdescottes][:julian] from comment #15)
> Created attachment 8928188 [details]
> request-bug1408339-v2.md
> 
> Sorry Francois, I just got an update about the requirements for this probe.
> It should target all users rather than only the devtools-onboarding
> experiment users. 
> 
> Most of the the content remains valid but I assume it needs to be reviewed
> again.

I don't believe that this changes any of my review answers, so datareview+.

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8925521 [details]
Bug 1408339 - add a preference to drive the devtools onboarding experiment;

https://reviewboard.mozilla.org/r/196600/#review204100

> You could inline the existing code that already forces the pref to true:
>   if (hasDevToolsFlag || hasToolbarPref || this.isDevToolsUser() || !isOnboardingExperiment) {
> (and keep your comment before isOnboardingExperiment definition)

I didn't go for it because it triggers eslint max line violations on 2 places, forcing me to break lines and making the code harder to follow. I think having the block dedicated to the experiment really as a standalone also has value in order to cleanup later on.

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8926456 [details]
Bug 1408339 - measure onboarding devtools user detection with telemetry;, datareview=francois

https://reviewboard.mozilla.org/r/197706/#review204104

> Yes the onboarding preference will be switched remotely to enable the experiment. I am not sure when the flipping occurs though. 
> Is it randomly during runtime? After a restart? Does it prompt for a restart? Haven't really thought about that.
> 
> When the user restarts the browser, the probe will be called. But we can't be sure that they will restart during the experiment.
> Maybe we should add an observer on the onboarding experiment pref, just to make sure it gets called at least once for all the experiment users. 
> 
> I should gather a bit more information about the pref flipping mechanism though.

I didn't get any answer to my questions so far. But now that the probe is enabled for all users, I think it should gather enough data to be relevant, regardless of some users not restarting their browsers.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fb5208715ea
add a preference to drive the devtools onboarding experiment;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/dfdc0642a212
measure onboarding devtools user detection with telemetry;r=ochameau, datareview=francois

Comment 22

[Julian Descottes [:jdescottes]]

Harald: should we try to uplift this. It's the only missing piece for the experiment from my point of view but I don't know if we still want to run this in Beta 58?

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/0fb5208715ea
https://hg.mozilla.org/mozilla-central/rev/dfdc0642a212

Comment 24

[:Harald Kirschner :digitarald]

Yes, we should uplift. This can’t be tested on pre-release so doesn’t make sense to bake on Nightly for several weeks. Most changes are also only about Telemetry.

I’ll create the experiment draft tomorrow.

Comment 25

[Julian Descottes [:jdescottes]]

Comment on attachment 8925521 [details]
Bug 1408339 - add a preference to drive the devtools onboarding experiment;

Approval Request Comment
[Feature/Bug causing the regression]: shield study for feature work started in Bug 1361080
[User impact if declined]: We would like to run an experiment in Beta for the devtools onboarding flow, this patch contains the pref that will allow us to enable the feature only for a subset of users.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: If needed the STRs to verify this are:
- use a new profile
- in about:config: 
  - set devtools.enabled to false
  - set devtools.selfxss.count to 0 
  - set devtools.onboarding.experiment to true
- try to open devtools with a keyboard shortcut (e.g. F12) -> a tab should open on about:devtools
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this patch is just adding a bool pref that guards a feature currently disabled. The code change is minimal, and it won't have any impact for users until we enable the experiment for some of them.
[String changes made/needed]: none

Comment 26

[Julian Descottes [:jdescottes]]

Comment on attachment 8926456 [details]
Bug 1408339 - measure onboarding devtools user detection with telemetry;, datareview=francois

Approval Request Comment
[Feature/Bug causing the regression]: shield study for feature work started in Bug 1361080
[User impact if declined]: This patch contains a telemetry probe that will help us check if part of our study logic is correct.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this patch is just adding a new telemetry scalar.
[String changes made/needed]: none

Comment 27

[Gerry Chang [:gchang]]

Hi :julian,
Do we have QA signoff for this? If not, can you find a QA to verify this? We have to get a signoff before starting the shield study.

Comment 28

[Julian Descottes [:jdescottes]]

We don't have a QA sign off at the moment. Sent an email Cornel Ionce and Iulia Cristescu, cc-ing them here.

For the shield study, I merged the three necessary patches to uplift in Bug 1418266, is this ok with you? They were quite small and I felt like uplifting all 3 of them separately would just be more work for no added value. If there's any issue with the feature, the patch can still be backed out as a single unit.

Comment 29

[Gerry Chang [:gchang]]

Bug 1418266 was uplift to Beta58.

Comment 30

[François Marier [:francois]]

(In reply to François Marier [:francois] from comment #14)
> Comment on attachment 8927403 [details]
> request-bug1408339.md
>
> 7) Is the data collection covered by the existing Firefox privacy notice?
> 
> No.

For the record, that was a typo. I meant "yes, it's covered by the existing privacy notice."