fix(3741): refactor remote-feature-flag controller with generateDeterministicRandomNumber and getMetaMetricsId #5097
Conversation
DDDDDanica
force-pushed
the
fix/3742-threshold
branch
3 times, most recently
from
41f11dc to
24571f9
Compare
DDDDDanica
force-pushed
the
fix/3742-threshold
branch
from
24571f9 to
6e48d4b
Compare
| MOBILE_MIN: '00000000-0000-0000-0000-000000000000', | ||
| MOBILE_MAX: 'ffffffff-ffff-ffff-ffff-ffffffffffff', |
There was a problem hiding this comment.
| MOBILE_MIN: '00000000-0000-0000-0000-000000000000', | |
| MOBILE_MAX: 'ffffffff-ffff-ffff-ffff-ffffffffffff', | |
| MOBILE_MIN: '00000000-0000-4000-0000-000000000000', | |
| MOBILE_MAX: 'ffffffff-ffff-4fff-ffff-ffffffffffff', |
There was a problem hiding this comment.
good catch ! resolved: 035745f
| const metaMetricsIdResult = this.#getMetaMetricsId(); | ||
| const metaMetricsId = | ||
| (await this.#getMetaMetricsId) || generateFallbackMetaMetricsId(); | ||
| const thresholdValue = generateDeterministicRandomNumber(metaMetricsId); | ||
| metaMetricsIdResult instanceof Promise | ||
| ? await metaMetricsIdResult | ||
| : metaMetricsIdResult; | ||
|
|
||
| const clientType = this.#clientConfigApiService.getClient(); | ||
| const thresholdValue = generateDeterministicRandomNumber( | ||
| clientType, | ||
| metaMetricsId, | ||
| ); |
There was a problem hiding this comment.
Q: i don't really understand what you did here.
Can you elaborate more on this part please? Is it only to force the return type not to be optional or undefined as the MetaMetrics getMetaMetricsId always return a string even if the signature says it can return undefined?
What would you think about modifying the signature of MetaMetrics.getMetaMetricsId to drop undefined?
There was a problem hiding this comment.
Yes definitely! We no longer handle undefined values - the signature doesn't actually include undefined. It's about handling both synchronous and asynchronous returns in a type-safe way.
About modifying MetaMetrics.getMetaMetricsId - it is defined in client side while mobile is async and extension is sync, and we will rely on client side to always give a value back 🙏🏻
| @@ -59,7 +59,7 @@ function createController( | |||
| state: Partial<RemoteFeatureFlagControllerState>; | |||
| clientConfigApiService: AbstractClientConfigApiService; | |||
| disabled: boolean; | |||
| getMetaMetricsId: Promise<string | undefined>; | |||
| getMetaMetricsId: () => Promise<string> | string; | |||
There was a problem hiding this comment.
Note that this function is synchronous on both extension and mobile. It's typed as asynchronous on mobile by mistake, but it's not.
There was a problem hiding this comment.
Oh that's good to know.
@joaoloureirop could you make adjustments in your side of implementation when initializing the controller to adapt to synchronous pattern?
changes can be found: 15428da
There was a problem hiding this comment.
Thanks Mark & Danica. I'll update mobile PR
| * Gets the client type configured for this service. | ||
| * @returns The configured ClientType | ||
| */ | ||
| public getClient(): ClientType { |
There was a problem hiding this comment.
this one is to get the client type we passed in the service configuration. Then when we can decide which algorithm we use inside generateDeterministicRandomNumber
| const chr = char.charCodeAt(0); | ||
| return ((acc << BITS_PER_CHAR) - acc + chr) & maxValue; | ||
| }, 0); | ||
| return hash / maxValue; |
There was a problem hiding this comment.
The "max value" here should be the maximum possible hash value, not the maximum possible integer size. If the max here doesn't match the maximum hash size, then our distribution will be skewed.
There was a problem hiding this comment.
It's really important that we guarantee a uniform distribution, so that a 1% rollout actually results in 1% of users having a feature enabled (rather than 10% or 99% or 0.001%). I'm requesting changes mainly because this doesn't yet seem guaranteed.
Perhaps we can split this PR in two though? The getMetraMetricsId related changes are totally distinct, and much simpler.
DDDDDanica
force-pushed
the
fix/3742-threshold
branch
from
15428da to
3ff22dc
Compare
Explanation
Changes introduced in this PR including:
metaMetricsIdand rely on client side always returning a valuegenerateDeterministicRandomNumberto handle both mobile(uuidv4) and extension(hex) sidegetMetaMetricsIdto only handle synchronous (extension) functionReferences
Address feedback about
Changelog
@metamask/remote-feature-flag-controllergetMetaMetricsIdto handle both synchronous functiongenerateDeterministicRandomNumberto handle both mobile(uuidv4) and extension(hex) sidemetaMetricsIdChecklist