Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Support staged rollout of migration to Rust Crypto #12184

Merged
merged 2 commits into from
Jan 31, 2024

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Jan 29, 2024

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

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

@BillCarsonFr BillCarsonFr added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Jan 29, 2024
@BillCarsonFr BillCarsonFr marked this pull request as ready for review January 29, 2024 20:50
@BillCarsonFr BillCarsonFr requested review from a team as code owners January 29, 2024 20:51
Copy link
Member

@richvdh richvdh left a 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)) {
Copy link
Member

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.

Copy link
Member Author

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?)

Copy link
Member

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.

Copy link
Member

@andybalaam andybalaam left a 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.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be !== ?

Copy link
Member Author

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.

@t3chguy
Copy link
Member

t3chguy commented Jan 30, 2024

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?

@BillCarsonFr
Copy link
Member Author

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.
This could have been a nice intermediate step but we decided skip it and directly rollout using our own flag. (FTR the ios rollout was done like that, first rollout of posthog users then all users.

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

@t3chguy
Copy link
Member

t3chguy commented Jan 30, 2024

Using posthog phased rollout would only allow us to rollout users that did opt-in in the analytics.

I don't believe this is the case. The features function of Posthog should work regardless of anon/pseudon/opt-out.

@BillCarsonFr
Copy link
Member Author

Using posthog phased rollout would only allow us to rollout users that did opt-in in the analytics.

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.

@t3chguy
Copy link
Member

t3chguy commented Jan 30, 2024

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

@pmaier1
Copy link

pmaier1 commented Jan 30, 2024

@pmaier1 can you validate

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.

@BillCarsonFr BillCarsonFr removed the request for review from t3chguy January 31, 2024 14:11
@robintown robintown removed their request for review January 31, 2024 15:09
@BillCarsonFr BillCarsonFr added this pull request to the merge queue Jan 31, 2024
Merged via the queue into develop with commit a5f9df5 Jan 31, 2024
22 checks passed
@BillCarsonFr BillCarsonFr deleted the valere/staged_rollout_migration_rust branch January 31, 2024 16:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Element-R: Support staged rollout of migration to Rust Crypto
6 participants