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

feat!: improve eval performance, restructure lib, support flag metadata #1120

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
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
4 changes: 2 additions & 2 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
[submodule "libs/providers/flagd/schemas"]
path = libs/providers/flagd/schemas
url = https://github.com/open-feature/schemas.git
url = https://github.com/open-feature/flagd-schemas.git
[submodule "libs/providers/flagd-web/schemas"]
path = libs/providers/flagd-web/schemas
url = https://github.com/open-feature/schemas
url = https://github.com/open-feature/flagd-schemas.git
[submodule "libs/providers/flagd/spec"]
path = libs/providers/flagd/spec
url = https://github.com/open-feature/spec.git
Expand Down
2 changes: 1 addition & 1 deletion libs/shared/flagd-core/flagd-schemas
Submodule flagd-schemas updated 45 files
+2 −2 .release-please-manifest.json
+19 −0 json/CHANGELOG.md
+27 −10 json/flagd_definitions_test.go
+35 −3 json/flags.json
+28 −3 json/flags.yaml
+15 −20 json/targeting.json
+161 −140 json/targeting.yaml
+1 −1 json/test/flags/negative/empty-variants.json
+1 −1 json/test/flags/negative/malformed-flag.json
+1 −2 json/test/flags/negative/missing-variants.json
+1 −1 json/test/flags/negative/mixed-variant-types.ffconfig.json
+1 −1 json/test/flags/negative/no-default-variant.json
+1 −1 json/test/flags/negative/state-set-incorrectly.json
+16 −0 json/test/flags/negative/with-invalid-flag-metadata.json
+16 −0 json/test/flags/negative/with-invalid-flag-set-metadata.json
+16 −0 json/test/flags/negative/with-invalid-targeting.json
+1 −0 json/test/flags/positive/example-simple.flagd.json
+1 −1 json/test/flags/positive/example.flagd.json
+29 −0 json/test/flags/positive/with-metadata.json
+26 −0 json/test/flags/positive/with-valid-targeting.json
+0 −26 json/test/negative/fractional-invalid-bucketing.json
+0 −25 json/test/negative/fractional-invalid-weighting.json
+0 −23 json/test/negative/invalid-ends-with-param.json
+0 −23 json/test/negative/invalid-flagd-props.json
+0 −23 json/test/negative/invalid-starts-with-param.json
+0 −23 json/test/negative/sem-ver-invalid-range-specifier.json
+0 −23 json/test/negative/sem-ver-invalid-ver-expression.json
+0 −74 json/test/positive/basic-json-ops.json
+0 −161 json/test/positive/custom-ops.json
+0 −71 json/test/positive/if-shorthand.json
+9 −0 json/test/targeting/negative/fractional-invalid-bucketing.json
+8 −0 json/test/targeting/negative/fractional-invalid-weighting.json
+9 −0 json/test/targeting/negative/invalid-ends-with-param.json
+9 −0 json/test/targeting/negative/invalid-flagd-props.json
+9 −0 json/test/targeting/negative/invalid-starts-with-param.json
+9 −0 json/test/targeting/negative/sem-ver-invalid-range-specifier.json
+9 −0 json/test/targeting/negative/sem-ver-invalid-ver-expression.json
+61 −0 json/test/targeting/positive/basic-json-ops.json
+114 −0 json/test/targeting/positive/custom-ops.json
+35 −0 json/test/targeting/positive/if-shorthand.json
+1 −1 json/version.txt
+14 −0 protobuf/CHANGELOG.md
+3 −0 protobuf/flagd/evaluation/v1/evaluation.proto
+2 −0 protobuf/schema/v1/schema.proto
+2 −0 protobuf/sync/v1/sync_service.proto
5 changes: 3 additions & 2 deletions libs/shared/flagd-core/package.json
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
{
"name": "@openfeature/flagd-core",
"version": "0.2.5",
"license": "Apache-2.0",
"scripts": {
"publish-if-not-exists": "cp $NPM_CONFIG_USERCONFIG .npmrc && if [ \"$(npm show $npm_package_name@$npm_package_version version)\" = \"$(npm run current-version -s)\" ]; then echo 'already published, skipping'; else npm publish --access public; fi",
"current-version": "echo $npm_package_version"
},
"peerDependencies": {
"@openfeature/core": ">=0.0.16"
"@openfeature/core": ">=1.6.0"
},
"dependencies": {
"ajv": "^8.12.0",
"tslib": "^2.3.0"
}
}
}
17 changes: 11 additions & 6 deletions libs/shared/flagd-core/src/lib/feature-flag.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import type { Logger } from '@openfeature/core';
import { FeatureFlag, Flag } from './feature-flag';

const logger: Logger = {
error: jest.fn(),
warn: jest.fn(),
info: jest.fn(),
debug: jest.fn(),
};

