feat(AudienceEvaluator): Add the ability to provide custom condition evaluators #288

jamesopti · 2019-06-07T20:58:28Z

Summary

This PR (replacing #216) adds the ability for an SDK consumer to pass their own set of condition evaluators when creating an optimizely instance.

The core motivator for this is to be able to evaluate WEB audience conditions using the Javascript SDK as part of the EDGE project (go/edge).

Future fullstack use cases for this functionality might include audience condition plugins, where the customer would define their own match types and provide their own matchers.

/**
 * Condition evaluators for conditions that can be evaluated in a Cloudflare worker using
 * information like cookies, query parameters, userAgent, etc...
 */
function getConditionEvaluators(inputs) {
  return {
    cookies: {
      evaluate: matchCookies.match.bind(this, { cookies: inputs.cookies }),
    },
    device: {
      evaluate: matchDevice.match.bind(this, { device: Detect.parseUA(inputs.ua).device }),
    },
    ...
    /* Other custom condition evaluators */
  }
}

var optlyInstance = optimizely.createInstance({
  datafile: datafile,
  conditionEvaluators: getConditionEvaluators(inputs),
  skipJSONValidation: true
});

Test plan

Fixed existing tests. May need to add more unit tests.

Issues

https://optimizely.atlassian.net/browse/CJS-3034

rebase with latest audience editor changes updates per code review

coveralls · 2019-06-07T21:35:15Z

Coverage decreased (-0.07%) to 97.531% when pulling 4da5d4a on james/custom_condition_evaluators_OG into bd49de0 on master.

lpappone · 2019-06-07T21:37:29Z

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js

@@ -56,7 +56,6 @@ EVALUATORS_BY_MATCH_TYPE[SUBSTRING_MATCH_TYPE] = substringEvaluator;
 */
 function evaluate(condition, userAttributes, logger) {
  if (condition.type !== CUSTOM_ATTRIBUTE_CONDITION_TYPE) {
-    logger.log(LOG_LEVEL.WARNING, sprintf(LOG_MESSAGES.UNKNOWN_CONDITION_TYPE, MODULE_NAME, JSON.stringify(condition)));
    return null;
  }


delete? because we're handling this in typeToEvaluate - we only call this if there's a type.

mjc1283 · 2019-06-11T16:57:33Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

+ * @constructor
+ */
+function AudienceEvaluator(logger, UNSTABLE_conditionEvaluators) {
+  this.logger = logger;


Instead of passing logger, we can import a singleton logger and use it directly (example from project_config_manager)

mikeproeng37

Mostly looks good. I just have a question.

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

mikeproeng37 · 2019-06-11T20:20:47Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

+ * @constructor
+ */
+function AudienceEvaluator(logger, UNSTABLE_conditionEvaluators) {
+  this.logger = logger;


packages/optimizely-sdk/lib/core/audience_evaluator/index.js

nchilada

This code looks really good. My main feedback relates to the interface, particularly if we think there's a chance that we'll eventually expose this kind of interface publicly. I'm going off the code sample in the PR description:

/**
 * Condition evaluators for conditions that can be evaluated in a Cloudflare worker using
 * information like cookies, query parameters, userAgent, etc...
 */
function getConditionEvaluators(inputs) {
  return {
    cookies: {
      evaluate: matchCookies.match.bind(this, { cookies: inputs.cookies }),
    },
    device: {
      evaluate: matchDevice.match.bind(this, { device: Detect.parseUA(inputs.ua).device }),
    },
    ...
    /* Other custom condition evaluators */
  }
}

var optlyInstance = optimizely.createInstance({
  datafile: datafile,
  conditionEvaluators: getConditionEvaluators(inputs),
  skipJSONValidation: true
});

My comments:

Why require customers to pass an object with an evaluate function rather than passing a naked function? Just because that's the shape of the custom_attribute_condition_evaluator module?
Should we think more deeply about all the context that we pass to the evaluate functions? It looks like we're passing the condition, the user attributes, and a logger. I wonder if we'll eventually need to document what to do with the logger, and I think we ought to pass the user ID in addition to the user attributes.

At any rate, I found the code sample very useful! Only one nit: it's a little confusing to capture inputs at the moment the SDK is initialized. Instead, can we declare a module-level variable named userState or something? I'd be happy to make this change in the PR description / commit message since it is de-facto documentation for the new feature.

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

packages/optimizely-sdk/lib/utils/enums/index.js

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.tests.js

packages/optimizely-sdk/lib/core/decision_service/index.js

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

packages/optimizely-sdk/lib/optimizely/index.tests.js

lpappone · 2019-06-14T02:57:40Z

@mikeng13 @mjc1283 Could you guys weigh in on whether this is something you plan to consider exposing to customers? A lot of Nikhil's concerns seem to center on that, and it's not something that I feel I have the context to decide.

In the meantime to try to address a couple questions:

Why require customers to pass an object with an evaluate function rather than passing a naked function? Just because that's the shape of the custom_attribute_condition_evaluator module?

Yes, I think that was the case. Maybe @jamesopti has more to say.

I wonder if we'll eventually need to document what to do with the logger, and I think we ought to pass the user ID in addition to the user attributes.

I don't think we're passing a logger anymore - can you comment in the code if you see it? What do you think the user ID is needed for?

nchilada

Why require customers to pass an object with an evaluate function rather than passing a naked function? Just because that's the shape of the custom_attribute_condition_evaluator module?

Yes, I think that was the case. Maybe @jamesopti has more to say.

🙏

I don't think we're passing a logger anymore - can you comment in the code if you see it?

Hmm, maybe there's a bug here? You're right that we're only passing the condition and user attributes to the evaluator, but the "custom_attribute" evaluator still expects to receive a logger. We should fix this and add a unit test to make sure we're correctly calling the evaluator.

What do you think the user ID is needed for?

For customer-defined audience conditions, the user ID might be more useful than user attributes, since user attributes are primarily relevant for "custom_attribute" conditions. I'm sure the customer can stash the user ID in some closure, along with other user-specific state, but passing the user ID seems like a nice touch.

Perhaps we can pass named arguments (read: an "options" object, in the case of JavaScript) to the customer's callback. This way we can start passing the user ID in the future without breaking previously-instrumented callbacks. This is what we do for notification listeners.

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

nchilada · 2019-06-14T23:23:15Z

Why require customers to pass an object with an evaluate function rather than passing a naked function? Just because that's the shape of the custom_attribute_condition_evaluator module?

Yes, I think that was the case. Maybe @jamesopti has more to say.

🙏

@lpappone ohh, my bad, I didn't realize you were authoring this! I can try to close out some of my comment threads if you're ready to move forward

Co-Authored-By: Nikhil Chelliah <[email protected]>

lpappone · 2019-06-17T17:41:23Z

@lpappone ohh, my bad, I didn't realize you were authoring this! I can try to close out some of my comment threads if you're ready to move forward

I resolved things that seemed resolvable.

nchilada

Looking good! Just a couple outstanding points of discussion

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

nchilada · 2019-06-17T18:35:32Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

+    return null;
+  }
+  try {
+    return evaluator.evaluate(condition, userAttributes);


I still think we need to think about the arguments we're passing here.

Primarily we need to pass a logger or update the custom_attribute_condition_evaluator to import the logger directly

Can we pass an object with named properties?

This is a good point.

We DO need to pass a logger - I mistakenly omitted this. We can't have custom_attribute_condition_evaluator import it as its stateful to this instance of AudienceEvaluator

Can we pass an object with named properties?

WDYM exactly? The custom_attribute_condition_evaluator needs the userAttributes map to do it's matching, but other matchers won't necessarily need them.

Oh. I'm proposing named parameters like this:

Suggested change

return evaluator.evaluate(condition, userAttributes);

return evaluator.evaluate({

condition: condition,

userAttributes: userAttributes,

});

This is how we invoke customer's notification listener callbacks (example) and, arguably, is how we ought to accept inputs to all of our public API methods.

Ok, doing that here would involve changing a lot of established code (like this). I don't think it makes a ton of sense to make that change in this PR.

Hmm, the changes ought to be limited to custom_attribute_condition_evaluator.evaluate and its unit tests. I think this is the best time to take this on - we're starting to move towards a public API, and we're prepare to introduce many more evaluators than custom_attribute_condition_evaluator

mikeproeng37

Changes lgtm

jamesopti

I cant request changes on my own PR. Haha. Added to @nchilada 's comment though that we do need to update passing the logger.

jamesopti · 2019-06-17T21:41:59Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

+    return null;
+  }
+  try {
+    return evaluator.evaluate(condition, userAttributes);


This is a good point.

We DO need to pass a logger - I mistakenly omitted this. We can't have custom_attribute_condition_evaluator import it as its stateful to this instance of AudienceEvaluator

Can we pass an object with named properties?

WDYM exactly? The custom_attribute_condition_evaluator needs the userAttributes map to do it's matching, but other matchers won't necessarily need them.

lpappone · 2019-06-18T17:22:12Z

Ok, added the logger. @nchilada I am also not sure what you intended with "Can we pass an object with named properties?"

nchilada

Thanks for the fix!

nchilada · 2019-06-18T22:08:37Z

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

+    return null;
+  }
+  try {
+    return evaluator.evaluate(condition, userAttributes);


Oh. I'm proposing named parameters like this:

Suggested change

return evaluator.evaluate(condition, userAttributes);

return evaluator.evaluate({

condition: condition,

userAttributes: userAttributes,

});

This is how we invoke customer's notification listener callbacks (example) and, arguably, is how we ought to accept inputs to all of our public API methods.

lpappone · 2019-06-19T03:04:34Z

@nchilada your "request changes" is blocking this from being merged, despite @mikeng13 approving it. Can you re-review please?

lpappone · 2019-06-19T21:18:23Z

@mikeng13 @mjc1283 @jamesopti
Can you guys (at least a couple of you) weigh in on @nchilada's suggestion to pass an object with named properties to the custom_attribute_condition_evaluator? I'm not familiar enough with the SDK to have a strong opinion, and I don't want to put in the time to do the work unless there's some consensus that it's the right thing to do.

nchilada

@mikeng13 says we'll try to clean up the audience-condition evaluation interface in the future

…condition evaluator (#389) ## Summary #288 introduced an exploratory new feature, the ability to pass custom condition evaluators to the SDK constructor via the option `UNSTABLE_conditionEvaluators`. The AudienceEvaluator (and its tests) were refactored as a part of that PR, but no tests were added to assert that the `UNSTABLE_conditionEvaluators` worked when passed. This PR introduces tests to ensure that functionality continues to work. ## Test plan Unit tests added to assert that: - You cannot override the built in `custom_attribute` evaluator - A passed in custom condition evaluator will evaluate conditions of that type - That the audience condition, user attributes, and logger instance are all passed to the custom evaluator ## Issues - CJS-3815

add the ability to provide custom condition evaluators

2ff9d2e

rebase with latest audience editor changes updates per code review

jamesopti force-pushed the james/custom_condition_evaluators_OG branch from 0112ff9 to 2ff9d2e Compare June 7, 2019 21:11

James Fox and others added 3 commits June 7, 2019 14:24

fix tests

7015e7d

update lint command

db7addd

update function name

fda872d

lpappone reviewed Jun 7, 2019

View reviewed changes

remove unneccessary check

713212d

lpappone mentioned this pull request Jun 7, 2019

DO NOT REVIEW - SEE https://github.com/optimizely/javascript-sdk/pull/288 #216

Closed

lpappone changed the title ~~James/custom condition evaluators og~~ feat(AudienceEvaluator): Add the ability to provide custom condition evaluators Jun 7, 2019

lpappone requested review from jordangarcia, mjc1283, nchilada and mikeproeng37 June 7, 2019 22:15

mjc1283 reviewed Jun 11, 2019

View reviewed changes

mikeproeng37 reviewed Jun 11, 2019

View reviewed changes

lpappone added 3 commits June 11, 2019 16:14

use singleton logger

fcbc80e

fix tests

f7e8816

fix lint errors

5ba2116

nchilada suggested changes Jun 13, 2019

View reviewed changes

nchilada reviewed Jun 14, 2019

View reviewed changes

packages/optimizely-sdk/lib/core/audience_evaluator/index.js Show resolved Hide resolved

lpappone and others added 2 commits June 17, 2019 10:39

Update packages/optimizely-sdk/lib/utils/enums/index.js

e4fd82e

Co-Authored-By: Nikhil Chelliah <[email protected]>

put back some accidentally deleted things

b68e444

nchilada suggested changes Jun 17, 2019

View reviewed changes

mikeproeng37 approved these changes Jun 17, 2019

View reviewed changes

jamesopti commented Jun 17, 2019

View reviewed changes

pass logger to evaluate

69001d7

nchilada reviewed Jun 18, 2019

View reviewed changes

add todo

0eeb31d

nchilada approved these changes Jun 19, 2019

View reviewed changes

Merge branch 'master' into james/custom_condition_evaluators_OG

4da5d4a

mikeproeng37 removed the request for review from jordangarcia June 19, 2019 22:24

mikeproeng37 merged commit ed884fb into master Jun 19, 2019

nchilada deleted the james/custom_condition_evaluators_OG branch June 24, 2019 18:49

jamesopti mentioned this pull request Jan 24, 2020

chore (Regression Test)[CJS-3815]: Add tests for passing in a custom condition evaluator #389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(AudienceEvaluator): Add the ability to provide custom condition evaluators #288

feat(AudienceEvaluator): Add the ability to provide custom condition evaluators #288

jamesopti commented Jun 7, 2019 •

edited by nchilada

Loading

coveralls commented Jun 7, 2019 •

edited

Loading

lpappone Jun 7, 2019

mjc1283 Jun 11, 2019

mikeproeng37 Jun 11, 2019

mikeproeng37 left a comment

mikeproeng37 Jun 11, 2019

nchilada left a comment •

edited

Loading

lpappone commented Jun 14, 2019

nchilada left a comment

nchilada commented Jun 14, 2019

lpappone commented Jun 17, 2019

nchilada left a comment

nchilada Jun 17, 2019

jamesopti Jun 17, 2019 •

edited by nchilada

Loading

nchilada Jun 18, 2019

lpappone Jun 19, 2019

nchilada Jun 19, 2019

mikeproeng37 left a comment

jamesopti left a comment

jamesopti Jun 17, 2019 •

edited by nchilada

Loading

lpappone commented Jun 18, 2019

nchilada left a comment

nchilada Jun 18, 2019

lpappone commented Jun 19, 2019

lpappone commented Jun 19, 2019

nchilada left a comment

-    return evaluator.evaluate(condition, userAttributes);
+    return evaluator.evaluate({
+      condition: condition,
+      userAttributes: userAttributes,
+    });

feat(AudienceEvaluator): Add the ability to provide custom condition evaluators #288

feat(AudienceEvaluator): Add the ability to provide custom condition evaluators #288

Conversation

jamesopti commented Jun 7, 2019 • edited by nchilada Loading

Summary

Test plan

Issues

coveralls commented Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeproeng37 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchilada left a comment • edited Loading

Choose a reason for hiding this comment

lpappone commented Jun 14, 2019

nchilada left a comment

Choose a reason for hiding this comment

nchilada commented Jun 14, 2019

lpappone commented Jun 17, 2019

nchilada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesopti Jun 17, 2019 • edited by nchilada Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeproeng37 left a comment

Choose a reason for hiding this comment

jamesopti left a comment

Choose a reason for hiding this comment

jamesopti Jun 17, 2019 • edited by nchilada Loading

Choose a reason for hiding this comment

lpappone commented Jun 18, 2019

nchilada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpappone commented Jun 19, 2019

lpappone commented Jun 19, 2019

nchilada left a comment

Choose a reason for hiding this comment

jamesopti commented Jun 7, 2019 •

edited by nchilada

Loading

coveralls commented Jun 7, 2019 •

edited

Loading

nchilada left a comment •

edited

Loading

jamesopti Jun 17, 2019 •

edited by nchilada

Loading

jamesopti Jun 17, 2019 •

edited by nchilada

Loading