feat(3742): Feature Flag Values with Scope Based on threshold

[DDDDDanica]

Explanation

This PR added ability for @metamask/remote-feature-flag-controller to take in a new param metametricsId and applied to the featureFlag processing. Furthermore, a threshold will be generated using a simple function, which generates a deterministic random group number between 0 and 1 from metametricsId. If encounters a flag from api with scope as type: "threshold", controller will drill down to return the value that's matching the threshold generated based on metametricsId.
Note: metametricsId will always be provided from client, a random SHA-256 hash will be used as fallback

References

Fixes https://github.com/MetaMask/MetaMask-planning/issues/3742

Changelog

@metamask/remote-feature-flag-controller

• ADDED: Added support for threshold-based feature flag scoping

Checklist

• I've updated the test suite for new or updated code as appropriate
• I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
• I've highlighted breaking changes using the "BREAKING" category above as appropriate
• I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

[NicolasMassart]

The only thing to change is Metametrics is actually a UUIDv4 and hot a hex hash

[NicolasMassart]

The format we use must be UUIDv4.
Easy to spot, the UUIDv4 must have it 13th digit to be a 4:
6D796265-7374-(4)953-6D65-74616D61736B

[NicolasMassart]

these are not valid UUIDv4.

• Check 123e4567-e89b-12d3-a456-426614174000 is UUIDv1
• Check 987fcdeb-51a2-3c4b-9876-543210fedcba is UUIDv3
• Check a1b2c3d4-e5f6-7890-abcd-ef1234567890 is UUIDv7
• Check f9e8d7c6-b5a4-3210-9876-543210fedcba is UUIDv3

[NicolasMassart]

Not a valid UUIDv4, this is UUIDv3

[DDDDDanica]

Good catch ! addressed 93d8351

[joaoloureirop]

Hi Danica.

Can we add a Promise value that will return the metametrics id?

Mobile controller initialization architecture makes it difficult to await for such value, it would help a lot having the option to pass a promise inside the controller.

I've tested with these changes.

diff --git a/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts b/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
index 37c8d64c..7ded1f5a 100644
--- a/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
+++ b/packages/remote-feature-flag-controller/src/remote-feature-flag-controller.ts
@@ -105,6 +105,8 @@ export class RemoteFeatureFlagController extends BaseController<
 
   #metaMetricsId?: string | undefined;
 
+  #getMetaMetricsId?: Promise<string | undefined>;
+
   /**
    * Constructs a new RemoteFeatureFlagController instance.
    *
@@ -115,6 +117,7 @@ export class RemoteFeatureFlagController extends BaseController<
    * @param options.fetchInterval - The interval in milliseconds before cached flags expire. Defaults to 1 day.
    * @param options.disabled - Determines if the controller should be disabled initially. Defaults to false.
    * @param options.metaMetricsId - Determines the threshold value for the feature flag to return
+   * @param options.getMetaMetricsId - Promise that will return MetaMetricsId
    */
   constructor({
     messenger,
@@ -123,11 +126,13 @@ export class RemoteFeatureFlagController extends BaseController<
     fetchInterval = DEFAULT_CACHE_DURATION,
     disabled = false,
     metaMetricsId,
+    getMetaMetricsId,
   }: {
     messenger: RemoteFeatureFlagControllerMessenger;
     state?: Partial<RemoteFeatureFlagControllerState>;
     clientConfigApiService: AbstractClientConfigApiService;
     metaMetricsId?: string | undefined;
+    getMetaMetricsId?: Promise<string | undefined>,
     fetchInterval?: number;
     disabled?: boolean;
   }) {
@@ -145,6 +150,7 @@ export class RemoteFeatureFlagController extends BaseController<
     this.#disabled = disabled;
     this.#clientConfigApiService = clientConfigApiService;
     this.#metaMetricsId = metaMetricsId;
+    this.#getMetaMetricsId = getMetaMetricsId;
   }
 
   /**
@@ -193,9 +199,9 @@ export class RemoteFeatureFlagController extends BaseController<
    * @param remoteFeatureFlags - The new feature flags to cache.
    * @private
    */
