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.

Sign up for GitHub

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

Jump to bottom

feature (multi-rollout): Added support for multiple rollouts. #247

Merged
merged 7 commits into from
Apr 1, 2020
Merged

feature (multi-rollout): Added support for multiple rollouts. #247

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c182a86
Initial commit.
yasirfolio3 Mar 25, 2020
b047d85
tests added.
yasirfolio3 Mar 26, 2020
83dd14a
Unit tests added.
yasirfolio3 Mar 27, 2020
ebcf60a
Merge branch 'master' into yasir/targeted-rollout
yasirfolio3 Mar 27, 2020
db3666f
nit fixed.
yasirfolio3 Mar 31, 2020
edced35
Updated implementation.
yasirfolio3 Apr 1, 2020
760a86c
Updated local helper function parameters to references.
yasirfolio3 Apr 1, 2020
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
46 changes: 44 additions & 2 deletions pkg/decision/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/****************************************************************************
* Copyright 2019, Optimizely, Inc. and contributors *
* Copyright 2019-2020, Optimizely, Inc. and contributors *
* *
* Licensed under the Apache License, Version 2.0 (the "License"); *
* you may not use this file except in compliance with the License. *
Expand Down Expand Up @@ -150,6 +150,48 @@ var testExp1112 = entities.Experiment{
entities.Range{EntityID: "2222", EndOfRange: 10000},
},
}
var testExp1117Var2223 = entities.Variation{ID: "2223", Key: "2223"}
var testAudience5556 = entities.Audience{ID: "5556"}
var testExp1117 = entities.Experiment{
AudienceConditionTree: &entities.TreeNode{
Operator: "and",
Nodes: []*entities.TreeNode{
&entities.TreeNode{Item: "test_audience_5556"},
},
},
ID: "1117",
Key: testExp1111Key,
Variations: map[string]entities.Variation{
"2223": testExp1117Var2223,
},
VariationKeyToIDMap: map[string]string{
"2223": "2223",
},
TrafficAllocation: []entities.Range{
entities.Range{EntityID: "2223", EndOfRange: 10000},
},
}
var testExp1118Var2224 = entities.Variation{ID: "2224", Key: "2224"}
var testAudience5557 = entities.Audience{ID: "5557"}
var testExp1118 = entities.Experiment{
AudienceConditionTree: &entities.TreeNode{
Operator: "and",
Nodes: []*entities.TreeNode{
&entities.TreeNode{Item: "test_audience_5557"},
},
},
ID: "1118",
Key: testExp1111Key,
Variations: map[string]entities.Variation{
"2224": testExp1118Var2224,
},
VariationKeyToIDMap: map[string]string{
"2224": "2224",
},
TrafficAllocation: []entities.Range{
entities.Range{EntityID: "2224", EndOfRange: 10000},
},
}

const testFeatRollout3334Key = "test_feature_rollout_3334_key"

Expand All @@ -158,7 +200,7 @@ var testFeatRollout3334 = entities.Feature{
Key: testFeatRollout3334Key,
Rollout: entities.Rollout{
ID: "4444",
Experiments: []entities.Experiment{testExp1112},
Experiments: []entities.Experiment{testExp1112, testExp1117, testExp1118},
},
}

Expand Down
72 changes: 39 additions & 33 deletions pkg/decision/rollout_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,22 @@ import (

"github.com/optimizely/go-sdk/pkg/decision/evaluator"
"github.com/optimizely/go-sdk/pkg/decision/reasons"
"github.com/optimizely/go-sdk/pkg/entities"
"github.com/optimizely/go-sdk/pkg/logging"

"github.com/optimizely/go-sdk/pkg/entities"
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert it.

)

// RolloutService makes a feature decision for a given feature rollout
type RolloutService struct {
audienceTreeEvaluator evaluator.TreeEvaluator
experimentBucketerService ExperimentService
logger logging.OptimizelyLogProducer
logger logging.OptimizelyLogProducer
}

// NewRolloutService returns a new instance of the Rollout service
func NewRolloutService(sdkKey string) *RolloutService {
return &RolloutService{
logger:logging.GetLogger(sdkKey, "RolloutService"),
logger: logging.GetLogger(sdkKey, "RolloutService"),
audienceTreeEvaluator: evaluator.NewMixedTreeEvaluator(),
experimentBucketerService: NewExperimentBucketerService(logging.GetLogger(sdkKey, "ExperimentBucketerService")),
}
Expand All @@ -60,38 +61,43 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user
return featureDecision, nil
}

// For now, Rollouts is just a single experiment layer
experiment := rollout.Experiments[0]
experimentDecisionContext := ExperimentDecisionContext{
Experiment: &experiment,
ProjectConfig: decisionContext.ProjectConfig,
}

// if user fails rollout targeting rule we return out of it
if experiment.AudienceConditionTree != nil {
condTreeParams := entities.NewTreeParameters(&userContext, decisionContext.ProjectConfig.GetAudienceMap())
evalResult, _ := r.audienceTreeEvaluator.Evaluate(experiment.AudienceConditionTree, condTreeParams)
if !evalResult {
featureDecision.Reason = reasons.FailedRolloutTargeting
r.logger.Debug(fmt.Sprintf(`User "%s" failed targeting for feature rollout with key "%s".`, userContext.ID, feature.Key))
return featureDecision, nil
index := 0
for index < numberOfExperiments {
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be numberOfExperiments-1

experiment := rollout.Experiments[index]
experimentDecisionContext := ExperimentDecisionContext{
Experiment: &experiment,
ProjectConfig: decisionContext.ProjectConfig,
}
if experiment.AudienceConditionTree != nil {
condTreeParams := entities.NewTreeParameters(&userContext, decisionContext.ProjectConfig.GetAudienceMap())
evalResult, _ := r.audienceTreeEvaluator.Evaluate(experiment.AudienceConditionTree, condTreeParams)
if !evalResult { // Evaluate this user for the next rule
featureDecision.Reason = reasons.FailedRolloutTargeting
r.logger.Debug(fmt.Sprintf(`User "%s" failed targeting for feature rollout with key "%s".`, userContext.ID, feature.Key))
Copy link
Contributor

Choose a reason for hiding this comment

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

are we loggin in other sdks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we do.

index++
continue
}
}
decision, _ := r.experimentBucketerService.GetDecision(experimentDecisionContext, userContext)
if decision.Variation == nil && index < numberOfExperiments-1 {
// Evaluate fall back rule / last rule now
index = numberOfExperiments - 1
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be break

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add everyoneElse rule.

}
// translate the experiment reason into a more rollouts-appropriate reason
switch decision.Reason {
case reasons.NotBucketedIntoVariation:
featureDecision.Decision = Decision{Reason: reasons.FailedRolloutBucketing}
case reasons.BucketedIntoVariation:
featureDecision.Decision = Decision{Reason: reasons.BucketedIntoRollout}
default:
featureDecision.Decision = decision.Decision
}
}

decision, _ := r.experimentBucketerService.GetDecision(experimentDecisionContext, userContext)
// translate the experiment reason into a more rollouts-appropriate reason
switch decision.Reason {
case reasons.NotBucketedIntoVariation:
featureDecision.Decision = Decision{Reason: reasons.FailedRolloutBucketing}
case reasons.BucketedIntoVariation:
featureDecision.Decision = Decision{Reason: reasons.BucketedIntoRollout}
default:
featureDecision.Decision = decision.Decision
featureDecision.Experiment = experiment
featureDecision.Variation = decision.Variation
r.logger.Debug(fmt.Sprintf(`Decision made for user "%s" for feature rollout with key "%s": %s.`, userContext.ID, feature.Key, featureDecision.Reason))
break
}

featureDecision.Experiment = experiment
featureDecision.Variation = decision.Variation
r.logger.Debug(fmt.Sprintf(`Decision made for user "%s" for feature rollout with key "%s": %s.`, userContext.ID, feature.Key, featureDecision.Reason))

return featureDecision, nil
}
150 changes: 129 additions & 21 deletions pkg/decision/rollout_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/****************************************************************************
* Copyright 2019, Optimizely, Inc. and contributors *
* Copyright 2019-2020, Optimizely, Inc. and contributors *
* *
* Licensed under the Apache License, Version 2.0 (the "License"); *
* you may not use this file except in compliance with the License. *
Expand All @@ -26,25 +26,24 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/suite"

)

type RolloutServiceTestSuite struct {
suite.Suite
mockConfig *mockProjectConfig
mockAudienceTreeEvaluator *MockAudienceTreeEvaluator
mockExperimentService *MockExperimentDecisionService
testExperimentDecisionContext ExperimentDecisionContext
testFeatureDecisionContext FeatureDecisionContext
testConditionTreeParams *entities.TreeParameters
testUserContext entities.UserContext
mockConfig *mockProjectConfig
mockAudienceTreeEvaluator *MockAudienceTreeEvaluator
mockExperimentService *MockExperimentDecisionService
testExperiment1112DecisionContext ExperimentDecisionContext
Copy link
Contributor

Choose a reason for hiding this comment

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

what is it 1112?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it represents the experimentID.

testFeatureDecisionContext FeatureDecisionContext
testConditionTreeParams *entities.TreeParameters
testUserContext entities.UserContext
}

func (s *RolloutServiceTestSuite) SetupTest() {
s.mockConfig = new(mockProjectConfig)
s.mockAudienceTreeEvaluator = new(MockAudienceTreeEvaluator)
s.mockExperimentService = new(MockExperimentDecisionService)
s.testExperimentDecisionContext = ExperimentDecisionContext{
s.testExperiment1112DecisionContext = ExperimentDecisionContext{
Experiment: &testExp1112,
ProjectConfig: s.mockConfig,
}
Expand All @@ -55,6 +54,8 @@ func (s *RolloutServiceTestSuite) SetupTest() {

testAudienceMap := map[string]entities.Audience{
"5555": testAudience5555,
"5556": testAudience5556,
"5557": testAudience5557,
}
s.testUserContext = entities.UserContext{
ID: "test_user",
Expand All @@ -63,19 +64,53 @@ func (s *RolloutServiceTestSuite) SetupTest() {
s.mockConfig.On("GetAudienceMap").Return(testAudienceMap)
}

func (s *RolloutServiceTestSuite) TestGetDecisionWithEmptyRolloutID() {

testRolloutService := RolloutService{}
feature := testFeatRollout3334
feature.Rollout.ID = ""
featureDecisionContext := FeatureDecisionContext{
Feature: &feature,
ProjectConfig: s.mockConfig,
}
expectedFeatureDecision := FeatureDecision{
Source: Rollout,
Decision: Decision{Reason: reasons.NoRolloutForFeature},
}
decision, _ := testRolloutService.GetDecision(featureDecisionContext, s.testUserContext)
s.Equal(expectedFeatureDecision, decision)
}

func (s *RolloutServiceTestSuite) TestGetDecisionWithNoExperiments() {

testRolloutService := RolloutService{}
feature := testFeatRollout3334
feature.Rollout.Experiments = []entities.Experiment{}
featureDecisionContext := FeatureDecisionContext{
Feature: &feature,
ProjectConfig: s.mockConfig,
}
expectedFeatureDecision := FeatureDecision{
Source: Rollout,
Decision: Decision{Reason: reasons.RolloutHasNoExperiments},
}
decision, _ := testRolloutService.GetDecision(featureDecisionContext, s.testUserContext)
s.Equal(expectedFeatureDecision, decision)
}

func (s *RolloutServiceTestSuite) TestGetDecisionHappyPath() {
// Test experiment passes targeting and bucketing
testExperimentBucketerDecision := ExperimentDecision{
Variation: &testExp1112Var2222,
Decision: Decision{Reason: reasons.BucketedIntoVariation},
}
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(true, true)
s.mockExperimentService.On("GetDecision", s.testExperimentDecisionContext, s.testUserContext).Return(testExperimentBucketerDecision, nil)
s.mockExperimentService.On("GetDecision", s.testExperiment1112DecisionContext, s.testUserContext).Return(testExperimentBucketerDecision, nil)

testRolloutService := RolloutService{
audienceTreeEvaluator: s.mockAudienceTreeEvaluator,
experimentBucketerService: s.mockExperimentService,
logger:logging.GetLogger("sdkKey", "RolloutService"),
logger: logging.GetLogger("sdkKey", "RolloutService"),
}
expectedFeatureDecision := FeatureDecision{
Experiment: testExp1112,
Expand All @@ -89,27 +124,98 @@ func (s *RolloutServiceTestSuite) TestGetDecisionHappyPath() {
s.mockExperimentService.AssertExpectations(s.T())
}

func (s *RolloutServiceTestSuite) TestGetDecisionFailsBucketing() {
func (s *RolloutServiceTestSuite) TestGetDecisionFallbacksToLastWhenFailsBucketing() {
// Test experiment passes targeting but not bucketing
testExperimentBucketerDecision := ExperimentDecision{
testExperiment1112BucketerDecision := ExperimentDecision{
Decision: Decision{
Reason: reasons.NotBucketedIntoVariation,
},
}

testExperiment1118BucketerDecision := ExperimentDecision{
Variation: &testExp1118Var2224,
Decision: Decision{Reason: reasons.BucketedIntoVariation},
}
experiment1118DecisionContext := ExperimentDecisionContext{
Experiment: &testExp1118,
ProjectConfig: s.mockConfig,
}
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(true, true)
s.mockExperimentService.On("GetDecision", s.testExperimentDecisionContext, s.testUserContext).Return(testExperimentBucketerDecision, nil)
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1118.AudienceConditionTree, s.testConditionTreeParams).Return(true, true)
s.mockExperimentService.On("GetDecision", s.testExperiment1112DecisionContext, s.testUserContext).Return(testExperiment1112BucketerDecision, nil)
s.mockExperimentService.On("GetDecision", experiment1118DecisionContext, s.testUserContext).Return(testExperiment1118BucketerDecision, nil)

testRolloutService := RolloutService{
audienceTreeEvaluator: s.mockAudienceTreeEvaluator,
experimentBucketerService: s.mockExperimentService,
logger:logging.GetLogger("sdkKey", "RolloutService"),
logger: logging.GetLogger("sdkKey", "RolloutService"),
}
expectedFeatureDecision := FeatureDecision{
Experiment: testExp1118,
Variation: &testExp1118Var2224,
Source: Rollout,
Decision: Decision{Reason: reasons.BucketedIntoRollout},
}
decision, _ := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext)
s.Equal(expectedFeatureDecision, decision)
s.mockAudienceTreeEvaluator.AssertExpectations(s.T())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also assert, number of rolloutrules experiments were in it? and the returned mocked roleout rule was the last one. Also throw exception for all other roleout rules so I can be sure, it didn't iterate once fallen into audience evaluation but didn't bucket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since there are 3 rollout rules experiments in this test, we have only mocked response for Evaluate and GetDecision methods of 1st and last rule, This makes sure that if these methods were called for any other experiments, the test will fail. We assert that the returned rollout rule was the last one already by providing testExp1118 in expectedFeatureDecision.

s.mockExperimentService.AssertExpectations(s.T())
}

func (s *RolloutServiceTestSuite) TestGetDecisionWhenFallbackBucketingFails() {
// Test experiment passes targeting but not bucketing
testExperiment1112BucketerDecision := ExperimentDecision{
Decision: Decision{
Reason: reasons.FailedRolloutBucketing,
Reason: reasons.NotBucketedIntoVariation,
},
Experiment: testExp1112,
}
testExperiment1118DecisionContext := ExperimentDecisionContext{
Experiment: &testExp1118,
ProjectConfig: s.mockConfig,
}
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(true, true)
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1118.AudienceConditionTree, s.testConditionTreeParams).Return(true, true)
s.mockExperimentService.On("GetDecision", s.testExperiment1112DecisionContext, s.testUserContext).Return(testExperiment1112BucketerDecision, nil)
s.mockExperimentService.On("GetDecision", testExperiment1118DecisionContext, s.testUserContext).Return(testExperiment1112BucketerDecision, nil)

testRolloutService := RolloutService{
audienceTreeEvaluator: s.mockAudienceTreeEvaluator,
experimentBucketerService: s.mockExperimentService,
logger: logging.GetLogger("sdkKey", "RolloutService"),
}
expectedFeatureDecision := FeatureDecision{
Experiment: testExp1118,
Source: Rollout,
Decision: Decision{Reason: reasons.FailedRolloutBucketing},
}
decision, _ := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext)
s.Equal(expectedFeatureDecision, decision)
s.mockAudienceTreeEvaluator.AssertExpectations(s.T())
s.mockExperimentService.AssertExpectations(s.T())
}

func (s *RolloutServiceTestSuite) TestEvaluatesNextIfPreviousTargetingFails() {
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(false, true)
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1117.AudienceConditionTree, s.testConditionTreeParams).Return(true, true)
experiment1117DecisionContext := ExperimentDecisionContext{
Experiment: &testExp1117,
ProjectConfig: s.mockConfig,
}
testExperimentBucketerDecision := ExperimentDecision{
Variation: &testExp1117Var2223,
Decision: Decision{Reason: reasons.BucketedIntoVariation},
}
s.mockExperimentService.On("GetDecision", experiment1117DecisionContext, s.testUserContext).Return(testExperimentBucketerDecision, nil)

testRolloutService := RolloutService{
audienceTreeEvaluator: s.mockAudienceTreeEvaluator,
experimentBucketerService: s.mockExperimentService,
logger: logging.GetLogger("sdkKey", "RolloutService"),
}
expectedFeatureDecision := FeatureDecision{
Experiment: testExp1117,
Variation: &testExp1117Var2223,
Source: Rollout,
Decision: Decision{Reason: reasons.BucketedIntoRollout},
}
decision, _ := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext)
s.Equal(expectedFeatureDecision, decision)
Expand All @@ -119,10 +225,12 @@ func (s *RolloutServiceTestSuite) TestGetDecisionFailsBucketing() {

func (s *RolloutServiceTestSuite) TestGetDecisionFailsTargeting() {
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1112.AudienceConditionTree, s.testConditionTreeParams).Return(false, true)
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1117.AudienceConditionTree, s.testConditionTreeParams).Return(false, true)
s.mockAudienceTreeEvaluator.On("Evaluate", testExp1118.AudienceConditionTree, s.testConditionTreeParams).Return(false, true)
testRolloutService := RolloutService{
audienceTreeEvaluator: s.mockAudienceTreeEvaluator,
experimentBucketerService: s.mockExperimentService,
logger:logging.GetLogger("sdkKey", "RolloutService"),
logger: logging.GetLogger("sdkKey", "RolloutService"),
}
expectedFeatureDecision := FeatureDecision{
Decision: Decision{
Expand All @@ -139,7 +247,7 @@ func (s *RolloutServiceTestSuite) TestGetDecisionFailsTargeting() {
func TestNewRolloutService(t *testing.T) {
rolloutService := NewRolloutService("")
assert.IsType(t, &evaluator.MixedTreeEvaluator{}, rolloutService.audienceTreeEvaluator)
assert.IsType(t, &ExperimentBucketerService{logger:logging.GetLogger("sdkKey", "ExperimentBucketerService")}, rolloutService.experimentBucketerService)
assert.IsType(t, &ExperimentBucketerService{logger: logging.GetLogger("sdkKey", "ExperimentBucketerService")}, rolloutService.experimentBucketerService)
}

func TestRolloutServiceTestSuite(t *testing.T) {
Expand Down