From 329c2b825e38f1ab5b6551c68eb625d71b01fa83 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Wed, 13 May 2026 14:47:03 -0700 Subject: [PATCH] [AI-FSSDK] [FSSDK-12369] Add local holdouts support with includedRules field and per-rule evaluation --- pkg/cmab/service_test.go | 10 + pkg/config/datafileprojectconfig/config.go | 26 ++- .../entities/entities.go | 4 + .../datafileprojectconfig/mappers/holdout.go | 38 ++- .../mappers/holdout_test.go | 219 ++++++++++++++++-- pkg/config/interface.go | 4 + pkg/decision/composite_feature_service.go | 7 +- .../evaluator/audience_evaluator_test.go | 10 + pkg/decision/feature_experiment_service.go | 24 ++ pkg/decision/helpers_test.go | 10 + pkg/decision/holdout_service.go | 60 ++++- pkg/decision/holdout_service_test.go | 103 ++++++++ pkg/decision/rollout_service.go | 38 +++ pkg/entities/experiment.go | 8 + 14 files changed, 532 insertions(+), 29 deletions(-) diff --git a/pkg/cmab/service_test.go b/pkg/cmab/service_test.go index 4675d773..8ea9f90b 100644 --- a/pkg/cmab/service_test.go +++ b/pkg/cmab/service_test.go @@ -235,6 +235,16 @@ func (m *MockProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Hol return args.Get(0).([]entities.Holdout) } +func (m *MockProjectConfig) GetGlobalHoldouts() []entities.Holdout { + args := m.Called() + return args.Get(0).([]entities.Holdout) +} + +func (m *MockProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout { + args := m.Called(ruleID) + return args.Get(0).([]entities.Holdout) +} + type CmabServiceTestSuite struct { suite.Suite mockClient *MockCmabClient diff --git a/pkg/config/datafileprojectconfig/config.go b/pkg/config/datafileprojectconfig/config.go index eeeb341a..8ed69b17 100644 --- a/pkg/config/datafileprojectconfig/config.go +++ b/pkg/config/datafileprojectconfig/config.go @@ -63,6 +63,7 @@ type DatafileProjectConfig struct { holdouts []entities.Holdout holdoutIDMap map[string]entities.Holdout flagHoldoutsMap map[string][]entities.Holdout + ruleHoldoutsMap map[string][]entities.Holdout } // GetDatafile returns a string representation of the environment's datafile @@ -284,7 +285,8 @@ func (c DatafileProjectConfig) GetRegion() string { return c.region } -// GetHoldoutsForFlag returns all holdouts applicable to the given feature flag +// GetHoldoutsForFlag returns all holdouts applicable to the given feature flag. +// This returns global holdouts (includedRules == nil) for flag-level evaluation. func (c DatafileProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Holdout { if holdouts, exists := c.flagHoldoutsMap[featureKey]; exists { return holdouts @@ -292,6 +294,25 @@ func (c DatafileProjectConfig) GetHoldoutsForFlag(featureKey string) []entities. return []entities.Holdout{} } +// GetGlobalHoldouts returns all global holdouts (those with includedRules == nil). +func (c DatafileProjectConfig) GetGlobalHoldouts() []entities.Holdout { + global := []entities.Holdout{} + for _, h := range c.holdouts { + if h.IsGlobal() { + global = append(global, h) + } + } + return global +} + +// GetHoldoutsForRule returns all local holdouts targeting the given rule ID. +func (c DatafileProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout { + if holdouts, exists := c.ruleHoldoutsMap[ruleID]; exists { + return holdouts + } + return []entities.Holdout{} +} + // NewDatafileProjectConfig initializes a new datafile from a json byte array using the default JSON datafile parser func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogProducer) (*DatafileProjectConfig, error) { datafile, err := Parse(jsonDatafile) @@ -338,7 +359,7 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP audienceMap, audienceSegmentList := mappers.MapAudiences(append(datafile.TypedAudiences, datafile.Audiences...)) flagVariationsMap := mappers.MapFlagVariations(featureMap) - holdouts, holdoutIDMap, flagHoldoutsMap := mappers.MapHoldouts(datafile.Holdouts, featureMap) + holdouts, holdoutIDMap, flagHoldoutsMap, ruleHoldoutsMap := mappers.MapHoldouts(datafile.Holdouts, featureMap) attributeKeyMap := make(map[string]entities.Attribute) attributeIDToKeyMap := make(map[string]string) @@ -384,6 +405,7 @@ func NewDatafileProjectConfig(jsonDatafile []byte, logger logging.OptimizelyLogP holdouts: holdouts, holdoutIDMap: holdoutIDMap, flagHoldoutsMap: flagHoldoutsMap, + ruleHoldoutsMap: ruleHoldoutsMap, } logger.Info("Datafile is valid.") diff --git a/pkg/config/datafileprojectconfig/entities/entities.go b/pkg/config/datafileprojectconfig/entities/entities.go index 0e68966a..c6292f3a 100644 --- a/pkg/config/datafileprojectconfig/entities/entities.go +++ b/pkg/config/datafileprojectconfig/entities/entities.go @@ -123,6 +123,10 @@ type Holdout struct { AudienceConditions interface{} `json:"audienceConditions"` Variations []Variation `json:"variations"` TrafficAllocation []TrafficAllocation `json:"trafficAllocation"` + // IncludedRules is an optional list of rule IDs this holdout targets. + // nil means global holdout (applies to all rules); a non-nil slice (even + // empty) means local holdout (applies only to listed rules). + IncludedRules []string `json:"includedRules,omitempty"` } // Integration represents a integration from the Optimizely datafile diff --git a/pkg/config/datafileprojectconfig/mappers/holdout.go b/pkg/config/datafileprojectconfig/mappers/holdout.go index 4a51d8a6..c64fa0d3 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout.go @@ -22,18 +22,24 @@ import ( "github.com/optimizely/go-sdk/v2/pkg/entities" ) -// MapHoldouts maps the raw datafile holdout entities to SDK Holdout entities -// All running holdouts apply to all flags +// MapHoldouts maps the raw datafile holdout entities to SDK Holdout entities. +// It returns: +// - holdoutList: all mapped running holdouts +// - holdoutIDMap: map from holdout ID to holdout +// - flagHoldoutsMap: map from feature key to global holdouts (for flag-level evaluation) +// - ruleHoldoutsMap: map from rule ID to local holdouts targeting that rule func MapHoldouts(holdouts []datafileEntities.Holdout, featureMap map[string]entities.Feature) ( holdoutList []entities.Holdout, holdoutIDMap map[string]entities.Holdout, flagHoldoutsMap map[string][]entities.Holdout, + ruleHoldoutsMap map[string][]entities.Holdout, ) { holdoutList = []entities.Holdout{} holdoutIDMap = make(map[string]entities.Holdout) flagHoldoutsMap = make(map[string][]entities.Holdout) + ruleHoldoutsMap = make(map[string][]entities.Holdout) - runningHoldouts := []entities.Holdout{} + globalHoldouts := []entities.Holdout{} for _, holdout := range holdouts { // Only process running holdouts @@ -44,17 +50,26 @@ func MapHoldouts(holdouts []datafileEntities.Holdout, featureMap map[string]enti mappedHoldout := mapHoldout(holdout) holdoutList = append(holdoutList, mappedHoldout) holdoutIDMap[holdout.ID] = mappedHoldout - runningHoldouts = append(runningHoldouts, mappedHoldout) + + if mappedHoldout.IsGlobal() { + // Global holdout: applies to all rules across all flags + globalHoldouts = append(globalHoldouts, mappedHoldout) + } else { + // Local holdout: register per targeted rule ID + for _, ruleID := range mappedHoldout.IncludedRules { + ruleHoldoutsMap[ruleID] = append(ruleHoldoutsMap[ruleID], mappedHoldout) + } + } } - // All running holdouts apply to all flags + // Populate flagHoldoutsMap with global holdouts only (flag-level evaluation) for _, feature := range featureMap { - if len(runningHoldouts) > 0 { - flagHoldoutsMap[feature.Key] = runningHoldouts + if len(globalHoldouts) > 0 { + flagHoldoutsMap[feature.Key] = globalHoldouts } } - return holdoutList, holdoutIDMap, flagHoldoutsMap + return holdoutList, holdoutIDMap, flagHoldoutsMap, ruleHoldoutsMap } func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout { @@ -98,6 +113,12 @@ func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout { } } + // Preserve IncludedRules as-is (nil = global, non-nil slice = local) + var includedRules []string + if datafileHoldout.IncludedRules != nil { + includedRules = datafileHoldout.IncludedRules + } + return entities.Holdout{ ID: datafileHoldout.ID, Key: datafileHoldout.Key, @@ -107,5 +128,6 @@ func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout { Variations: variations, TrafficAllocation: trafficAllocation, AudienceConditionTree: audienceConditionTree, + IncludedRules: includedRules, } } diff --git a/pkg/config/datafileprojectconfig/mappers/holdout_test.go b/pkg/config/datafileprojectconfig/mappers/holdout_test.go index 36df6245..82509744 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout_test.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout_test.go @@ -28,15 +28,16 @@ func TestMapHoldoutsEmpty(t *testing.T) { rawHoldouts := []datafileEntities.Holdout{} featureMap := map[string]entities.Feature{} - holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + holdoutList, holdoutIDMap, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) assert.Empty(t, holdoutList) assert.Empty(t, holdoutIDMap) assert.Empty(t, flagHoldoutsMap) + assert.Empty(t, ruleHoldoutsMap) } func TestMapHoldoutsAppliestoAllFlags(t *testing.T) { - // Running holdouts apply to all flags + // Running holdouts with no includedRules are global — apply to all flags rawHoldouts := []datafileEntities.Holdout{ { ID: "holdout_1", @@ -57,15 +58,16 @@ func TestMapHoldoutsAppliestoAllFlags(t *testing.T) { "feature_3": {ID: "feature_3", Key: "feature_3"}, } - holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + holdoutList, holdoutIDMap, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) // Verify holdout list and ID map assert.Len(t, holdoutList, 1) assert.Len(t, holdoutIDMap, 1) assert.Equal(t, "holdout_1", holdoutList[0].ID) assert.Equal(t, "running_holdout", holdoutList[0].Key) + assert.True(t, holdoutList[0].IsGlobal()) - // Running holdout should apply to all flags + // Global holdout should apply to all flags assert.Contains(t, flagHoldoutsMap, "feature_1") assert.Contains(t, flagHoldoutsMap, "feature_2") assert.Contains(t, flagHoldoutsMap, "feature_3") @@ -73,8 +75,10 @@ func TestMapHoldoutsAppliestoAllFlags(t *testing.T) { assert.Len(t, flagHoldoutsMap["feature_1"], 1) assert.Len(t, flagHoldoutsMap["feature_2"], 1) assert.Len(t, flagHoldoutsMap["feature_3"], 1) -} + // Global holdout should NOT appear in ruleHoldoutsMap + assert.Empty(t, ruleHoldoutsMap) +} func TestMapHoldoutsNotRunning(t *testing.T) { // Holdout with non-Running status should be filtered out @@ -93,16 +97,17 @@ func TestMapHoldoutsNotRunning(t *testing.T) { "feature_1": {ID: "feature_1", Key: "feature_1"}, } - holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + holdoutList, holdoutIDMap, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) // Non-running holdouts should be filtered out assert.Empty(t, holdoutList) assert.Empty(t, holdoutIDMap) assert.Empty(t, flagHoldoutsMap) + assert.Empty(t, ruleHoldoutsMap) } func TestMapHoldoutsMultipleHoldouts(t *testing.T) { - // Multiple running holdouts all apply to all flags + // Multiple running global holdouts all apply to all flags rawHoldouts := []datafileEntities.Holdout{ { ID: "holdout_1", @@ -133,13 +138,13 @@ func TestMapHoldoutsMultipleHoldouts(t *testing.T) { "feature_2": {ID: "feature_2", Key: "feature_2"}, } - holdoutList, holdoutIDMap, flagHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + holdoutList, holdoutIDMap, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) // Verify both holdouts are in the list assert.Len(t, holdoutList, 2) assert.Len(t, holdoutIDMap, 2) - // Both features should get both holdouts + // Both features should get both global holdouts assert.Contains(t, flagHoldoutsMap, "feature_1") assert.Len(t, flagHoldoutsMap["feature_1"], 2) assert.Equal(t, "holdout_1", flagHoldoutsMap["feature_1"][0].Key) @@ -147,11 +152,10 @@ func TestMapHoldoutsMultipleHoldouts(t *testing.T) { assert.Contains(t, flagHoldoutsMap, "feature_2") assert.Len(t, flagHoldoutsMap["feature_2"], 2) - assert.Equal(t, "holdout_1", flagHoldoutsMap["feature_2"][0].Key) - assert.Equal(t, "holdout_2", flagHoldoutsMap["feature_2"][1].Key) -} - + // No local holdouts + assert.Empty(t, ruleHoldoutsMap) +} func TestMapHoldoutsWithAudienceConditions(t *testing.T) { rawHoldouts := []datafileEntities.Holdout{ @@ -174,7 +178,7 @@ func TestMapHoldoutsWithAudienceConditions(t *testing.T) { "feature_1": {ID: "feature_1", Key: "feature_1"}, } - holdoutList, _, _ := MapHoldouts(rawHoldouts, featureMap) + holdoutList, _, _, _ := MapHoldouts(rawHoldouts, featureMap) // Verify audience conditions are mapped assert.Len(t, holdoutList, 1) @@ -214,7 +218,7 @@ func TestMapHoldoutsVariationsMapping(t *testing.T) { "feature_1": {ID: "feature_1", Key: "feature_1"}, } - holdoutList, _, _ := MapHoldouts(rawHoldouts, featureMap) + holdoutList, _, _, _ := MapHoldouts(rawHoldouts, featureMap) // Verify variations are mapped correctly assert.Len(t, holdoutList, 1) @@ -229,3 +233,188 @@ func TestMapHoldoutsVariationsMapping(t *testing.T) { assert.Equal(t, "var_2", holdoutList[0].TrafficAllocation[1].EntityID) assert.Equal(t, 10000, holdoutList[0].TrafficAllocation[1].EndOfRange) } + +// TestMapLocalHoldoutSingleRule tests a local holdout targeting a single rule. +func TestMapLocalHoldoutSingleRule(t *testing.T) { + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "local_holdout_1", + Key: "local_holdout", + Status: "Running", + IncludedRules: []string{"rule_123"}, + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, holdoutIDMap, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + assert.Len(t, holdoutList, 1) + assert.Len(t, holdoutIDMap, 1) + assert.False(t, holdoutList[0].IsGlobal()) + assert.Equal(t, []string{"rule_123"}, holdoutList[0].IncludedRules) + + // Local holdout should NOT appear in flagHoldoutsMap + assert.Empty(t, flagHoldoutsMap) + + // Local holdout SHOULD appear in ruleHoldoutsMap under its rule ID + assert.Len(t, ruleHoldoutsMap, 1) + assert.Contains(t, ruleHoldoutsMap, "rule_123") + assert.Len(t, ruleHoldoutsMap["rule_123"], 1) + assert.Equal(t, "local_holdout_1", ruleHoldoutsMap["rule_123"][0].ID) +} + +// TestMapLocalHoldoutMultipleRules tests a local holdout targeting multiple rules. +func TestMapLocalHoldoutMultipleRules(t *testing.T) { + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "local_holdout_1", + Key: "local_holdout", + Status: "Running", + IncludedRules: []string{"rule_1", "rule_2", "rule_3"}, + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + + featureMap := map[string]entities.Feature{} + + _, _, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + // Local holdout should NOT appear in flagHoldoutsMap + assert.Empty(t, flagHoldoutsMap) + + // Each rule gets an entry + assert.Contains(t, ruleHoldoutsMap, "rule_1") + assert.Contains(t, ruleHoldoutsMap, "rule_2") + assert.Contains(t, ruleHoldoutsMap, "rule_3") + assert.Len(t, ruleHoldoutsMap["rule_1"], 1) + assert.Len(t, ruleHoldoutsMap["rule_2"], 1) + assert.Len(t, ruleHoldoutsMap["rule_3"], 1) +} + +// TestMapEmptyIncludedRulesIsLocal tests that an empty (non-nil) IncludedRules is local. +func TestMapEmptyIncludedRulesIsLocal(t *testing.T) { + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "holdout_empty_rules", + Key: "empty_rules_holdout", + Status: "Running", + IncludedRules: []string{}, // empty slice = local holdout with no rules + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, _, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + assert.Len(t, holdoutList, 1) + // Empty IncludedRules is NOT nil, so it is local (not global) + assert.False(t, holdoutList[0].IsGlobal()) + + // Should not appear in flagHoldoutsMap + assert.Empty(t, flagHoldoutsMap) + // No rule IDs to register, so ruleHoldoutsMap is also empty + assert.Empty(t, ruleHoldoutsMap) +} + +// TestMapMixedGlobalAndLocalHoldouts tests a mix of global and local holdouts. +func TestMapMixedGlobalAndLocalHoldouts(t *testing.T) { + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "global_holdout", + Key: "global", + Status: "Running", + // nil IncludedRules = global + Variations: []datafileEntities.Variation{ + {ID: "gvar", Key: "gvar"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "gvar", EndOfRange: 5000}, + }, + }, + { + ID: "local_holdout", + Key: "local", + Status: "Running", + IncludedRules: []string{"rule_abc"}, + Variations: []datafileEntities.Variation{ + {ID: "lvar", Key: "lvar"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "lvar", EndOfRange: 3000}, + }, + }, + } + + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, _, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + assert.Len(t, holdoutList, 2) + + // Global holdout in flagHoldoutsMap + assert.Len(t, flagHoldoutsMap["feature_1"], 1) + assert.Equal(t, "global_holdout", flagHoldoutsMap["feature_1"][0].ID) + + // Local holdout in ruleHoldoutsMap + assert.Contains(t, ruleHoldoutsMap, "rule_abc") + assert.Len(t, ruleHoldoutsMap["rule_abc"], 1) + assert.Equal(t, "local_holdout", ruleHoldoutsMap["rule_abc"][0].ID) +} + +// TestMapBackwardCompatibilityOldDatafile tests that old datafiles without +// includedRules field still parse correctly as global holdouts. +func TestMapBackwardCompatibilityOldDatafile(t *testing.T) { + // Old datafile holdout has no IncludedRules field (nil by default) + rawHoldouts := []datafileEntities.Holdout{ + { + ID: "old_holdout", + Key: "old_holdout", + Status: "Running", + // IncludedRules not set, defaults to nil = global + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + + featureMap := map[string]entities.Feature{ + "feature_1": {ID: "feature_1", Key: "feature_1"}, + } + + holdoutList, _, flagHoldoutsMap, ruleHoldoutsMap := MapHoldouts(rawHoldouts, featureMap) + + assert.Len(t, holdoutList, 1) + assert.Nil(t, holdoutList[0].IncludedRules) + assert.True(t, holdoutList[0].IsGlobal()) + + // Should appear in flagHoldoutsMap as before + assert.Len(t, flagHoldoutsMap["feature_1"], 1) + assert.Empty(t, ruleHoldoutsMap) +} diff --git a/pkg/config/interface.go b/pkg/config/interface.go index 6d45c14a..ff77debf 100644 --- a/pkg/config/interface.go +++ b/pkg/config/interface.go @@ -57,6 +57,10 @@ type ProjectConfig interface { GetFlagVariationsMap() map[string][]entities.Variation GetRegion() string GetHoldoutsForFlag(featureKey string) []entities.Holdout + // GetGlobalHoldouts returns holdouts with includedRules == nil (applies to all rules). + GetGlobalHoldouts() []entities.Holdout + // GetHoldoutsForRule returns local holdouts targeting the given rule ID. + GetHoldoutsForRule(ruleID string) []entities.Holdout } // ProjectConfigManager maintains an instance of the ProjectConfig diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index 2c1b2b54..8fae1198 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -31,12 +31,13 @@ type CompositeFeatureService struct { // NewCompositeFeatureService returns a new instance of the CompositeFeatureService func NewCompositeFeatureService(sdkKey string, compositeExperimentService ExperimentService) *CompositeFeatureService { + holdoutSvc := NewHoldoutService(sdkKey) return &CompositeFeatureService{ logger: logging.GetLogger(sdkKey, "CompositeFeatureService"), featureServices: []FeatureService{ - NewHoldoutService(sdkKey), - NewFeatureExperimentService(logging.GetLogger(sdkKey, "FeatureExperimentService"), compositeExperimentService), - NewRolloutService(sdkKey), + holdoutSvc, + NewFeatureExperimentService(logging.GetLogger(sdkKey, "FeatureExperimentService"), compositeExperimentService).WithHoldoutService(holdoutSvc), + NewRolloutService(sdkKey).WithHoldoutService(holdoutSvc), }, } } diff --git a/pkg/decision/evaluator/audience_evaluator_test.go b/pkg/decision/evaluator/audience_evaluator_test.go index 75897aed..e23dd86a 100644 --- a/pkg/decision/evaluator/audience_evaluator_test.go +++ b/pkg/decision/evaluator/audience_evaluator_test.go @@ -205,6 +205,16 @@ func (m *MockProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Hol return args.Get(0).([]entities.Holdout) } +func (m *MockProjectConfig) GetGlobalHoldouts() []entities.Holdout { + args := m.Called() + return args.Get(0).([]entities.Holdout) +} + +func (m *MockProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout { + args := m.Called(ruleID) + return args.Get(0).([]entities.Holdout) +} + // MockLogger is a mock implementation of OptimizelyLogProducer // (This declaration has been removed to resolve the redeclaration error) diff --git a/pkg/decision/feature_experiment_service.go b/pkg/decision/feature_experiment_service.go index f2bc3689..1b9ae3e1 100644 --- a/pkg/decision/feature_experiment_service.go +++ b/pkg/decision/feature_experiment_service.go @@ -28,6 +28,7 @@ import ( // FeatureExperimentService helps evaluate feature test associated with the feature type FeatureExperimentService struct { compositeExperimentService ExperimentService + holdoutService *HoldoutService logger logging.OptimizelyLogProducer } @@ -36,9 +37,17 @@ func NewFeatureExperimentService(logger logging.OptimizelyLogProducer, composite return &FeatureExperimentService{ logger: logger, compositeExperimentService: compositeExperimentService, + holdoutService: nil, // set via WithHoldoutService when available } } +// WithHoldoutService sets the HoldoutService on the FeatureExperimentService for +// local holdout evaluation. Returns the same receiver for chaining. +func (f *FeatureExperimentService) WithHoldoutService(hs *HoldoutService) *FeatureExperimentService { + f.holdoutService = hs + return f +} + // GetDecision returns a decision for the given feature test and user context func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext, options *decide.Options) (FeatureDecision, decide.DecisionReasons, error) { feature := decisionContext.Feature @@ -46,6 +55,21 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon // @TODO this can be improved by getting group ID first and determining experiment and then bucketing in experiment for _, featureExperiment := range feature.FeatureExperiments { + // Check local holdouts targeting this experiment rule BEFORE forced decisions. + if f.holdoutService != nil { + localDecision, localReasons := f.holdoutService.EvaluateLocalHoldouts( + featureExperiment.ID, + feature.Key, + decisionContext, + userContext, + options, + ) + reasons.Append(localReasons) + if localDecision != nil { + return *localDecision, reasons, nil + } + } + // Checking for forced decision if decisionContext.ForcedDecisionService != nil { forcedDecision, _reasons, err := decisionContext.ForcedDecisionService.FindValidatedForcedDecision(decisionContext.ProjectConfig, OptimizelyDecisionContext{FlagKey: feature.Key, RuleKey: featureExperiment.Key}, options) diff --git a/pkg/decision/helpers_test.go b/pkg/decision/helpers_test.go index 8c7a94cc..7f308dbd 100644 --- a/pkg/decision/helpers_test.go +++ b/pkg/decision/helpers_test.go @@ -64,6 +64,16 @@ func (c *mockProjectConfig) GetHoldoutsForFlag(featureKey string) []entities.Hol return args.Get(0).([]entities.Holdout) } +func (c *mockProjectConfig) GetGlobalHoldouts() []entities.Holdout { + args := c.Called() + return args.Get(0).([]entities.Holdout) +} + +func (c *mockProjectConfig) GetHoldoutsForRule(ruleID string) []entities.Holdout { + args := c.Called(ruleID) + return args.Get(0).([]entities.Holdout) +} + type MockExperimentDecisionService struct { mock.Mock } diff --git a/pkg/decision/holdout_service.go b/pkg/decision/holdout_service.go index 48fae2ad..223bb229 100644 --- a/pkg/decision/holdout_service.go +++ b/pkg/decision/holdout_service.go @@ -45,11 +45,14 @@ func NewHoldoutService(sdkKey string) *HoldoutService { } } -// GetDecision returns a decision for holdouts associated with the feature +// GetDecision evaluates global holdouts (includedRules == nil) at the flag level. +// Local holdouts (includedRules != nil) are evaluated per-rule inside +// FeatureExperimentService and RolloutService. func (h HoldoutService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext, options *decide.Options) (FeatureDecision, decide.DecisionReasons, error) { feature := decisionContext.Feature reasons := decide.NewDecisionReasons(options) + // GetHoldoutsForFlag returns only global holdouts after the mapper update. holdouts := decisionContext.ProjectConfig.GetHoldoutsForFlag(feature.Key) for i := range holdouts { @@ -153,3 +156,58 @@ type decisionResult struct { result bool reasons decide.DecisionReasons } + +// EvaluateLocalHoldouts checks if the user hits any local holdout that targets the +// given rule ID. It returns a non-nil FeatureDecision when the user is in a holdout, +// signalling that rule evaluation should be skipped. +func (h HoldoutService) EvaluateLocalHoldouts( + ruleID string, + featureKey string, + decisionContext FeatureDecisionContext, + userContext entities.UserContext, + options *decide.Options, +) (holdoutDecision *FeatureDecision, reasons decide.DecisionReasons) { + reasons = decide.NewDecisionReasons(options) + + localHoldouts := decisionContext.ProjectConfig.GetHoldoutsForRule(ruleID) + for i := range localHoldouts { + holdout := &localHoldouts[i] + h.logger.Debug(fmt.Sprintf("Evaluating local holdout %s for rule %s", holdout.Key, ruleID)) + + inAudience := h.checkIfUserInHoldoutAudience(holdout, userContext, decisionContext.ProjectConfig, options) + reasons.Append(inAudience.reasons) + if !inAudience.result { + continue + } + + bucketingID, err := userContext.GetBucketingID() + if err != nil { + msg := reasons.AddInfo("Error computing bucketing ID for local holdout %q: %q", holdout.Key, err.Error()) + h.logger.Debug(msg) + } + + experimentForBucketing := entities.Experiment{ + ID: holdout.ID, + Key: holdout.Key, + Variations: holdout.Variations, + TrafficAllocation: holdout.TrafficAllocation, + AudienceIds: holdout.AudienceIds, + AudienceConditions: holdout.AudienceConditions, + AudienceConditionTree: holdout.AudienceConditionTree, + } + + variation, _, _ := h.bucketer.Bucket(bucketingID, experimentForBucketing, entities.Group{}) + if variation != nil { + reason := reasons.AddInfo("User %s is in local holdout %s for rule %s.", userContext.ID, holdout.Key, ruleID) + h.logger.Info(reason) + decision := &FeatureDecision{ + Experiment: experimentForBucketing, + Variation: variation, + Source: Holdout, + } + return decision, reasons + } + } + + return nil, reasons +} diff --git a/pkg/decision/holdout_service_test.go b/pkg/decision/holdout_service_test.go index 9a5e4a71..254a9979 100644 --- a/pkg/decision/holdout_service_test.go +++ b/pkg/decision/holdout_service_test.go @@ -335,6 +335,109 @@ func (s *HoldoutServiceTestSuite) TestCheckIfUserInHoldoutAudienceWithConditionT s.mockAudienceTreeEvaluator.AssertExpectations(s.T()) } +// TestEvaluateLocalHoldoutsNoHoldouts checks that EvaluateLocalHoldouts returns nil when +// there are no local holdouts for the rule. +func (s *HoldoutServiceTestSuite) TestEvaluateLocalHoldoutsNoHoldouts() { + s.mockConfig.On("GetHoldoutsForRule", "rule_123").Return([]entities.Holdout{}) + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + localDecision, _ := testHoldoutService.EvaluateLocalHoldouts( + "rule_123", + "test_feature_with_holdout", + s.testFeatureDecisionContext, + s.testUserContext, + s.options, + ) + + s.Nil(localDecision) +} + +// TestEvaluateLocalHoldoutsUserBucketed checks that a user bucketed into a local holdout +// gets a non-nil decision with source Holdout. +func (s *HoldoutServiceTestSuite) TestEvaluateLocalHoldoutsUserBucketed() { + localHoldout := entities.Holdout{ + ID: "local_holdout_1", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + AudienceConditionTree: nil, + IncludedRules: []string{"rule_123"}, + Variations: map[string]entities.Variation{ + "holdout_var_1": testHoldoutVar1, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "holdout_var_1", EndOfRange: 10000}, + }, + } + + s.mockConfig.On("GetHoldoutsForRule", "rule_123").Return([]entities.Holdout{localHoldout}) + s.mockBucketer.On("Bucket", "test_user_holdout", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&testHoldoutVar1, reasons.Reason(""), nil) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + localDecision, _ := testHoldoutService.EvaluateLocalHoldouts( + "rule_123", + "test_feature_with_holdout", + s.testFeatureDecisionContext, + s.testUserContext, + s.options, + ) + + s.NotNil(localDecision) + s.Equal(Holdout, localDecision.Source) + s.Equal(testHoldoutVar1.ID, localDecision.Variation.ID) + s.mockBucketer.AssertExpectations(s.T()) +} + +// TestEvaluateLocalHoldoutsUserNotBucketed checks that when a user misses the local +// holdout traffic allocation, EvaluateLocalHoldouts returns nil. +func (s *HoldoutServiceTestSuite) TestEvaluateLocalHoldoutsUserNotBucketed() { + localHoldout := entities.Holdout{ + ID: "local_holdout_1", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + AudienceConditionTree: nil, + IncludedRules: []string{"rule_123"}, + Variations: map[string]entities.Variation{ + "holdout_var_1": testHoldoutVar1, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "holdout_var_1", EndOfRange: 0}, // 0% traffic + }, + } + + s.mockConfig.On("GetHoldoutsForRule", "rule_123").Return([]entities.Holdout{localHoldout}) + s.mockBucketer.On("Bucket", "test_user_holdout", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(nil, reasons.Reason(""), nil) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testHoldoutService := HoldoutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + bucketer: s.mockBucketer, + logger: s.mockLogger, + } + + localDecision, _ := testHoldoutService.EvaluateLocalHoldouts( + "rule_123", + "test_feature_with_holdout", + s.testFeatureDecisionContext, + s.testUserContext, + s.options, + ) + + s.Nil(localDecision) +} + func TestHoldoutServiceTestSuite(t *testing.T) { suite.Run(t, new(HoldoutServiceTestSuite)) } diff --git a/pkg/decision/rollout_service.go b/pkg/decision/rollout_service.go index 13c1402c..44441b32 100644 --- a/pkg/decision/rollout_service.go +++ b/pkg/decision/rollout_service.go @@ -32,6 +32,7 @@ import ( type RolloutService struct { audienceTreeEvaluator evaluator.TreeEvaluator experimentBucketerService ExperimentService + holdoutService *HoldoutService logger logging.OptimizelyLogProducer } @@ -42,9 +43,16 @@ func NewRolloutService(sdkKey string) *RolloutService { logger: logger, audienceTreeEvaluator: evaluator.NewMixedTreeEvaluator(logger), experimentBucketerService: NewExperimentBucketerService(logging.GetLogger(sdkKey, "ExperimentBucketerService")), + holdoutService: nil, // set via WithHoldoutService } } +// WithHoldoutService sets the HoldoutService for local holdout evaluation per rollout rule. +func (r *RolloutService) WithHoldoutService(hs *HoldoutService) *RolloutService { + r.holdoutService = hs + return r +} + // GetDecision returns a decision for the given feature and user context func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext, options *decide.Options) (FeatureDecision, decide.DecisionReasons, error) { featureDecision := FeatureDecision{ @@ -102,6 +110,21 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user loggingKey := strconv.Itoa(index + 1) experiment := &rollout.Experiments[index] + // Check local holdouts targeting this delivery rule before forced decisions. + if r.holdoutService != nil { + localDecision, localReasons := r.holdoutService.EvaluateLocalHoldouts( + experiment.ID, + feature.Key, + decisionContext, + userContext, + options, + ) + reasons.Append(localReasons) + if localDecision != nil { + return *localDecision, reasons, nil + } + } + // Checking for forced decision if forcedDecision := checkForForcedDecision(experiment); forcedDecision != nil { return *forcedDecision, reasons, nil @@ -132,6 +155,21 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user // fall back rule / last rule experiment := &rollout.Experiments[numberOfExperiments-1] + // Check local holdouts targeting the last (everyone-else) rule as well. + if r.holdoutService != nil { + localDecision, localReasons := r.holdoutService.EvaluateLocalHoldouts( + experiment.ID, + feature.Key, + decisionContext, + userContext, + options, + ) + reasons.Append(localReasons) + if localDecision != nil { + return *localDecision, reasons, nil + } + } + // Checking for forced decision if forcedDecision := checkForForcedDecision(experiment); forcedDecision != nil { return *forcedDecision, reasons, nil diff --git a/pkg/entities/experiment.go b/pkg/entities/experiment.go index 0de662b6..8a340915 100644 --- a/pkg/entities/experiment.go +++ b/pkg/entities/experiment.go @@ -90,4 +90,12 @@ type Holdout struct { Variations map[string]Variation // keyed by variation ID TrafficAllocation []Range AudienceConditionTree *TreeNode + // IncludedRules is nil for global holdouts; a non-nil slice (even empty) + // indicates a local holdout that targets only the listed rule IDs. + IncludedRules []string +} + +// IsGlobal returns true when the holdout applies to all rules (includedRules == nil). +func (h Holdout) IsGlobal() bool { + return h.IncludedRules == nil }