-
Notifications
You must be signed in to change notification settings - Fork 32
[FSSDK-11172] feat: update decision service to handle CMAB #602
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
Open
muzahidul-opti
wants to merge
22
commits into
master
Choose a base branch
from
muzahid/cmab-decision
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,920
−119
Open
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
b40adda
wip: cmab client done
muzahidul-opti 9c9b9b0
Update test cases
muzahidul-opti d998e0b
wip: add test cases for cmab services
muzahidul-opti c427a46
Merge branch 'master' into muzahid/cmab-service
muzahidul-opti 8e168f6
Added log for cmab decision
muzahidul-opti 0c4d072
Update copyright date
muzahidul-opti f342ae2
CMAB decision implemented
muzahidul-opti ebbde3c
CmabService testcases added for sync getDecision function
muzahidul-opti 92d3140
Add factory method for DefaultCmabService
muzahidul-opti c6507b0
DefaultDecision initializer updated
muzahidul-opti b51c228
Add operation type enum
muzahidul-opti e9d01ac
CMAB not supported in sync mode
muzahidul-opti e827b27
Reuse experiment bucketing logic
muzahidul-opti 445006b
Add seperate method for group exlusion, add bucketToEntityId method
muzahidul-opti b9ce0f5
Update bucketing logic for cmab experiment
muzahidul-opti 093e372
Add test cases for cmab experiement
muzahidul-opti f2cc7df
Add test cases for cmab decision options
muzahidul-opti f0c1d5b
Merge branch 'master' into muzahid/cmab-decision
muzahidul-opti 384bfa8
Add test cases for bucketToEntity
muzahidul-opti 87bcecf
Return feature dicision with nil variation for CMAB fetch error
muzahidul-opti d1067d6
Add code doc and fix linting issue
muzahidul-opti 0d761c9
Update cmab entity id matching logic
muzahidul-opti File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,4 +71,7 @@ extension Experiment { | |
return status == .running | ||
} | ||
|
||
var isCmab: Bool { | ||
return cmab != nil | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,43 +37,16 @@ class DefaultBucketer: OPTBucketer { | |
bucketingId: String) -> DecisionResponse<Variation> { | ||
let reasons = DecisionReasons() | ||
|
||
var mutexAllowed = true | ||
|
||
// check for mutex | ||
|
||
let group = config.project.groups.filter { $0.getExperiment(id: experiment.id) != nil }.first | ||
|
||
if let group = group { | ||
switch group.policy { | ||
case .overlapping: | ||
break | ||
case .random: | ||
let decisionResponse = bucketToExperiment(config: config, | ||
group: group, | ||
bucketingId: bucketingId) | ||
reasons.merge(decisionResponse.reasons) | ||
if let mutexExperiment = decisionResponse.result { | ||
if mutexExperiment.id == experiment.id { | ||
mutexAllowed = true | ||
|
||
let info = LogMessage.userBucketedIntoExperimentInGroup(bucketingId, experiment.key, group.id) | ||
logger.i(info) | ||
reasons.addInfo(info) | ||
} else { | ||
mutexAllowed = false | ||
|
||
let info = LogMessage.userNotBucketedIntoExperimentInGroup(bucketingId, experiment.key, group.id) | ||
logger.i(info) | ||
reasons.addInfo(info) | ||
} | ||
} else { | ||
mutexAllowed = false | ||
|
||
let info = LogMessage.userNotBucketedIntoAnyExperimentInGroup(bucketingId, group.id) | ||
logger.i(info) | ||
reasons.addInfo(info) | ||
} | ||
} | ||
// Check mutex rules | ||
let mutexAllowed = checkMutexRules( | ||
config: config, | ||
experiment: experiment, | ||
bucketingId: bucketingId, | ||
reasons: reasons | ||
) | ||
|
||
if !mutexAllowed { | ||
return DecisionResponse(result: nil, reasons: reasons) | ||
} | ||
|
||
if !mutexAllowed { return DecisionResponse(result: nil, reasons: reasons) } | ||
Comment on lines
+48
to
52
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. clean up redundant? 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. Yes, reuse the mutex logic. |
||
|
@@ -120,6 +93,83 @@ class DefaultBucketer: OPTBucketer { | |
return DecisionResponse(result: nil, reasons: reasons) | ||
} | ||
|
||
/// Checks if an experiment is allowed to run based on mutex rules | ||
/// - Parameters: | ||
/// - config: The project configuration | ||
/// - experiment: The experiment to check | ||
/// - bucketingId: The bucketing ID for the user | ||
/// - reasons: Decision reasons to track the mutex check process | ||
/// - Returns: A boolean indicating if the experiment is allowed to run | ||
private func checkMutexRules( | ||
config: ProjectConfig, | ||
experiment: Experiment, | ||
bucketingId: String, | ||
reasons: DecisionReasons | ||
) -> Bool { | ||
// Find the group containing this experiment | ||
let group = config.project.groups.filter { $0.getExperiment(id: experiment.id) != nil }.first | ||
|
||
guard let group = group else { | ||
return true // No group found, experiment is allowed | ||
} | ||
|
||
switch group.policy { | ||
case .overlapping: | ||
return true // Overlapping experiments are always allowed | ||
|
||
case .random: | ||
let decisionResponse = bucketToExperiment( | ||
config: config, | ||
group: group, | ||
bucketingId: bucketingId | ||
) | ||
reasons.merge(decisionResponse.reasons) | ||
|
||
guard let mutexExperiment = decisionResponse.result else { | ||
let info = LogMessage.userNotBucketedIntoAnyExperimentInGroup(bucketingId, group.id) | ||
logger.i(info) | ||
reasons.addInfo(info) | ||
return false | ||
} | ||
|
||
let isAllowed = mutexExperiment.id == experiment.id | ||
let info = isAllowed | ||
? LogMessage.userBucketedIntoExperimentInGroup(bucketingId, experiment.key, group.id) | ||
: LogMessage.userNotBucketedIntoExperimentInGroup(bucketingId, experiment.key, group.id) | ||
|
||
logger.i(info) | ||
reasons.addInfo(info) | ||
return isAllowed | ||
} | ||
} | ||
|
||
func bucketToEntityId(config: ProjectConfig, | ||
experiment: Experiment, | ||
bucketingId: String, | ||
trafficAllocation: [TrafficAllocation]) -> DecisionResponse<String> { | ||
|
||
let reasons = DecisionReasons() | ||
|
||
// Check mutex rules | ||
let mutexAllowed = checkMutexRules( | ||
config: config, | ||
experiment: experiment, | ||
bucketingId: bucketingId, | ||
reasons: reasons | ||
) | ||
|
||
if !mutexAllowed { | ||
return DecisionResponse(result: nil, reasons: reasons) | ||
} | ||
|
||
let hashId = makeHashIdFromBucketingId(bucketingId: bucketingId, entityId: experiment.id) | ||
let bucketValue = self.generateBucketValue(bucketingId: hashId) | ||
|
||
let entityId = allocateTraffic(trafficAllocation: trafficAllocation, bucketValue: bucketValue) | ||
|
||
return DecisionResponse(result: entityId, reasons: reasons) | ||
} | ||
|
||
func bucketToVariation(experiment: ExperimentCore, | ||
bucketingId: String) -> DecisionResponse<Variation> { | ||
let reasons = DecisionReasons() | ||
|
@@ -153,7 +203,7 @@ class DefaultBucketer: OPTBucketer { | |
for bucket in trafficAllocation where bucketValue < bucket.endOfRange { | ||
return bucket.entityId | ||
} | ||
|
||
return nil | ||
} | ||
|
||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 like this timeout is too large - is this consistent for all SDKs? @raju-opti