Closed
Bug 1502111
Opened 6 years ago
Closed 6 years ago
[Shield] Study: IndexedDB Storage Initialization Errors, beta 64, nightly 65
Categories
(WebExtensions :: Storage, enhancement, P1)
WebExtensions
Storage
Tracking
(firefox64+ fixed, firefox65+ fixed, firefox66+ fixed)
People
(Reporter: rpl, Assigned: rpl)
References
Details
User Story
Attachments
(8 files)
(deleted),
text/x-github-pull-request
|
rhelmer
:
review+
|
Details |
(deleted),
application/zip
|
rhelmer
:
review+
|
Details |
(deleted),
text/plain
|
chutten
:
review+
|
Details |
(deleted),
application/x-xpinstall
|
Details | |
(deleted),
text/x-github-pull-request
|
rhelmer
:
review+
|
Details |
(deleted),
application/zip
|
rhelmer
:
review+
|
Details |
(deleted),
application/x-xpinstall
|
Details | |
(deleted),
application/x-xpinstall
|
Details |
In Bug 1474562 we have enabled on Nightly the storage.local API backend to migrate from JSON to indexedDB.
During the time the migration has been enabled on Nightly, we have been looking into the telemetry related to the "backend migration results" that we collect and we noticed that for some users the migration is not currently able to complete successfully because of errors raised from IndexedDB when we are opening a database (but before any data is actually stored in the newly opened db).
There errors seems likely related to some known QuotaManager issues (e.g. https://bugzilla.mozilla.org/buglist.cgi?bug_id=944918%2C1423917).
In order to verify the incidence of these storage initialization issues on non-Nightly channels (where we can't turn on the actual storage.local data migration yet), we need to write a shield study extension which tries to open an IndexedDB database under the same conditions of the storage.local IDB data migrations, and then collect and reports the results.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
User Story: (updated)
Priority: -- → P1
Assignee | ||
Comment 1•6 years ago
|
||
Hi Robert,
the attached link is related to a github pull request which contains the current version of the sources of this shield study addon, so that you may be able to more easily add inline comment if/when needed as part of the review (and I'm going to be able to add any additional change that may arise from the review by pushing in that branch).
I'm also going to attach separately the extension xpi or zip archive.
Attachment #9020092 -
Flags: review?(rhelmer)
Assignee | ||
Comment 2•6 years ago
|
||
The built extension in zip archive format (created from the github repo using web-ext build).
Attachment #9020095 -
Flags: review?(rhelmer)
Updated•6 years ago
|
Attachment #9020095 -
Flags: review?(rhelmer) → review+
Updated•6 years ago
|
Attachment #9020092 -
Flags: review?(rhelmer) → review+
Assignee | ||
Updated•6 years ago
|
User Story: (updated)
Assignee | ||
Comment 3•6 years ago
|
||
Hi :chutten,
The attached markdown contains a data reviewed request related to the "shield-study-addon" telemetry ping recorded by the attached shield study webextension.
The telemetry collected by this study extension are actually a subset of the ones described in the Bug 1470213 data review request (but I felt that an explicit and separate data review request was still needed even if the data is the same, as we are recording them into a different telemetry ping).
The following are the parts of the study extension that are collecting these data and then recording them in the "shield-study-addon" ping (in case having a look to actual implementation may be helpful to the data review):
- https://github.com/rpl/storage-init-errors-shield-study/blob/eda15697310bbd1c59088ca10abf6243ae180e2e/src/privileged/testIDBOpen/api.js#L54-L72
- https://github.com/rpl/storage-init-errors-shield-study/blob/eda15697310bbd1c59088ca10abf6243ae180e2e/src/background.js#L114-L128
In the study extension's github repository I also included a TELEMETRY.md file with some details about the telemetry recorded by the extension:
- https://github.com/rpl/storage-init-errors-shield-study/blob/master/docs/TELEMETRY.md
Attachment #9023272 -
Flags: review?(chutten)
Comment 4•6 years ago
|
||
Comment on attachment 9023272 [details]
storage-init-error-shield-study-DataReviewRequest.md
Preliminary notes:
Thank you for requesting review. Changing the mechanism of collection is indeed criteria for review, so you were right in doing so. (Though, in this case the shield-study-addon pings don't look to contain anything of sufficiently-different character to be treated differently than measurements on the main or event ping, but eh :)
Thank you also for the links to prior reviews and code.
Interestingly, your questions do not require that you know the name of the failure, just its incidence rates. I presume you wish to see if the character of the failure rates on beta match that of Nightly and that's why you want directly-comparable data.
DATA COLLECTION REVIEW RESPONSE:
Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?
Yes. See TELEMETRY.md: https://github.com/rpl/storage-init-errors-shield-study/blob/master/docs/TELEMETRY.md#shield-study-addon-pings-specific-to-this-study
Is there a control mechanism that allows the user to turn the data collection on and off?
Yes. Standard study and Telemetry mechanisms apply.
If the request is for permanent data collection, is there someone who will monitor the data over time?
N/A, limited to study boundaries.
Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **
Category 1, Technical.
Is the data collection request for default-on or default-off?
Default on for specific channels and versions.
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)?
Nothing new, but it's worth noting once more that this collection contains the names of DOMExceptions, truncated to 80 characters.
Is the data collection covered by the existing Firefox privacy notice?
Yes.
Does there need to be a check-in in the future to determine whether to renew the data?
No, this study will run its course.
---
Result: datareview+
Attachment #9023272 -
Flags: review?(chutten) → review+
Assignee | ||
Comment 5•6 years ago
|
||
Comment on attachment 9020095 [details]
storage-init-errors-shield-study-1.0.0.zip
Hi Micheal,
Could you sign storage-init-errors-shield-study-1.0.0.zip so that it can be verified on a Beta builds from QA?
Thanks in advance for your help.
Flags: needinfo?(mcooper)
Comment 6•6 years ago
|
||
Comment 7•6 years ago
|
||
I've signed the add-on for QA testing. This add-on is signed with the key for development, and as such can only be used on unbranded builds with 'xpinstall.signatures.dev-root' set to true. QA has been informed of this process change.
Once the add-on has QA signoff and Firefox peer review, it can receive final signing for deployment.
Flags: needinfo?(mcooper)
Assignee | ||
Comment 8•6 years ago
|
||
Hi Krupa,
follows the QA request related to the Shield Study extension xpi (attachment 9023748 [details], which is signed for QA testing with the key for development, as mentioned in comment 7).
Let me know if there are any additional details that are needed and missing in this QA request.
-------
QA Request
We are the Addons team, and we would would like to request testing help with the "storage-init-errors Shield Study" (storage-init-errors-shield-study@shield.mozilla.org)
Project name: storage-init-errors-shield-study
Description: A tiny Shield Study which checks that QuotaManagerService/IndexedDB are initializing successfully.
Study owner: Luca Greco (lgreco@mozilla.com)
Priority: normal
Tracking Bug: 1502111
https://bugzilla.mozilla.org/1502111
PHD document: https://docs.google.com/document/d/1oAJsjFDGov1FiovzkVA1OPM96KgmqL5bh2GkBM99VUQ
Channels: Beta and Nightly
Platform: Firefox Desktop (OSX/Windows/Linux)
Locales: any
Complexity: Simple one-time passive data collection effort (single branch). Telemetry sent using shield-study-addon Telemetry ping.
Testing instructions: https://github.com/rpl/storage-init-errors-shield-study/blob/master/docs/TESTPLAN.md
Assignee | ||
Comment 9•6 years ago
|
||
Needinfo for Matt related to the PHD https://docs.google.com/document/d/1oAJsjFDGov1FiovzkVA1OPM96KgmqL5bh2GkBM99VUQ,
to double-check if there is any information missing or not yet complete.
Flags: needinfo?(mgrimes)
Assignee | ||
Comment 10•6 years ago
|
||
Needinfo Krupa on "QA request" added in comment 8.
Flags: needinfo?(kraj)
Assignee | ||
Comment 11•6 years ago
|
||
I'm clearing the needinfo assigned to Krupa, as the QA verification has been started.
Flags: needinfo?(kraj)
Assignee | ||
Comment 12•6 years ago
|
||
Hi Ben and Matt,
during the study QA verification, QA has pointed out that this shield study extension is currently auto-uninstalling itself as soon as it has been able to fully run once.
This is currently a behavior I implemented on purpose, but I wanted to double-check with you if it may make sense (from a data analysis point of view, as well as from a "shield studies conventions" point of view) to keep the shield study installed and enabled until it is expired, so that it may have the chance to try to run the IndexedDB test (and report back the result) more than once (e.g. one reason that currently comes to my mind could be "to be able to detect if some of the QuotaManagerService/IndexedDb errors detected were transient or consistent across Firefox restarts").
Flags: needinfo?(bmiroglio)
Comment 13•6 years ago
|
||
Luca, if it's not too much trouble, it is preferred to keep the extension enabled for the duration of the experiment.
Flags: needinfo?(bmiroglio)
Assignee | ||
Comment 14•6 years ago
|
||
Hi Robert,
Do you mind to review this github pull request which includes some small changes to the shield study addon code?
(it removes a couple of lines from the background script to leave the study installed until it expires, which is related to comment 13, and it also adds the "hidden" property to the manifest.json file).
Attachment #9025430 -
Flags: review?(rhelmer)
Assignee | ||
Comment 15•6 years ago
|
||
The built extension for v.1.0.1 in zip archive format (created from the github pull request using web-ext build).
Attachment #9025432 -
Flags: review?(rhelmer)
Assignee | ||
Comment 16•6 years ago
|
||
Comment on attachment 9025432 [details]
storage-init-errors-shield-study-1.0.1.zip
Hi Micheal,
Could you sign storage-init-errors-shield-study-1.0.1.zip with the key for development?
so that QA can verify it (while the github pull request which includes these small changes gets peer-reviewed).
Thanks in advance!
Flags: needinfo?(mcooper)
Updated•6 years ago
|
Attachment #9025430 -
Flags: review?(rhelmer) → review+
Updated•6 years ago
|
Attachment #9025432 -
Flags: review?(rhelmer) → review+
Comment 18•6 years ago
|
||
We have finished testing the "IndexedDB" experiment.
QA’s recommendation: GREEN - SHIP IT
Reasoning:
- We haven’t found any issues during testing.
Testing Summary:
- Full test suite: https://testrail.stage.mozaws.net/index.php?/plans/view/13684
Tested Platforms:
- Windows 10 x64
- Windows 7 x64
- Mac 10.14
- Mac 10.13
- Ubuntu 14.04
- Arch Linux 4.16
Tested Firefox versions:
- Nightly 65
- Beta 64
Comment 19•6 years ago
|
||
Comment 20•6 years ago
|
||
Since this has received code review r+ and a green QA status, I've signed it with the prod key. It is now ready to ship.
Comment 21•6 years ago
|
||
[Tracking Requested - why for this release]: Study will run in both beta and nightly.
status-firefox64:
--- → affected
status-firefox65:
--- → affected
tracking-firefox64:
--- → ?
tracking-firefox65:
--- → ?
Summary: [Shield] Study: IndexedDB Storage Initialization Errors → [Shield] Study: IndexedDB Storage Initialization Errors, beta 64, nightly 65
Comment 22•6 years ago
|
||
Shell or Luca, please request legal review for this study, here: https://bugzilla.mozilla.org/enter_bug.cgi?product=Legal&component=Product%20-%20data
Flags: needinfo?(sescalante)
Flags: needinfo?(lgreco)
Updated•6 years ago
|
Comment 24•6 years ago
|
||
Given the timing wouldn't this be beta 65 / nightly 66 at this point?
Comment 25•6 years ago
|
||
r+ science
So the science looks good enough. Per the comment above (c8) and studySetup.js this appears to be a single branch study to compare against our standard baseline. It should be noted that this has less scientific validity than a true control/variant a/b test which may be preferable in the future or other circumstances to lessen the potential for assignment/temporal asymmetries.
Working on the recipe. I'm going to target Beta 64+ and Nightly 65+ since we're in that awkward launch window.
(Clearing resolved NI's above)
Flags: needinfo?(mgrimes)
Flags: needinfo?(lgreco)
Comment 26•6 years ago
|
||
Ryan, the team would like to launch this study this week. Can we get your approval? Thanks.
Flags: needinfo?(ryanvm)
Updated•6 years ago
|
status-firefox66:
--- → affected
tracking-firefox66:
--- → +
Comment 28•6 years ago
|
||
a=me to run on Beta64+ and Nightly65+
Flags: needinfo?(ryanvm)
Flags: needinfo?(jcristau)
Comment 29•6 years ago
|
||
Forgot to update, we're live on this since yesterday.
Comment 30•6 years ago
|
||
Going to call this fixed for 64 so it drops off my radar for release, as a beta/nightly study.
Comment 31•6 years ago
|
||
Josh, please pause enrollment on this study, but leave it running, per Ben's email.
Flags: needinfo?(jgaunt)
Comment 33•6 years ago
|
||
Recipes 655 and 656 have been disabled.
Updated•6 years ago
|
Assignee | ||
Comment 34•6 years ago
|
||
Marking "resolved,fixed", as this shield study has been completed.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•