-
-
Notifications
You must be signed in to change notification settings - Fork 827
Support staged rollout of migration to Rust Crypto #12184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
|
||
// If the default config is to use rust crypto, and the user is on legacy crypto, | ||
// we want to check if we should migrate the current user. | ||
if (!useRustCrypto && defaultUseRustCrypto && Number.isInteger(migrationPercent)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't bother with the check on defaultUseRustCrypto. It's fiddly to implement and I don't think it's hugely helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be strange that if feature_rust_crypto
is explicitly set to false
that existing login can be migrated? (particularly when the default for staged_rollout_percent
will be 100?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Counterpoint: it's strange that setting staged_rollout_percent does nothing unless you also remember to set feature_rust_crypto. But you make a good point. I have no strong feelings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks!
credentials, | ||
homeserver, | ||
}, workerInfo) => { | ||
test.skip(workerInfo.project.name === "Rust Crypto", "This test only works with Rust crypto."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be !==
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to test legacy to rust migration (feature_rust_crypto from false
to true
).
In the project Rust Crypto
, the app is started with feature_rust_crypto: true
But anyhow it doesn't matter much as we anyhow override the cryptoBackend
by intercepting http://localhost:8080/config.json*
, so it would test twice the same thing.
The Element Web team just had a chat and we'd prefer an approach built atop Posthog's phased rollout as it'd give us access to better analytics and give the Product team more control for similar rollouts and experiments. Is this something you'd be able to switch to? |
Using posthog phased rollout would only allow us to rollout users that did opt-in in the analytics. We also have in place monitoring that allows us to keep track of WebR penetration https://posthog.hss.element.io/dashboard/1196 @pmaier1 can you validate |
I don't believe this is the case. The features function of Posthog should work regardless of anon/pseudon/opt-out. |
From my understanding, a Pre-requesite for phased rollout is to identify users. And we don't do it unless at least opt'in for pseudonimous analytics is done. Also I think that when we identify, we store the id in account data so it's shared across device and this would migrate all devices of same users in same rollout. And it's not a property that we want. |
Posthog can also operate in anonymous mode and without you sending any data beyond that identification, this then isn't tracking, nor is it breaking anonymity, complying with our privacy policy and allowing the feature of features |
We have a very tight timeline and want to get this done. It won't be possible to change the staged roll-out approach in this time frame. While I agree it would be good to have some more data, I don't think it's an absolute necessity. Please, let's move on and get this out. |
Fixes element-hq/element-web#26869
Note regarding the quality gate: There is a playwright test that is covering the part that is uncovered by unit tests (MatrixClientPeg)
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.