Closed
Bug 1408339
Opened 7 years ago
Closed 7 years ago
[dt-onboarding] Prepare for a shield study about the devtools onboarding flow on beta
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 59
People
(Reporter: jdescottes, Assigned: jdescottes)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
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
Assignee | ||
Updated•7 years ago
|
Blocks: dt-onboarding
Assignee | ||
Updated•7 years ago
|
No longer blocks: dt-addon-uishim
Assignee | ||
Comment 1•7 years ago
|
||
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.
Updated•7 years ago
|
Flags: needinfo?(hkirschner)
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
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)
Attachment #8925521 -
Flags: review?(poirot.alex) → review+
Comment 12•7 years ago
|
||
mozreview-review |
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?
Attachment #8926456 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
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.
Attachment #8927403 -
Flags: review?(francois) → review+
Assignee | ||
Comment 15•7 years ago
|
||
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.
Attachment #8927403 -
Attachment is obsolete: true
Attachment #8928188 -
Flags: review?(francois)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
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+.
Attachment #8928188 -
Flags: review?(francois) → review+
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(hkirschner)
Comment 21•7 years ago
|
||
Pushed by jdescottes@mozilla.com: 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
Assignee | ||
Comment 22•7 years ago
|
||
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?
Flags: needinfo?(hkirschner)
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0fb5208715ea https://hg.mozilla.org/mozilla-central/rev/dfdc0642a212
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Comment 24•7 years ago
|
||
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.
Flags: needinfo?(hkirschner)
Assignee | ||
Comment 25•7 years ago
|
||
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
Attachment #8925521 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 26•7 years ago
|
||
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
Attachment #8926456 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•7 years ago
|
Attachment #8926456 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•7 years ago
|
Attachment #8925521 -
Flags: approval-mozilla-beta?
Comment 27•7 years ago
|
||
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.
Flags: needinfo?(jdescottes)
Assignee | ||
Comment 28•7 years ago
|
||
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.
Flags: needinfo?(jdescottes) → needinfo?(gchang)
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 30•6 years ago
|
||
(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."
status-firefox59:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•