Bug 1442042 - gradual roll-out of TLS 1.3 to release channel #96
Conversation
| } | ||
| } | ||
|
|
||
| async function getRandomnessSeed() { |
There was a problem hiding this comment.
nit: this doesn't need to be async (in fact some would say you could ditch the function altogether)
There was a problem hiding this comment.
Hm, yeah I think the only benefit to having the function vs. calling ClientID.getClientID() directly is to make it clear this is the seed... I don't think it's a big deal though, I will just remove the function.
| return ClientID.getClientID(); | ||
| } | ||
|
|
||
| async function generateVariate(seed, label) { |
There was a problem hiding this comment.
It does const hash = await hasher.digest(...) below
| // initially roll out to 10%, we want to control this on the client rather than | ||
| // depending on server-side throttling, as throttling cannot account for any | ||
| // other concurrent gradual roll-outs. | ||
| const ENABLE_PROB = 0.9; |
There was a problem hiding this comment.
talked on IRC but either this should be 0.1 or it should be DISABLE_PROB and the logic below should change
|
|
||
| let {classes: Cc, interfaces: Ci, utils: Cu} = Components; | ||
|
|
||
| Cu.import("resource://gre/modules/Preferences.jsm"); |
There was a problem hiding this comment.
probably easier to just use Services.prefs instead
|
|
||
| debug("Installing"); | ||
|
|
||
| let variate = await generateVariate(await getRandomnessSeed(), data.id); |
There was a problem hiding this comment.
Is there any point passing in data.id here? why not just hash the client id?
There was a problem hiding this comment.
Hey @ekr did you have a reason for this ^?
There was a problem hiding this comment.
This way if you run multiple experiments based on this template they independently select their cohorts rather than having the same people get every experiment
rhelmer
force-pushed
the
bug1442042-tls-1.3-gradual-rollout
branch
2 times, most recently
from
8d9a43c
to
d5feba2
Compare
rhelmer
force-pushed
the
bug1442042-tls-1.3-gradual-rollout
branch
from
d5feba2
to
a2cca68
Compare
|
@Standard8 could you please squash+merge this if it lgty? Thanks! |
|
@aswan, pauljt in https://bugzilla.mozilla.org/show_bug.cgi?id=1442042 points out that we really don't need to do anything on each startup... since the seed is always the same (clientID + addonID), I wouldn't expect the decision to change unless one of these did, which would require an upgrade or install of the add-on. So, I think we should be able to just move all the work to the |
rhelmer
force-pushed
the
bug1442042-tls-1.3-gradual-rollout
branch
from
24acad1
to
c790500
Compare
|
@aswan ugh, scratch that - @ekr just reminded me that we're using the default branch here so that the pref will revert if the add-on doesn't start / is removed for any reason. I've reverted this branch to be the same as the XPI in https://bugzilla.mozilla.org/show_bug.cgi?id=1442042, so I think we're good to go here. |
No description provided.