fetch+run shield studies soon after first startup

RESOLVED FIXED

Status

Shield
Add-on
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 months ago
Currently the shield recipe client sets its own timer which runs every 24 hours. This precludes downloading+running recipes in early startup (such as onboarding and retention-related experiments).

This work is tracked in https://github.com/mozilla/normandy/pull/895

Since this runs early in startup and only on the first run, we can't rely on the gofaster update mechanism - only Firefox versions that ship with this feature will be able to take advantage of it.
(Assignee)

Updated

3 months ago
Summary: fetch+run shield studies soon after startup → fetch+run shield studies soon after first startup
(Assignee)

Updated

3 months ago
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 2

3 months ago
This patch is the same as https://github.com/mozilla/normandy/pull/895, with a minor change to one of the tests so it will apply to both mozilla-central and mozilla-beta repos.

Comment 3

3 months ago
mozreview-review
Comment on attachment 8890442 [details]
Bug 1383338 - fetch and run shield studies soon after UI startup

https://reviewboard.mozilla.org/r/161560/#review166872

Can you update the version number in install.rdf.in to 55.1? That will compare greater than what is there now, so it updates correctly, but will compare less than future versions, especially 61 that I'm working on in bug 1384652.
Attachment #8890442 - Flags: review?(mcooper) → review-
Comment hidden (mozreview-request)

Comment 5

3 months ago
mozreview-review
Comment on attachment 8890442 [details]
Bug 1383338 - fetch and run shield studies soon after UI startup

https://reviewboard.mozilla.org/r/161560/#review166896
Attachment #8890442 - Flags: review?(mcooper) → review+
Comment hidden (mozreview-request)
Comment on attachment 8890442 [details]
Bug 1383338 - fetch and run shield studies soon after UI startup

https://reviewboard.mozilla.org/r/161560/#review166916

rs=me
Attachment #8890442 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 8

3 months ago
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02874c4ecef4
fetch and run shield studies soon after UI startup r=Gijs,mythmon
(Assignee)

Comment 9

3 months ago
Comment on attachment 8890442 [details]
Bug 1383338 - fetch and run shield studies soon after UI startup

Approval Request Comment
[Feature/Bug causing the regression]: bug 1383338
[User impact if declined]: inability to run retention tests for first-time users
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, please contact shield team (#normandy in IRC) to coordinate
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: runs once on first run only, easy to test
[String changes made/needed]: none
Attachment #8890442 - Flags: approval-mozilla-beta?

Comment 10

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/02874c4ecef4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Comment on attachment 8890442 [details]
Bug 1383338 - fetch and run shield studies soon after UI startup

run shield client on first run, beta55+

should be in 55.0b13
Attachment #8890442 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox55: --- → affected

Comment 12

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/bc34fc74f1e1
status-firefox55: affected → fixed
this seems to have introduced a large improvement in fileIO:
== Change summary for alert #8376 (as of July 26 2017 21:05 UTC) ==

Improvements:

 95%  tp5n nonmain_startup_fileio windows7-32 opt e10s     66,880.17 -> 3,217.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8376

this is not on the main thread, are we pushing much fileIO after startup completely?
(Assignee)

Comment 14

3 months ago
(In reply to Joel Maher ( :jmaher) (UTC-9) (PTO: back August 2nd) from comment #13)
> this seems to have introduced a large improvement in fileIO:
> == Change summary for alert #8376 (as of July 26 2017 21:05 UTC) ==
> 
> Improvements:
> 
>  95%  tp5n nonmain_startup_fileio windows7-32 opt e10s     66,880.17 ->
> 3,217.00
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=8376
> 
> this is not on the main thread, are we pushing much fileIO after startup
> completely?

That's interesting. I did not expect that but we are indeed waiting until after UI startup to do anything now. I wasn't aware that shield was doing much IO before startup before this change, though...
(Assignee)

Updated

3 months ago
Depends on: 1385177
Could you or someone please submit a request on pi-requests@mozilla.com with the details needed so we can test this?
Tried on #normandy but got no answer there.

Thanks!
Flags: needinfo?(rhelmer)
(Assignee)

Comment 16

3 months ago
(In reply to Cornel Ionce [:cornel_ionce], Desktop Release QA from comment #15)
> Could you or someone please submit a request on pi-requests@mozilla.com with
> the details needed so we can test this?
> Tried on #normandy but got no answer there.
> 
> Thanks!

Osmose, can you help with this? I'm thinking maybe we provide a test URL and instructions for QA to ensure that first-run works, that only filtered recipes run, etc.

Also note that we need to backport bug 1385177 (disable self-support detection in shield) before this can be effectively tested, that should land momentarily.
Flags: needinfo?(rhelmer) → needinfo?(mkelly)
I'm working on getting a Beta build on trypush for testing, but found a bug.

RecipeRunner.run is async, but is called synchronously from init(). By the time the recipes are executed, the first run pref has already been set to false, and the normandy.isFirstRun filter fails on the first run.
Flags: needinfo?(mkelly)
Status: RESOLVED → REOPENED
Flags: needinfo?(rhelmer)
Resolution: FIXED → ---
(Assignee)

Comment 18

3 months ago
About to push a fix for this.
Flags: needinfo?(rhelmer)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8890442 - Attachment is obsolete: true

Comment 20

3 months ago
mozreview-review
Comment on attachment 8891498 [details]
Bug 1383338 - await shield recipe runner so recipes have a chance to see if this is first run

https://reviewboard.mozilla.org/r/162618/#review167974

This fixes the bug I observed in my test build. Thanks!
Attachment #8891498 - Flags: review+
Comment hidden (mozreview-request)
(Assignee)

Comment 22

3 months ago
Comment on attachment 8891498 [details]
Bug 1383338 - await shield recipe runner so recipes have a chance to see if this is first run

Approval Request Comment
[Feature/Bug causing the regression]: bug 1383338 (followup)
[User impact if declined]: inability to run retention tests for first-time users
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, please contact shield team (#normandy in IRC) to coordinate
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: runs once on first run only, easy to test
[String changes made/needed]: none
Attachment #8891498 - Flags: approval-mozilla-beta?

Comment 23

3 months ago
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d776548e6846
await shield recipe runner so recipes have a chance to see if this is first run r=mkelly
QA request with testing steps has been sent to pi-request.

Comment 25

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d776548e6846
Status: REOPENED → RESOLVED
Last Resolved: 3 months ago3 months ago
Resolution: --- → FIXED
status-firefox55: fixed → affected
Comment on attachment 8891498 [details]
Bug 1383338 - await shield recipe runner so recipes have a chance to see if this is first run

followup fix needed for 55 rc1
Attachment #8891498 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 27

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/2ccefa352a99
status-firefox55: affected → fixed
Depends on: 1385915
Depends on: 1390824
No longer depends on: 1390824
You need to log in before you can comment on or make changes to this bug.