From fa460ea326e5436619ce4c41644e02cbafbb789d Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Tue, 29 Oct 2019 14:21:47 -0700 Subject: [PATCH 01/16] Override experiment service --- pkg/decision/composite_experiment_service.go | 29 +++++++-- pkg/decision/experiment_override_service.go | 68 ++++++++++++++++++++ pkg/decision/reasons/reason.go | 6 ++ 3 files changed, 98 insertions(+), 5 deletions(-) create mode 100644 pkg/decision/experiment_override_service.go diff --git a/pkg/decision/composite_experiment_service.go b/pkg/decision/composite_experiment_service.go index 37eaf95aa..4accae0d9 100644 --- a/pkg/decision/composite_experiment_service.go +++ b/pkg/decision/composite_experiment_service.go @@ -32,18 +32,37 @@ type CompositeExperimentService struct { experimentServices []ExperimentService } +// CESOptionFunc allows optional customization of the CompositeExperimentService returned from NewCompositeExperimentService +type CESOptionFunc func(*CompositeExperimentService) + +// WithOverrides prepends an ExperimentOverrideService of the argument map to service.experimentServices +func WithOverrides(overrides map[OverrideKey]string) CESOptionFunc { + return func(service *CompositeExperimentService) { + service.experimentServices = append( + []ExperimentService{NewExperimentOverrideService(overrides)}, + service.experimentServices..., + ) + } +} + // NewCompositeExperimentService creates a new instance of the CompositeExperimentService -func NewCompositeExperimentService() *CompositeExperimentService { +func NewCompositeExperimentService(options ...CESOptionFunc) *CompositeExperimentService { // These decision services are applied in order: - // 1. Whitelist - // 2. Bucketing - // @TODO(mng): Prepend forced variation - return &CompositeExperimentService{ + // 1. Overrides (only when the WithOverrides option is used) + // 2. Whitelist + // 3. Bucketing + compositeExperimentService := &CompositeExperimentService{ experimentServices: []ExperimentService{ NewExperimentWhitelistService(), NewExperimentBucketerService(), }, } + + for _, option := range options { + option(compositeExperimentService) + } + + return compositeExperimentService } // GetDecision returns a decision for the given experiment and user context diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go new file mode 100644 index 000000000..9a21982ed --- /dev/null +++ b/pkg/decision/experiment_override_service.go @@ -0,0 +1,68 @@ +/**************************************************************************** + * Copyright 2019, 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. * + * You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, software * + * distributed under the License is distributed on an "AS IS" BASIS, * + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * + * See the License for the specific language governing permissions and * + * limitations under the License. * + ***************************************************************************/ + +// Package decision // +package decision + +import ( + "errors" + + "github.com/optimizely/go-sdk/pkg/decision/reasons" + "github.com/optimizely/go-sdk/pkg/entities" +) + +// OverrideKey is the type of keys in the Overrides map of ExperimentOverrideService +type OverrideKey struct { + experiment, user string +} + +// ExperimentOverrideService makes a decision using a given map of (experiment key, user id) to variation keys +// Implements the ExperimentService interface +type ExperimentOverrideService struct { + Overrides map[OverrideKey]string +} + +// NewExperimentOverrideService returns a pointer to an initialized ExperimentOverrideService +func NewExperimentOverrideService(overrides map[OverrideKey]string) *ExperimentOverrideService { + return &ExperimentOverrideService{ + Overrides: overrides, + } +} + +// GetDecision returns a decision with a variation when a variation assignment is found in the configured overrides for the given user and experiment +func (s ExperimentOverrideService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) { + decision := ExperimentDecision{} + + if decisionContext.Experiment == nil { + return decision, errors.New("decisionContext Experiment is nil") + } + + variationKey, ok := s.Overrides[OverrideKey{experiment: decisionContext.Experiment.Key, user: userContext.ID}] + if !ok { + decision.Reason = reasons.NoOverrideVariationForUser + return decision, nil + } + + variation, ok := decisionContext.Experiment.Variations[variationKey] + if !ok { + decision.Reason = reasons.InvalidOverrideVariationForUser + return decision, nil + } + + decision.Variation = &variation + decision.Reason = reasons.OverrideVariationFound + return decision, nil +} diff --git a/pkg/decision/reasons/reason.go b/pkg/decision/reasons/reason.go index cb01775ea..4ca7ddf2f 100644 --- a/pkg/decision/reasons/reason.go +++ b/pkg/decision/reasons/reason.go @@ -43,4 +43,10 @@ const ( InvalidWhitelistVariationAssignment Reason = "Invalid whitelist variation assignment" // WhitelistVariationAssignmentFound - a valid variation assignment was found for the given user and experiment WhitelistVariationAssignmentFound Reason = "Whitelist variation assignment found" + // NoOverrideVariationForUser - No override variation was found for the given user and experiment + NoOverrideVariationForUser Reason = "No override variation for user" + // InvalidOverrideVariationForUser - An override variation was found for the given user and experiment, but no variation with that key exists in the given experiment + InvalidOverrideVariationForUser Reason = "No override variation for user" + // OverrideVariationFound - A valid override variation was found for the given user and experiment + OverrideVariationFound Reason = "Override variation found" ) From 5d78a4d414e0aa797ef7c6a309d6183374a709fc Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Tue, 29 Oct 2019 14:36:49 -0700 Subject: [PATCH 02/16] Basic test for experiment overrides --- .../experiment_override_service_test.go | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 pkg/decision/experiment_override_service_test.go diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go new file mode 100644 index 000000000..a38d634e6 --- /dev/null +++ b/pkg/decision/experiment_override_service_test.go @@ -0,0 +1,60 @@ +/**************************************************************************** + * Copyright 2019, 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. * + * You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, software * + * distributed under the License is distributed on an "AS IS" BASIS, * + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * + * See the License for the specific language governing permissions and * + * limitations under the License. * + ***************************************************************************/ + +// Package decision // +package decision + +import ( + "testing" + + "github.com/optimizely/go-sdk/pkg/entities" + "github.com/stretchr/testify/suite" +) + +type ExperimentOverrideServiceTestSuite struct { + suite.Suite + mockConfig *mockProjectConfig + overrides map[OverrideKey]string + overrideService *ExperimentOverrideService +} + +func (s *ExperimentOverrideServiceTestSuite) SetupTest() { + s.mockConfig = new(mockProjectConfig) + s.overrides = make(map[OverrideKey]string) + s.overrideService = NewExperimentOverrideService(s.overrides) +} + +func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { + testDecisionContext := ExperimentDecisionContext{ + Experiment: &testExp1111, + ProjectConfig: s.mockConfig, + } + + testUserContext := entities.UserContext{ + ID: "test_user_1", + } + + s.overrides[OverrideKey{experiment: testExp1111.Key, user: testUserContext.ID}] = testExp1111Var2222.Key + + decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) + + s.NoError(err) + s.NotNil(decision.Variation) +} + +func TestExperimentOverridesTestSuite(t *testing.T) { + suite.Run(t, new(ExperimentOverrideServiceTestSuite)) +} From ecd5bf877948a934df46af0a26a25b599381ea5c Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Tue, 29 Oct 2019 15:57:55 -0700 Subject: [PATCH 03/16] Fix override service. Variations in experiment are keyed by id. --- pkg/decision/composite_service.go | 20 +++++++++++++++-- pkg/decision/experiment_override_service.go | 22 ++++++++++++------- .../experiment_override_service_test.go | 2 +- pkg/decision/reasons/reason.go | 6 ++--- 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/pkg/decision/composite_service.go b/pkg/decision/composite_service.go index 5f893d62a..0317f22d5 100644 --- a/pkg/decision/composite_service.go +++ b/pkg/decision/composite_service.go @@ -35,16 +35,32 @@ type CompositeService struct { notificationCenter notification.Center } +// CSOptionFunc allows customization of the CompositeService returned from NewCompositeService +type CSOptionFunc func(*CompositeService) + +// WithCompositeExperimentService sets a custom compositeExperimentService +func WithCompositeExperimentService(compositeExperimentService *CompositeExperimentService) CSOptionFunc { + return func(service *CompositeService) { + service.compositeExperimentService = compositeExperimentService + } +} + // NewCompositeService returns a new instance of the CompositeService with the defaults -func NewCompositeService(sdkKey string) *CompositeService { +func NewCompositeService(sdkKey string, options ...CSOptionFunc) *CompositeService { // @TODO: add factory method with option funcs to accept custom feature and experiment services compositeExperimentService := NewCompositeExperimentService() compositeFeatureDecisionService := NewCompositeFeatureService(compositeExperimentService) - return &CompositeService{ + compositeService := &CompositeService{ compositeExperimentService: compositeExperimentService, compositeFeatureService: compositeFeatureDecisionService, notificationCenter: registry.GetNotificationCenter(sdkKey), } + + for _, option := range options { + option(compositeService) + } + + return compositeService } // GetFeatureDecision returns a decision for the given feature key diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index 9a21982ed..b2cc6e9fe 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -19,14 +19,18 @@ package decision import ( "errors" + "fmt" "github.com/optimizely/go-sdk/pkg/decision/reasons" "github.com/optimizely/go-sdk/pkg/entities" + "github.com/optimizely/go-sdk/pkg/logging" ) +var eosLogger = logging.GetLogger("ExperimentOverrideService") + // OverrideKey is the type of keys in the Overrides map of ExperimentOverrideService type OverrideKey struct { - experiment, user string + Experiment, User string } // ExperimentOverrideService makes a decision using a given map of (experiment key, user id) to variation keys @@ -50,19 +54,21 @@ func (s ExperimentOverrideService) GetDecision(decisionContext ExperimentDecisio return decision, errors.New("decisionContext Experiment is nil") } - variationKey, ok := s.Overrides[OverrideKey{experiment: decisionContext.Experiment.Key, user: userContext.ID}] + variationKey, ok := s.Overrides[OverrideKey{Experiment: decisionContext.Experiment.Key, User: userContext.ID}] if !ok { decision.Reason = reasons.NoOverrideVariationForUser return decision, nil } - variation, ok := decisionContext.Experiment.Variations[variationKey] - if !ok { - decision.Reason = reasons.InvalidOverrideVariationForUser - return decision, nil + for _, variation := range decisionContext.Experiment.Variations { + if variation.Key == variationKey { + decision.Variation = &variation + decision.Reason = reasons.OverrideVariationFound + eosLogger.Info(fmt.Sprintf("Override variation %v found for user %v", variationKey, userContext.ID)) + return decision, nil + } } - decision.Variation = &variation - decision.Reason = reasons.OverrideVariationFound + decision.Reason = reasons.InvalidOverrideVariationForUser return decision, nil } diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go index a38d634e6..a1ed166b6 100644 --- a/pkg/decision/experiment_override_service_test.go +++ b/pkg/decision/experiment_override_service_test.go @@ -47,7 +47,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { ID: "test_user_1", } - s.overrides[OverrideKey{experiment: testExp1111.Key, user: testUserContext.ID}] = testExp1111Var2222.Key + s.overrides[OverrideKey{Experiment: testExp1111.Key, User: testUserContext.ID}] = testExp1111Var2222.Key decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) diff --git a/pkg/decision/reasons/reason.go b/pkg/decision/reasons/reason.go index 4ca7ddf2f..01cdf8e42 100644 --- a/pkg/decision/reasons/reason.go +++ b/pkg/decision/reasons/reason.go @@ -44,9 +44,9 @@ const ( // WhitelistVariationAssignmentFound - a valid variation assignment was found for the given user and experiment WhitelistVariationAssignmentFound Reason = "Whitelist variation assignment found" // NoOverrideVariationForUser - No override variation was found for the given user and experiment - NoOverrideVariationForUser Reason = "No override variation for user" + NoOverrideVariationForUser Reason = "No override variation assignment" // InvalidOverrideVariationForUser - An override variation was found for the given user and experiment, but no variation with that key exists in the given experiment - InvalidOverrideVariationForUser Reason = "No override variation for user" + InvalidOverrideVariationForUser Reason = "Invalid override variation assignment" // OverrideVariationFound - A valid override variation was found for the given user and experiment - OverrideVariationFound Reason = "Override variation found" + OverrideVariationFound Reason = "Override variation assignment found" ) From 32f282222e804bc0362da8472ece0154ea18763a Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Wed, 30 Oct 2019 14:09:57 -0700 Subject: [PATCH 04/16] Change option function to connect overrides map directly when calling NewCompositeService --- pkg/decision/composite_service.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/decision/composite_service.go b/pkg/decision/composite_service.go index 0317f22d5..797b2703a 100644 --- a/pkg/decision/composite_service.go +++ b/pkg/decision/composite_service.go @@ -38,10 +38,12 @@ type CompositeService struct { // CSOptionFunc allows customization of the CompositeService returned from NewCompositeService type CSOptionFunc func(*CompositeService) -// WithCompositeExperimentService sets a custom compositeExperimentService -func WithCompositeExperimentService(compositeExperimentService *CompositeExperimentService) CSOptionFunc { +// WithExperimentOverrides applies the argument experiment overrides to a composite service +func WithExperimentOverrides(experimentOverrides map[OverrideKey]string) CSOptionFunc { return func(service *CompositeService) { - service.compositeExperimentService = compositeExperimentService + service.compositeExperimentService = NewCompositeExperimentService( + WithOverrides(experimentOverrides), + ) } } From 94a28e70e53ca7d482c7f2655a59b8372f05b9a8 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Wed, 30 Oct 2019 16:07:20 -0700 Subject: [PATCH 05/16] Add main example --- examples/main.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/examples/main.go b/examples/main.go index ae0738b5c..eeec9aea7 100644 --- a/examples/main.go +++ b/examples/main.go @@ -8,6 +8,7 @@ import ( "time" "github.com/optimizely/go-sdk/pkg/client" + "github.com/optimizely/go-sdk/pkg/decision" "github.com/optimizely/go-sdk/pkg/entities" "github.com/optimizely/go-sdk/pkg/event" "github.com/optimizely/go-sdk/pkg/logging" @@ -67,4 +68,24 @@ func main() { client.WithBatchEventProcessor(event.DefaultBatchSize, event.DefaultEventQueueSize, event.DefaultEventFlushInterval), ) optimizelyClient.Close() + + /************* Setting experiment overrides (a.k.a. "forced variations") ********************/ + overrideKey := decision.OverrideKey{ + Experiment: "aaaa", + User: "Matt", + } + overrides := map[decision.OverrideKey]string{ + overrideKey: "variation_1", + } + compositeService := decision.NewCompositeService( + sdkKey, + decision.WithExperimentOverrides(overrides), + ) + optimizelyClient, _ = optimizelyFactory.Client( + client.WithDecisionService(compositeService), + ) + // Optimizely client now has "variation_1" forced for user "Matt" in experiment "aaaa" + // The forced variation will work regardless of whether "aaaa" is an A/B test or a Feature Test. + fmt.Printf("Is feature enabled? %v\n", enabled) + } From 9573948293d206ee43f98b179a4eb53633ed6ab9 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Thu, 31 Oct 2019 16:15:53 -0700 Subject: [PATCH 06/16] Use OverridesStore interface instead of map --- pkg/decision/composite_experiment_service.go | 2 +- pkg/decision/composite_service.go | 2 +- pkg/decision/experiment_override_service.go | 23 ++++++++++++++++--- .../experiment_override_service_test.go | 10 ++++---- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/pkg/decision/composite_experiment_service.go b/pkg/decision/composite_experiment_service.go index 4accae0d9..d02e26425 100644 --- a/pkg/decision/composite_experiment_service.go +++ b/pkg/decision/composite_experiment_service.go @@ -36,7 +36,7 @@ type CompositeExperimentService struct { type CESOptionFunc func(*CompositeExperimentService) // WithOverrides prepends an ExperimentOverrideService of the argument map to service.experimentServices -func WithOverrides(overrides map[OverrideKey]string) CESOptionFunc { +func WithOverrides(overrides OverrideStore) CESOptionFunc { return func(service *CompositeExperimentService) { service.experimentServices = append( []ExperimentService{NewExperimentOverrideService(overrides)}, diff --git a/pkg/decision/composite_service.go b/pkg/decision/composite_service.go index 797b2703a..0de1849bd 100644 --- a/pkg/decision/composite_service.go +++ b/pkg/decision/composite_service.go @@ -39,7 +39,7 @@ type CompositeService struct { type CSOptionFunc func(*CompositeService) // WithExperimentOverrides applies the argument experiment overrides to a composite service -func WithExperimentOverrides(experimentOverrides map[OverrideKey]string) CSOptionFunc { +func WithExperimentOverrides(experimentOverrides OverrideStore) CSOptionFunc { return func(service *CompositeService) { service.compositeExperimentService = NewCompositeExperimentService( WithOverrides(experimentOverrides), diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index b2cc6e9fe..5ada96dd2 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -28,19 +28,36 @@ import ( var eosLogger = logging.GetLogger("ExperimentOverrideService") +// OverrideStore provides read access to overrides +type OverrideStore interface { + // Returns a variation associated with overrideKey + GetVariation(overrideKey OverrideKey) (string, bool) +} + // OverrideKey is the type of keys in the Overrides map of ExperimentOverrideService type OverrideKey struct { Experiment, User string } +// MapOverrides is a map-based implementation of OverrideStore +type MapOverrides struct { + overrides map[OverrideKey]string +} + +// GetVariation returns the override associated with the given key in the map +func (m *MapOverrides) GetVariation(overrideKey OverrideKey) (string, bool) { + variationKey, ok := m.overrides[overrideKey] + return variationKey, ok +} + // ExperimentOverrideService makes a decision using a given map of (experiment key, user id) to variation keys // Implements the ExperimentService interface type ExperimentOverrideService struct { - Overrides map[OverrideKey]string + Overrides OverrideStore } // NewExperimentOverrideService returns a pointer to an initialized ExperimentOverrideService -func NewExperimentOverrideService(overrides map[OverrideKey]string) *ExperimentOverrideService { +func NewExperimentOverrideService(overrides OverrideStore) *ExperimentOverrideService { return &ExperimentOverrideService{ Overrides: overrides, } @@ -54,7 +71,7 @@ func (s ExperimentOverrideService) GetDecision(decisionContext ExperimentDecisio return decision, errors.New("decisionContext Experiment is nil") } - variationKey, ok := s.Overrides[OverrideKey{Experiment: decisionContext.Experiment.Key, User: userContext.ID}] + variationKey, ok := s.Overrides.GetVariation(OverrideKey{Experiment: decisionContext.Experiment.Key, User: userContext.ID}) if !ok { decision.Reason = reasons.NoOverrideVariationForUser return decision, nil diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go index a1ed166b6..15440245b 100644 --- a/pkg/decision/experiment_override_service_test.go +++ b/pkg/decision/experiment_override_service_test.go @@ -33,8 +33,12 @@ type ExperimentOverrideServiceTestSuite struct { func (s *ExperimentOverrideServiceTestSuite) SetupTest() { s.mockConfig = new(mockProjectConfig) - s.overrides = make(map[OverrideKey]string) - s.overrideService = NewExperimentOverrideService(s.overrides) + mapOverrides := &MapOverrides{ + overrides: map[OverrideKey]string{ + OverrideKey{Experiment: testExp1111.Key, User: "test_user_1"}: testExp1111Var2222.Key, + }, + } + s.overrideService = NewExperimentOverrideService(mapOverrides) } func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { @@ -47,8 +51,6 @@ func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { ID: "test_user_1", } - s.overrides[OverrideKey{Experiment: testExp1111.Key, User: testUserContext.ID}] = testExp1111Var2222.Key - decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) From 870e509333e3a4e48e45034f8690cb5fc02837b0 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 1 Nov 2019 09:51:47 -0700 Subject: [PATCH 07/16] Fixes --- examples/main.go | 6 ++--- pkg/decision/composite_experiment_service.go | 2 +- pkg/decision/composite_service.go | 8 +++++++ pkg/decision/experiment_override_service.go | 24 +++++++++---------- .../experiment_override_service_test.go | 6 ++--- 5 files changed, 26 insertions(+), 20 deletions(-) diff --git a/examples/main.go b/examples/main.go index eeec9aea7..3e63eb600 100644 --- a/examples/main.go +++ b/examples/main.go @@ -72,20 +72,18 @@ func main() { /************* Setting experiment overrides (a.k.a. "forced variations") ********************/ overrideKey := decision.OverrideKey{ Experiment: "aaaa", - User: "Matt", + UserID: "Matt", } overrides := map[decision.OverrideKey]string{ overrideKey: "variation_1", } compositeService := decision.NewCompositeService( sdkKey, - decision.WithExperimentOverrides(overrides), + decision.WithExperimentOverridesMap(overrides), ) optimizelyClient, _ = optimizelyFactory.Client( client.WithDecisionService(compositeService), ) // Optimizely client now has "variation_1" forced for user "Matt" in experiment "aaaa" // The forced variation will work regardless of whether "aaaa" is an A/B test or a Feature Test. - fmt.Printf("Is feature enabled? %v\n", enabled) - } diff --git a/pkg/decision/composite_experiment_service.go b/pkg/decision/composite_experiment_service.go index d02e26425..885f431ab 100644 --- a/pkg/decision/composite_experiment_service.go +++ b/pkg/decision/composite_experiment_service.go @@ -35,7 +35,7 @@ type CompositeExperimentService struct { // CESOptionFunc allows optional customization of the CompositeExperimentService returned from NewCompositeExperimentService type CESOptionFunc func(*CompositeExperimentService) -// WithOverrides prepends an ExperimentOverrideService of the argument map to service.experimentServices +// WithOverrides prepends an ExperimentOverrideService to service.experimentServices func WithOverrides(overrides OverrideStore) CESOptionFunc { return func(service *CompositeExperimentService) { service.experimentServices = append( diff --git a/pkg/decision/composite_service.go b/pkg/decision/composite_service.go index 0de1849bd..20d3d33a9 100644 --- a/pkg/decision/composite_service.go +++ b/pkg/decision/composite_service.go @@ -47,6 +47,14 @@ func WithExperimentOverrides(experimentOverrides OverrideStore) CSOptionFunc { } } +// WithExperimentOverridesMap applies the overrides in the argument map to a composite service +func WithExperimentOverridesMap(overridesMap map[OverrideKey]string) CSOptionFunc { + overridesStore := &mapOverridesStore{ + overridesMap: overridesMap, + } + return WithExperimentOverrides(overridesStore) +} + // NewCompositeService returns a new instance of the CompositeService with the defaults func NewCompositeService(sdkKey string, options ...CSOptionFunc) *CompositeService { // @TODO: add factory method with option funcs to accept custom feature and experiment services diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index 5ada96dd2..672b74100 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -28,29 +28,29 @@ import ( var eosLogger = logging.GetLogger("ExperimentOverrideService") +// OverrideKey represents the user ID and experiment associated with an override variation +type OverrideKey struct { + Experiment, UserID string +} + // OverrideStore provides read access to overrides type OverrideStore interface { // Returns a variation associated with overrideKey GetVariation(overrideKey OverrideKey) (string, bool) } -// OverrideKey is the type of keys in the Overrides map of ExperimentOverrideService -type OverrideKey struct { - Experiment, User string -} - -// MapOverrides is a map-based implementation of OverrideStore -type MapOverrides struct { - overrides map[OverrideKey]string +// MapOverridesStore is a map-based implementation of OverrideStore +type mapOverridesStore struct { + overridesMap map[OverrideKey]string } // GetVariation returns the override associated with the given key in the map -func (m *MapOverrides) GetVariation(overrideKey OverrideKey) (string, bool) { - variationKey, ok := m.overrides[overrideKey] +func (m *mapOverridesStore) GetVariation(overrideKey OverrideKey) (string, bool) { + variationKey, ok := m.overridesMap[overrideKey] return variationKey, ok } -// ExperimentOverrideService makes a decision using a given map of (experiment key, user id) to variation keys +// ExperimentOverrideService makes a decision using an OverridesStore // Implements the ExperimentService interface type ExperimentOverrideService struct { Overrides OverrideStore @@ -71,7 +71,7 @@ func (s ExperimentOverrideService) GetDecision(decisionContext ExperimentDecisio return decision, errors.New("decisionContext Experiment is nil") } - variationKey, ok := s.Overrides.GetVariation(OverrideKey{Experiment: decisionContext.Experiment.Key, User: userContext.ID}) + variationKey, ok := s.Overrides.GetVariation(OverrideKey{Experiment: decisionContext.Experiment.Key, UserID: userContext.ID}) if !ok { decision.Reason = reasons.NoOverrideVariationForUser return decision, nil diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go index 15440245b..e74b1a593 100644 --- a/pkg/decision/experiment_override_service_test.go +++ b/pkg/decision/experiment_override_service_test.go @@ -33,9 +33,9 @@ type ExperimentOverrideServiceTestSuite struct { func (s *ExperimentOverrideServiceTestSuite) SetupTest() { s.mockConfig = new(mockProjectConfig) - mapOverrides := &MapOverrides{ - overrides: map[OverrideKey]string{ - OverrideKey{Experiment: testExp1111.Key, User: "test_user_1"}: testExp1111Var2222.Key, + mapOverrides := &mapOverridesStore{ + overridesMap: map[OverrideKey]string{ + OverrideKey{Experiment: testExp1111.Key, UserID: "test_user_1"}: testExp1111Var2222.Key, }, } s.overrideService = NewExperimentOverrideService(mapOverrides) From 1ea197a9515280a69ae593ef7c4de1fb389a16da Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 1 Nov 2019 10:19:46 -0700 Subject: [PATCH 08/16] Apply exp overrides to feature decisions --- examples/main.go | 4 ++-- pkg/decision/composite_service.go | 8 ++++++-- pkg/decision/experiment_override_service.go | 12 ++++++------ pkg/decision/experiment_override_service_test.go | 6 +++--- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/examples/main.go b/examples/main.go index 3e63eb600..75bcd8d84 100644 --- a/examples/main.go +++ b/examples/main.go @@ -70,11 +70,11 @@ func main() { optimizelyClient.Close() /************* Setting experiment overrides (a.k.a. "forced variations") ********************/ - overrideKey := decision.OverrideKey{ + overrideKey := decision.ExperimentOverrideKey{ Experiment: "aaaa", UserID: "Matt", } - overrides := map[decision.OverrideKey]string{ + overrides := map[decision.ExperimentOverrideKey]string{ overrideKey: "variation_1", } compositeService := decision.NewCompositeService( diff --git a/pkg/decision/composite_service.go b/pkg/decision/composite_service.go index 20d3d33a9..496b6b0df 100644 --- a/pkg/decision/composite_service.go +++ b/pkg/decision/composite_service.go @@ -39,16 +39,20 @@ type CompositeService struct { type CSOptionFunc func(*CompositeService) // WithExperimentOverrides applies the argument experiment overrides to a composite service +// Note: This overwrites both compositeExperimentService and compositeFeatureService. The +// overrides will be applied to both. func WithExperimentOverrides(experimentOverrides OverrideStore) CSOptionFunc { return func(service *CompositeService) { - service.compositeExperimentService = NewCompositeExperimentService( + expService := NewCompositeExperimentService( WithOverrides(experimentOverrides), ) + service.compositeExperimentService = expService + service.compositeFeatureService = NewCompositeFeatureService(expService) } } // WithExperimentOverridesMap applies the overrides in the argument map to a composite service -func WithExperimentOverridesMap(overridesMap map[OverrideKey]string) CSOptionFunc { +func WithExperimentOverridesMap(overridesMap map[ExperimentOverrideKey]string) CSOptionFunc { overridesStore := &mapOverridesStore{ overridesMap: overridesMap, } diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index 672b74100..b2e056d05 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -28,24 +28,24 @@ import ( var eosLogger = logging.GetLogger("ExperimentOverrideService") -// OverrideKey represents the user ID and experiment associated with an override variation -type OverrideKey struct { +// ExperimentOverrideKey represents the user ID and experiment associated with an override variation +type ExperimentOverrideKey struct { Experiment, UserID string } // OverrideStore provides read access to overrides type OverrideStore interface { // Returns a variation associated with overrideKey - GetVariation(overrideKey OverrideKey) (string, bool) + GetVariation(overrideKey ExperimentOverrideKey) (string, bool) } // MapOverridesStore is a map-based implementation of OverrideStore type mapOverridesStore struct { - overridesMap map[OverrideKey]string + overridesMap map[ExperimentOverrideKey]string } // GetVariation returns the override associated with the given key in the map -func (m *mapOverridesStore) GetVariation(overrideKey OverrideKey) (string, bool) { +func (m *mapOverridesStore) GetVariation(overrideKey ExperimentOverrideKey) (string, bool) { variationKey, ok := m.overridesMap[overrideKey] return variationKey, ok } @@ -71,7 +71,7 @@ func (s ExperimentOverrideService) GetDecision(decisionContext ExperimentDecisio return decision, errors.New("decisionContext Experiment is nil") } - variationKey, ok := s.Overrides.GetVariation(OverrideKey{Experiment: decisionContext.Experiment.Key, UserID: userContext.ID}) + variationKey, ok := s.Overrides.GetVariation(ExperimentOverrideKey{Experiment: decisionContext.Experiment.Key, UserID: userContext.ID}) if !ok { decision.Reason = reasons.NoOverrideVariationForUser return decision, nil diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go index e74b1a593..51b7fb6e0 100644 --- a/pkg/decision/experiment_override_service_test.go +++ b/pkg/decision/experiment_override_service_test.go @@ -27,15 +27,15 @@ import ( type ExperimentOverrideServiceTestSuite struct { suite.Suite mockConfig *mockProjectConfig - overrides map[OverrideKey]string + overrides map[ExperimentOverrideKey]string overrideService *ExperimentOverrideService } func (s *ExperimentOverrideServiceTestSuite) SetupTest() { s.mockConfig = new(mockProjectConfig) mapOverrides := &mapOverridesStore{ - overridesMap: map[OverrideKey]string{ - OverrideKey{Experiment: testExp1111.Key, UserID: "test_user_1"}: testExp1111Var2222.Key, + overridesMap: map[ExperimentOverrideKey]string{ + ExperimentOverrideKey{Experiment: testExp1111.Key, UserID: "test_user_1"}: testExp1111Var2222.Key, }, } s.overrideService = NewExperimentOverrideService(mapOverrides) From 9a45d6503fb5f0044abc3921c03928de06edcbcf Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 1 Nov 2019 11:04:02 -0700 Subject: [PATCH 09/16] Rename and privatize --- pkg/decision/composite_experiment_service.go | 9 ++++----- pkg/decision/experiment_override_service.go | 12 ++++++------ pkg/decision/experiment_override_service_test.go | 6 +++--- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/pkg/decision/composite_experiment_service.go b/pkg/decision/composite_experiment_service.go index 885f431ab..437afe487 100644 --- a/pkg/decision/composite_experiment_service.go +++ b/pkg/decision/composite_experiment_service.go @@ -39,7 +39,7 @@ type CESOptionFunc func(*CompositeExperimentService) func WithOverrides(overrides OverrideStore) CESOptionFunc { return func(service *CompositeExperimentService) { service.experimentServices = append( - []ExperimentService{NewExperimentOverrideService(overrides)}, + []ExperimentService{newExperimentOverrideService(overrides)}, service.experimentServices..., ) } @@ -47,10 +47,9 @@ func WithOverrides(overrides OverrideStore) CESOptionFunc { // NewCompositeExperimentService creates a new instance of the CompositeExperimentService func NewCompositeExperimentService(options ...CESOptionFunc) *CompositeExperimentService { - // These decision services are applied in order: - // 1. Overrides (only when the WithOverrides option is used) - // 2. Whitelist - // 3. Bucketing + // By default, these decision services are applied in order: + // 1. Whitelist + // 2. Bucketing compositeExperimentService := &CompositeExperimentService{ experimentServices: []ExperimentService{ NewExperimentWhitelistService(), diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index b2e056d05..707034ddd 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -30,7 +30,7 @@ var eosLogger = logging.GetLogger("ExperimentOverrideService") // ExperimentOverrideKey represents the user ID and experiment associated with an override variation type ExperimentOverrideKey struct { - Experiment, UserID string + ExperimentKey, UserID string } // OverrideStore provides read access to overrides @@ -52,26 +52,26 @@ func (m *mapOverridesStore) GetVariation(overrideKey ExperimentOverrideKey) (str // ExperimentOverrideService makes a decision using an OverridesStore // Implements the ExperimentService interface -type ExperimentOverrideService struct { +type experimentOverrideService struct { Overrides OverrideStore } // NewExperimentOverrideService returns a pointer to an initialized ExperimentOverrideService -func NewExperimentOverrideService(overrides OverrideStore) *ExperimentOverrideService { - return &ExperimentOverrideService{ +func newExperimentOverrideService(overrides OverrideStore) *experimentOverrideService { + return &experimentOverrideService{ Overrides: overrides, } } // GetDecision returns a decision with a variation when a variation assignment is found in the configured overrides for the given user and experiment -func (s ExperimentOverrideService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) { +func (s experimentOverrideService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) { decision := ExperimentDecision{} if decisionContext.Experiment == nil { return decision, errors.New("decisionContext Experiment is nil") } - variationKey, ok := s.Overrides.GetVariation(ExperimentOverrideKey{Experiment: decisionContext.Experiment.Key, UserID: userContext.ID}) + variationKey, ok := s.Overrides.GetVariation(ExperimentOverrideKey{ExperimentKey: decisionContext.Experiment.Key, UserID: userContext.ID}) if !ok { decision.Reason = reasons.NoOverrideVariationForUser return decision, nil diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go index 51b7fb6e0..85c1bb20c 100644 --- a/pkg/decision/experiment_override_service_test.go +++ b/pkg/decision/experiment_override_service_test.go @@ -28,17 +28,17 @@ type ExperimentOverrideServiceTestSuite struct { suite.Suite mockConfig *mockProjectConfig overrides map[ExperimentOverrideKey]string - overrideService *ExperimentOverrideService + overrideService *experimentOverrideService } func (s *ExperimentOverrideServiceTestSuite) SetupTest() { s.mockConfig = new(mockProjectConfig) mapOverrides := &mapOverridesStore{ overridesMap: map[ExperimentOverrideKey]string{ - ExperimentOverrideKey{Experiment: testExp1111.Key, UserID: "test_user_1"}: testExp1111Var2222.Key, + ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}: testExp1111Var2222.Key, }, } - s.overrideService = NewExperimentOverrideService(mapOverrides) + s.overrideService = newExperimentOverrideService(mapOverrides) } func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { From d4e84702b329a4607d9fe785b017a717077c7804 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 1 Nov 2019 12:48:14 -0700 Subject: [PATCH 10/16] Finish tests --- pkg/decision/experiment_override_service.go | 9 +- .../experiment_override_service_test.go | 89 +++++++++++++++++-- pkg/decision/reasons/reason.go | 12 +-- 3 files changed, 93 insertions(+), 17 deletions(-) diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index 707034ddd..f5cc3303e 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -63,7 +63,7 @@ func newExperimentOverrideService(overrides OverrideStore) *experimentOverrideSe } } -// GetDecision returns a decision with a variation when a variation assignment is found in the configured overrides for the given user and experiment +// GetDecision returns a decision with a variation when the store returns a variation assignment for the given user and experiment func (s experimentOverrideService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) { decision := ExperimentDecision{} @@ -73,19 +73,20 @@ func (s experimentOverrideService) GetDecision(decisionContext ExperimentDecisio variationKey, ok := s.Overrides.GetVariation(ExperimentOverrideKey{ExperimentKey: decisionContext.Experiment.Key, UserID: userContext.ID}) if !ok { - decision.Reason = reasons.NoOverrideVariationForUser + decision.Reason = reasons.NoOverrideVariationAssignment return decision, nil } + // TODO(Matt): Add a VariationsByKey map to the Experiment struct, and use it to look up Variation by key for _, variation := range decisionContext.Experiment.Variations { if variation.Key == variationKey { decision.Variation = &variation - decision.Reason = reasons.OverrideVariationFound + decision.Reason = reasons.OverrideVariationAssignmentFound eosLogger.Info(fmt.Sprintf("Override variation %v found for user %v", variationKey, userContext.ID)) return decision, nil } } - decision.Reason = reasons.InvalidOverrideVariationForUser + decision.Reason = reasons.InvalidOverrideVariationAssignment return decision, nil } diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go index 85c1bb20c..27bfa7905 100644 --- a/pkg/decision/experiment_override_service_test.go +++ b/pkg/decision/experiment_override_service_test.go @@ -20,6 +20,7 @@ package decision import ( "testing" + "github.com/optimizely/go-sdk/pkg/decision/reasons" "github.com/optimizely/go-sdk/pkg/entities" "github.com/stretchr/testify/suite" ) @@ -33,12 +34,10 @@ type ExperimentOverrideServiceTestSuite struct { func (s *ExperimentOverrideServiceTestSuite) SetupTest() { s.mockConfig = new(mockProjectConfig) - mapOverrides := &mapOverridesStore{ - overridesMap: map[ExperimentOverrideKey]string{ - ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}: testExp1111Var2222.Key, - }, - } - s.overrideService = newExperimentOverrideService(mapOverrides) + s.overrides = make(map[ExperimentOverrideKey]string) + s.overrideService = newExperimentOverrideService(&mapOverridesStore{ + overridesMap: s.overrides, + }) } func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { @@ -46,15 +45,91 @@ func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { Experiment: &testExp1111, ProjectConfig: s.mockConfig, } + testUserContext := entities.UserContext{ + ID: "test_user_1", + } + s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}] = testExp1111Var2222.Key + decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) + s.NoError(err) + s.NotNil(decision.Variation) + s.Exactly(decision.Variation.Key, testExp1111Var2222.Key) + s.Exactly(decision.Reason, reasons.OverrideVariationAssignmentFound) +} +func (s *ExperimentOverrideServiceTestSuite) TestNilDecisionContextExperiment() { + testDecisionContext := ExperimentDecisionContext{ + ProjectConfig: s.mockConfig, + } testUserContext := entities.UserContext{ ID: "test_user_1", } + decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) + s.Error(err) + s.Nil(decision.Variation) +} +func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForExperiment() { + testDecisionContext := ExperimentDecisionContext{ + Experiment: &testExp1111, + ProjectConfig: s.mockConfig, + } + testUserContext := entities.UserContext{ + ID: "test_user_1", + } + // The decision context refers to testExp1111, but this override is for another experiment + s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_1"}] = testExp1113Var2224.Key decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) + s.NoError(err) + s.Nil(decision.Variation) + s.Exactly(decision.Reason, reasons.NoOverrideVariationAssignment) +} +func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUser() { + testDecisionContext := ExperimentDecisionContext{ + Experiment: &testExp1111, + ProjectConfig: s.mockConfig, + } + testUserContext := entities.UserContext{ + ID: "test_user_1", + } + // The user context refers to "test_user_1", but this override is for another user + s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_2"}] = testExp1111Var2222.Key + decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) - s.NotNil(decision.Variation) + s.Nil(decision.Variation) + s.Exactly(decision.Reason, reasons.NoOverrideVariationAssignment) +} + +func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUserOrExperiment() { + testDecisionContext := ExperimentDecisionContext{ + Experiment: &testExp1111, + ProjectConfig: s.mockConfig, + } + testUserContext := entities.UserContext{ + ID: "test_user_1", + } + // This override is for both a different user and a different experiment than the ones in the contexts above + s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1113.Key, UserID: "test_user_3"}] = testExp1111Var2222.Key + decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) + s.NoError(err) + s.Nil(decision.Variation) + s.Exactly(decision.Reason, reasons.NoOverrideVariationAssignment) +} + +func (s *ExperimentOverrideServiceTestSuite) TestInvalidVariationInOverride() { + testDecisionContext := ExperimentDecisionContext{ + Experiment: &testExp1111, + ProjectConfig: s.mockConfig, + } + testUserContext := entities.UserContext{ + ID: "test_user_1", + } + // This override variation key does not exist in the experiment + s.overrides[ExperimentOverrideKey{ExperimentKey: testExp1111.Key, UserID: "test_user_1"}] = "invalid_variation_key" + decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) + s.NoError(err) + s.Nil(decision.Variation) + s.Exactly(decision.Reason, reasons.InvalidOverrideVariationAssignment) } func TestExperimentOverridesTestSuite(t *testing.T) { diff --git a/pkg/decision/reasons/reason.go b/pkg/decision/reasons/reason.go index 01cdf8e42..24a8b0f92 100644 --- a/pkg/decision/reasons/reason.go +++ b/pkg/decision/reasons/reason.go @@ -43,10 +43,10 @@ const ( InvalidWhitelistVariationAssignment Reason = "Invalid whitelist variation assignment" // WhitelistVariationAssignmentFound - a valid variation assignment was found for the given user and experiment WhitelistVariationAssignmentFound Reason = "Whitelist variation assignment found" - // NoOverrideVariationForUser - No override variation was found for the given user and experiment - NoOverrideVariationForUser Reason = "No override variation assignment" - // InvalidOverrideVariationForUser - An override variation was found for the given user and experiment, but no variation with that key exists in the given experiment - InvalidOverrideVariationForUser Reason = "Invalid override variation assignment" - // OverrideVariationFound - A valid override variation was found for the given user and experiment - OverrideVariationFound Reason = "Override variation assignment found" + // NoOverrideVariationAssignment - No override variation was found for the given user and experiment + NoOverrideVariationAssignment Reason = "No override variation assignment" + // InvalidOverrideVariationAssignment - An override variation was found for the given user and experiment, but no variation with that key exists in the given experiment + InvalidOverrideVariationAssignment Reason = "Invalid override variation assignment" + // OverrideVariationAssignmentFound - A valid override variation was found for the given user and experiment + OverrideVariationAssignmentFound Reason = "Override variation assignment found" ) From d54013652d38c927102a0474a0552b5d109ab3bd Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 1 Nov 2019 12:57:01 -0700 Subject: [PATCH 11/16] Make things public again --- pkg/decision/composite_experiment_service.go | 2 +- pkg/decision/composite_service.go | 2 +- pkg/decision/experiment_override_service.go | 12 ++++++------ pkg/decision/experiment_override_service_test.go | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/decision/composite_experiment_service.go b/pkg/decision/composite_experiment_service.go index 437afe487..71a41db92 100644 --- a/pkg/decision/composite_experiment_service.go +++ b/pkg/decision/composite_experiment_service.go @@ -39,7 +39,7 @@ type CESOptionFunc func(*CompositeExperimentService) func WithOverrides(overrides OverrideStore) CESOptionFunc { return func(service *CompositeExperimentService) { service.experimentServices = append( - []ExperimentService{newExperimentOverrideService(overrides)}, + []ExperimentService{NewExperimentOverrideService(overrides)}, service.experimentServices..., ) } diff --git a/pkg/decision/composite_service.go b/pkg/decision/composite_service.go index 496b6b0df..8fcd32197 100644 --- a/pkg/decision/composite_service.go +++ b/pkg/decision/composite_service.go @@ -53,7 +53,7 @@ func WithExperimentOverrides(experimentOverrides OverrideStore) CSOptionFunc { // WithExperimentOverridesMap applies the overrides in the argument map to a composite service func WithExperimentOverridesMap(overridesMap map[ExperimentOverrideKey]string) CSOptionFunc { - overridesStore := &mapOverridesStore{ + overridesStore := &MapOverridesStore{ overridesMap: overridesMap, } return WithExperimentOverrides(overridesStore) diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index f5cc3303e..cc68e5ac0 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -40,31 +40,31 @@ type OverrideStore interface { } // MapOverridesStore is a map-based implementation of OverrideStore -type mapOverridesStore struct { +type MapOverridesStore struct { overridesMap map[ExperimentOverrideKey]string } // GetVariation returns the override associated with the given key in the map -func (m *mapOverridesStore) GetVariation(overrideKey ExperimentOverrideKey) (string, bool) { +func (m *MapOverridesStore) GetVariation(overrideKey ExperimentOverrideKey) (string, bool) { variationKey, ok := m.overridesMap[overrideKey] return variationKey, ok } // ExperimentOverrideService makes a decision using an OverridesStore // Implements the ExperimentService interface -type experimentOverrideService struct { +type ExperimentOverrideService struct { Overrides OverrideStore } // NewExperimentOverrideService returns a pointer to an initialized ExperimentOverrideService -func newExperimentOverrideService(overrides OverrideStore) *experimentOverrideService { - return &experimentOverrideService{ +func NewExperimentOverrideService(overrides OverrideStore) *ExperimentOverrideService { + return &ExperimentOverrideService{ Overrides: overrides, } } // GetDecision returns a decision with a variation when the store returns a variation assignment for the given user and experiment -func (s experimentOverrideService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) { +func (s ExperimentOverrideService) GetDecision(decisionContext ExperimentDecisionContext, userContext entities.UserContext) (ExperimentDecision, error) { decision := ExperimentDecision{} if decisionContext.Experiment == nil { diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go index 27bfa7905..6e132e471 100644 --- a/pkg/decision/experiment_override_service_test.go +++ b/pkg/decision/experiment_override_service_test.go @@ -29,13 +29,13 @@ type ExperimentOverrideServiceTestSuite struct { suite.Suite mockConfig *mockProjectConfig overrides map[ExperimentOverrideKey]string - overrideService *experimentOverrideService + overrideService *ExperimentOverrideService } func (s *ExperimentOverrideServiceTestSuite) SetupTest() { s.mockConfig = new(mockProjectConfig) s.overrides = make(map[ExperimentOverrideKey]string) - s.overrideService = newExperimentOverrideService(&mapOverridesStore{ + s.overrideService = NewExperimentOverrideService(&MapOverridesStore{ overridesMap: s.overrides, }) } From 69856c9ae87b4e6af49b009a8b861a8fdbfbd837 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 1 Nov 2019 13:16:46 -0700 Subject: [PATCH 12/16] Rneame to ExperimentOverrideStore --- pkg/decision/composite_experiment_service.go | 2 +- pkg/decision/composite_service.go | 2 +- pkg/decision/experiment_override_service.go | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/decision/composite_experiment_service.go b/pkg/decision/composite_experiment_service.go index 71a41db92..2f83193d1 100644 --- a/pkg/decision/composite_experiment_service.go +++ b/pkg/decision/composite_experiment_service.go @@ -36,7 +36,7 @@ type CompositeExperimentService struct { type CESOptionFunc func(*CompositeExperimentService) // WithOverrides prepends an ExperimentOverrideService to service.experimentServices -func WithOverrides(overrides OverrideStore) CESOptionFunc { +func WithOverrides(overrides ExperimentOverrideStore) CESOptionFunc { return func(service *CompositeExperimentService) { service.experimentServices = append( []ExperimentService{NewExperimentOverrideService(overrides)}, diff --git a/pkg/decision/composite_service.go b/pkg/decision/composite_service.go index 8fcd32197..8c853d322 100644 --- a/pkg/decision/composite_service.go +++ b/pkg/decision/composite_service.go @@ -41,7 +41,7 @@ type CSOptionFunc func(*CompositeService) // WithExperimentOverrides applies the argument experiment overrides to a composite service // Note: This overwrites both compositeExperimentService and compositeFeatureService. The // overrides will be applied to both. -func WithExperimentOverrides(experimentOverrides OverrideStore) CSOptionFunc { +func WithExperimentOverrides(experimentOverrides ExperimentOverrideStore) CSOptionFunc { return func(service *CompositeService) { expService := NewCompositeExperimentService( WithOverrides(experimentOverrides), diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index cc68e5ac0..35d605ac3 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -33,8 +33,8 @@ type ExperimentOverrideKey struct { ExperimentKey, UserID string } -// OverrideStore provides read access to overrides -type OverrideStore interface { +// ExperimentOverrideStore provides read access to overrides +type ExperimentOverrideStore interface { // Returns a variation associated with overrideKey GetVariation(overrideKey ExperimentOverrideKey) (string, bool) } @@ -50,14 +50,14 @@ func (m *MapOverridesStore) GetVariation(overrideKey ExperimentOverrideKey) (str return variationKey, ok } -// ExperimentOverrideService makes a decision using an OverridesStore +// ExperimentOverrideService makes a decision using an ExperimentOverridesStore // Implements the ExperimentService interface type ExperimentOverrideService struct { - Overrides OverrideStore + Overrides ExperimentOverrideStore } // NewExperimentOverrideService returns a pointer to an initialized ExperimentOverrideService -func NewExperimentOverrideService(overrides OverrideStore) *ExperimentOverrideService { +func NewExperimentOverrideService(overrides ExperimentOverrideStore) *ExperimentOverrideService { return &ExperimentOverrideService{ Overrides: overrides, } From dc0a3a16a2bf6d58c342de8d25960008a08a3aad Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 1 Nov 2019 14:27:26 -0700 Subject: [PATCH 13/16] Test WithOverrides of NewCompositeExperimentService --- .../composite_experiment_service_test.go | 23 +++++++++++++++++++ .../experiment_override_service_test.go | 12 +++++----- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/pkg/decision/composite_experiment_service_test.go b/pkg/decision/composite_experiment_service_test.go index c1297d87e..fcf5ae860 100644 --- a/pkg/decision/composite_experiment_service_test.go +++ b/pkg/decision/composite_experiment_service_test.go @@ -158,6 +158,29 @@ func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentService() { s.IsType(&ExperimentBucketerService{}, compositeExperimentService.experimentServices[1]) } +// testExperimentOverrides implements ExperimentOverridesStore for tests +type testExperimentOverrides struct{} + +func (o *testExperimentOverrides) GetVariation(overrideKey ExperimentOverrideKey) (string, bool) { + return testExp1111Var2222.Key, true +} + +func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentServiceWithOverrides() { + compositeExperimentService := NewCompositeExperimentService( + WithOverrides(&testExperimentOverrides{}), + ) + s.NotEmpty(compositeExperimentService.experimentServices) + overrideService := compositeExperimentService.experimentServices[0] + s.IsType(&ExperimentOverrideService{}, overrideService) + testUserContext := entities.UserContext{ + ID: "test_user_1", + } + decision, err := overrideService.GetDecision(s.testDecisionContext, testUserContext) + s.NoError(err) + s.NotNil(decision.Variation) + s.Equal(decision.Variation.Key, testExp1111Var2222.Key) +} + func TestCompositeExperimentTestSuite(t *testing.T) { suite.Run(t, new(CompositeExperimentTestSuite)) } diff --git a/pkg/decision/experiment_override_service_test.go b/pkg/decision/experiment_override_service_test.go index 6e132e471..ca50b84e1 100644 --- a/pkg/decision/experiment_override_service_test.go +++ b/pkg/decision/experiment_override_service_test.go @@ -52,8 +52,8 @@ func (s *ExperimentOverrideServiceTestSuite) TestOverridesIncludeVariation() { decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.NotNil(decision.Variation) - s.Exactly(decision.Variation.Key, testExp1111Var2222.Key) - s.Exactly(decision.Reason, reasons.OverrideVariationAssignmentFound) + s.Exactly(testExp1111Var2222.Key, decision.Variation.Key) + s.Exactly(reasons.OverrideVariationAssignmentFound, decision.Reason) } func (s *ExperimentOverrideServiceTestSuite) TestNilDecisionContextExperiment() { @@ -81,7 +81,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForExperiment() { decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.Nil(decision.Variation) - s.Exactly(decision.Reason, reasons.NoOverrideVariationAssignment) + s.Exactly(reasons.NoOverrideVariationAssignment, decision.Reason) } func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUser() { @@ -97,7 +97,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUser() { decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.Nil(decision.Variation) - s.Exactly(decision.Reason, reasons.NoOverrideVariationAssignment) + s.Exactly(reasons.NoOverrideVariationAssignment, decision.Reason) } func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUserOrExperiment() { @@ -113,7 +113,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestNoOverrideForUserOrExperiment() decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.Nil(decision.Variation) - s.Exactly(decision.Reason, reasons.NoOverrideVariationAssignment) + s.Exactly(reasons.NoOverrideVariationAssignment, decision.Reason) } func (s *ExperimentOverrideServiceTestSuite) TestInvalidVariationInOverride() { @@ -129,7 +129,7 @@ func (s *ExperimentOverrideServiceTestSuite) TestInvalidVariationInOverride() { decision, err := s.overrideService.GetDecision(testDecisionContext, testUserContext) s.NoError(err) s.Nil(decision.Variation) - s.Exactly(decision.Reason, reasons.InvalidOverrideVariationAssignment) + s.Exactly(reasons.InvalidOverrideVariationAssignment, decision.Reason) } func TestExperimentOverridesTestSuite(t *testing.T) { From c2123b29631ebad34b6d01859ac530a7bc58cc72 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 1 Nov 2019 15:50:13 -0700 Subject: [PATCH 14/16] Revert higher level changes for now --- examples/main.go | 19 ----------- pkg/decision/composite_experiment_service.go | 26 +++----------- .../composite_experiment_service_test.go | 23 ------------- pkg/decision/composite_service.go | 34 ++----------------- 4 files changed, 6 insertions(+), 96 deletions(-) diff --git a/examples/main.go b/examples/main.go index 75bcd8d84..ae0738b5c 100644 --- a/examples/main.go +++ b/examples/main.go @@ -8,7 +8,6 @@ import ( "time" "github.com/optimizely/go-sdk/pkg/client" - "github.com/optimizely/go-sdk/pkg/decision" "github.com/optimizely/go-sdk/pkg/entities" "github.com/optimizely/go-sdk/pkg/event" "github.com/optimizely/go-sdk/pkg/logging" @@ -68,22 +67,4 @@ func main() { client.WithBatchEventProcessor(event.DefaultBatchSize, event.DefaultEventQueueSize, event.DefaultEventFlushInterval), ) optimizelyClient.Close() - - /************* Setting experiment overrides (a.k.a. "forced variations") ********************/ - overrideKey := decision.ExperimentOverrideKey{ - Experiment: "aaaa", - UserID: "Matt", - } - overrides := map[decision.ExperimentOverrideKey]string{ - overrideKey: "variation_1", - } - compositeService := decision.NewCompositeService( - sdkKey, - decision.WithExperimentOverridesMap(overrides), - ) - optimizelyClient, _ = optimizelyFactory.Client( - client.WithDecisionService(compositeService), - ) - // Optimizely client now has "variation_1" forced for user "Matt" in experiment "aaaa" - // The forced variation will work regardless of whether "aaaa" is an A/B test or a Feature Test. } diff --git a/pkg/decision/composite_experiment_service.go b/pkg/decision/composite_experiment_service.go index 2f83193d1..37eaf95aa 100644 --- a/pkg/decision/composite_experiment_service.go +++ b/pkg/decision/composite_experiment_service.go @@ -32,36 +32,18 @@ type CompositeExperimentService struct { experimentServices []ExperimentService } -// CESOptionFunc allows optional customization of the CompositeExperimentService returned from NewCompositeExperimentService -type CESOptionFunc func(*CompositeExperimentService) - -// WithOverrides prepends an ExperimentOverrideService to service.experimentServices -func WithOverrides(overrides ExperimentOverrideStore) CESOptionFunc { - return func(service *CompositeExperimentService) { - service.experimentServices = append( - []ExperimentService{NewExperimentOverrideService(overrides)}, - service.experimentServices..., - ) - } -} - // NewCompositeExperimentService creates a new instance of the CompositeExperimentService -func NewCompositeExperimentService(options ...CESOptionFunc) *CompositeExperimentService { - // By default, these decision services are applied in order: +func NewCompositeExperimentService() *CompositeExperimentService { + // These decision services are applied in order: // 1. Whitelist // 2. Bucketing - compositeExperimentService := &CompositeExperimentService{ + // @TODO(mng): Prepend forced variation + return &CompositeExperimentService{ experimentServices: []ExperimentService{ NewExperimentWhitelistService(), NewExperimentBucketerService(), }, } - - for _, option := range options { - option(compositeExperimentService) - } - - return compositeExperimentService } // GetDecision returns a decision for the given experiment and user context diff --git a/pkg/decision/composite_experiment_service_test.go b/pkg/decision/composite_experiment_service_test.go index fcf5ae860..c1297d87e 100644 --- a/pkg/decision/composite_experiment_service_test.go +++ b/pkg/decision/composite_experiment_service_test.go @@ -158,29 +158,6 @@ func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentService() { s.IsType(&ExperimentBucketerService{}, compositeExperimentService.experimentServices[1]) } -// testExperimentOverrides implements ExperimentOverridesStore for tests -type testExperimentOverrides struct{} - -func (o *testExperimentOverrides) GetVariation(overrideKey ExperimentOverrideKey) (string, bool) { - return testExp1111Var2222.Key, true -} - -func (s *CompositeExperimentTestSuite) TestNewCompositeExperimentServiceWithOverrides() { - compositeExperimentService := NewCompositeExperimentService( - WithOverrides(&testExperimentOverrides{}), - ) - s.NotEmpty(compositeExperimentService.experimentServices) - overrideService := compositeExperimentService.experimentServices[0] - s.IsType(&ExperimentOverrideService{}, overrideService) - testUserContext := entities.UserContext{ - ID: "test_user_1", - } - decision, err := overrideService.GetDecision(s.testDecisionContext, testUserContext) - s.NoError(err) - s.NotNil(decision.Variation) - s.Equal(decision.Variation.Key, testExp1111Var2222.Key) -} - func TestCompositeExperimentTestSuite(t *testing.T) { suite.Run(t, new(CompositeExperimentTestSuite)) } diff --git a/pkg/decision/composite_service.go b/pkg/decision/composite_service.go index 8c853d322..5f893d62a 100644 --- a/pkg/decision/composite_service.go +++ b/pkg/decision/composite_service.go @@ -35,46 +35,16 @@ type CompositeService struct { notificationCenter notification.Center } -// CSOptionFunc allows customization of the CompositeService returned from NewCompositeService -type CSOptionFunc func(*CompositeService) - -// WithExperimentOverrides applies the argument experiment overrides to a composite service -// Note: This overwrites both compositeExperimentService and compositeFeatureService. The -// overrides will be applied to both. -func WithExperimentOverrides(experimentOverrides ExperimentOverrideStore) CSOptionFunc { - return func(service *CompositeService) { - expService := NewCompositeExperimentService( - WithOverrides(experimentOverrides), - ) - service.compositeExperimentService = expService - service.compositeFeatureService = NewCompositeFeatureService(expService) - } -} - -// WithExperimentOverridesMap applies the overrides in the argument map to a composite service -func WithExperimentOverridesMap(overridesMap map[ExperimentOverrideKey]string) CSOptionFunc { - overridesStore := &MapOverridesStore{ - overridesMap: overridesMap, - } - return WithExperimentOverrides(overridesStore) -} - // NewCompositeService returns a new instance of the CompositeService with the defaults -func NewCompositeService(sdkKey string, options ...CSOptionFunc) *CompositeService { +func NewCompositeService(sdkKey string) *CompositeService { // @TODO: add factory method with option funcs to accept custom feature and experiment services compositeExperimentService := NewCompositeExperimentService() compositeFeatureDecisionService := NewCompositeFeatureService(compositeExperimentService) - compositeService := &CompositeService{ + return &CompositeService{ compositeExperimentService: compositeExperimentService, compositeFeatureService: compositeFeatureDecisionService, notificationCenter: registry.GetNotificationCenter(sdkKey), } - - for _, option := range options { - option(compositeService) - } - - return compositeService } // GetFeatureDecision returns a decision for the given feature key From cfbbc1417d2639613e8bf3da29257eb5e1ba2605 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 1 Nov 2019 15:55:07 -0700 Subject: [PATCH 15/16] Fix lint --- pkg/decision/experiment_override_service.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index 35d605ac3..0d60b6358 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -79,6 +79,7 @@ func (s ExperimentOverrideService) GetDecision(decisionContext ExperimentDecisio // TODO(Matt): Add a VariationsByKey map to the Experiment struct, and use it to look up Variation by key for _, variation := range decisionContext.Experiment.Variations { + variation := variation if variation.Key == variationKey { decision.Variation = &variation decision.Reason = reasons.OverrideVariationAssignmentFound From 1445e837f68d586f028209b17ac932679ba12132 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Mon, 4 Nov 2019 11:02:56 -0800 Subject: [PATCH 16/16] Review fixes --- pkg/decision/experiment_override_service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/decision/experiment_override_service.go b/pkg/decision/experiment_override_service.go index 0d60b6358..9386d1fc8 100644 --- a/pkg/decision/experiment_override_service.go +++ b/pkg/decision/experiment_override_service.go @@ -77,13 +77,13 @@ func (s ExperimentOverrideService) GetDecision(decisionContext ExperimentDecisio return decision, nil } - // TODO(Matt): Add a VariationsByKey map to the Experiment struct, and use it to look up Variation by key + // TODO(Matt): Implement and use a way to access variations by key for _, variation := range decisionContext.Experiment.Variations { variation := variation if variation.Key == variationKey { decision.Variation = &variation decision.Reason = reasons.OverrideVariationAssignmentFound - eosLogger.Info(fmt.Sprintf("Override variation %v found for user %v", variationKey, userContext.ID)) + eosLogger.Debug(fmt.Sprintf("Override variation %v found for user %v", variationKey, userContext.ID)) return decision, nil } }