diff --git a/pkg/client/client.go b/pkg/client/client.go index 9f4f232c..838f782d 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2025, Optimizely, Inc. and contributors * + * Copyright 2019-2026, 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. * @@ -248,27 +248,18 @@ func (o *OptimizelyClient) dispatchDecisionEvents(projectConfig config.ProjectCo o.EventProcessor.ProcessEvent(ue) eventSent = true } - if o.sendHoldoutImpression(projectConfig, featureDecision, usrContext, key) { - eventSent = true + for _, hd := range featureDecision.HoldoutDecisions { + if hd.Variation == nil { + continue + } + if hue, ok := event.CreateImpressionUserEvent(projectConfig, hd.Experiment, hd.Variation, usrContext, key, hd.Experiment.Key, decision.Holdout, hd.Variation.FeatureEnabled, nil); ok { + o.EventProcessor.ProcessEvent(hue) + eventSent = true + } } return eventSent } -// sendHoldoutImpression emits a holdout impression event when a holdout was bypassed for a -// targeted delivery rule (the decision carries the bucketed holdout experiment/variation). -// It returns true if an event was dispatched. -func (o *OptimizelyClient) sendHoldoutImpression(projectConfig config.ProjectConfig, featureDecision decision.FeatureDecision, userContext entities.UserContext, key string) bool { - if featureDecision.HoldoutExperiment == nil || featureDecision.HoldoutVariation == nil { - return false - } - if hue, hok := event.CreateImpressionUserEvent(projectConfig, *featureDecision.HoldoutExperiment, - featureDecision.HoldoutVariation, userContext, key, featureDecision.HoldoutExperiment.Key, decision.Holdout, featureDecision.HoldoutVariation.FeatureEnabled, nil); hok { - o.EventProcessor.ProcessEvent(hue) - return true - } - return false -} - func (o *OptimizelyClient) decideForKeys(userContext OptimizelyUserContext, keys []string, options *decide.Options) map[string]OptimizelyDecision { var err error defer func() { @@ -546,8 +537,6 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit featureDecision.Variation, userContext, featureKey, featureDecision.Experiment.Key, featureDecision.Source, result, featureDecision.CmabUUID); ok && featureDecision.Source != "" { o.EventProcessor.ProcessEvent(ue) } - // Send holdout impression when holdout is bypassed for targeted delivery - o.sendHoldoutImpression(decisionContext.ProjectConfig, featureDecision, userContext, featureKey) return result, err } @@ -913,8 +902,6 @@ func (o *OptimizelyClient) GetDetailedFeatureDecisionUnsafe(featureKey string, u featureDecision.Variation, userContext, featureKey, featureDecision.Experiment.Key, featureDecision.Source, decisionInfo.Enabled, featureDecision.CmabUUID); ok { o.EventProcessor.ProcessEvent(ue) } - // Send holdout impression when holdout is bypassed for targeted delivery - o.sendHoldoutImpression(decisionContext.ProjectConfig, featureDecision, userContext, featureKey) } } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index c1b2f873..0db21c19 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2020,2022-2025 Optimizely, Inc. and contributors * + * Copyright 2019-2020,2022-2026 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. * @@ -3271,3 +3271,223 @@ func TestClientTestSuiteTrackEvent(t *testing.T) { func TestClientTestSuiteTrackNotification(t *testing.T) { suite.Run(t, new(ClientTestSuiteTrackNotification)) } + +type localHoldoutClientFixture struct { + client *OptimizelyClient + config *MockProjectConfig + feature entities.Feature + eventProcessor *MockEventProcessor +} + +func newLocalHoldoutClientFixture(featureDecision decision.FeatureDecision) localHoldoutClientFixture { + mockConfig := new(MockProjectConfig) + mockConfigManager := new(MockProjectConfigManager) + mockDecisionService := new(MockDecisionService) + mockEventProcessor := new(MockEventProcessor) + + feature := makeTestFeatureWithExperiment("feature_1", makeTestExperiment("exp_1")) + mockConfig.On("GetFeatureByKey", feature.Key).Return(feature, nil) + mockConfigManager.On("GetConfig").Return(mockConfig, nil) + mockDecisionService.On("GetFeatureDecision", mock.Anything, mock.Anything, mock.Anything).Return(featureDecision, decide.NewDecisionReasons(nil), nil) + mockEventProcessor.On("ProcessEvent", mock.AnythingOfType("event.UserEvent")) + + return localHoldoutClientFixture{ + client: &OptimizelyClient{ + ConfigManager: mockConfigManager, + DecisionService: mockDecisionService, + EventProcessor: mockEventProcessor, + defaultDecideOptions: &decide.Options{}, + logger: logging.GetLogger("", ""), + tracer: &MockTracer{}, + }, + config: mockConfig, + feature: feature, + eventProcessor: mockEventProcessor, + } +} + +func makeLocalHoldoutDecision(holdoutKey string) decision.FeatureDecision { + variation := makeTestVariation("holdout_off", false) + return decision.FeatureDecision{ + Experiment: makeTestExperimentWithVariations(holdoutKey, []entities.Variation{variation}), + Variation: &variation, + Source: decision.Holdout, + } +} + +func processedImpressions(ep *MockEventProcessor, ruleType string) []event.UserEvent { + var events []event.UserEvent + for _, call := range ep.Calls { + if call.Method != "ProcessEvent" { + continue + } + ue := call.Arguments.Get(0).(event.UserEvent) + if ue.Impression != nil && ue.Impression.Metadata.RuleType == ruleType { + events = append(events, ue) + } + } + return events +} + +func TestDecideLocalHoldoutHitThenLaterRuleServes(t *testing.T) { + servedVariation := makeTestVariation("on", true) + servedExperiment := makeTestExperimentWithVariations("exp_2", []entities.Variation{servedVariation}) + f := newLocalHoldoutClientFixture(decision.FeatureDecision{ + Experiment: servedExperiment, + Variation: &servedVariation, + Source: decision.FeatureTest, + HoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("local_holdout")}, + }) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + d := f.client.decide(&userContext, f.feature.Key, &decide.Options{}) + + assert.Equal(t, "on", d.VariationKey) + assert.Equal(t, "exp_2", d.RuleKey) + assert.True(t, d.Enabled) + holdoutEvents := processedImpressions(f.eventProcessor, decision.Holdout) + assert.Len(t, holdoutEvents, 1) + assert.Equal(t, "local_holdout", holdoutEvents[0].Impression.Metadata.RuleKey) + assert.Equal(t, f.feature.Key, holdoutEvents[0].Impression.Metadata.FlagKey) + assert.Len(t, processedImpressions(f.eventProcessor, decision.FeatureTest), 1) +} + +func TestDecideLocalHoldoutHitsOnSeveralRulesSendSeveralEvents(t *testing.T) { + f := newLocalHoldoutClientFixture(decision.FeatureDecision{ + Source: decision.Rollout, + HoldoutDecisions: []decision.FeatureDecision{ + makeLocalHoldoutDecision("local_holdout"), + makeLocalHoldoutDecision("local_holdout"), + }, + }) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + d := f.client.decide(&userContext, f.feature.Key, &decide.Options{}) + + assert.Equal(t, "", d.VariationKey) + assert.False(t, d.Enabled) + // MockProjectConfig has sendFlagDecisions off: holdout events still go out, no rollout event does. + assert.Len(t, processedImpressions(f.eventProcessor, decision.Holdout), 2) + assert.Len(t, f.eventProcessor.Calls, 2) +} + +func TestDecideLocalHoldoutOnDeliveryRuleSendsEventWithSendFlagDecisionsOff(t *testing.T) { + servedVariation := makeTestVariation("on", true) + f := newLocalHoldoutClientFixture(decision.FeatureDecision{ + Experiment: makeTestExperimentWithVariations("rollout_2", []entities.Variation{servedVariation}), + Variation: &servedVariation, + Source: decision.Rollout, + HoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("delivery_holdout")}, + }) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + d := f.client.decide(&userContext, f.feature.Key, &decide.Options{}) + + assert.Equal(t, "rollout_2", d.RuleKey) + assert.Len(t, processedImpressions(f.eventProcessor, decision.Holdout), 1) + assert.Len(t, f.eventProcessor.Calls, 1) +} + +func TestDecideDisableDecisionEventSuppressesLocalHoldoutEvents(t *testing.T) { + f := newLocalHoldoutClientFixture(decision.FeatureDecision{ + Source: decision.Rollout, + HoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("local_holdout")}, + }) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + f.client.decide(&userContext, f.feature.Key, &decide.Options{DisableDecisionEvent: true}) + + f.eventProcessor.AssertNotCalled(t, "ProcessEvent", mock.Anything) +} + +func TestDecideDecisionErrorSuppressesLocalHoldoutEvents(t *testing.T) { + holdoutDecision := decision.FeatureDecision{ + Source: decision.FeatureTest, + HoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("local_holdout")}, + } + f := newLocalHoldoutClientFixture(holdoutDecision) + cmabErrorMessage := "Failed to fetch CMAB data for experiment exp_1." + mockDecisionService := f.client.DecisionService.(*MockDecisionService) + mockDecisionService.ExpectedCalls = nil + mockDecisionService.On("GetFeatureDecision", mock.Anything, mock.Anything, mock.Anything).Return(holdoutDecision, decide.NewDecisionReasons(nil), errors.New(cmabErrorMessage)) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + d := f.client.decide(&userContext, f.feature.Key, &decide.Options{}) + + assert.Equal(t, "", d.VariationKey) + assert.Equal(t, "", d.RuleKey) + assert.False(t, d.Enabled) + assert.Contains(t, d.Reasons, cmabErrorMessage) + f.eventProcessor.AssertNotCalled(t, "ProcessEvent", mock.Anything) +} + +func makeExcludeTDBypassDecision(localHits ...decision.FeatureDecision) decision.FeatureDecision { + servedVariation := makeTestVariation("on", true) + globalHit := makeLocalHoldoutDecision("global_holdout") + return decision.FeatureDecision{ + Experiment: makeTestExperimentWithVariations("rollout_1", []entities.Variation{servedVariation}), + Variation: &servedVariation, + Source: decision.Rollout, + HoldoutExperiment: &globalHit.Experiment, + HoldoutVariation: globalHit.Variation, + HoldoutDecisions: append([]decision.FeatureDecision{globalHit}, localHits...), + } +} + +func TestLegacyAPIsSendNoHoldoutImpression(t *testing.T) { + servedVariation := makeTestVariation("on", true) + localHit := decision.FeatureDecision{ + Experiment: makeTestExperimentWithVariations("exp_2", []entities.Variation{servedVariation}), + Variation: &servedVariation, + Source: decision.FeatureTest, + HoldoutDecisions: []decision.FeatureDecision{makeLocalHoldoutDecision("local_holdout")}, + } + decisions := map[string]decision.FeatureDecision{ + "local holdout hit": localHit, + "global excludeTD bypassed": makeExcludeTDBypassDecision(), + } + userContext := entities.UserContext{ID: "test_user_1"} + + for name, featureDecision := range decisions { + t.Run(name+"/IsFeatureEnabled", func(t *testing.T) { + f := newLocalHoldoutClientFixture(featureDecision) + enabled, err := f.client.IsFeatureEnabled(f.feature.Key, userContext) + assert.NoError(t, err) + assert.True(t, enabled) + assert.Empty(t, processedImpressions(f.eventProcessor, decision.Holdout)) + }) + t.Run(name+"/GetFeatureVariable", func(t *testing.T) { + f := newLocalHoldoutClientFixture(featureDecision) + f.client.notificationCenter = notification.NewNotificationCenter() + f.config.On("GetVariableByKey", f.feature.Key, "var_1").Return(entities.Variable{Key: "var_1", DefaultValue: "x", Type: entities.String}, nil) + _, _, err := f.client.GetFeatureVariable(f.feature.Key, "var_1", userContext) + assert.NoError(t, err) + assert.Empty(t, processedImpressions(f.eventProcessor, decision.Holdout)) + }) + t.Run(name+"/GetDetailedFeatureDecisionUnsafe", func(t *testing.T) { + f := newLocalHoldoutClientFixture(featureDecision) + _, err := f.client.GetDetailedFeatureDecisionUnsafe(f.feature.Key, userContext, false) + assert.NoError(t, err) + assert.Empty(t, processedImpressions(f.eventProcessor, decision.Holdout)) + }) + } +} + +func TestDecideSendsGlobalExcludeTDHoldoutFirstThenLocalHits(t *testing.T) { + f := newLocalHoldoutClientFixture(makeExcludeTDBypassDecision( + makeLocalHoldoutDecision("experiment_holdout"), + makeLocalHoldoutDecision("delivery_holdout"), + )) + userContext := newOptimizelyUserContext(f.client, "test_user_1", nil, nil, nil) + + d := f.client.decide(&userContext, f.feature.Key, &decide.Options{}) + + assert.Equal(t, "rollout_1", d.RuleKey) + holdoutEvents := processedImpressions(f.eventProcessor, decision.Holdout) + var ruleKeys []string + for _, e := range holdoutEvents { + ruleKeys = append(ruleKeys, e.Impression.Metadata.RuleKey) + } + assert.Equal(t, []string{"global_holdout", "experiment_holdout", "delivery_holdout"}, ruleKeys) + assert.Len(t, f.eventProcessor.Calls, 3) +} diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index 073e7ebf..63ee7bbb 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -66,6 +66,7 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont } var featureDecision FeatureDecision + var holdoutDecisions []FeatureDecision var err error for _, featureDecisionService := range f.featureServices { var decisionReasons decide.DecisionReasons @@ -77,24 +78,34 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont return FeatureDecision{}, reasons, err } + holdoutDecisions = append(holdoutDecisions, featureDecision.HoldoutDecisions...) + featureDecision.HoldoutDecisions = holdoutDecisions if featureDecision.Variation != nil { - return featureDecision, reasons, err + return withFirstHoldout(featureDecision), reasons, err } } - return featureDecision, reasons, err + return withFirstHoldout(featureDecision), reasons, err +} + +// withFirstHoldout populates the deprecated HoldoutExperiment/HoldoutVariation fields from the first holdout hit. +func withFirstHoldout(featureDecision FeatureDecision) FeatureDecision { + if len(featureDecision.HoldoutDecisions) > 0 { + first := featureDecision.HoldoutDecisions[0] + featureDecision.HoldoutExperiment = &first.Experiment + featureDecision.HoldoutVariation = first.Variation + } + return featureDecision } // getDecisionWithExcludedTD handles the exclude_targeted_deliveries holdout logic. // When a holdout has ExcludeTargetedDeliveries set, AB/MAB/CMAB experiments are // blocked (holdout returned) but targeted delivery rules are allowed through. func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision FeatureDecision, decisionContext FeatureDecisionContext, userContext entities.UserContext, options *decide.Options, reasons decide.DecisionReasons) (FeatureDecision, decide.DecisionReasons, error) { - holdoutExp := holdoutDecision.Experiment - holdoutVar := holdoutDecision.Variation - reasons.AddInfo("Holdout \"%s\" has excludeTargetedDeliveries enabled, continuing to rollout evaluation.", holdoutDecision.Holdout.Key) // Skip experiment evaluation entirely (A/B/MAB/CMAB are blocked by holdout). // Evaluate rollout service only (targeted deliveries are excluded from holdout blocking). + holdoutDecisions := []FeatureDecision{holdoutDecision} if f.rolloutService != nil { rolloutDecision, rolloutReasons, err := f.rolloutService.GetDecision(decisionContext, userContext, options) reasons.Append(rolloutReasons) @@ -103,10 +114,10 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu reasons.AddError(err.Error()) return FeatureDecision{}, reasons, err } + holdoutDecisions = append(holdoutDecisions, rolloutDecision.HoldoutDecisions...) if rolloutDecision.Variation != nil { - rolloutDecision.HoldoutExperiment = &holdoutExp - rolloutDecision.HoldoutVariation = holdoutVar - return rolloutDecision, reasons, nil + rolloutDecision.HoldoutDecisions = holdoutDecisions + return withFirstHoldout(rolloutDecision), reasons, nil } } @@ -114,9 +125,8 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu // Match the rollout service's no-match behavior (see RolloutService.GetDecision), // so a served impression under sendFlagDecisions carries ruleType "rollout" rather // than a blank value. - Source: Rollout, - HoldoutExperiment: &holdoutExp, - HoldoutVariation: holdoutVar, + Source: Rollout, + HoldoutDecisions: holdoutDecisions, } - return emptyDecision, reasons, nil + return withFirstHoldout(emptyDecision), reasons, nil } diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index 54f38f91..dab07ff5 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2025, Optimizely, Inc. and contributors * + * Copyright 2019-2026, 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. * @@ -350,9 +350,10 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { rolloutVar := entities.Variation{ID: "rollout_var", Key: "rollout_variation"} rolloutDecision := FeatureDecision{ - Variation: &rolloutVar, - Source: Rollout, - Experiment: entities.Experiment{ID: "rollout_1", Key: "rollout_rule"}, + Variation: &rolloutVar, + Source: Rollout, + Experiment: entities.Experiment{ID: "rollout_1", Key: "rollout_rule"}, + HoldoutDecisions: []FeatureDecision{{Source: Holdout, Experiment: entities.Experiment{Key: "local_holdout"}}}, } mockFeatureService := new(MockFeatureDecisionService) @@ -385,6 +386,13 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { assert.NotNil(t, decision.HoldoutExperiment) assert.NotNil(t, decision.HoldoutVariation) assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) + hds := decision.HoldoutDecisions + if assert.Len(t, hds, 2) { + assert.Equal(t, Holdout, hds[0].Source) + assert.Equal(t, holdout.Key, hds[0].Holdout.Key) + assert.Equal(t, holdoutVar.ID, hds[0].Variation.ID) + assert.Equal(t, rolloutDecision.HoldoutDecisions[0], hds[1]) + } mockFeatureService.AssertNotCalled(t, "GetDecision") mockRolloutService.AssertExpectations(t) @@ -432,7 +440,7 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { options := &decide.Options{IncludeReasons: true} decisionReasons := decide.NewDecisionReasons(options) - emptyDecision := FeatureDecision{} + emptyDecision := FeatureDecision{HoldoutDecisions: []FeatureDecision{{Source: Holdout, Experiment: entities.Experiment{Key: "local_holdout"}}}} mockRolloutService.On("GetDecision", decisionContext, userContext, options).Return(emptyDecision, decisionReasons, nil) compositeFeatureService := &CompositeFeatureService{ @@ -452,6 +460,13 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { assert.NotNil(t, decision.HoldoutExperiment) assert.NotNil(t, decision.HoldoutVariation) assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) + hds := decision.HoldoutDecisions + if assert.Len(t, hds, 2) { + assert.Equal(t, Holdout, hds[0].Source) + assert.Equal(t, holdout.Key, hds[0].Holdout.Key) + assert.Equal(t, holdoutVar.ID, hds[0].Variation.ID) + assert.Equal(t, emptyDecision.HoldoutDecisions[0], hds[1]) + } mockFeatureService.AssertNotCalled(t, "GetDecision") mockRolloutService.AssertExpectations(t) @@ -509,6 +524,39 @@ func TestLocalHoldoutIgnoresExcludeTargetedDeliveries(t *testing.T) { assert.Equal(t, Holdout, decision.Source) } +func (s *CompositeFeatureServiceTestSuite) TestGetDecisionAccumulatesHoldoutDecisions() { + testUserContext := entities.UserContext{ID: "test_user_1"} + expHoldout := FeatureDecision{Source: Holdout, Experiment: entities.Experiment{Key: "exp_holdout"}, Variation: &entities.Variation{Key: "hv"}} + rolloutHoldout := FeatureDecision{Source: Holdout, Experiment: entities.Experiment{Key: "rollout_holdout"}, Variation: &entities.Variation{Key: "hv"}} + + s.mockFeatureService.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return( + FeatureDecision{HoldoutDecisions: []FeatureDecision{expHoldout}}, s.reasons, nil) + s.mockFeatureService2.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return( + FeatureDecision{Source: Rollout, Variation: &testExp1113Var2223, HoldoutDecisions: []FeatureDecision{rolloutHoldout}}, s.reasons, nil) + + compositeFeatureService := &CompositeFeatureService{ + featureServices: []FeatureService{s.mockFeatureService, s.mockFeatureService2}, + logger: logging.GetLogger("sdkKey", "CompositeFeatureService"), + } + decision, _, err := compositeFeatureService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Equal(&testExp1113Var2223, decision.Variation) + s.Equal([]FeatureDecision{expHoldout, rolloutHoldout}, decision.HoldoutDecisions) + s.Equal("exp_holdout", decision.HoldoutExperiment.Key) + s.Equal(expHoldout.Variation, decision.HoldoutVariation) + + // No rule serves: the final empty decision still carries every hit. + s.mockFeatureService2.ExpectedCalls = nil + s.mockFeatureService2.On("GetDecision", s.testFeatureDecisionContext, testUserContext, s.options).Return( + FeatureDecision{Source: Rollout, HoldoutDecisions: []FeatureDecision{rolloutHoldout}}, s.reasons, nil) + decision, _, err = compositeFeatureService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Nil(decision.Variation) + s.Equal([]FeatureDecision{expHoldout, rolloutHoldout}, decision.HoldoutDecisions) +} + func (s *CompositeFeatureServiceTestSuite) TestNewCompositeFeatureService() { // Assert that the service is instantiated with the correct child services in the right order compositeExperimentService := NewCompositeExperimentService("") diff --git a/pkg/decision/entities.go b/pkg/decision/entities.go index e663854c..3fd60d91 100644 --- a/pkg/decision/entities.go +++ b/pkg/decision/entities.go @@ -74,11 +74,15 @@ type FeatureDecision struct { // Holdout is the holdout the user was bucketed into, set by HoldoutService so the // composite service can inspect holdout properties (e.g. ExcludeTargetedDeliveries). Holdout *entities.Holdout - // HoldoutExperiment and HoldoutVariation are set by CompositeFeatureService only when a - // holdout with ExcludeTargetedDeliveries is bypassed for a targeted delivery rule. They let - // the client emit a holdout impression alongside the served rollout decision. + // HoldoutExperiment and HoldoutVariation mirror the first entry of HoldoutDecisions. + // + // Deprecated: use HoldoutDecisions, which carries every holdout hit for the flag. HoldoutExperiment *entities.Experiment - HoldoutVariation *entities.Variation + // Deprecated: use HoldoutDecisions. + HoldoutVariation *entities.Variation + // HoldoutDecisions holds every holdout hit for the flag in dispatch order: a global holdout + // bypassed by ExcludeTargetedDeliveries first, then experiment-rule hits, then delivery-rule hits. + HoldoutDecisions []FeatureDecision } // ExperimentDecision contains the decision information about an experiment diff --git a/pkg/decision/feature_experiment_service.go b/pkg/decision/feature_experiment_service.go index 3561846f..29e551a6 100644 --- a/pkg/decision/feature_experiment_service.go +++ b/pkg/decision/feature_experiment_service.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2021, Optimizely, Inc. and contributors * + * Copyright 2019-2021, 2026, 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. * @@ -45,6 +45,7 @@ func NewFeatureExperimentService(logger logging.OptimizelyLogProducer, composite func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionContext, userContext entities.UserContext, options *decide.Options) (FeatureDecision, decide.DecisionReasons, error) { feature := decisionContext.Feature reasons := decide.NewDecisionReasons(options) + var holdoutDecisions []FeatureDecision // @TODO this can be improved by getting group ID first and determining experiment and then bucketing in experiment for _, featureExperiment := range feature.FeatureExperiments { @@ -54,22 +55,22 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon reasons.Append(_reasons) if err == nil { featureDecision := FeatureDecision{ - Experiment: featureExperiment, - Variation: forcedDecision, - Source: FeatureTest, + Experiment: featureExperiment, + Variation: forcedDecision, + Source: FeatureTest, + HoldoutDecisions: holdoutDecisions, } return featureDecision, reasons, nil } } - // [FSSDK-12369] Check local holdouts targeting this experiment rule. // Local holdouts are evaluated after forced decisions but before audience/traffic checks. if f.holdoutService != nil { holdoutDecision, holdoutReasons, _ := f.holdoutService.GetLocalDecisionForRule(featureExperiment.ID, decisionContext.ProjectConfig, userContext, options) reasons.Append(holdoutReasons) if holdoutDecision.Variation != nil { - // User is in a local holdout — return holdout decision, skip rule evaluation - return holdoutDecision, reasons, nil + holdoutDecisions = append(holdoutDecisions, holdoutDecision) + continue } } @@ -99,16 +100,17 @@ func (f FeatureExperimentService) GetDecision(decisionContext FeatureDecisionCon // Variation not nil means we got a decision and should return it if experimentDecision.Variation != nil { featureDecision := FeatureDecision{ - Experiment: experiment, - Decision: experimentDecision.Decision, - Variation: experimentDecision.Variation, - Source: FeatureTest, - CmabUUID: experimentDecision.CmabUUID, + Experiment: experiment, + Decision: experimentDecision.Decision, + Variation: experimentDecision.Variation, + Source: FeatureTest, + CmabUUID: experimentDecision.CmabUUID, + HoldoutDecisions: holdoutDecisions, } return featureDecision, reasons, err } } - return FeatureDecision{}, reasons, nil + return FeatureDecision{HoldoutDecisions: holdoutDecisions}, reasons, nil } diff --git a/pkg/decision/feature_experiment_service_test.go b/pkg/decision/feature_experiment_service_test.go index 66e5a9b3..0087e535 100644 --- a/pkg/decision/feature_experiment_service_test.go +++ b/pkg/decision/feature_experiment_service_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2025, Optimizely, Inc. and contributors * + * Copyright 2019-2026, 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. * @@ -335,6 +335,138 @@ func (s *FeatureExperimentServiceTestSuite) TestForcedDecisionBeats100PercentLoc s.mockConfig.AssertNotCalled(s.T(), "GetHoldoutsForRule") } +func (s *FeatureExperimentServiceTestSuite) TestLocalHoldoutHitSkipsRuleAndNextRuleServes() { + testUserContext := entities.UserContext{ID: "test_user_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var": {ID: "ho_var", Key: "ho_var"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var", EndOfRange: 10000}}, + } + s.mockConfig.On("GetHoldoutsForRule", testExp1113.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetHoldoutsForRule", testExp1114.ID).Return([]entities.Holdout{}) + + expectedVariation := testExp1114.Variations["2225"] + exp1114Context := ExperimentDecisionContext{Experiment: &testExp1114, ProjectConfig: s.mockConfig} + s.mockExperimentService.On("GetDecision", exp1114Context, testUserContext, s.options).Return(ExperimentDecision{Variation: &expectedVariation}, s.reasons, nil) + + featureExperimentService := &FeatureExperimentService{ + compositeExperimentService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: logging.GetLogger("sdkKey", "FeatureExperimentService"), + } + + decision, _, err := featureExperimentService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Equal(&expectedVariation, decision.Variation) + s.Equal(testExp1114.Key, decision.Experiment.Key) + s.Equal(FeatureTest, decision.Source) + s.Require().Len(decision.HoldoutDecisions, 1) + s.Equal(localHoldout.Key, decision.HoldoutDecisions[0].Experiment.Key) + s.Equal(Holdout, decision.HoldoutDecisions[0].Source) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision", ExperimentDecisionContext{Experiment: &testExp1113, ProjectConfig: s.mockConfig}, testUserContext, s.options) + s.mockExperimentService.AssertExpectations(s.T()) +} + +func (s *FeatureExperimentServiceTestSuite) TestLocalHoldoutOnAllRulesNoRuleServes() { + testUserContext := entities.UserContext{ID: "test_user_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var": {ID: "ho_var", Key: "ho_var"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var", EndOfRange: 10000}}, + } + s.mockConfig.On("GetHoldoutsForRule", testExp1113.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetHoldoutsForRule", testExp1114.ID).Return([]entities.Holdout{localHoldout}) + + featureExperimentService := &FeatureExperimentService{ + compositeExperimentService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: logging.GetLogger("sdkKey", "FeatureExperimentService"), + } + + decision, _, err := featureExperimentService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Nil(decision.Variation) + s.Len(decision.HoldoutDecisions, 2) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") +} + +func (s *FeatureExperimentServiceTestSuite) TestLocalHoldoutHitThenForcedDecisionOnLaterRule() { + testUserContext := entities.UserContext{ID: "test_user_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var": {ID: "ho_var", Key: "ho_var"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var", EndOfRange: 10000}}, + } + s.mockConfig.On("GetHoldoutsForRule", testExp1113.ID).Return([]entities.Holdout{localHoldout}) + forcedVariation := testExp1114.Variations["2225"] + s.mockConfig.On("GetFlagVariationsMap").Return(map[string][]entities.Variation{ + s.testFeatureDecisionContext.Feature.Key: {forcedVariation}, + }) + s.testFeatureDecisionContext.ForcedDecisionService.SetForcedDecision( + OptimizelyDecisionContext{FlagKey: s.testFeatureDecisionContext.Feature.Key, RuleKey: testExp1114.Key}, + OptimizelyForcedDecision{VariationKey: forcedVariation.Key}, + ) + + featureExperimentService := &FeatureExperimentService{ + compositeExperimentService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: logging.GetLogger("sdkKey", "FeatureExperimentService"), + } + + decision, _, err := featureExperimentService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.NoError(err) + s.Equal(forcedVariation.Key, decision.Variation.Key) + s.Equal(testExp1114.Key, decision.Experiment.Key) + s.Require().Len(decision.HoldoutDecisions, 1) + s.Equal(localHoldout.Key, decision.HoldoutDecisions[0].Experiment.Key) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") +} + +func (s *FeatureExperimentServiceTestSuite) TestLocalHoldoutHitThenCmabErrorOnLaterRule() { + testUserContext := entities.UserContext{ID: "test_user_1"} + localHoldout := entities.Holdout{ + ID: "local_holdout", + Key: "local_holdout", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var": {ID: "ho_var", Key: "ho_var"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var", EndOfRange: 10000}}, + } + cmabExperiment := testExp1114 + cmabExperiment.Cmab = &entities.Cmab{AttributeIds: []string{"attr1"}, TrafficAllocation: 10000} + s.mockConfig.On("GetHoldoutsForRule", testExp1113.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetHoldoutsForRule", cmabExperiment.ID).Return([]entities.Holdout{}) + + cmabContext := ExperimentDecisionContext{Experiment: &cmabExperiment, ProjectConfig: s.mockConfig} + cmabError := errors.New("Failed to fetch CMAB data for experiment " + cmabExperiment.Key + ".") + s.mockExperimentService.On("GetDecision", cmabContext, testUserContext, s.options).Return(ExperimentDecision{}, s.reasons, cmabError) + + feature := testFeat3335 + feature.FeatureExperiments = []entities.Experiment{testExp1113, cmabExperiment} + s.testFeatureDecisionContext.Feature = &feature + + featureExperimentService := &FeatureExperimentService{ + compositeExperimentService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: logging.GetLogger("sdkKey", "FeatureExperimentService"), + } + + decision, _, err := featureExperimentService.GetDecision(s.testFeatureDecisionContext, testUserContext, s.options) + + s.ErrorIs(err, cmabError) + s.Nil(decision.Variation) + s.Empty(decision.HoldoutDecisions) + s.mockExperimentService.AssertExpectations(s.T()) +} + func TestFeatureExperimentServiceTestSuite(t *testing.T) { suite.Run(t, new(FeatureExperimentServiceTestSuite)) } diff --git a/pkg/decision/rollout_service.go b/pkg/decision/rollout_service.go index c4cfcf4d..441635a9 100644 --- a/pkg/decision/rollout_service.go +++ b/pkg/decision/rollout_service.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2021, Optimizely, Inc. and contributors * + * Copyright 2019-2021, 2026, 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. * @@ -109,11 +109,11 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return *forcedDecision, reasons, nil } - // [FSSDK-12369] Check local holdouts targeting this rollout rule. // Local holdouts are evaluated after forced decisions but before audience/traffic checks. if holdoutDecision, holdoutReasons := r.getLocalHoldoutDecision(experiment, decisionContext, userContext, options); holdoutDecision != nil { reasons.Append(holdoutReasons) - return *holdoutDecision, reasons, nil + featureDecision.HoldoutDecisions = append(featureDecision.HoldoutDecisions, *holdoutDecision) + continue } experimentDecisionContext := getExperimentDecisionContext(experiment) @@ -146,10 +146,10 @@ func (r RolloutService) GetDecision(decisionContext FeatureDecisionContext, user return *forcedDecision, reasons, nil } - // [FSSDK-12369] Check local holdouts for the fallback/everyone else rule if holdoutDecision, holdoutReasons := r.getLocalHoldoutDecision(experiment, decisionContext, userContext, options); holdoutDecision != nil { reasons.Append(holdoutReasons) - return *holdoutDecision, reasons, nil + featureDecision.HoldoutDecisions = append(featureDecision.HoldoutDecisions, *holdoutDecision) + return featureDecision, reasons, nil } experimentDecisionContext := getExperimentDecisionContext(experiment) diff --git a/pkg/decision/rollout_service_test.go b/pkg/decision/rollout_service_test.go index 8ddc7c1f..61d11c21 100644 --- a/pkg/decision/rollout_service_test.go +++ b/pkg/decision/rollout_service_test.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019-2021, Optimizely, Inc. and contributors * + * Copyright 2019-2021, 2026, 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. * @@ -381,9 +381,8 @@ func (s *RolloutServiceTestSuite) TestGetDecisionFailsTargeting() { s.mockLogger.AssertExpectations(s.T()) } -// TestGetDecisionLocalHoldout_RegularRule_UserBucketed verifies that when a user is bucketed -// into a local holdout targeting a regular rollout rule, the holdout decision is returned -// immediately — audience and traffic checks for the rule are skipped. +// TestGetDecisionLocalHoldout_RegularRule_UserBucketed verifies that a local holdout hit on a +// regular rollout rule skips only that rule; the next rule serves and the hit is recorded. func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_RegularRule_UserBucketed() { holdoutVar := entities.Variation{ID: "ho_var_1", Key: "holdout_variation"} localHoldout := entities.Holdout{ @@ -398,6 +397,14 @@ func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_RegularRule_UserBu }, } s.mockConfig.On("GetHoldoutsForRule", testExp1112.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetHoldoutsForRule", testExp1117.ID).Return([]entities.Holdout{}) + s.mockAudienceTreeEvaluator.On("Evaluate", testExp1117.AudienceConditionTree, s.testConditionTreeParams, mock.Anything).Return(true, true, s.reasons) + expectedVariation := testExp1117Var2223 + exp1117DecisionContext := ExperimentDecisionContext{Experiment: &testExp1117, ProjectConfig: s.mockConfig} + s.mockExperimentService.On("GetDecision", exp1117DecisionContext, s.testUserContext, s.options).Return(ExperimentDecision{ + Variation: &expectedVariation, + Decision: Decision{Reason: reasons.BucketedIntoVariation}, + }, s.reasons, nil) s.mockLogger.On("Debug", mock.Anything).Return() s.mockLogger.On("Info", mock.Anything).Return() @@ -411,9 +418,49 @@ func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_RegularRule_UserBu decision, _, err := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) s.NoError(err) - s.NotNil(decision.Variation) - s.Equal(Holdout, decision.Source) - // Audience and experiment bucketer must not be called — holdout returned early + s.Equal(&expectedVariation, decision.Variation) + s.Equal(testExp1117.Key, decision.Experiment.Key) + s.Equal(Rollout, decision.Source) + s.Require().Len(decision.HoldoutDecisions, 1) + s.Equal(Holdout, decision.HoldoutDecisions[0].Source) + s.Equal(holdoutVar.Key, decision.HoldoutDecisions[0].Variation.Key) + // Rule 1112 is skipped entirely: no audience evaluation or bucketing for it + s.mockAudienceTreeEvaluator.AssertNotCalled(s.T(), "Evaluate", testExp1112.AudienceConditionTree, mock.Anything, mock.Anything) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision", s.testExperiment1112DecisionContext, mock.Anything, mock.Anything) + s.mockExperimentService.AssertExpectations(s.T()) +} + +// TestGetDecisionLocalHoldout_AllRulesHeldOut verifies that hits on every rule each get recorded +// and no rule serves. +func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_AllRulesHeldOut() { + localHoldout := entities.Holdout{ + ID: "local_holdout_rule", + Key: "local_holdout_for_rule", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{ + "ho_var_1": {ID: "ho_var_1", Key: "holdout_variation"}, + }, + TrafficAllocation: []entities.Range{ + {EntityID: "ho_var_1", EndOfRange: 10000}, + }, + } + s.mockConfig.On("GetHoldoutsForRule", mock.Anything).Return([]entities.Holdout{localHoldout}) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testRolloutService := RolloutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + experimentBucketerService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: s.mockLogger, + } + + decision, _, err := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.Nil(decision.Variation) + s.Equal(Rollout, decision.Source) + s.Len(decision.HoldoutDecisions, 3) s.mockAudienceTreeEvaluator.AssertNotCalled(s.T(), "Evaluate") s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") } @@ -461,8 +508,7 @@ func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_RegularRule_UserMi } // TestGetDecisionLocalHoldout_FallbackRule_UserBucketed verifies that a local holdout -// targeting the everyone-else (fallback) rule is evaluated and returns a holdout decision -// when the user is bucketed into it. +// targeting the everyone-else (fallback) rule skips it and records the hit. func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_FallbackRule_UserBucketed() { holdoutVar := entities.Variation{ID: "ho_var_1", Key: "holdout_variation"} holdoutMiss := entities.Holdout{ @@ -509,8 +555,10 @@ func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldout_FallbackRule_UserB decision, _, err := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) s.NoError(err) - s.NotNil(decision.Variation) - s.Equal(Holdout, decision.Source) + s.Nil(decision.Variation) + s.Equal(Rollout, decision.Source) + s.Require().Len(decision.HoldoutDecisions, 1) + s.Equal(holdoutHit.Key, decision.HoldoutDecisions[0].Experiment.Key) s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") s.mockConfig.AssertExpectations(s.T()) } @@ -545,6 +593,42 @@ func (s *RolloutServiceTestSuite) TestForcedDecisionBeatsLocalHoldout_Rollout() s.mockConfig.AssertNotCalled(s.T(), "GetHoldoutsForRule") } +func (s *RolloutServiceTestSuite) TestGetDecisionLocalHoldoutHitThenForcedDecisionOnLaterRule() { + localHoldout := entities.Holdout{ + ID: "local_holdout_rule", + Key: "local_holdout_for_rule", + Status: entities.HoldoutStatusRunning, + Variations: map[string]entities.Variation{"ho_var_1": {ID: "ho_var_1", Key: "holdout_variation"}}, + TrafficAllocation: []entities.Range{{EntityID: "ho_var_1", EndOfRange: 10000}}, + } + s.mockConfig.On("GetHoldoutsForRule", testExp1112.ID).Return([]entities.Holdout{localHoldout}) + s.mockConfig.On("GetFlagVariationsMap").Return(map[string][]entities.Variation{ + s.testFeatureDecisionContext.Feature.Key: {testExp1117Var2223}, + }) + s.testFeatureDecisionContext.ForcedDecisionService.SetForcedDecision( + OptimizelyDecisionContext{FlagKey: s.testFeatureDecisionContext.Feature.Key, RuleKey: testExp1117.Key}, + OptimizelyForcedDecision{VariationKey: testExp1117Var2223.Key}, + ) + s.mockLogger.On("Debug", mock.Anything).Return() + s.mockLogger.On("Info", mock.Anything).Return() + + testRolloutService := RolloutService{ + audienceTreeEvaluator: s.mockAudienceTreeEvaluator, + experimentBucketerService: s.mockExperimentService, + holdoutService: NewHoldoutService("test_sdk_key"), + logger: s.mockLogger, + } + + decision, _, err := testRolloutService.GetDecision(s.testFeatureDecisionContext, s.testUserContext, s.options) + + s.NoError(err) + s.Equal(testExp1117Var2223.Key, decision.Variation.Key) + s.Equal(reasons.ForcedDecisionFound, decision.Reason) + s.Require().Len(decision.HoldoutDecisions, 1) + s.Equal(localHoldout.Key, decision.HoldoutDecisions[0].Experiment.Key) + s.mockExperimentService.AssertNotCalled(s.T(), "GetDecision") +} + func TestNewRolloutService(t *testing.T) { rolloutService := NewRolloutService("") assert.IsType(t, &evaluator.MixedTreeEvaluator{}, rolloutService.audienceTreeEvaluator)