[sc-154361] #1 Implement target evaluation and 'OFF' evaluation. #12

kinyoklion · 2022-06-03T17:55:36Z

This PR adds support for evaluating targets/context targets in flags as well as handling the evaluation of flags that are off.

…ing evaluator.

shortcut-integration · 2022-06-03T17:55:39Z

This pull request has been linked to Shortcut Story #154361: Implement evaluation: Scaffold, Operators, Targets.

kinyoklion · 2022-06-03T17:56:09Z

sdk-common/__tests__/Context.test.ts

  });

  it('should get the same values', () => {
-    expect(context?.valueForKind('user', new AttributeReference('cat'))).toEqual('calico');


I changed the order of these params to allow for the kind to be optional and simplify evaluation logic.

kinyoklion · 2022-06-03T17:58:14Z

server-sdk-common/__tests__/evaluation/Evaluator.test.ts

@@ -0,0 +1,127 @@
+import { Context, LDContext } from '@launchdarkly/js-sdk-common';


Currently there are very limited tests for just the couple things the evaluator supports.

kinyoklion · 2022-06-03T17:59:39Z

server-sdk-common/src/evaluation/EvalResult.ts

@@ -0,0 +1,38 @@
+import { LDEvaluationDetail, LDEvaluationReason } from '../api';


In the node implementation it was very difficult to get errors to propagate out of the evaluation. This was due to a combination of methods that had a return value independent of their callback value. In hopes to reduce the complexity some, aside from using async/away, I am trying to make sure any layer can easily propagate an error result.

kinyoklion · 2022-06-03T18:01:07Z

server-sdk-common/src/evaluation/Evaluator.ts

+    return EvalResult.ForError(ErrorKinds.FlagNotFound, 'Temporary');
+  }
+
+  // private async ruleMatchContext(


I started working on this and then realized I need to work on the reviver/replacer for JSON first. Currently node doesn't have anything like that, so the eval code has to check every possible combination of null/undefined for every single JSON field.

Additionally it has no way to pre-compute AttributeReferences.

So, I am going to pause this and implement that functionality and then come back.

Could you say more about what you mean by "the eval code has to check every possible combination of null/undefined for every single JSON field"?

kinyoklion · 2022-06-03T18:01:41Z

server-sdk-common/src/evaluation/Operations.ts

+/**
+ * Allows checking if a specific operator is defined and allows execution of an operator on data.
+ *
+ * @internal


I may be missing some of these, but in the end I will audit what appears in the public interface to make sure nothing has leaked out.

kinyoklion · 2022-06-03T18:02:53Z

server-sdk-common/src/evaluation/collection.ts

@@ -0,0 +1,46 @@
+/**


In the node implementation we use the async package to handle all kinds of callback hell. If we are using async/await, then we don't need it, but we do need some mechanism for some common iteration strategies.

kinyoklion · 2022-06-03T18:03:30Z

server-sdk-common/src/evaluation/data/Flag.ts

 import { Versioned } from './Versioned';

-type VariationOrRollout = number | Rollout;
+type VariationOrRollout = { variation: number; } | { rollout: Rollout };


kinyoklion · 2022-06-03T18:04:36Z

server-sdk-common/src/evaluation/evalTargets.ts

@@ -0,0 +1,47 @@
+import { Context } from '@launchdarkly/js-sdk-common';


Target evaluation doesn't use any queries, so I have done it as free functions instead of incorporating it into the evaluator. Prerequisites and rule evaluation are going to require queries, so will either be encapsulated objects for those purposes, or directly integrated into evaluator.

kinyoklion · 2022-06-03T18:08:49Z

server-sdk-common/src/evaluation/collection.ts

+  operator: (val: T) => U | undefined,
+): U | undefined {
+  let res;
+  collection?.some((item) => {


I dislike JS naming of things, but 'some' is equivalent to the more sensible language of 'any'. In that it stops iteration once any return value is truthy.

So it is a common way to iterate until a condition is true. In JS you general want to use prefer for loops, or collection methods like some versus using for in/of. This is because there is a substantial performance difference because range based for loops use generators and iterators and have much more runtime cost.

So, this just gives us an easy way to get the first result that is defined.

some by itself just uses the return value for truthiness, so we could not get the actual result.

eli-darkly · 2022-06-30T02:09:30Z

sdk-common/__tests__/Context.test.ts

-    expect(context?.valueForKind('user', new AttributeReference('cat'))).toEqual('calico');
-    expect(context?.valueForKind('user', new AttributeReference('name'))).toEqual('context name');
+    expect(context?.valueForKind(new AttributeReference('cat'), 'user')).toEqual('calico');
+    expect(context?.valueForKind(new AttributeReference('name'), 'user')).toEqual('context name');


Not a big deal, but: I'm unclear on why you're using context? in all the tests like this. I mean, if line 63 fails somehow and context ends up being null/undefined, then the "should create a context" test will fail with a clear error. And I would think that if these other tests failed by throwing a null reference exception, that would also be fairly clear. Using ?. in all these expectations means, if I understand correctly, that it will instead be telling us "expected for valueForKind to return 'calico', but it returned undefined", etc.— which is a bit misleading since the problem has nothing to do with valueForKind. Plus, if we got past the first line, we know context is not null/undefined, so what's the point of repeating ?. every time?

eli-darkly · 2022-06-30T02:10:57Z

server-sdk-common/__tests__/evaluation/Evaluator.test.ts

+  // @ts-ignore
+  it(`produces the expected evaluation result for context: ${context.key} ${context.kind} targets: ${flag.targets?.map((t) => `${t.values}, ${t.variation}`)} context targets: ${flag.contextTargets?.map((t) => `${t.contextKind}: ${t.values}, ${t.variation}`)}`, async () => {
+    const result = await evaluator.evaluate(flag, Context.FromLDContext(context)!);
+    expect(result?.isError).toEqual(expected?.isError);


Similar to my earlier comment, I think I would prefer a more specific failure here if result is null/undefined (which it shouldn't ever be, right?).

eli-darkly · 2022-06-30T02:13:38Z

server-sdk-common/src/evaluation/Reasons.ts

+ *
+ * @internal
+ */
+export default class Reasons {


Is there a particular reason to make this a separate non-public class, instead of having these be static methods of LDEvaluationReason? We do the latter in other SDKs, mainly because it facilitates testing in other projects that use the SDK.

The main reason at the moment is all the external interface is composed of interfaces. Which I cannot add static methods to.

It is possible we could change some of them to classes and depend on the generated interfaces, but I have not yet considered doing this.

eli-darkly · 2022-06-30T02:16:27Z

server-sdk-common/src/evaluation/evalTargets.ts

+
+function evalTarget(flag: Flag, target: Target, context: Context): EvalResult | undefined {
+  const contextKey = context.key(target.contextKind);
+  if (contextKey !== undefined) {


Would one potential solution to the "always having to check specifically for null or undefined" issue be to ensure that context.key can never return any potentially truthy type other than string? I mean, if (contextKey) is only a problem if we think it could be a boolean/number/etc., right?

On the flag data from LD we now transform to undefined if there are nulls. I can similarly ensure that for the context as well, assuming I am not already. Most of the checks I am using type guard now instead of direct comparisons. Aside from a few cases like these.

kinyoklion added 10 commits June 1, 2022 13:19

Add bucketing and common tests.

c695f15

Correct spelling.

30325bf

Check numeric conversion step.

13cf885

Add operators and tests.

3667ae8

Add unrecognized operator test.

a8ab5d9

Add interfaces for the data we use during evaluations. Start scaffold…

a28b852

…ing evaluator.

Cleanup

d0ed14c

Add target evaluation.

1bfb4dd

Merge main.

0158058

Add documentation and tests.

8d08641

kinyoklion commented Jun 3, 2022

View reviewed changes

Linting.

8941124

kinyoklion commented Jun 3, 2022

View reviewed changes

kinyoklion marked this pull request as ready for review June 3, 2022 18:05

kinyoklion commented Jun 3, 2022

View reviewed changes

kinyoklion requested review from aengelberg, keelerm84 and yusinto June 3, 2022 18:08

kinyoklion changed the title ~~[sc-154361] Implement target evaluation and 'OFF' evaluation.~~ [sc-154361] #1 Implement target evaluation and 'OFF' evaluation. Jun 8, 2022

kinyoklion requested a review from eli-darkly June 22, 2022 16:20

eli-darkly reviewed Jun 30, 2022

View reviewed changes

eli-darkly approved these changes Jun 30, 2022

View reviewed changes

kinyoklion merged commit 34f38c6 into main Jul 5, 2022

kinyoklion deleted the rlamb/sc-154361/target-evaluation branch July 5, 2022 15:56

		@@ -0,0 +1,127 @@
		import { Context, LDContext } from '@launchdarkly/js-sdk-common';

		@@ -0,0 +1,38 @@
		import { LDEvaluationDetail, LDEvaluationReason } from '../api';

		@@ -0,0 +1,47 @@
		import { Context } from '@launchdarkly/js-sdk-common';

[sc-154361] #1 Implement target evaluation and 'OFF' evaluation. #12

[sc-154361] #1 Implement target evaluation and 'OFF' evaluation. #12

Uh oh!

Conversation

kinyoklion commented Jun 3, 2022

Uh oh!

shortcut-integration bot commented Jun 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants