Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(3741): refactor remote-feature-flag controller with generateDeterministicRandomNumber and getMetaMetricsId #5097

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,23 @@ describe('ClientConfigApiService', () => {
});
});

describe('getClient', () => {
it('returns configured client type', () => {
const clientConfigApiService = new ClientConfigApiService({
fetch: createMockFetch({}),
config: {
client: ClientType.Extension,
distribution: DistributionType.Main,
environment: EnvironmentType.Production,
},
});

expect(clientConfigApiService.getClient()).toStrictEqual(
ClientType.Extension,
);
});
});

describe('circuit breaker', () => {
it('opens the circuit breaker after consecutive failures', async () => {
const mockFetch = createMockFetch({ error: networkError });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,14 @@ export class ClientConfigApiService {
};
}

/**
* Gets the client type configured for this service.
* @returns The configured ClientType
*/
public getClient(): ClientType {
Copy link
Member

Choose a reason for hiding this comment

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

What is this method for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return this.#client;
}

/**
* Flattens an array of feature flag objects into a single feature flags object.
* @param responseData - Array of objects containing feature flag key-value pairs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import type {
RemoteFeatureFlagControllerStateChangeEvent,
} from './remote-feature-flag-controller';
import type { FeatureFlags } from './remote-feature-flag-controller-types';
import * as userSegmentationUtils from './utils/user-segmentation-utils';
import { ClientType } from './remote-feature-flag-controller-types';

const MOCK_FLAGS: FeatureFlags = {
feature1: true,
Expand Down Expand Up @@ -42,7 +42,6 @@ const MOCK_FLAGS_WITH_THRESHOLD = {
};

const MOCK_METRICS_ID = 'f9e8d7c6-b5a4-4210-9876-543210fedcba';
const MOCK_METRICS_ID_2 = '987fcdeb-51a2-4c4b-9876-543210fedcba';

/**
* Creates a controller instance with default parameters for testing
Expand All @@ -51,6 +50,7 @@ const MOCK_METRICS_ID_2 = '987fcdeb-51a2-4c4b-9876-543210fedcba';
* @param options.state - The initial controller state
* @param options.clientConfigApiService - The client config API service instance
* @param options.disabled - Whether the controller should start disabled
* @param options.getMetaMetricsId - The function to get the metaMetricsId
* @returns A configured RemoteFeatureFlagController instance
*/
function createController(
Expand All @@ -59,7 +59,7 @@ function createController(
state: Partial<RemoteFeatureFlagControllerState>;
clientConfigApiService: AbstractClientConfigApiService;
disabled: boolean;
getMetaMetricsId: Promise<string | undefined>;
getMetaMetricsId: () => Promise<string> | string;
Copy link
Member

Choose a reason for hiding this comment

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

Note that this function is synchronous on both extension and mobile. It's typed as asynchronous on mobile by mistake, but it's not.

Copy link
Contributor Author

@DDDDDanica DDDDDanica Jan 6, 2025

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Thanks Mark & Danica. I'll update mobile PR

}> = {},
) {
return new RemoteFeatureFlagController({
Expand All @@ -68,7 +68,8 @@ function createController(
clientConfigApiService:
options.clientConfigApiService ?? buildClientConfigApiService(),
disabled: options.disabled,
getMetaMetricsId: options.getMetaMetricsId,
getMetaMetricsId:
options.getMetaMetricsId ?? (() => Promise.resolve(MOCK_METRICS_ID)),
});
}

Expand Down Expand Up @@ -219,6 +220,7 @@ describe('RemoteFeatureFlagController', () => {
});

const clientConfigApiService = {
getClient: () => ClientType.Mobile,
fetchRemoteFeatureFlags: fetchSpy,
} as AbstractClientConfigApiService;

Expand Down Expand Up @@ -271,7 +273,7 @@ describe('RemoteFeatureFlagController', () => {
});
const controller = createController({
clientConfigApiService,
getMetaMetricsId: Promise.resolve(MOCK_METRICS_ID),
getMetaMetricsId: () => Promise.resolve(MOCK_METRICS_ID),
});
await controller.updateRemoteFeatureFlags();

Expand All @@ -289,7 +291,7 @@ describe('RemoteFeatureFlagController', () => {
});
const controller = createController({
clientConfigApiService,
getMetaMetricsId: Promise.resolve(MOCK_METRICS_ID),
getMetaMetricsId: () => Promise.resolve(MOCK_METRICS_ID),
});
await controller.updateRemoteFeatureFlags();

Expand All @@ -298,23 +300,22 @@ describe('RemoteFeatureFlagController', () => {
expect(nonThresholdFlags).toStrictEqual(MOCK_FLAGS);
});

it('uses a fallback metaMetricsId if none is provided', async () => {
jest
.spyOn(userSegmentationUtils, 'generateFallbackMetaMetricsId')
.mockReturnValue(MOCK_METRICS_ID_2);
it('handles synchronous metaMetricsId', async () => {
const clientConfigApiService = buildClientConfigApiService({
remoteFeatureFlags: MOCK_FLAGS_WITH_THRESHOLD,
});
const controller = createController({
clientConfigApiService,
getMetaMetricsId: () => MOCK_METRICS_ID,
});

await controller.updateRemoteFeatureFlags();

expect(
controller.state.remoteFeatureFlags.testFlagForThreshold,
).toStrictEqual({
name: 'groupA',
value: 'valueA',
name: 'groupC',
value: 'valueC',
});
});
});
Expand Down Expand Up @@ -399,18 +400,22 @@ function getControllerMessenger(
* @param options.remoteFeatureFlags - Optional feature flags data to return
* @param options.cacheTimestamp - Optional timestamp to use for the cache
* @param options.error - Optional error to simulate API failure
* @param options.client - The client type to return from getClient
* @returns A mock client config API service
*/
function buildClientConfigApiService({
remoteFeatureFlags,
cacheTimestamp,
error,
client,
}: {
remoteFeatureFlags?: FeatureFlags;
cacheTimestamp?: number;
error?: Error;
client?: ClientType;
} = {}): AbstractClientConfigApiService {
return {
getClient: jest.fn().mockReturnValue(client ?? ClientType.Mobile),
fetchRemoteFeatureFlags: jest.fn().mockImplementation(() => {
if (error) {
return Promise.reject(error);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import type {
import {
generateDeterministicRandomNumber,
isFeatureFlagWithScopeValue,
generateFallbackMetaMetricsId,
} from './utils/user-segmentation-utils';

// === GENERAL ===
Expand Down Expand Up @@ -103,7 +102,7 @@ export class RemoteFeatureFlagController extends BaseController<

#inProgressFlagUpdate?: Promise<ServiceResponse>;

#getMetaMetricsId?: Promise<string | undefined>;
#getMetaMetricsId: () => string | Promise<string>;

/**
* Constructs a new RemoteFeatureFlagController instance.
Expand All @@ -127,7 +126,7 @@ export class RemoteFeatureFlagController extends BaseController<
messenger: RemoteFeatureFlagControllerMessenger;
state?: Partial<RemoteFeatureFlagControllerState>;
clientConfigApiService: AbstractClientConfigApiService;
getMetaMetricsId?: Promise<string | undefined>;
getMetaMetricsId: () => string | Promise<string>;
fetchInterval?: number;
disabled?: boolean;
}) {
Expand Down Expand Up @@ -209,9 +208,17 @@ export class RemoteFeatureFlagController extends BaseController<
remoteFeatureFlags: FeatureFlags,
): Promise<FeatureFlags> {
const processedRemoteFeatureFlags: FeatureFlags = {};
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,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙏🏻


for (const [
remoteFeatureFlagName,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
import { validate as uuidValidate, version as uuidVersion } from 'uuid';
import { v4 as uuidv4 } from 'uuid';

import { ClientType } from '../remote-feature-flag-controller-types';
import {
generateDeterministicRandomNumber,
isFeatureFlagWithScopeValue,
generateFallbackMetaMetricsId,
} from './user-segmentation-utils';

const MOCK_METRICS_IDS = [
'123e4567-e89b-4456-a456-426614174000',
'987fcdeb-51a2-4c4b-9876-543210fedcba',
'a1b2c3d4-e5f6-4890-abcd-ef1234567890',
'f9e8d7c6-b5a4-4210-9876-543210fedcba',
];
const MOCK_METRICS_IDS = {
MOBILE_VALID: '123e4567-e89b-4456-a456-426614174000',
EXTENSION_VALID:
'0x86bacb9b2bf9a7e8d2b147eadb95ac9aaa26842327cd24afc8bd4b3c1d136420',
MOBILE_MIN: '00000000-0000-0000-0000-000000000000',
MOBILE_MAX: 'ffffffff-ffff-ffff-ffff-ffffffffffff',
EXTENSION_MIN: `0x${'0'.repeat(64)}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch ! resolved: 035745f

EXTENSION_MAX: `0x${'f'.repeat(64)}`,
};

const MOCK_FEATURE_FLAGS = {
VALID: {
Expand All @@ -31,26 +34,112 @@ const MOCK_FEATURE_FLAGS = {

describe('user-segmentation-utils', () => {
describe('generateDeterministicRandomNumber', () => {
it('generates consistent numbers for the same input', () => {
const result1 = generateDeterministicRandomNumber(MOCK_METRICS_IDS[0]);
const result2 = generateDeterministicRandomNumber(MOCK_METRICS_IDS[0]);
describe('Mobile client (uuidv4)', () => {
it('generates consistent results for same uuidv4', () => {
const result1 = generateDeterministicRandomNumber(
ClientType.Mobile,
MOCK_METRICS_IDS.MOBILE_VALID,
);
const result2 = generateDeterministicRandomNumber(
ClientType.Mobile,
MOCK_METRICS_IDS.MOBILE_VALID,
);
expect(result1).toBe(result2);
});

expect(result1).toBe(result2);
});
it('handles minimum uuidv4 value', () => {
const result = generateDeterministicRandomNumber(
ClientType.Mobile,
MOCK_METRICS_IDS.MOBILE_MIN,
);
expect(result).toBeGreaterThanOrEqual(0);
expect(result).toBeLessThanOrEqual(1);
});

it('generates numbers between 0 and 1', () => {
MOCK_METRICS_IDS.forEach((id) => {
const result = generateDeterministicRandomNumber(id);
it('handles maximum uuidv4 value', () => {
const result = generateDeterministicRandomNumber(
ClientType.Mobile,
MOCK_METRICS_IDS.MOBILE_MAX,
);
expect(result).toBeGreaterThanOrEqual(0);
expect(result).toBeLessThanOrEqual(1);
});
});

it('generates different numbers for different inputs', () => {
const result1 = generateDeterministicRandomNumber(MOCK_METRICS_IDS[0]);
const result2 = generateDeterministicRandomNumber(MOCK_METRICS_IDS[1]);
describe('Extension client (hex string)', () => {
it('generates consistent results for same hex', () => {
const result1 = generateDeterministicRandomNumber(
ClientType.Extension,
MOCK_METRICS_IDS.EXTENSION_VALID,
);
const result2 = generateDeterministicRandomNumber(
ClientType.Extension,
MOCK_METRICS_IDS.EXTENSION_VALID,
);
expect(result1).toBe(result2);
});

it('handles minimum hex value', () => {
const result = generateDeterministicRandomNumber(
ClientType.Extension,
MOCK_METRICS_IDS.EXTENSION_MIN,
);
expect(result).toBe(0);
});

it('handles maximum hex value', () => {
const result = generateDeterministicRandomNumber(
ClientType.Extension,
MOCK_METRICS_IDS.EXTENSION_MAX,
);
expect(result).toBe(1);
});
});

describe('Distribution validation', () => {
it('produces uniform distribution', () => {
const samples = 1000;
const buckets = 10;
const distribution = new Array(buckets).fill(0);

// Generate samples using valid UUIDs
Array.from({ length: samples }).forEach(() => {
const randomUUID = uuidv4();
const value = generateDeterministicRandomNumber(
ClientType.Mobile,
randomUUID,
);
const bucketIndex = Math.floor(value * buckets);
distribution[bucketIndex] += 1;
});

// Check distribution
const expectedPerBucket = samples / buckets;
const tolerance = expectedPerBucket * 0.3; // 30% tolerance

distribution.forEach((count) => {
expect(count).toBeGreaterThanOrEqual(expectedPerBucket - tolerance);
expect(count).toBeLessThanOrEqual(expectedPerBucket + tolerance);
});
});
});

describe('Client type handling', () => {
it('defaults to Extension handling for undefined client type', () => {
const undefinedClient = 'undefined' as unknown as ClientType;
const result = generateDeterministicRandomNumber(
undefinedClient,
MOCK_METRICS_IDS.EXTENSION_VALID,
);

// Should match Extension result
const extensionResult = generateDeterministicRandomNumber(
ClientType.Extension,
MOCK_METRICS_IDS.EXTENSION_VALID,
);

expect(result1).not.toBe(result2);
expect(result).toBe(extensionResult);
});
});
});

Expand All @@ -75,12 +164,4 @@ describe('user-segmentation-utils', () => {
).toBe(false);
});
});

describe('generateFallbackMetaMetricsId', () => {
it('returns a valid uuidv4', () => {
const result = generateFallbackMetaMetricsId();
expect(uuidValidate(result)).toBe(true);
expect(uuidVersion(result)).toBe(4);
});
});
});
Loading
Loading