feat: Make a/b tests independent #7511
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.test.ts
Show resolved
Hide resolved
packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| const seed = metaMetricsId + featureFlagName; | ||
| const hashBuffer = sha256(seed); |
There was a problem hiding this comment.
If async is OK here we should consider using crypto.subtle.digest
There was a problem hiding this comment.
@FrederikBolding can you expand on that ?
I've used sha256 from noble/hashes to keep consistency with what I've noticed in other packages.
There was a problem hiding this comment.
It doesn't seem to be published/exported yet in the current version of metamask/utils
There was a problem hiding this comment.
It was published earlier today!
There was a problem hiding this comment.
SHA-256 using native code will perform better across mobile and extension! We automatically fall back to noble if unavailable
packages/remote-feature-flag-controller/src/utils/user-segmentation-utils.ts
Show resolved
Hide resolved
|
LGTM! |
|
Why was |
Sorry @Gudahtt, reflex from the mobile repo |
|
|
||
| if (Array.isArray(processedValue) && thresholdValue) { | ||
| if (Array.isArray(processedValue)) { | ||
| const deterministicSeed = createDeterministicSeed( |
There was a problem hiding this comment.
This is a slow cryptographic operation and it runs once per flag, but we don't need to run this for all flags. It's unnecessary unless the flag has a threshold.
Perhaps we can calculate this later, after we've determined that there is a threshold on this specific flag? That way it can be skipped for non-threshold flags, which is the majority of them.
There was a problem hiding this comment.
It's also not ideal to run this every time flags get updated 🤔.
| * | ||
| * @param metaMetricsId - The user's MetaMetrics ID (must be non-empty) | ||
| * @param featureFlagName - The feature flag name to create unique seed per flag | ||
| * @returns A hex string with '0x' prefix suitable for generateDeterministicRandomNumber |
There was a problem hiding this comment.
The 0x-prefixed input for generateDeterministicRandomNumber is meant to support legacy metrics IDs. We had hoped to remove that functionality when dropping support for those legacy IDs. Migrating to a format we planned to deprecate is not ideal.
There was a problem hiding this comment.
The function generateDeterministicRandomNumber attempts to deterministically generate a number between 0 and 1 cheaply. That's why we didn't use a hash function.
With a hash function, it's relatively easy to ensure that the output is uniform in distribution across different inputs (we can use a hash function known to have this property, we don't need to be careful to maintain this guarantee ourselves). It's quite a bit simpler than the approach taken now in generateDeterministicRandomNumber.
If we're using a hash function anyway, there is no need to keep generateDeterministicRandomNumber around, it serves no purpose. We can derive a number between 0 and 1 directly from the hash function. I don't think it makes much sense to keep it around but pass an artificially-generated legacy metrics ID to it.
I'd recommend reconsidering this approach. Either we should attempt to find a cheaper way to derive the threshold without using a hash function, or align on that as a solution and drop the unnecessary extra code.
|
After thinking about this further, a hash function is the only viable approach I can think of. We could hash |
Explanation
Right now, only the ID is used as a seed to determine what bucket the user falls into in an A/B test.
Due to this, if there’s more than 1 A/B test configured, they will never be independent of each other
Solution:
Define the distribution using profile/metametrics ID + A/B test ID as a seed instead of just profile/metametrics ID
References
Checklist
Note
Uses a per-flag deterministic seed (metaMetricsId + flag name) for threshold selection to decouple A/B assignments across flags.
createDeterministicSeed(metaMetricsId, remoteFeatureFlagName)→generateDeterministicRandomNumber(seed).createDeterministicSeed(SHA-256 via@noble/hashes), lowercases inputs, returns0x-prefixed 32-byte hex; throws on empty ID.createDeterministicSeed(format, casing, errors).@noble/hashes.Written by Cursor Bugbot for commit 526f5fe. This will update automatically on new commits. Configure here.