-  #updateCache(remoteFeatureFlags: FeatureFlags) {
+  async #updateCache(remoteFeatureFlags: FeatureFlags) {
     const processedRemoteFeatureFlags =
-      this.#processRemoteFeatureFlags(remoteFeatureFlags);
+      await this.#processRemoteFeatureFlags(remoteFeatureFlags);
     this.update(() => {
       return {
         remoteFeatureFlags: processedRemoteFeatureFlags,
@@ -204,10 +210,12 @@ export class RemoteFeatureFlagController extends BaseController<
     });
   }
 
-  #processRemoteFeatureFlags(remoteFeatureFlags: FeatureFlags): FeatureFlags {
+  async #processRemoteFeatureFlags(remoteFeatureFlags: FeatureFlags): Promise<FeatureFlags> {
     const processedRemoteFeatureFlags: FeatureFlags = {};
+
+    const metaMetricsId = this.#metaMetricsId ?? await this.#getMetaMetricsId;
     const thresholdValue = generateDeterministicRandomNumber(
-      this.#metaMetricsId || generateFallbackMetaMetricsId(),
+      metaMetricsId || generateFallbackMetaMetricsId(),
     );
 
     for (const [

Moblie PR with the promise value changes MetaMask/metamask-mobile@8769485

[gauthierpetetin]

based on metaMetricsId

[DDDDDanica]

🕵🏻‍♂️ good catch ! addressed 63a7274

[NicolasMassart]

Looks good to me unless we really want ot use the promise param instead of metametricsId value directly.
Leaves this decision between you and @joaoloureirop.
Nice job!

[DDDDDanica]

@joaoloureirop changed to a promise in 1f94695

[joaoloureirop]

Looks good!

[NicolasMassart]

Looks good to me.

[Gudahtt]

This is non-deterministic, and therefore unsuitable for determining whether a feature flag should be enabled or not

[Gudahtt]

I guess the best way of handling this would be to accept a callback for generating the metametrics ID? Ideally we'd use a messenger action, but that would be tricky because of the separate metrics implementations, so a callback should work instead.

[Gudahtt]

Or as you've just pointed out in the extension PR @DDDDDanica , we can rely on getMetaMetricsId to always return a value (i.e. to generate if it's missing). But we'll still want to remove this fallback if we're expecting the callback to always return a value, and we'll want to update the type of the callback

[DDDDDanica]

Discussed in slack thread, we will remove the callback and add this logic to client side if undefined 🙏🏻

[Gudahtt]

We should be careful here about the expected size of the metaMetricsId. If the number we're dividing by here isn't the max size of the ID, we'll get an uneven distribution.

i.e. if our target threshold is 10% and 0xffffffff is double the max hash, then this will result in only 5% of users getting the feature. But if 0xffffffff is 1/10 of the max hash, it would result in ~99% of users having the feature enabled.

Correct distribution here is dependent upon each of the constants we're using here being exactly correct. e.g. the 5 bit shift above, and the 0xffffffff here. Ideally we'd assert in code that the max here is truly the max ID (using the length of the ID)

[Gudahtt]

What's especially tricky is that the exact length/contents of the ID might differ between MetaMask versions or clients. Maybe not, but I haven't verified that. That could confuse things even more.

[Gudahtt]

Ideas for validating the distribution:

• Assertion on metametrics length, and derive constants from expected length
• Test cases for minimum and maximum ID (should get values 0 and 1 respectively )
• Validate that all production metametrics IDs in both clients are of the expected length

[DDDDDanica]

What's especially tricky is that the exact length/contents of the ID might differ between MetaMask versions or clients. Maybe not, but I haven't verified that. That could confuse things even more.

I had a discussion with @NicolasMassart on this topic, and we decided to go for uuidV4 on this one.

Currently for mobile side, metaMetricsId is uuidv4 format so we expect 36-character string, while in extension side is a hash hex with 66-characters string but it seems to be planned to migrate to uuidv4