-
Notifications
You must be signed in to change notification settings - Fork 4
Add built-in TargetingFilter impl #5
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
Changes from all commits
0b5c81c
ddbcfda
ee83ae8
a0d6627
9f88e1c
cfb565c
de65379
27bed19
bf545cc
2f66e46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,7 @@ | |
| "eol-last": [ | ||
| "error", | ||
| "always" | ||
| ] | ||
| ], | ||
| "no-trailing-spaces": "warn" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| import { IFeatureFilter } from "./FeatureFilter"; | ||
| import { createHash } from "crypto"; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The portal team used CryptoJS. I imagine either works- but might make sense to align on one https://msazure.visualstudio.com/Azure%20AppConfig/_git/Portal?path=/AppConfigPortalExtension/AppConfigPortalExtension/Client/Resource/FeatureManager/ResourceFeatureFlagsBlade.ts&version=GBmaryanne/feature/splitExperimentation&_a=contents We will need to use Base64 in the future if that helps decide. |
||
|
|
||
| type TargetingFilterParameters = { | ||
| Audience: { | ||
| DefaultRolloutPercentage: number; | ||
Eskibear marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Users?: string[]; | ||
| Groups?: { | ||
| Name: string; | ||
| RolloutPercentage: number; | ||
| }[]; | ||
| Exclusion?: { | ||
| Users?: string[]; | ||
| Groups?: string[]; | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| type TargetingFilterEvaluationContext = { | ||
| featureName: string; | ||
| parameters: TargetingFilterParameters; | ||
| } | ||
|
|
||
| type TargetingFilterAppContext = { | ||
| userId?: string; | ||
| groups?: string[]; | ||
| } | ||
|
|
||
| export class TargetingFilter implements IFeatureFilter { | ||
| name: string = "Microsoft.Targeting"; | ||
|
|
||
| evaluate(context: TargetingFilterEvaluationContext, appContext?: TargetingFilterAppContext): boolean | Promise<boolean> { | ||
Eskibear marked this conversation as resolved.
Show resolved
Hide resolved
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels like a lie- since we don't ever return Promise. I prefer marking this async and only doing Promise.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rossgrambo Good catch. That was a miss. evaluate(context: TargetingFilterEvaluationContext, appContext?: TargetingFilterAppContext): booleanWDYT? |
||
| const { featureName, parameters } = context; | ||
| TargetingFilter.#validateParameters(parameters); | ||
|
|
||
| if (appContext === undefined) { | ||
| throw new Error("The app context is required for targeting filter."); | ||
| } | ||
|
|
||
| if (parameters.Audience.Exclusion !== undefined) { | ||
| // check if the user is in the exclusion list | ||
| if (appContext?.userId !== undefined && | ||
| parameters.Audience.Exclusion.Users !== undefined && | ||
| parameters.Audience.Exclusion.Users.includes(appContext.userId)) { | ||
| return false; | ||
| } | ||
| // check if the user is in a group within exclusion list | ||
| if (appContext?.groups !== undefined && | ||
| parameters.Audience.Exclusion.Groups !== undefined) { | ||
| for (const excludedGroup of parameters.Audience.Exclusion.Groups) { | ||
| if (appContext.groups.includes(excludedGroup)) { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // check if the user is being targeted directly | ||
| if (appContext?.userId !== undefined && | ||
| parameters.Audience.Users !== undefined && | ||
| parameters.Audience.Users.includes(appContext.userId)) { | ||
| return true; | ||
| } | ||
|
|
||
| // check if the user is in a group that is being targeted | ||
| if (appContext?.groups !== undefined && | ||
| parameters.Audience.Groups !== undefined) { | ||
| for (const group of parameters.Audience.Groups) { | ||
| if (appContext.groups.includes(group.Name)) { | ||
| const audienceContextId = constructAudienceContextId(featureName, appContext.userId, group.Name); | ||
| const rolloutPercentage = group.RolloutPercentage; | ||
| if (TargetingFilter.#isTargeted(audienceContextId, rolloutPercentage)) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // check if the user is being targeted by a default rollout percentage | ||
| const defaultContextId = constructAudienceContextId(featureName, appContext?.userId); | ||
| return TargetingFilter.#isTargeted(defaultContextId, parameters.Audience.DefaultRolloutPercentage); | ||
| } | ||
|
|
||
| static #isTargeted(audienceContextId: string, rolloutPercentage: number): boolean { | ||
| if (rolloutPercentage === 100) { | ||
| return true; | ||
| } | ||
| // Cryptographic hashing algorithms ensure adequate entropy across hash values. | ||
| const contextMarker = stringToUint32(audienceContextId); | ||
| const contextPercentage = (contextMarker / 0xFFFFFFFF) * 100; | ||
| return contextPercentage < rolloutPercentage; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Eskibear Did you validate that this produces the same number as dotnet/python? |
||
| } | ||
|
|
||
| static #validateParameters(parameters: TargetingFilterParameters): void { | ||
| if (parameters.Audience.DefaultRolloutPercentage < 0 || parameters.Audience.DefaultRolloutPercentage > 100) { | ||
| throw new Error("Audience.DefaultRolloutPercentage must be a number between 0 and 100."); | ||
| } | ||
| // validate RolloutPercentage for each group | ||
| if (parameters.Audience.Groups !== undefined) { | ||
| for (const group of parameters.Audience.Groups) { | ||
| if (group.RolloutPercentage < 0 || group.RolloutPercentage > 100) { | ||
| throw new Error(`RolloutPercentage of group ${group.Name} must be a number between 0 and 100.`); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "between 0 and 100" makes it sound like it needs to be 1-99. Could do any of:
|
||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Constructs the context id for the audience. | ||
| * The context id is used to determine if the user is part of the audience for a feature. | ||
| * If groupName is provided, the context id is constructed as follows: | ||
| * userId + "\n" + featureName + "\n" + groupName | ||
| * Otherwise, the context id is constructed as follows: | ||
| * userId + "\n" + featureName | ||
| * | ||
| * @param featureName name of the feature | ||
| * @param userId userId from app context | ||
| * @param groupName group name from app context | ||
| * @returns a string that represents the context id for the audience | ||
| */ | ||
| function constructAudienceContextId(featureName: string, userId: string | undefined, groupName?: string) { | ||
| let contextId = `${userId ?? ""}\n${featureName}`; | ||
| if (groupName !== undefined) { | ||
| contextId += `\n${groupName}`; | ||
| } | ||
| return contextId | ||
| } | ||
|
|
||
| function stringToUint32(str: string): number { | ||
| // Create a SHA-256 hash of the string | ||
| const hash = createHash("sha256").update(str).digest(); | ||
|
|
||
| // Get the first 4 bytes of the hash | ||
| const first4Bytes = hash.subarray(0, 4); | ||
|
|
||
| // Convert the 4 bytes to a uint32 with little-endian encoding | ||
| const uint32 = first4Bytes.readUInt32LE(0); | ||
| return uint32; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| import * as chai from "chai"; | ||
| import * as chaiAsPromised from "chai-as-promised"; | ||
| chai.use(chaiAsPromised); | ||
| const expect = chai.expect; | ||
|
|
||
| import { FeatureManager, ConfigurationMapFeatureFlagProvider } from "./exportedApi"; | ||
|
|
||
| const complexTargetingFeature = { | ||
| "id": "ComplexTargeting", | ||
| "description": "A feature flag using a targeting filter, that will return true for Alice, Stage1, and 50% of Stage2. Dave and Stage3 are excluded. The default rollout percentage is 25%.", | ||
| "enabled": true, | ||
| "conditions": { | ||
| "client_filters": [ | ||
| { | ||
| "name": "Microsoft.Targeting", | ||
| "parameters": { | ||
| "Audience": { | ||
| "Users": [ | ||
| "Alice" | ||
| ], | ||
| "Groups": [ | ||
| { | ||
| "Name": "Stage1", | ||
| "RolloutPercentage": 100 | ||
| }, | ||
| { | ||
| "Name": "Stage2", | ||
| "RolloutPercentage": 50 | ||
| } | ||
| ], | ||
| "DefaultRolloutPercentage": 25, | ||
| "Exclusion": { | ||
| "Users": ["Dave"], | ||
| "Groups": ["Stage3"] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| }; | ||
|
|
||
| const createTargetingFeatureWithRolloutPercentage = (name: string, defaultRolloutPercentage: number, groups?: { Name: string, RolloutPercentage: number }[]) => { | ||
| const featureFlag = { | ||
| "id": name, | ||
| "description": "A feature flag using a targeting filter with invalid parameters.", | ||
| "enabled": true, | ||
| "conditions": { | ||
| "client_filters": [ | ||
| { | ||
| "name": "Microsoft.Targeting", | ||
| "parameters": { | ||
| "Audience": { | ||
| "DefaultRolloutPercentage": defaultRolloutPercentage | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| }; | ||
| if (groups && groups.length > 0) { | ||
| (featureFlag.conditions.client_filters[0].parameters.Audience as any).Groups = groups; | ||
| } | ||
| return featureFlag; | ||
| }; | ||
|
|
||
| describe("targeting filter", () => { | ||
| it("should validate parameters", () => { | ||
| const dataSource = new Map(); | ||
| dataSource.set("feature_management", { | ||
| feature_flags: [ | ||
| createTargetingFeatureWithRolloutPercentage("InvalidTargeting1", -1), | ||
| createTargetingFeatureWithRolloutPercentage("InvalidTargeting2", 101), | ||
| // invalid group rollout percentage | ||
| createTargetingFeatureWithRolloutPercentage("InvalidTargeting3", 25, [{ Name: "Stage1", RolloutPercentage: -1 }]), | ||
| createTargetingFeatureWithRolloutPercentage("InvalidTargeting4", 25, [{ Name: "Stage1", RolloutPercentage: 101 }]), | ||
| ] | ||
| }); | ||
|
|
||
| const provider = new ConfigurationMapFeatureFlagProvider(dataSource); | ||
| const featureManager = new FeatureManager(provider); | ||
|
|
||
| return Promise.all([ | ||
| expect(featureManager.isEnabled("InvalidTargeting1", {})).eventually.rejectedWith("Audience.DefaultRolloutPercentage must be a number between 0 and 100."), | ||
| expect(featureManager.isEnabled("InvalidTargeting2", {})).eventually.rejectedWith("Audience.DefaultRolloutPercentage must be a number between 0 and 100."), | ||
| expect(featureManager.isEnabled("InvalidTargeting3", {})).eventually.rejectedWith("RolloutPercentage of group Stage1 must be a number between 0 and 100."), | ||
| expect(featureManager.isEnabled("InvalidTargeting4", {})).eventually.rejectedWith("RolloutPercentage of group Stage1 must be a number between 0 and 100."), | ||
| ]); | ||
| }); | ||
|
|
||
| it("should evaluate feature with targeting filter", () => { | ||
| const dataSource = new Map(); | ||
| dataSource.set("feature_management", { | ||
| feature_flags: [complexTargetingFeature] | ||
| }); | ||
|
|
||
| const provider = new ConfigurationMapFeatureFlagProvider(dataSource); | ||
| const featureManager = new FeatureManager(provider); | ||
|
|
||
| return Promise.all([ | ||
| // default rollout 25% | ||
| // - "Aiden\nComplexTargeting": ~62.9% | ||
| expect(featureManager.isEnabled("ComplexTargeting", { userId: "Aiden" })).eventually.eq(false, "Aiden is not in the 25% default rollout"), | ||
|
|
||
| // - "Blossom\nComplexTargeting": ~20.2% | ||
| expect(featureManager.isEnabled("ComplexTargeting", { userId: "Blossom" })).eventually.eq(true, "Blossom is in the 25% default rollout"), | ||
| expect(featureManager.isEnabled("ComplexTargeting", { userId: "Alice" })).eventually.eq(true, "Alice is directly targeted"), | ||
|
|
||
| // Stage1 group is 100% rollout | ||
| expect(featureManager.isEnabled("ComplexTargeting", { userId: "Aiden", groups: ["Stage1"] })).eventually.eq(true, "Aiden is in because Stage1 is 100% rollout"), | ||
|
|
||
| // Stage2 group is 50% rollout | ||
| // - "\nComplexTargeting\nStage2": ~78.7% >= 50% (Stage2 is 50% rollout) | ||
| // - "\nComplexTargeting": ~38.9% >= 25% (default rollout percentage) | ||
| expect(featureManager.isEnabled("ComplexTargeting", { groups: ["Stage2"] })).eventually.eq(false, "Empty user is not in the 50% rollout of group Stage2"), | ||
|
|
||
| // - "Aiden\nComplexTargeting\nStage2": ~15.6% | ||
| expect(featureManager.isEnabled("ComplexTargeting", { userId: "Aiden", groups: ["Stage2"] })).eventually.eq(true, "Aiden is in the 50% rollout of group Stage2"), | ||
|
|
||
| // - "Chris\nComplexTargeting\nStage2": 55.3% >= 50% (Stage2 is 50% rollout) | ||
| // - "Chris\nComplexTargeting": 72.3% >= 25% (default rollout percentage) | ||
| expect(featureManager.isEnabled("ComplexTargeting", { userId: "Chris", groups: ["Stage2"] })).eventually.eq(false, "Chris is not in the 50% rollout of group Stage2"), | ||
|
|
||
| // exclusions | ||
| expect(featureManager.isEnabled("ComplexTargeting", { groups: ["Stage3"] })).eventually.eq(false, "Stage3 group is excluded"), | ||
| expect(featureManager.isEnabled("ComplexTargeting", { userId: "Alice", groups: ["Stage3"] })).eventually.eq(false, "Alice is excluded because she is part of Stage3 group"), | ||
| expect(featureManager.isEnabled("ComplexTargeting", { userId: "Blossom", groups: ["Stage3"] })).eventually.eq(false, "Blossom is excluded because she is part of Stage3 group"), | ||
| expect(featureManager.isEnabled("ComplexTargeting", { userId: "Dave", groups: ["Stage1"] })).eventually.eq(false, "Dave is excluded because he is in the exclusion list"), | ||
| ]); | ||
| }); | ||
|
|
||
| it("should throw error if app context is not provided", () => { | ||
| const dataSource = new Map(); | ||
| dataSource.set("feature_management", { | ||
| feature_flags: [complexTargetingFeature] | ||
| }); | ||
|
|
||
| const provider = new ConfigurationMapFeatureFlagProvider(dataSource); | ||
| const featureManager = new FeatureManager(provider); | ||
|
|
||
| return expect(featureManager.isEnabled("ComplexTargeting")).eventually.rejectedWith("The app context is required for targeting filter."); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface should just return Promise. The calling code is forced to use await anyway since this function might return a Promise. If await is used on a non-promise, it simply wraps it in an immediately resolved Promise anyway.
I believe the implementation can just stay "return true" and things will work fine- as long as the function is marked async.