-
Notifications
You must be signed in to change notification settings - Fork 83
[FSSDK-11127] implement cmab service #1011
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
Conversation
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.
Looks good! A few suggestions to consider.
/** | ||
* Get variation id for the user | ||
* @param {OptimizelyUserContext} userContext | ||
* @param {string} experimentId |
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.
* @param {string} experimentId | |
* @param {string} ruleId |
getDecision( | ||
projectConfig: ProjectConfig, | ||
userContext: OptimizelyUserContext, | ||
experimentId: string, |
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.
experimentId: string, | |
ruleId: string, |
} | ||
} | ||
|
||
const variation = await this.fetchVariation(ruleId, userContext.getUserId(), filteredAttributes); |
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.
const variation = await this.fetchVariation(ruleId, userContext.getUserId(), filteredAttributes); | |
const variation = await this.fetchDecision(ruleId, userContext.getUserId(), filteredAttributes); |
const cmabAttributeIds = experiment.cmab.attributeIds; | ||
|
||
Object.keys(attributes).forEach((key) => { | ||
const attributeId = projectConfig.attributeKeyMap[key].id; |
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.
What about user attributes not in projectConfig.attributes?
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.
We can start with cmab.attributes? We can avoid walk-through all the user attributes.
In this way, we can also create the fixed order of filtered attributes -
for aId in cmab.attributes {
aKey = project.atrributes[aId]
if aKey in UserContext {
filteredAttribute.add()
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.
Updated.
also added test for attribute order insensitivity.
} | ||
|
||
const cachedValue = await this.cmabCache.get(cacheKey); | ||
const attributesHash = String(murmurhash.v3(JSON.stringify(filteredAttributes))); |
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.
Can we sort by keys? Same set of attributes can be passed in different orders in UserContext?
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.
LGTM
Summary
Test plan
Issues