describe('Flagd flag structure', () => {
it('should be constructed with valid input - boolean', () => {
const input: Flag = {
Expand All @@ -12,12 +20,11 @@ describe('Flagd flag structure', () => {
targeting: '',
};

const ff = new FeatureFlag(input);
const ff = new FeatureFlag('test', input, logger);

expect(ff).toBeTruthy();
expect(ff.state).toBe('ENABLED');
expect(ff.defaultVariant).toBe('off');
expect(ff.targeting).toBe('');
expect(ff.variants.get('on')).toBeTruthy();
expect(ff.variants.get('off')).toBeFalsy();
});
Expand All @@ -33,12 +40,11 @@ describe('Flagd flag structure', () => {
targeting: '',
};

const ff = new FeatureFlag(input);
const ff = new FeatureFlag('test', input, logger);

expect(ff).toBeTruthy();
expect(ff.state).toBe('ENABLED');
expect(ff.defaultVariant).toBe('one');
expect(ff.targeting).toBe('');
expect(ff.variants.get('one')).toBe(1.0);
expect(ff.variants.get('two')).toBe(2.0);
});
Expand All @@ -60,12 +66,11 @@ describe('Flagd flag structure', () => {
targeting: '',
};

const ff = new FeatureFlag(input);
const ff = new FeatureFlag('test', input, logger);

expect(ff).toBeTruthy();
expect(ff.state).toBe('ENABLED');
expect(ff.defaultVariant).toBe('pi2');
expect(ff.targeting).toBe('');
expect(ff.variants.get('pi2')).toStrictEqual({ value: 3.14, accuracy: 2 });
expect(ff.variants.get('pi5')).toStrictEqual({ value: 3.14159, accuracy: 5 });
});
Expand Down
93 changes: 85 additions & 8 deletions libs/shared/flagd-core/src/lib/feature-flag.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
import { FlagValue, ParseError } from '@openfeature/core';
import type {
FlagValue,
FlagMetadata,
ResolutionDetails,
JsonValue,
Logger,
ResolutionReason,
EvaluationContext,
} from '@openfeature/core';
import { ParseError, StandardResolutionReasons, GeneralError, TypeMismatchError } from '@openfeature/core';
import { sha1 } from 'object-hash';
import { Targeting } from './targeting/targeting';

/**
* Flagd flag configuration structure mapping to schema definition.
Expand All @@ -9,27 +19,49 @@ export interface Flag {
defaultVariant: string;
variants: { [key: string]: FlagValue };
targeting?: string;
metadata?: FlagMetadata;
}

type ResolutionDetailsWithFlagMetadata<T> = Required<Pick<ResolutionDetails<T>, 'flagMetadata'>> & ResolutionDetails<T>;

/**
* Flagd flag configuration structure for internal reference.
*/
export class FeatureFlag {
private readonly _key: string;
private readonly _state: 'ENABLED' | 'DISABLED';
private readonly _defaultVariant: string;
private readonly _variants: Map<string, FlagValue>;
private readonly _targeting: unknown;
private readonly _hash: string;
private readonly _metadata: FlagMetadata;
private readonly _targeting?: Targeting;
private readonly _targetingParseError?: Error;

constructor(flag: Flag) {
constructor(key: string, flag: Flag, logger: Logger) {
this._key = key;
this._state = flag['state'];
this._defaultVariant = flag['defaultVariant'];
this._variants = new Map<string, FlagValue>(Object.entries(flag['variants']));
this._targeting = flag['targeting'];
this._metadata = flag['metadata'] ?? {};

if (flag.targeting && Object.keys(flag.targeting).length > 0) {
try {
this._targeting = new Targeting(flag.targeting, logger);
} catch (err) {
const message = `Invalid targeting configuration for flag '${key}'`;
logger.warn(message);
this._targetingParseError = new ParseError(message, { cause: err });
}
}
this._hash = sha1(flag);

this.validateStructure();
}

get key(): string {
return this._key;
}

get hash(): string {
return this._hash;
}
Expand All @@ -42,14 +74,59 @@ export class FeatureFlag {
return this._defaultVariant;
}

get targeting(): unknown {
return this._targeting;
}

get variants(): Map<string, FlagValue> {
return this._variants;
}

get metadata(): FlagMetadata {
return this._metadata;
}

evaluate(evalCtx: EvaluationContext): ResolutionDetailsWithFlagMetadata<JsonValue> {
let variant: string;
let reason: ResolutionReason;

if (this._targetingParseError) {
throw this._targetingParseError;
} else if (!this._targeting) {
variant = this._defaultVariant;
reason = StandardResolutionReasons.STATIC;
} else {
let targetingResolution: JsonValue;
try {
targetingResolution = this._targeting.evaluate(this._key, evalCtx);
} catch (e) {
throw new GeneralError(`Error evaluating targeting rule for flag '${this._key}'`, { cause: e });
}

// Return default variant if targeting resolution is null or undefined
if (targetingResolution === null || targetingResolution === undefined) {
variant = this._defaultVariant;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is required to properly support short-circuiting json logic. If we used !targetingResolution, it would be true for any falsy values.

reason = StandardResolutionReasons.DEFAULT;
} else {
// Obtain resolution in string. This is useful for short-circuiting json logic
variant = targetingResolution.toString();
reason = StandardResolutionReasons.TARGETING_MATCH;
}
}

if (typeof variant !== 'string') {
throw new TypeMismatchError(`Variant must be a string, but found '${typeof variant}'`);
}

const resolvedVariant = this._variants.get(variant);
if (resolvedVariant === undefined) {
throw new GeneralError(`Variant '${variant}' not found in flag with key '${this._key}'`);
}

return {
value: resolvedVariant,
reason,
variant,
flagMetadata: this.metadata,
};
}

validateStructure() {
// basic validation, ideally this sort of thing is caught by IDEs and other schema validation before we get here
// consistent with Java/Go and other implementations, we only warn for schema validation, but we fail for this sort of basic structural errors
Expand Down
Loading
Loading