From 54292699a8147206ef475d470ca5b4faa9e280e1 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Tue, 21 Jul 2026 13:47:02 -0500 Subject: [PATCH 01/10] [AI-FSSDK] [FSSDK-12735] Add holdout exclude_targeted_deliveries support --- .../entities/entities.go | 5 +- .../datafileprojectconfig/mappers/holdout.go | 5 +- .../mappers/holdout_test.go | 43 +++ pkg/decision/composite_feature_service.go | 41 ++- .../composite_feature_service_test.go | 259 ++++++++++++++++++ pkg/decision/entities.go | 3 +- pkg/decision/holdout_service.go | 3 +- pkg/entities/experiment.go | 5 +- 8 files changed, 354 insertions(+), 10 deletions(-) diff --git a/pkg/config/datafileprojectconfig/entities/entities.go b/pkg/config/datafileprojectconfig/entities/entities.go index 57d2e46a9..391308f69 100644 --- a/pkg/config/datafileprojectconfig/entities/entities.go +++ b/pkg/config/datafileprojectconfig/entities/entities.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019,2021-2025, 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. * @@ -128,7 +128,8 @@ type Holdout struct { TrafficAllocation []TrafficAllocation `json:"trafficAllocation"` // IncludedRules carries per-rule targeting for local holdouts. Required on // `localHoldouts` entries; ignored/stripped on `holdouts` entries at parse time. - IncludedRules *[]string `json:"includedRules,omitempty"` + IncludedRules *[]string `json:"includedRules,omitempty"` + ExcludeTargetedDeliveries bool `json:"excludeTargetedDeliveries"` } // 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 c0050ff99..4a8ed0fa2 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2025, Optimizely, Inc. and contributors * + * Copyright 2025-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. * @@ -176,6 +176,7 @@ func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout { Variations: variations, TrafficAllocation: trafficAllocation, AudienceConditionTree: audienceConditionTree, - IncludedRules: datafileHoldout.IncludedRules, + IncludedRules: datafileHoldout.IncludedRules, + ExcludeTargetedDeliveries: datafileHoldout.ExcludeTargetedDeliveries, } } diff --git a/pkg/config/datafileprojectconfig/mappers/holdout_test.go b/pkg/config/datafileprojectconfig/mappers/holdout_test.go index a44b01251..e4e51799a 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout_test.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout_test.go @@ -561,6 +561,49 @@ func TestMapHoldoutsIsGlobalProperty(t *testing.T) { assert.False(t, localHoldoutWithRules.IsGlobal(), "non-nil IncludedRules with rules should NOT be global") } +func TestMapHoldoutsExcludeTargetedDeliveriesMapped(t *testing.T) { + rawGlobal := []datafileEntities.Holdout{ + { + ID: "holdout_etd", + Key: "holdout_with_etd", + Status: "Running", + ExcludeTargetedDeliveries: true, + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + + holdoutList, _, _, _ := MapHoldouts(rawGlobal, nil, &captureLogger{}) + + assert.Len(t, holdoutList, 1) + assert.True(t, holdoutList[0].ExcludeTargetedDeliveries) +} + +func TestMapHoldoutsExcludeTargetedDeliveriesDefaultsFalse(t *testing.T) { + rawGlobal := []datafileEntities.Holdout{ + { + ID: "holdout_no_etd", + Key: "holdout_without_etd", + Status: "Running", + Variations: []datafileEntities.Variation{ + {ID: "var_1", Key: "variation_1"}, + }, + TrafficAllocation: []datafileEntities.TrafficAllocation{ + {EntityID: "var_1", EndOfRange: 10000}, + }, + }, + } + + holdoutList, _, _, _ := MapHoldouts(rawGlobal, nil, &captureLogger{}) + + assert.Len(t, holdoutList, 1) + assert.False(t, holdoutList[0].ExcludeTargetedDeliveries) +} + func TestMapHoldoutsDuplicateIDsAcrossSectionsLogsWarning(t *testing.T) { // If the same holdout ID appears in both sections, a warning must be logged // and the later (local) entry overwrites the earlier (global) one in the ID map. diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index cb3168cf0..e58f33463 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.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. * @@ -52,6 +52,9 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont holdoutDecision, holdoutReasons, _ := f.holdoutService.GetGlobalDecision(decisionContext, userContext, options) reasons.Append(holdoutReasons) if holdoutDecision.Variation != nil { + if holdoutDecision.Holdout != nil && holdoutDecision.Holdout.ExcludeTargetedDeliveries { + return f.getDecisionWithExcludedTD(holdoutDecision, decisionContext, userContext, options, reasons) + } return holdoutDecision, reasons, nil } } @@ -65,7 +68,6 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont if err != nil { f.logger.Debug(err.Error()) reasons.AddError(err.Error()) - // Return the error to let the caller handle it properly return FeatureDecision{}, reasons, err } @@ -75,3 +77,38 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont } return featureDecision, reasons, err } + +// 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) { + // Check feature experiment service (AB/MAB/CMAB) — if matched, block with holdout + if len(f.featureServices) > 0 { + expDecision, expReasons, err := f.featureServices[0].GetDecision(decisionContext, userContext, options) + reasons.Append(expReasons) + if err != nil { + f.logger.Debug(err.Error()) + reasons.AddError(err.Error()) + return FeatureDecision{}, reasons, err + } + if expDecision.Variation != nil { + return holdoutDecision, reasons, nil + } + } + + // Check rollout service (targeted deliveries) — if matched, allow through + if len(f.featureServices) > 1 { + rolloutDecision, rolloutReasons, err := f.featureServices[1].GetDecision(decisionContext, userContext, options) + reasons.Append(rolloutReasons) + if err != nil { + f.logger.Debug(err.Error()) + reasons.AddError(err.Error()) + return FeatureDecision{}, reasons, err + } + if rolloutDecision.Variation != nil { + return rolloutDecision, reasons, nil + } + } + + return holdoutDecision, reasons, nil +} diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index 06cbff369..2f547bcb1 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -25,6 +25,8 @@ import ( "github.com/optimizely/go-sdk/v2/pkg/entities" "github.com/optimizely/go-sdk/v2/pkg/logging" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/suite" ) @@ -198,6 +200,263 @@ func (s *CompositeFeatureServiceTestSuite) TestGetDecisionWithCmabError() { s.mockFeatureService2.AssertNotCalled(s.T(), "GetDecision") } +func TestExcludeTDFalseBlocksEverything(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + holdoutVar := entities.Variation{ID: "holdout_var", Key: "holdout_variation"} + holdout := entities.Holdout{ + ID: "holdout_etd_false", + Key: "holdout_exclude_td_false", + Status: entities.HoldoutStatusRunning, + ExcludeTargetedDeliveries: false, + Variations: map[string]entities.Variation{"holdout_var": holdoutVar}, + TrafficAllocation: []entities.Range{{EntityID: "holdout_var", EndOfRange: 10000}}, + } + + mockConfig.On("GetGlobalHoldouts").Return([]entities.Holdout{holdout}) + mockConfig.On("GetAudienceMap").Return(map[string]entities.Audience{}) + mockBucketer.On("Bucket", "test_user", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&holdoutVar, reasons.Reason(""), nil) + mockLogger.On("Debug", mock.Anything).Return() + mockLogger.On("Info", mock.Anything).Return() + + holdoutService := &HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + mockFeatureService := new(MockFeatureDecisionService) + mockRolloutService := new(MockFeatureDecisionService) + + compositeFeatureService := &CompositeFeatureService{ + holdoutService: holdoutService, + featureServices: []FeatureService{mockFeatureService, mockRolloutService}, + logger: logging.GetLogger("", "CompositeFeatureService"), + } + + feature := entities.Feature{ID: "feat_1", Key: "test_feature"} + decisionContext := FeatureDecisionContext{ + Feature: &feature, + ProjectConfig: mockConfig, + } + userContext := entities.UserContext{ID: "test_user"} + options := &decide.Options{} + + decision, _, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) + + assert.NoError(t, err) + assert.NotNil(t, decision.Variation) + assert.Equal(t, holdoutVar.ID, decision.Variation.ID) + assert.Equal(t, Holdout, decision.Source) + mockFeatureService.AssertNotCalled(t, "GetDecision") + mockRolloutService.AssertNotCalled(t, "GetDecision") +} + +func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + holdoutVar := entities.Variation{ID: "holdout_var", Key: "holdout_variation"} + holdout := entities.Holdout{ + ID: "holdout_etd_true", + Key: "holdout_exclude_td_true", + Status: entities.HoldoutStatusRunning, + ExcludeTargetedDeliveries: true, + Variations: map[string]entities.Variation{"holdout_var": holdoutVar}, + TrafficAllocation: []entities.Range{{EntityID: "holdout_var", EndOfRange: 10000}}, + } + + mockConfig.On("GetGlobalHoldouts").Return([]entities.Holdout{holdout}) + mockConfig.On("GetAudienceMap").Return(map[string]entities.Audience{}) + mockBucketer.On("Bucket", "test_user", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&holdoutVar, reasons.Reason(""), nil) + mockLogger.On("Debug", mock.Anything).Return() + mockLogger.On("Info", mock.Anything).Return() + + holdoutService := &HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + abVar := entities.Variation{ID: "ab_var", Key: "ab_variation"} + abDecision := FeatureDecision{ + Variation: &abVar, + Source: FeatureTest, + Experiment: entities.Experiment{ID: "exp_1", Key: "ab_experiment"}, + } + + mockFeatureService := new(MockFeatureDecisionService) + mockRolloutService := new(MockFeatureDecisionService) + + feature := entities.Feature{ID: "feat_1", Key: "test_feature"} + decisionContext := FeatureDecisionContext{ + Feature: &feature, + ProjectConfig: mockConfig, + } + userContext := entities.UserContext{ID: "test_user"} + options := &decide.Options{} + decisionReasons := decide.NewDecisionReasons(options) + + mockFeatureService.On("GetDecision", decisionContext, userContext, options).Return(abDecision, decisionReasons, nil) + + compositeFeatureService := &CompositeFeatureService{ + holdoutService: holdoutService, + featureServices: []FeatureService{mockFeatureService, mockRolloutService}, + logger: logging.GetLogger("", "CompositeFeatureService"), + } + + decision, _, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) + + assert.NoError(t, err) + assert.NotNil(t, decision.Variation) + assert.Equal(t, holdoutVar.ID, decision.Variation.ID) + assert.Equal(t, Holdout, decision.Source) + mockFeatureService.AssertExpectations(t) + mockRolloutService.AssertNotCalled(t, "GetDecision") +} + +func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + holdoutVar := entities.Variation{ID: "holdout_var", Key: "holdout_variation"} + holdout := entities.Holdout{ + ID: "holdout_etd_true", + Key: "holdout_exclude_td_true", + Status: entities.HoldoutStatusRunning, + ExcludeTargetedDeliveries: true, + Variations: map[string]entities.Variation{"holdout_var": holdoutVar}, + TrafficAllocation: []entities.Range{{EntityID: "holdout_var", EndOfRange: 10000}}, + } + + mockConfig.On("GetGlobalHoldouts").Return([]entities.Holdout{holdout}) + mockConfig.On("GetAudienceMap").Return(map[string]entities.Audience{}) + mockBucketer.On("Bucket", "test_user", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&holdoutVar, reasons.Reason(""), nil) + mockLogger.On("Debug", mock.Anything).Return() + mockLogger.On("Info", mock.Anything).Return() + + holdoutService := &HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + rolloutVar := entities.Variation{ID: "rollout_var", Key: "rollout_variation"} + rolloutDecision := FeatureDecision{ + Variation: &rolloutVar, + Source: Rollout, + Experiment: entities.Experiment{ID: "rollout_1", Key: "rollout_rule"}, + } + + mockFeatureService := new(MockFeatureDecisionService) + mockRolloutService := new(MockFeatureDecisionService) + + feature := entities.Feature{ID: "feat_1", Key: "test_feature"} + decisionContext := FeatureDecisionContext{ + Feature: &feature, + ProjectConfig: mockConfig, + } + userContext := entities.UserContext{ID: "test_user"} + options := &decide.Options{} + decisionReasons := decide.NewDecisionReasons(options) + + emptyDecision := FeatureDecision{} + mockFeatureService.On("GetDecision", decisionContext, userContext, options).Return(emptyDecision, decisionReasons, nil) + mockRolloutService.On("GetDecision", decisionContext, userContext, options).Return(rolloutDecision, decisionReasons, nil) + + compositeFeatureService := &CompositeFeatureService{ + holdoutService: holdoutService, + featureServices: []FeatureService{mockFeatureService, mockRolloutService}, + logger: logging.GetLogger("", "CompositeFeatureService"), + } + + decision, _, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) + + assert.NoError(t, err) + assert.NotNil(t, decision.Variation) + assert.Equal(t, rolloutVar.ID, decision.Variation.ID) + assert.Equal(t, Rollout, decision.Source) + mockFeatureService.AssertExpectations(t) + mockRolloutService.AssertExpectations(t) +} + +func TestExcludeTDTrueNoDownstreamMatchReturnsHoldout(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + holdoutVar := entities.Variation{ID: "holdout_var", Key: "holdout_variation"} + holdout := entities.Holdout{ + ID: "holdout_etd_true", + Key: "holdout_exclude_td_true", + Status: entities.HoldoutStatusRunning, + ExcludeTargetedDeliveries: true, + Variations: map[string]entities.Variation{"holdout_var": holdoutVar}, + TrafficAllocation: []entities.Range{{EntityID: "holdout_var", EndOfRange: 10000}}, + } + + mockConfig.On("GetGlobalHoldouts").Return([]entities.Holdout{holdout}) + mockConfig.On("GetAudienceMap").Return(map[string]entities.Audience{}) + mockBucketer.On("Bucket", "test_user", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&holdoutVar, reasons.Reason(""), nil) + mockLogger.On("Debug", mock.Anything).Return() + mockLogger.On("Info", mock.Anything).Return() + + holdoutService := &HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + mockFeatureService := new(MockFeatureDecisionService) + mockRolloutService := new(MockFeatureDecisionService) + + feature := entities.Feature{ID: "feat_1", Key: "test_feature"} + decisionContext := FeatureDecisionContext{ + Feature: &feature, + ProjectConfig: mockConfig, + } + userContext := entities.UserContext{ID: "test_user"} + options := &decide.Options{} + decisionReasons := decide.NewDecisionReasons(options) + + emptyDecision := FeatureDecision{} + mockFeatureService.On("GetDecision", decisionContext, userContext, options).Return(emptyDecision, decisionReasons, nil) + mockRolloutService.On("GetDecision", decisionContext, userContext, options).Return(emptyDecision, decisionReasons, nil) + + compositeFeatureService := &CompositeFeatureService{ + holdoutService: holdoutService, + featureServices: []FeatureService{mockFeatureService, mockRolloutService}, + logger: logging.GetLogger("", "CompositeFeatureService"), + } + + decision, _, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) + + assert.NoError(t, err) + assert.NotNil(t, decision.Variation) + assert.Equal(t, holdoutVar.ID, decision.Variation.ID) + assert.Equal(t, Holdout, decision.Source) + mockFeatureService.AssertExpectations(t) + mockRolloutService.AssertExpectations(t) +} + +func TestExcludeTDMissingFieldDefaultsFalse(t *testing.T) { + holdout := entities.Holdout{ + ID: "holdout_no_etd", + Key: "holdout_missing_field", + Status: entities.HoldoutStatusRunning, + } + + assert.False(t, holdout.ExcludeTargetedDeliveries) +} + 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 fec60903b..428c0773c 100644 --- a/pkg/decision/entities.go +++ b/pkg/decision/entities.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. * @@ -70,6 +70,7 @@ type FeatureDecision struct { Source Source Experiment entities.Experiment Variation *entities.Variation + Holdout *entities.Holdout CmabUUID *string } diff --git a/pkg/decision/holdout_service.go b/pkg/decision/holdout_service.go index 95b6a1f71..10594c088 100644 --- a/pkg/decision/holdout_service.go +++ b/pkg/decision/holdout_service.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2025, Optimizely, Inc. and contributors * + * Copyright 2025-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,6 +109,7 @@ func (h HoldoutService) GetGlobalDecision(decisionContext FeatureDecisionContext featureDecision := FeatureDecision{ Experiment: experimentForBucketing, Variation: variation, + Holdout: holdout, Source: Holdout, } return featureDecision, reasons, nil diff --git a/pkg/entities/experiment.go b/pkg/entities/experiment.go index 2798b02ff..e27a82085 100644 --- a/pkg/entities/experiment.go +++ b/pkg/entities/experiment.go @@ -1,5 +1,5 @@ /**************************************************************************** - * Copyright 2019,2021-2025 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. * @@ -93,7 +93,8 @@ type Holdout struct { // IncludedRules carries per-rule targeting for local holdouts. The datafile // section determines scope; `DatafileProjectConfig` strips `IncludedRules` on // entries from the `holdouts` section, so nil here is equivalent to global. - IncludedRules *[]string + IncludedRules *[]string + ExcludeTargetedDeliveries bool } // IsGlobal returns true if this holdout is global (applies to all rules across all flags). From 85ccaf033421ba397aa978d6de942cb9220591eb Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 22 Jul 2026 14:26:30 -0500 Subject: [PATCH 02/10] [AI-FSSDK] [FSSDK-12735] Send holdout events regardless of ETD flag, ignore ETD for local holdouts, return null when TD has no decision --- pkg/client/client.go | 21 ++++++++ pkg/decision/composite_feature_service.go | 11 +++- .../composite_feature_service_test.go | 53 +++++++++++++++++-- pkg/decision/entities.go | 4 +- 4 files changed, 83 insertions(+), 6 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index f55ddbba6..b52bc5172 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -223,6 +223,13 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string o.EventProcessor.ProcessEvent(ue) eventSent = true } + // Send holdout impression when holdout is bypassed for targeted delivery + if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { + if hue, hok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, *featureDecision.HoldoutExperiment, + featureDecision.HoldoutVariation, usrContext, key, featureDecision.HoldoutExperiment.Key, decision.Holdout, flagEnabled, nil); hok { + o.EventProcessor.ProcessEvent(hue) + } + } } variableMap := map[string]interface{}{} @@ -522,6 +529,13 @@ 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 + if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { + if hue, hok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, *featureDecision.HoldoutExperiment, + featureDecision.HoldoutVariation, userContext, featureKey, featureDecision.HoldoutExperiment.Key, decision.Holdout, result, nil); hok { + o.EventProcessor.ProcessEvent(hue) + } + } return result, err } @@ -887,6 +901,13 @@ 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 + if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { + if hue, hok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, *featureDecision.HoldoutExperiment, + featureDecision.HoldoutVariation, userContext, featureKey, featureDecision.HoldoutExperiment.Key, decision.Holdout, decisionInfo.Enabled, nil); hok { + o.EventProcessor.ProcessEvent(hue) + } + } } } diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index e58f33463..c6770e5b4 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -82,6 +82,9 @@ func (f CompositeFeatureService) GetDecision(decisionContext FeatureDecisionCont // 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 + // Check feature experiment service (AB/MAB/CMAB) — if matched, block with holdout if len(f.featureServices) > 0 { expDecision, expReasons, err := f.featureServices[0].GetDecision(decisionContext, userContext, options) @@ -106,9 +109,15 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu return FeatureDecision{}, reasons, err } if rolloutDecision.Variation != nil { + rolloutDecision.HoldoutExperiment = &holdoutExp + rolloutDecision.HoldoutVariation = holdoutVar return rolloutDecision, reasons, nil } } - return holdoutDecision, reasons, nil + emptyDecision := FeatureDecision{ + HoldoutExperiment: &holdoutExp, + HoldoutVariation: holdoutVar, + } + return emptyDecision, reasons, nil } diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index 2f547bcb1..084919eb9 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -383,11 +383,14 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { assert.NotNil(t, decision.Variation) assert.Equal(t, rolloutVar.ID, decision.Variation.ID) assert.Equal(t, Rollout, decision.Source) + assert.NotNil(t, decision.HoldoutExperiment) + assert.NotNil(t, decision.HoldoutVariation) + assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) mockFeatureService.AssertExpectations(t) mockRolloutService.AssertExpectations(t) } -func TestExcludeTDTrueNoDownstreamMatchReturnsHoldout(t *testing.T) { +func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { mockConfig := new(mockProjectConfig) mockBucketer := new(MockExperimentBucketer) mockAudienceEval := new(MockAudienceTreeEvaluator) @@ -440,9 +443,11 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsHoldout(t *testing.T) { decision, _, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) assert.NoError(t, err) - assert.NotNil(t, decision.Variation) - assert.Equal(t, holdoutVar.ID, decision.Variation.ID) - assert.Equal(t, Holdout, decision.Source) + assert.Nil(t, decision.Variation) + assert.Equal(t, "", decision.Source) + assert.NotNil(t, decision.HoldoutExperiment) + assert.NotNil(t, decision.HoldoutVariation) + assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) mockFeatureService.AssertExpectations(t) mockRolloutService.AssertExpectations(t) } @@ -457,6 +462,46 @@ func TestExcludeTDMissingFieldDefaultsFalse(t *testing.T) { assert.False(t, holdout.ExcludeTargetedDeliveries) } +func TestLocalHoldoutIgnoresExcludeTargetedDeliveries(t *testing.T) { + mockConfig := new(mockProjectConfig) + mockBucketer := new(MockExperimentBucketer) + mockAudienceEval := new(MockAudienceTreeEvaluator) + mockLogger := new(MockLogger) + + holdoutVar := entities.Variation{ID: "local_holdout_var", Key: "local_holdout_variation"} + localHoldout := entities.Holdout{ + ID: "local_holdout_etd", + Key: "local_holdout_with_etd", + Status: entities.HoldoutStatusRunning, + ExcludeTargetedDeliveries: true, + Variations: map[string]entities.Variation{"local_holdout_var": holdoutVar}, + TrafficAllocation: []entities.Range{{EntityID: "local_holdout_var", EndOfRange: 10000}}, + } + + ruleID := "rule_123" + mockConfig.On("GetHoldoutsForRule", ruleID).Return([]entities.Holdout{localHoldout}) + mockConfig.On("GetAudienceMap").Return(map[string]entities.Audience{}) + mockBucketer.On("Bucket", "test_user", mock.AnythingOfType("entities.Experiment"), entities.Group{}).Return(&holdoutVar, reasons.Reason(""), nil) + mockLogger.On("Debug", mock.Anything).Return() + mockLogger.On("Info", mock.Anything).Return() + + holdoutService := &HoldoutService{ + audienceTreeEvaluator: mockAudienceEval, + bucketer: mockBucketer, + logger: mockLogger, + } + + userContext := entities.UserContext{ID: "test_user"} + options := &decide.Options{} + + decision, _, err := holdoutService.GetLocalDecisionForRule(ruleID, mockConfig, userContext, options) + + assert.NoError(t, err) + assert.NotNil(t, decision.Variation) + assert.Equal(t, holdoutVar.ID, decision.Variation.ID) + assert.Equal(t, Holdout, decision.Source) +} + 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 428c0773c..c3bafed8c 100644 --- a/pkg/decision/entities.go +++ b/pkg/decision/entities.go @@ -71,7 +71,9 @@ type FeatureDecision struct { Experiment entities.Experiment Variation *entities.Variation Holdout *entities.Holdout - CmabUUID *string + CmabUUID *string + HoldoutExperiment *entities.Experiment + HoldoutVariation *entities.Variation } // ExperimentDecision contains the decision information about an experiment From 43d7070929f517631a10fd7eab76bb9af472da39 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 22 Jul 2026 16:39:43 -0500 Subject: [PATCH 03/10] [AI-FSSDK] [FSSDK-12735] Fix decisionEventDispatched for holdout impression and add excludeTD bypass reason --- pkg/client/client.go | 1 + pkg/decision/composite_feature_service.go | 2 ++ .../composite_feature_service_test.go | 21 +++++++++++++------ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index b52bc5172..52ef6b0d8 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -228,6 +228,7 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string if hue, hok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, *featureDecision.HoldoutExperiment, featureDecision.HoldoutVariation, usrContext, key, featureDecision.HoldoutExperiment.Key, decision.Holdout, flagEnabled, nil); hok { o.EventProcessor.ProcessEvent(hue) + eventSent = true } } } diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index c6770e5b4..9ef3673a7 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -99,6 +99,8 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu } } + reasons.AddInfo("Holdout '%s' has excludeTargetedDeliveries enabled, continuing to rollout evaluation.", holdoutDecision.Holdout.Key) + // Check rollout service (targeted deliveries) — if matched, allow through if len(f.featureServices) > 1 { rolloutDecision, rolloutReasons, err := f.featureServices[1].GetDecision(decisionContext, userContext, options) diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index 084919eb9..2c5ab4aad 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -299,7 +299,7 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { ProjectConfig: mockConfig, } userContext := entities.UserContext{ID: "test_user"} - options := &decide.Options{} + options := &decide.Options{IncludeReasons: true} decisionReasons := decide.NewDecisionReasons(options) mockFeatureService.On("GetDecision", decisionContext, userContext, options).Return(abDecision, decisionReasons, nil) @@ -310,7 +310,7 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { logger: logging.GetLogger("", "CompositeFeatureService"), } - decision, _, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) + decision, decisionReasons, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) assert.NoError(t, err) assert.NotNil(t, decision.Variation) @@ -318,6 +318,9 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { assert.Equal(t, Holdout, decision.Source) mockFeatureService.AssertExpectations(t) mockRolloutService.AssertNotCalled(t, "GetDecision") + + reportedReasons := decisionReasons.ToReport() + assert.Contains(t, reportedReasons, "Holdout 'holdout_exclude_td_true' has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") } func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { @@ -364,7 +367,7 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { ProjectConfig: mockConfig, } userContext := entities.UserContext{ID: "test_user"} - options := &decide.Options{} + options := &decide.Options{IncludeReasons: true} decisionReasons := decide.NewDecisionReasons(options) emptyDecision := FeatureDecision{} @@ -377,7 +380,7 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { logger: logging.GetLogger("", "CompositeFeatureService"), } - decision, _, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) + decision, decisionReasons, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) assert.NoError(t, err) assert.NotNil(t, decision.Variation) @@ -388,6 +391,9 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) mockFeatureService.AssertExpectations(t) mockRolloutService.AssertExpectations(t) + + reportedReasons := decisionReasons.ToReport() + assert.Contains(t, reportedReasons, "Holdout 'holdout_exclude_td_true' has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") } func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { @@ -427,7 +433,7 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { ProjectConfig: mockConfig, } userContext := entities.UserContext{ID: "test_user"} - options := &decide.Options{} + options := &decide.Options{IncludeReasons: true} decisionReasons := decide.NewDecisionReasons(options) emptyDecision := FeatureDecision{} @@ -440,7 +446,7 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { logger: logging.GetLogger("", "CompositeFeatureService"), } - decision, _, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) + decision, resultReasons, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) assert.NoError(t, err) assert.Nil(t, decision.Variation) @@ -450,6 +456,9 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) mockFeatureService.AssertExpectations(t) mockRolloutService.AssertExpectations(t) + + reportedReasons := resultReasons.ToReport() + assert.Contains(t, reportedReasons, "Holdout 'holdout_exclude_td_true' has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") } func TestExcludeTDMissingFieldDefaultsFalse(t *testing.T) { From a117b02d0ba99f715f17237b368d1b4d73c907f5 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 29 Jul 2026 11:14:10 -0500 Subject: [PATCH 04/10] [FSSDK-12735] Re-trigger FSC tests after datafile updates From 28255d7857cf557e3bbb6fb08f48ab12351d563a Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 29 Jul 2026 16:06:42 -0500 Subject: [PATCH 05/10] Fix reason message --- pkg/decision/composite_feature_service.go | 2 +- pkg/decision/composite_feature_service_test.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index 9ef3673a7..f91f9df5a 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -99,7 +99,7 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu } } - reasons.AddInfo("Holdout '%s' has excludeTargetedDeliveries enabled, continuing to rollout evaluation.", holdoutDecision.Holdout.Key) + reasons.AddInfo("Holdout \"%s\" has excludeTargetedDeliveries enabled, continuing to rollout evaluation.", holdoutDecision.Holdout.Key) // Check rollout service (targeted deliveries) — if matched, allow through if len(f.featureServices) > 1 { diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index 2c5ab4aad..022f2a48f 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -320,7 +320,7 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { mockRolloutService.AssertNotCalled(t, "GetDecision") reportedReasons := decisionReasons.ToReport() - assert.Contains(t, reportedReasons, "Holdout 'holdout_exclude_td_true' has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") + assert.Contains(t, reportedReasons, "Holdout \"holdout_exclude_td_true\" has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") } func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { @@ -393,7 +393,7 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { mockRolloutService.AssertExpectations(t) reportedReasons := decisionReasons.ToReport() - assert.Contains(t, reportedReasons, "Holdout 'holdout_exclude_td_true' has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") + assert.Contains(t, reportedReasons, "Holdout \"holdout_exclude_td_true\" has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") } func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { @@ -458,7 +458,7 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { mockRolloutService.AssertExpectations(t) reportedReasons := resultReasons.ToReport() - assert.Contains(t, reportedReasons, "Holdout 'holdout_exclude_td_true' has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") + assert.Contains(t, reportedReasons, "Holdout \"holdout_exclude_td_true\" has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") } func TestExcludeTDMissingFieldDefaultsFalse(t *testing.T) { From 341b802617b9fd410411b15374607007802e6d21 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 29 Jul 2026 16:28:11 -0500 Subject: [PATCH 06/10] Fix Go implementation --- pkg/decision/composite_feature_service.go | 17 ++--------- .../composite_feature_service_test.go | 28 +++++++------------ 2 files changed, 12 insertions(+), 33 deletions(-) diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index f91f9df5a..6882f0dbf 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -85,23 +85,10 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu holdoutExp := holdoutDecision.Experiment holdoutVar := holdoutDecision.Variation - // Check feature experiment service (AB/MAB/CMAB) — if matched, block with holdout - if len(f.featureServices) > 0 { - expDecision, expReasons, err := f.featureServices[0].GetDecision(decisionContext, userContext, options) - reasons.Append(expReasons) - if err != nil { - f.logger.Debug(err.Error()) - reasons.AddError(err.Error()) - return FeatureDecision{}, reasons, err - } - if expDecision.Variation != nil { - return holdoutDecision, reasons, nil - } - } - reasons.AddInfo("Holdout \"%s\" has excludeTargetedDeliveries enabled, continuing to rollout evaluation.", holdoutDecision.Holdout.Key) - // Check rollout service (targeted deliveries) — if matched, allow through + // Skip experiment evaluation entirely (A/B/MAB/CMAB are blocked by holdout). + // Evaluate rollout service only (targeted deliveries are excluded from holdout blocking). if len(f.featureServices) > 1 { rolloutDecision, rolloutReasons, err := f.featureServices[1].GetDecision(decisionContext, userContext, options) reasons.Append(rolloutReasons) diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index 022f2a48f..f0bbc4dcf 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -283,13 +283,6 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { logger: mockLogger, } - abVar := entities.Variation{ID: "ab_var", Key: "ab_variation"} - abDecision := FeatureDecision{ - Variation: &abVar, - Source: FeatureTest, - Experiment: entities.Experiment{ID: "exp_1", Key: "ab_experiment"}, - } - mockFeatureService := new(MockFeatureDecisionService) mockRolloutService := new(MockFeatureDecisionService) @@ -302,7 +295,8 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { options := &decide.Options{IncludeReasons: true} decisionReasons := decide.NewDecisionReasons(options) - mockFeatureService.On("GetDecision", decisionContext, userContext, options).Return(abDecision, decisionReasons, nil) + emptyDecision := FeatureDecision{} + mockRolloutService.On("GetDecision", decisionContext, userContext, options).Return(emptyDecision, decisionReasons, nil) compositeFeatureService := &CompositeFeatureService{ holdoutService: holdoutService, @@ -313,11 +307,12 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { decision, decisionReasons, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) assert.NoError(t, err) - assert.NotNil(t, decision.Variation) - assert.Equal(t, holdoutVar.ID, decision.Variation.ID) - assert.Equal(t, Holdout, decision.Source) - mockFeatureService.AssertExpectations(t) - mockRolloutService.AssertNotCalled(t, "GetDecision") + assert.Nil(t, decision.Variation) + assert.NotNil(t, decision.HoldoutExperiment) + assert.NotNil(t, decision.HoldoutVariation) + assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) + mockFeatureService.AssertNotCalled(t, "GetDecision") + mockRolloutService.AssertExpectations(t) reportedReasons := decisionReasons.ToReport() assert.Contains(t, reportedReasons, "Holdout \"holdout_exclude_td_true\" has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") @@ -370,8 +365,6 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { options := &decide.Options{IncludeReasons: true} decisionReasons := decide.NewDecisionReasons(options) - emptyDecision := FeatureDecision{} - mockFeatureService.On("GetDecision", decisionContext, userContext, options).Return(emptyDecision, decisionReasons, nil) mockRolloutService.On("GetDecision", decisionContext, userContext, options).Return(rolloutDecision, decisionReasons, nil) compositeFeatureService := &CompositeFeatureService{ @@ -389,7 +382,7 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { assert.NotNil(t, decision.HoldoutExperiment) assert.NotNil(t, decision.HoldoutVariation) assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) - mockFeatureService.AssertExpectations(t) + mockFeatureService.AssertNotCalled(t, "GetDecision") mockRolloutService.AssertExpectations(t) reportedReasons := decisionReasons.ToReport() @@ -437,7 +430,6 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { decisionReasons := decide.NewDecisionReasons(options) emptyDecision := FeatureDecision{} - mockFeatureService.On("GetDecision", decisionContext, userContext, options).Return(emptyDecision, decisionReasons, nil) mockRolloutService.On("GetDecision", decisionContext, userContext, options).Return(emptyDecision, decisionReasons, nil) compositeFeatureService := &CompositeFeatureService{ @@ -454,7 +446,7 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { assert.NotNil(t, decision.HoldoutExperiment) assert.NotNil(t, decision.HoldoutVariation) assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID) - mockFeatureService.AssertExpectations(t) + mockFeatureService.AssertNotCalled(t, "GetDecision") mockRolloutService.AssertExpectations(t) reportedReasons := resultReasons.ToReport() From 45de1168f6e4fd0866c370c807a2e1d4594e306f Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 29 Jul 2026 16:56:10 -0500 Subject: [PATCH 07/10] Fix lint and failures --- pkg/client/client.go | 35 +++++++++++-------- .../entities/entities.go | 2 +- .../datafileprojectconfig/mappers/holdout.go | 18 +++++----- .../composite_feature_service_test.go | 8 ++--- pkg/entities/experiment.go | 2 +- 5 files changed, 35 insertions(+), 30 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 52ef6b0d8..1f90d096f 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -218,19 +218,7 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string } if !allOptions.DisableDecisionEvent { - if ue, ok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, featureDecision.Experiment, - featureDecision.Variation, usrContext, key, featureDecision.Experiment.Key, featureDecision.Source, flagEnabled, featureDecision.CmabUUID); ok { - o.EventProcessor.ProcessEvent(ue) - eventSent = true - } - // Send holdout impression when holdout is bypassed for targeted delivery - if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { - if hue, hok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, *featureDecision.HoldoutExperiment, - featureDecision.HoldoutVariation, usrContext, key, featureDecision.HoldoutExperiment.Key, decision.Holdout, flagEnabled, nil); hok { - o.EventProcessor.ProcessEvent(hue) - eventSent = true - } - } + eventSent = o.dispatchDecisionEvents(decisionContext.ProjectConfig, featureDecision, usrContext, key, flagEnabled) } variableMap := map[string]interface{}{} @@ -253,6 +241,23 @@ func (o *OptimizelyClient) decide(userContext *OptimizelyUserContext, key string return NewOptimizelyDecision(variationKey, ruleKey, key, flagEnabled, optimizelyJSON, *userContext, reasonsToReport) } +func (o *OptimizelyClient) dispatchDecisionEvents(projectConfig config.ProjectConfig, featureDecision decision.FeatureDecision, usrContext entities.UserContext, key string, flagEnabled bool) bool { + eventSent := false + if ue, ok := event.CreateImpressionUserEvent(projectConfig, featureDecision.Experiment, + featureDecision.Variation, usrContext, key, featureDecision.Experiment.Key, featureDecision.Source, flagEnabled, featureDecision.CmabUUID); ok { + o.EventProcessor.ProcessEvent(ue) + eventSent = true + } + if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { + if hue, hok := event.CreateImpressionUserEvent(projectConfig, *featureDecision.HoldoutExperiment, + featureDecision.HoldoutVariation, usrContext, key, featureDecision.HoldoutExperiment.Key, decision.Holdout, featureDecision.HoldoutVariation.FeatureEnabled, nil); hok { + o.EventProcessor.ProcessEvent(hue) + eventSent = true + } + } + return eventSent +} + func (o *OptimizelyClient) decideForKeys(userContext OptimizelyUserContext, keys []string, options *decide.Options) map[string]OptimizelyDecision { var err error defer func() { @@ -533,7 +538,7 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit // Send holdout impression when holdout is bypassed for targeted delivery if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { if hue, hok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, *featureDecision.HoldoutExperiment, - featureDecision.HoldoutVariation, userContext, featureKey, featureDecision.HoldoutExperiment.Key, decision.Holdout, result, nil); hok { + featureDecision.HoldoutVariation, userContext, featureKey, featureDecision.HoldoutExperiment.Key, decision.Holdout, featureDecision.HoldoutVariation.FeatureEnabled, nil); hok { o.EventProcessor.ProcessEvent(hue) } } @@ -905,7 +910,7 @@ func (o *OptimizelyClient) GetDetailedFeatureDecisionUnsafe(featureKey string, u // Send holdout impression when holdout is bypassed for targeted delivery if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { if hue, hok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, *featureDecision.HoldoutExperiment, - featureDecision.HoldoutVariation, userContext, featureKey, featureDecision.HoldoutExperiment.Key, decision.Holdout, decisionInfo.Enabled, nil); hok { + featureDecision.HoldoutVariation, userContext, featureKey, featureDecision.HoldoutExperiment.Key, decision.Holdout, featureDecision.HoldoutVariation.FeatureEnabled, nil); hok { o.EventProcessor.ProcessEvent(hue) } } diff --git a/pkg/config/datafileprojectconfig/entities/entities.go b/pkg/config/datafileprojectconfig/entities/entities.go index 391308f69..ec9deabf1 100644 --- a/pkg/config/datafileprojectconfig/entities/entities.go +++ b/pkg/config/datafileprojectconfig/entities/entities.go @@ -128,7 +128,7 @@ type Holdout struct { TrafficAllocation []TrafficAllocation `json:"trafficAllocation"` // IncludedRules carries per-rule targeting for local holdouts. Required on // `localHoldouts` entries; ignored/stripped on `holdouts` entries at parse time. - IncludedRules *[]string `json:"includedRules,omitempty"` + IncludedRules *[]string `json:"includedRules,omitempty"` ExcludeTargetedDeliveries bool `json:"excludeTargetedDeliveries"` } diff --git a/pkg/config/datafileprojectconfig/mappers/holdout.go b/pkg/config/datafileprojectconfig/mappers/holdout.go index 4a8ed0fa2..8d43f211e 100644 --- a/pkg/config/datafileprojectconfig/mappers/holdout.go +++ b/pkg/config/datafileprojectconfig/mappers/holdout.go @@ -168,15 +168,15 @@ func mapHoldout(datafileHoldout datafileEntities.Holdout) entities.Holdout { } return entities.Holdout{ - ID: datafileHoldout.ID, - Key: datafileHoldout.Key, - Status: entities.HoldoutStatus(datafileHoldout.Status), - AudienceIds: datafileHoldout.AudienceIds, - AudienceConditions: datafileHoldout.AudienceConditions, - Variations: variations, - TrafficAllocation: trafficAllocation, - AudienceConditionTree: audienceConditionTree, - IncludedRules: datafileHoldout.IncludedRules, + ID: datafileHoldout.ID, + Key: datafileHoldout.Key, + Status: entities.HoldoutStatus(datafileHoldout.Status), + AudienceIds: datafileHoldout.AudienceIds, + AudienceConditions: datafileHoldout.AudienceConditions, + Variations: variations, + TrafficAllocation: trafficAllocation, + AudienceConditionTree: audienceConditionTree, + IncludedRules: datafileHoldout.IncludedRules, ExcludeTargetedDeliveries: datafileHoldout.ExcludeTargetedDeliveries, } } diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index f0bbc4dcf..e00689017 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -304,7 +304,7 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { logger: logging.GetLogger("", "CompositeFeatureService"), } - decision, decisionReasons, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) + decision, resultReasons, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) assert.NoError(t, err) assert.Nil(t, decision.Variation) @@ -314,7 +314,7 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { mockFeatureService.AssertNotCalled(t, "GetDecision") mockRolloutService.AssertExpectations(t) - reportedReasons := decisionReasons.ToReport() + reportedReasons := resultReasons.ToReport() assert.Contains(t, reportedReasons, "Holdout \"holdout_exclude_td_true\" has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") } @@ -373,7 +373,7 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { logger: logging.GetLogger("", "CompositeFeatureService"), } - decision, decisionReasons, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) + decision, resultReasons, err := compositeFeatureService.GetDecision(decisionContext, userContext, options) assert.NoError(t, err) assert.NotNil(t, decision.Variation) @@ -385,7 +385,7 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { mockFeatureService.AssertNotCalled(t, "GetDecision") mockRolloutService.AssertExpectations(t) - reportedReasons := decisionReasons.ToReport() + reportedReasons := resultReasons.ToReport() assert.Contains(t, reportedReasons, "Holdout \"holdout_exclude_td_true\" has excludeTargetedDeliveries enabled, continuing to rollout evaluation.") } diff --git a/pkg/entities/experiment.go b/pkg/entities/experiment.go index e27a82085..d44638fc6 100644 --- a/pkg/entities/experiment.go +++ b/pkg/entities/experiment.go @@ -93,7 +93,7 @@ type Holdout struct { // IncludedRules carries per-rule targeting for local holdouts. The datafile // section determines scope; `DatafileProjectConfig` strips `IncludedRules` on // entries from the `holdouts` section, so nil here is equivalent to global. - IncludedRules *[]string + IncludedRules *[]string ExcludeTargetedDeliveries bool } From 69c0afd74a12d5be76a0908d3c758f5b25e49c41 Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Wed, 29 Jul 2026 17:18:25 -0500 Subject: [PATCH 08/10] [FSSDK-12735] Trigger FSC tests From 9323aad6fcf2be6350aa9690ade3a82c153a0053 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 30 Jul 2026 08:59:26 -0700 Subject: [PATCH 09/10] [FSSDK-12735] Address review: gofmt struct, named rolloutService, dedupe holdout impression - Regroup FeatureDecision fields so the struct is gofmt-clean and document who sets the Holdout / HoldoutExperiment / HoldoutVariation pointers - Reference rollout service via a named field instead of featureServices[1] so the ExcludeTargetedDeliveries path no longer depends on slice ordering - Extract sendHoldoutImpression helper and reuse it across the three client call sites to remove the duplicated impression block Co-Authored-By: Claude Opus 4.8 --- pkg/client/client.go | 37 ++++++++++--------- pkg/decision/composite_feature_service.go | 14 +++++-- .../composite_feature_service_test.go | 4 ++ pkg/decision/entities.go | 13 +++++-- 4 files changed, 42 insertions(+), 26 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 1f90d096f..9f4f232c5 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -248,16 +248,27 @@ func (o *OptimizelyClient) dispatchDecisionEvents(projectConfig config.ProjectCo o.EventProcessor.ProcessEvent(ue) eventSent = true } - if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { - if hue, hok := event.CreateImpressionUserEvent(projectConfig, *featureDecision.HoldoutExperiment, - featureDecision.HoldoutVariation, usrContext, key, featureDecision.HoldoutExperiment.Key, decision.Holdout, featureDecision.HoldoutVariation.FeatureEnabled, nil); hok { - o.EventProcessor.ProcessEvent(hue) - eventSent = true - } + if o.sendHoldoutImpression(projectConfig, featureDecision, usrContext, key) { + 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() { @@ -536,12 +547,7 @@ func (o *OptimizelyClient) IsFeatureEnabled(featureKey string, userContext entit o.EventProcessor.ProcessEvent(ue) } // Send holdout impression when holdout is bypassed for targeted delivery - if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { - if hue, hok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, *featureDecision.HoldoutExperiment, - featureDecision.HoldoutVariation, userContext, featureKey, featureDecision.HoldoutExperiment.Key, decision.Holdout, featureDecision.HoldoutVariation.FeatureEnabled, nil); hok { - o.EventProcessor.ProcessEvent(hue) - } - } + o.sendHoldoutImpression(decisionContext.ProjectConfig, featureDecision, userContext, featureKey) return result, err } @@ -908,12 +914,7 @@ func (o *OptimizelyClient) GetDetailedFeatureDecisionUnsafe(featureKey string, u o.EventProcessor.ProcessEvent(ue) } // Send holdout impression when holdout is bypassed for targeted delivery - if featureDecision.HoldoutExperiment != nil && featureDecision.HoldoutVariation != nil { - if hue, hok := event.CreateImpressionUserEvent(decisionContext.ProjectConfig, *featureDecision.HoldoutExperiment, - featureDecision.HoldoutVariation, userContext, featureKey, featureDecision.HoldoutExperiment.Key, decision.Holdout, featureDecision.HoldoutVariation.FeatureEnabled, nil); hok { - o.EventProcessor.ProcessEvent(hue) - } - } + o.sendHoldoutImpression(decisionContext.ProjectConfig, featureDecision, userContext, featureKey) } } diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index 6882f0dbf..f62f60b99 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -27,19 +27,25 @@ import ( type CompositeFeatureService struct { holdoutService *HoldoutService featureServices []FeatureService - logger logging.OptimizelyLogProducer + // rolloutService is the same instance held in featureServices; it is referenced directly + // (rather than by slice index) for the ExcludeTargetedDeliveries path, so the logic does not + // depend on the ordering of featureServices. + rolloutService FeatureService + logger logging.OptimizelyLogProducer } // NewCompositeFeatureService returns a new instance of the CompositeFeatureService func NewCompositeFeatureService(sdkKey string, compositeExperimentService ExperimentService) *CompositeFeatureService { holdoutService := NewHoldoutService(sdkKey) + rolloutService := NewRolloutService(sdkKey) return &CompositeFeatureService{ holdoutService: holdoutService, logger: logging.GetLogger(sdkKey, "CompositeFeatureService"), featureServices: []FeatureService{ NewFeatureExperimentService(logging.GetLogger(sdkKey, "FeatureExperimentService"), compositeExperimentService, holdoutService), - NewRolloutService(sdkKey), + rolloutService, }, + rolloutService: rolloutService, } } @@ -89,8 +95,8 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu // Skip experiment evaluation entirely (A/B/MAB/CMAB are blocked by holdout). // Evaluate rollout service only (targeted deliveries are excluded from holdout blocking). - if len(f.featureServices) > 1 { - rolloutDecision, rolloutReasons, err := f.featureServices[1].GetDecision(decisionContext, userContext, options) + if f.rolloutService != nil { + rolloutDecision, rolloutReasons, err := f.rolloutService.GetDecision(decisionContext, userContext, options) reasons.Append(rolloutReasons) if err != nil { f.logger.Debug(err.Error()) diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index e00689017..8e1b451f0 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -234,6 +234,7 @@ func TestExcludeTDFalseBlocksEverything(t *testing.T) { compositeFeatureService := &CompositeFeatureService{ holdoutService: holdoutService, featureServices: []FeatureService{mockFeatureService, mockRolloutService}, + rolloutService: mockRolloutService, logger: logging.GetLogger("", "CompositeFeatureService"), } @@ -301,6 +302,7 @@ func TestExcludeTDTrueBlocksABExperiment(t *testing.T) { compositeFeatureService := &CompositeFeatureService{ holdoutService: holdoutService, featureServices: []FeatureService{mockFeatureService, mockRolloutService}, + rolloutService: mockRolloutService, logger: logging.GetLogger("", "CompositeFeatureService"), } @@ -370,6 +372,7 @@ func TestExcludeTDTrueAllowsTDRollout(t *testing.T) { compositeFeatureService := &CompositeFeatureService{ holdoutService: holdoutService, featureServices: []FeatureService{mockFeatureService, mockRolloutService}, + rolloutService: mockRolloutService, logger: logging.GetLogger("", "CompositeFeatureService"), } @@ -435,6 +438,7 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { compositeFeatureService := &CompositeFeatureService{ holdoutService: holdoutService, featureServices: []FeatureService{mockFeatureService, mockRolloutService}, + rolloutService: mockRolloutService, logger: logging.GetLogger("", "CompositeFeatureService"), } diff --git a/pkg/decision/entities.go b/pkg/decision/entities.go index c3bafed8c..e663854cd 100644 --- a/pkg/decision/entities.go +++ b/pkg/decision/entities.go @@ -70,10 +70,15 @@ type FeatureDecision struct { Source Source Experiment entities.Experiment Variation *entities.Variation - Holdout *entities.Holdout - CmabUUID *string - HoldoutExperiment *entities.Experiment - HoldoutVariation *entities.Variation + CmabUUID *string + // 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 *entities.Experiment + HoldoutVariation *entities.Variation } // ExperimentDecision contains the decision information about an experiment From 1dbdf304a9174b06084b2d0c578d23b98e95ae42 Mon Sep 17 00:00:00 2001 From: Matjaz Pirnovar Date: Thu, 30 Jul 2026 10:18:51 -0700 Subject: [PATCH 10/10] Set rollout source on no-match ETD decision When an excludeTargetedDeliveries holdout is hit but no rollout rule matches, the returned emptyDecision left Source unset. Under sendFlagDecisions=true this produced a served impression with a blank ruleType, diverging from RolloutService's own no-match path (which returns Source "rollout") and from the Swift/Python reference SDKs. Set Source: Rollout on the no-match decision so the served impression carries ruleType "rollout". No effect on the FSC suite, whose ETD datafile has sendFlagDecisions off (the served impression is gated on that flag), but correct for that production config. Co-Authored-By: Claude Opus 4.8 --- pkg/decision/composite_feature_service.go | 4 ++++ pkg/decision/composite_feature_service_test.go | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/decision/composite_feature_service.go b/pkg/decision/composite_feature_service.go index f62f60b99..073e7ebf4 100644 --- a/pkg/decision/composite_feature_service.go +++ b/pkg/decision/composite_feature_service.go @@ -111,6 +111,10 @@ func (f CompositeFeatureService) getDecisionWithExcludedTD(holdoutDecision Featu } emptyDecision := FeatureDecision{ + // 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, } diff --git a/pkg/decision/composite_feature_service_test.go b/pkg/decision/composite_feature_service_test.go index 8e1b451f0..54f38f914 100644 --- a/pkg/decision/composite_feature_service_test.go +++ b/pkg/decision/composite_feature_service_test.go @@ -446,7 +446,9 @@ func TestExcludeTDTrueNoDownstreamMatchReturnsEmpty(t *testing.T) { assert.NoError(t, err) assert.Nil(t, decision.Variation) - assert.Equal(t, "", decision.Source) + // No downstream match still reports the rollout source, matching RolloutService's + // own no-match behavior, so a served impression carries ruleType "rollout". + assert.Equal(t, Rollout, decision.Source) assert.NotNil(t, decision.HoldoutExperiment) assert.NotNil(t, decision.HoldoutVariation) assert.Equal(t, holdoutVar.ID, decision.HoldoutVariation.ID)