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.

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

feat(segment-manager): Adds implementation for ODPSegmentManager #353

Merged
merged 24 commits into from
Nov 16, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions pkg/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ type OptimizelyClient struct {
// CreateUserContext creates a context of the user for which decision APIs will be called.
// A user context will be created successfully even when the SDK is not fully configured yet.
func (o *OptimizelyClient) CreateUserContext(userID string, attributes map[string]interface{}) OptimizelyUserContext {
// Passing qualified segments as nil initially since they will be fetched later
return newOptimizelyUserContext(o, userID, attributes, nil, nil)
}

Expand Down
10 changes: 5 additions & 5 deletions pkg/client/optimizely_user_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type OptimizelyUserContext struct {
}

// returns an instance of the optimizely user context.
func newOptimizelyUserContext(optimizely *OptimizelyClient, userID string, attributes map[string]interface{}, qualifiedSegments []string, forcedDecisionService *pkgDecision.ForcedDecisionService) OptimizelyUserContext {
func newOptimizelyUserContext(optimizely *OptimizelyClient, userID string, attributes map[string]interface{}, forcedDecisionService *pkgDecision.ForcedDecisionService, qualifiedSegments []string) OptimizelyUserContext {
Copy link

Choose a reason for hiding this comment

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

We need to differentiate cloning vs new context creation (so we can send IdenfityUser event only for the first creation not for cloing). We can keep it as is and fix it later when we work on the top-level integration.

Copy link
Contributor

Choose a reason for hiding this comment

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

what change you made for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be adding this in a separate PR which will be focused on integrating odpManager in userContext.

// store a copy of the provided attributes so it isn't affected by changes made afterwards.
if attributes == nil {
attributes = map[string]interface{}{}
Expand Down Expand Up @@ -115,21 +115,21 @@ func (o *OptimizelyUserContext) IsQualifiedFor(segment string) bool {
// all data required to deliver the flag or experiment.
func (o *OptimizelyUserContext) Decide(key string, options []decide.OptimizelyDecideOptions) OptimizelyDecision {
// use a copy of the user context so that any changes to the original context are not reflected inside the decision
userContextCopy := newOptimizelyUserContext(o.GetOptimizely(), o.GetUserID(), o.GetUserAttributes(), o.GetQualifiedSegments(), o.getForcedDecisionService())
userContextCopy := newOptimizelyUserContext(o.GetOptimizely(), o.GetUserID(), o.GetUserAttributes(), o.getForcedDecisionService(), o.GetQualifiedSegments())
return o.optimizely.decide(userContextCopy, key, convertDecideOptions(options))
}

// DecideAll returns a key-map of decision results for all active flag keys with options.
func (o *OptimizelyUserContext) DecideAll(options []decide.OptimizelyDecideOptions) map[string]OptimizelyDecision {
// use a copy of the user context so that any changes to the original context are not reflected inside the decision
userContextCopy := newOptimizelyUserContext(o.GetOptimizely(), o.GetUserID(), o.GetUserAttributes(), o.GetQualifiedSegments(), o.getForcedDecisionService())
userContextCopy := newOptimizelyUserContext(o.GetOptimizely(), o.GetUserID(), o.GetUserAttributes(), o.getForcedDecisionService(), o.GetQualifiedSegments())
return o.optimizely.decideAll(userContextCopy, convertDecideOptions(options))
}

// DecideForKeys returns a key-map of decision results for multiple flag keys and options.
func (o *OptimizelyUserContext) DecideForKeys(keys []string, options []decide.OptimizelyDecideOptions) map[string]OptimizelyDecision {
// use a copy of the user context so that any changes to the original context are not reflected inside the decision
userContextCopy := newOptimizelyUserContext(o.GetOptimizely(), o.GetUserID(), o.GetUserAttributes(), o.GetQualifiedSegments(), o.getForcedDecisionService())
userContextCopy := newOptimizelyUserContext(o.GetOptimizely(), o.GetUserID(), o.GetUserAttributes(), o.getForcedDecisionService(), o.GetQualifiedSegments())
return o.optimizely.decideForKeys(userContextCopy, keys, convertDecideOptions(options))
}

Expand Down Expand Up @@ -192,5 +192,5 @@ func copyQualifiedSegments(qualifiedSegments []string) (qualifiedSegmentsCopy []
}
qualifiedSegmentsCopy = make([]string, len(qualifiedSegments))
copy(qualifiedSegmentsCopy, qualifiedSegments)
return qualifiedSegmentsCopy
return
}
6 changes: 3 additions & 3 deletions pkg/client/optimizely_user_context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (s *OptimizelyUserContextTestSuite) SetupTest() {
func (s *OptimizelyUserContextTestSuite) TestOptimizelyUserContextWithAttributesAndSegments() {
attributes := map[string]interface{}{"key1": 1212, "key2": 1213}
segments := []string{"123"}
optimizelyUserContext := newOptimizelyUserContext(s.OptimizelyClient, s.userID, attributes, segments, nil)
optimizelyUserContext := newOptimizelyUserContext(s.OptimizelyClient, s.userID, attributes, nil, segments)

s.Equal(s.OptimizelyClient, optimizelyUserContext.GetOptimizely())
s.Equal(s.userID, optimizelyUserContext.GetUserID())
Expand All @@ -80,7 +80,7 @@ func (s *OptimizelyUserContextTestSuite) TestOptimizelyUserContextNoAttributesAn
func (s *OptimizelyUserContextTestSuite) TestUpatingProvidedUserContextHasNoImpactOnOptimizelyUserContext() {
attributes := map[string]interface{}{"k1": "v1", "k2": false}
segments := []string{"123"}
optimizelyUserContext := newOptimizelyUserContext(s.OptimizelyClient, s.userID, attributes, segments, nil)
optimizelyUserContext := newOptimizelyUserContext(s.OptimizelyClient, s.userID, attributes, nil, segments)

s.Equal(s.OptimizelyClient, optimizelyUserContext.GetOptimizely())
s.Equal(s.userID, optimizelyUserContext.GetUserID())
Expand Down Expand Up @@ -177,7 +177,7 @@ func (s *OptimizelyUserContextTestSuite) TestSetAndGetQualifiedSegments() {
userID := "1212121"
var attributes map[string]interface{}
qualifiedSegments := []string{"1", "2", "3"}
optimizelyUserContext := newOptimizelyUserContext(s.OptimizelyClient, userID, attributes, []string{}, nil)
optimizelyUserContext := newOptimizelyUserContext(s.OptimizelyClient, userID, attributes, nil, []string{})
s.Len(optimizelyUserContext.GetQualifiedSegments(), 0)

optimizelyUserContext.SetQualifiedSegments(nil)
Expand Down
1 change: 1 addition & 0 deletions pkg/config/datafileprojectconfig/mappers/audience.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func MapAudiences(audiences []datafileEntities.Audience) (audienceMap map[string
if err == nil {
audience.ConditionTree = conditionTree
}
// Only add unique segments to the list
for _, s := range fSegments {
if !odpSegmentsMap[s] {
odpSegmentsMap[s] = true
Expand Down
4 changes: 4 additions & 0 deletions pkg/config/datafileprojectconfig/mappers/condition_trees.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func buildConditionTree(conditions interface{}) (conditionTree *entities.TreeNod
retErr = err
return
}
// Extract odp segment from leaf node if applicable
extractSegment(&odpSegments, n)
}

Expand All @@ -102,6 +103,7 @@ func buildConditionTree(conditions interface{}) (conditionTree *entities.TreeNod
retErr = err
return
}
// Extract odp segment from leaf node if applicable
extractSegment(&odpSegments, n)
conditionTree.Operator = "or"
conditionTree.Nodes = append(conditionTree.Nodes, n)
Expand Down Expand Up @@ -209,11 +211,13 @@ func isValidOperator(op string) bool {
return false
}

// Extracts odpSegment from leaf node and adds it to odpSegments array
func extractSegment(odpSegments *[]string, node *entities.TreeNode) {
condition, ok := node.Item.(entities.Condition)
if !ok {
return
}
// Add segment to list only if match type is qualified and value is a non-empty string
if condition.Match == matchers.QualifiedMatchType {
if segment, ok := condition.Value.(string); ok && segment != "" {
*odpSegments = append(*odpSegments, segment)
Expand Down
46 changes: 10 additions & 36 deletions pkg/odp/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,42 +23,28 @@ import (
"github.com/optimizely/go-sdk/pkg/odp/utils"
)

// ConfigState is used to represent state of odp
type ConfigState int64

const (
// NotDetermined represents that odp service state is currently not determined
NotDetermined ConfigState = iota
// Integrated represents that odp service is integrated
Integrated
// NotIntegrated represents that odp service is not integrated
NotIntegrated
)

// Config is used to represent odp config
type Config interface {
Update(apiKey, apiHost string, segmentsToCheck []string) bool
GetAPIKey() string
GetAPIHost() string
GetSegmentsToCheck() []string
IsEventQueueingAllowed() bool
IsOdpServiceIntegrated() bool
}

// DefaultConfig represents default implementation of odp config
type DefaultConfig struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

i need to check name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

couldn't name this Config as it was the name of the interface, also ODPConfig is not recommended by linter since the config resides under odp package. Any suggestions?

apiKey, apiHost string
segmentsToCheck []string
odpServiceIntegrated ConfigState
lock sync.RWMutex
apiKey, apiHost string
segmentsToCheck []string
lock sync.RWMutex
}

// NewConfig creates and returns a new instance of DefaultConfig.
func NewConfig(apiKey, apiHost string, segmentsToCheck []string) *DefaultConfig {
return &DefaultConfig{
apiKey: apiKey,
apiHost: apiHost,
segmentsToCheck: segmentsToCheck,
odpServiceIntegrated: NotDetermined, // initially queueing allowed until the first datafile is parsed
apiKey: apiKey,
apiHost: apiHost,
segmentsToCheck: segmentsToCheck,
}
}

Expand All @@ -67,12 +53,6 @@ func (s *DefaultConfig) Update(apiKey, apiHost string, segmentsToCheck []string)
s.lock.Lock()
defer s.lock.Unlock()

// disable future event queueing if datafile has no ODP integrations.
s.odpServiceIntegrated = NotIntegrated
if apiKey != "" && apiHost != "" {
s.odpServiceIntegrated = Integrated
}

if s.apiKey == apiKey && s.apiHost == apiHost && utils.Equal(s.segmentsToCheck, segmentsToCheck) {
return false
}
Expand Down Expand Up @@ -108,13 +88,7 @@ func (s *DefaultConfig) GetSegmentsToCheck() []string {
return segmentsToCheck
}

// IsEventQueueingAllowed returns true if event queueing is allowed
func (s *DefaultConfig) IsEventQueueingAllowed() bool {
s.lock.RLock()
defer s.lock.RUnlock()
value := true
if s.odpServiceIntegrated == NotIntegrated {
value = false
}
return value
// IsOdpServiceIntegrated returns true if odp service is integrated
func (s *DefaultConfig) IsOdpServiceIntegrated() bool {
return s.GetAPIHost() != "" && s.GetAPIKey() != ""
}
20 changes: 8 additions & 12 deletions pkg/odp/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,15 @@ func (c *ConfigTestSuite) TestNewConfigWithValidValues() {
c.Equal(c.apiHost, c.config.GetAPIHost())
c.Equal(c.apiKey, c.config.GetAPIKey())
c.Equal(c.segmentsToCheck, c.config.GetSegmentsToCheck())
c.True(c.config.IsEventQueueingAllowed())
c.Equal(NotDetermined, c.config.odpServiceIntegrated)
c.True(c.config.IsOdpServiceIntegrated())
}

func (c *ConfigTestSuite) TestNewConfigWithEmptyValues() {
c.config = NewConfig("", "", nil)
c.Equal("", c.config.GetAPIHost())
c.Equal("", c.config.GetAPIKey())
c.Nil(c.config.GetSegmentsToCheck())
c.True(c.config.IsEventQueueingAllowed())
c.False(c.config.IsOdpServiceIntegrated())
}

func (c *ConfigTestSuite) TestUpdateWithValidValues() {
Expand All @@ -63,8 +62,7 @@ func (c *ConfigTestSuite) TestUpdateWithValidValues() {
c.Equal(expectedAPIKey, c.config.GetAPIKey())
c.Equal(expectedAPIHost, c.config.GetAPIHost())
c.Equal(expectedSegmentsList, c.config.GetSegmentsToCheck())
c.Equal(Integrated, c.config.odpServiceIntegrated)
c.True(c.config.IsEventQueueingAllowed())
c.True(c.config.IsOdpServiceIntegrated())
}

func (c *ConfigTestSuite) TestUpdateWithEmptyValues() {
Expand All @@ -75,17 +73,15 @@ func (c *ConfigTestSuite) TestUpdateWithEmptyValues() {
c.Equal(expectedAPIKey, c.config.GetAPIKey())
c.Equal(expectedAPIHost, c.config.GetAPIHost())
c.Equal(expectedSegmentsList, c.config.GetSegmentsToCheck())
c.Equal(NotIntegrated, c.config.odpServiceIntegrated)
c.False(c.config.IsEventQueueingAllowed())
c.False(c.config.IsOdpServiceIntegrated())
}

func (c *ConfigTestSuite) TestUpdateWithSameValues() {
c.False(c.config.Update(c.apiKey, c.apiHost, c.segmentsToCheck))
c.Equal(c.apiKey, c.config.GetAPIKey())
c.Equal(c.apiHost, c.config.GetAPIHost())
c.Equal(c.segmentsToCheck, c.config.GetSegmentsToCheck())
c.Equal(Integrated, c.config.odpServiceIntegrated)
c.True(c.config.IsEventQueueingAllowed())
c.True(c.config.IsOdpServiceIntegrated())
}

func (c *ConfigTestSuite) TestRaceCondition() {
Expand All @@ -111,8 +107,8 @@ func (c *ConfigTestSuite) TestRaceCondition() {
wg.Done()
}

isEventQueueingAllowed := func() {
c.config.IsEventQueueingAllowed()
isOdpServiceIntegrated := func() {
c.config.IsOdpServiceIntegrated()
wg.Done()
}

Expand All @@ -124,7 +120,7 @@ func (c *ConfigTestSuite) TestRaceCondition() {
go getAPIKey()
go getAPIHost()
go getSegmentsToCheck()
go isEventQueueingAllowed()
go isOdpServiceIntegrated()
}
wg.Wait()
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/odp/lru_cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,14 @@ func TestTimeout(t *testing.T) {
assert.Equal(t, 200, cache2.Lookup("2"))
assert.Equal(t, 300, cache2.Lookup("3"))
}

type TestCache struct {
}

func (l *TestCache) Save(key string, value interface{}) {
}
func (l *TestCache) Lookup(key string) interface{} {
return nil
}
func (l *TestCache) Reset() {
}
13 changes: 8 additions & 5 deletions pkg/odp/segment_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (

// SegmentManager represents the odp segment manager.
type SegmentManager interface {
FetchQualifiedSegments(userKey, userValue string, options []SegmentOption) (segments []string, err error)
FetchQualifiedSegments(userKey, userValue string, options []OptimizelySegmentOption) (segments []string, err error)
Reset()
}

Expand All @@ -37,11 +37,14 @@ type DefaultSegmentManager struct {
}

// NewSegmentManager creates and returns a new instance of DefaultSegmentManager.
func NewSegmentManager(cacheSize int, cacheTimeoutInSecs int64, odpConfig Config, apiManager SegmentAPIManager) *DefaultSegmentManager {
func NewSegmentManager(cache Cache, cacheSize int, cacheTimeoutInSecs int64, odpConfig Config, apiManager SegmentAPIManager) *DefaultSegmentManager {
segmentManager := DefaultSegmentManager{
odpConfig: odpConfig,
segmentAPIManager: apiManager,
segmentsCache: NewLRUCache(cacheSize, cacheTimeoutInSecs),
segmentsCache: cache,
}
if segmentManager.segmentsCache == nil {
segmentManager.segmentsCache = NewLRUCache(cacheSize, cacheTimeoutInSecs)
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
segmentsCache: cache,
}
if segmentManager.segmentsCache == nil {
segmentManager.segmentsCache = NewLRUCache(cacheSize, cacheTimeoutInSecs)
}
segmentsCache: cache ?? NewLRUCache(cacheSize, cacheTimeoutInSecs),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

golang does not support ternary operators such as ?? or ?:.

if segmentManager.odpConfig == nil {
segmentManager.odpConfig = NewConfig("", "", nil)
Expand All @@ -53,8 +56,8 @@ func NewSegmentManager(cacheSize int, cacheTimeoutInSecs int64, odpConfig Config
}

// FetchQualifiedSegments fetches and returns qualified segments
func (s *DefaultSegmentManager) FetchQualifiedSegments(userKey, userValue string, options []SegmentOption) (segments []string, err error) {
if s.odpConfig == nil || s.odpConfig.GetAPIHost() == "" || s.odpConfig.GetAPIKey() == "" {
func (s *DefaultSegmentManager) FetchQualifiedSegments(userKey, userValue string, options []OptimizelySegmentOption) (segments []string, err error) {
if s.odpConfig == nil || !s.odpConfig.IsOdpServiceIntegrated() {
return nil, fmt.Errorf(fetchSegmentsFailedError, "apiKey/apiHost not defined")
}

Expand Down
18 changes: 12 additions & 6 deletions pkg/odp/segment_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,27 +37,33 @@ type SegmentManagerTestSuite struct {
func (s *SegmentManagerTestSuite) SetupTest() {
s.segmentAPIManager = &MockSegmentAPIManager{}
s.config = NewConfig("", "", nil)
s.segmentManager = NewSegmentManager(10, 10, s.config, s.segmentAPIManager)
s.segmentManager = NewSegmentManager(nil, 10, 10, s.config, s.segmentAPIManager)
s.userValue = "test-user"
s.userKey = "vuid"
}

func (s *SegmentManagerTestSuite) TestNewSegmentManagerNilParameters() {
segmentManager := NewSegmentManager(0, 0, nil, nil)
segmentManager := NewSegmentManager(nil, 0, 0, nil, nil)
s.NotNil(segmentManager.segmentAPIManager)
s.NotNil(segmentManager.segmentsCache)
s.NotNil(segmentManager.odpConfig)
}

func (s *SegmentManagerTestSuite) TestNewSegmentManagerCustomCache() {
customCache := &TestCache{}
segmentManager := NewSegmentManager(customCache, 0, 0, nil, nil)
s.Equal(customCache, segmentManager.segmentsCache)
}

func (s *SegmentManagerTestSuite) TestFetchSegmentsNilConfig() {
segmentManager := NewSegmentManager(0, 0, nil, nil)
segmentManager := NewSegmentManager(nil, 0, 0, nil, nil)
segments, err := segmentManager.FetchQualifiedSegments(s.userKey, s.userValue, nil)
s.Nil(segments)
s.Error(err)
}

func (s *SegmentManagerTestSuite) TestFetchSegmentsNoSegmentsToCheckInConfig() {
segmentManager := NewSegmentManager(0, 0, NewConfig("a", "b", nil), nil)
segmentManager := NewSegmentManager(nil, 0, 0, NewConfig("a", "b", nil), nil)
segments, err := segmentManager.FetchQualifiedSegments(s.userKey, s.userValue, nil)
s.Empty(segments)
s.Nil(err)
Expand Down Expand Up @@ -100,7 +106,7 @@ func (s *SegmentManagerTestSuite) TestOptionsIgnoreCache() {
s.setCache(s.userKey, s.userValue, []string{"a"})
s.segmentAPIManager.On("FetchSegments", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)

segments, err := s.segmentManager.FetchQualifiedSegments(s.userKey, s.userValue, []SegmentOption{IgnoreCache})
segments, err := s.segmentManager.FetchQualifiedSegments(s.userKey, s.userValue, []OptimizelySegmentOption{IgnoreCache})
s.Nil(err)
s.Equal(expectedSegments, segments)
}
Expand All @@ -113,7 +119,7 @@ func (s *SegmentManagerTestSuite) TestOptionsResetCache() {
s.setCache(s.userKey, "456", []string{"a"})
s.segmentAPIManager.On("FetchSegments", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)

segments, err := s.segmentManager.FetchQualifiedSegments(s.userKey, s.userValue, []SegmentOption{ResetCache})
segments, err := s.segmentManager.FetchQualifiedSegments(s.userKey, s.userValue, []OptimizelySegmentOption{ResetCache})
s.Nil(err)
s.Equal(expectedSegments, segments)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/odp/segment_option.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
// Package odp //
package odp

// SegmentOption represents options controlling audience segments.
type SegmentOption int64
// OptimizelySegmentOption represents options controlling audience segments.
type OptimizelySegmentOption int

const (
// IgnoreCache ignores cache (save/lookup)
IgnoreCache SegmentOption = iota
IgnoreCache OptimizelySegmentOption = iota
// ResetCache resets cache
ResetCache
)