Skip to content

Commit e1c94f7

Browse files
[AI-FSSDK] [FSSDK-12958] Continue rule evaluation after local holdout hit (#407)
* [AI-FSSDK] [FSSDK-12958] Continue rule evaluation after local holdout hit * [AI-FSSDK] [FSSDK-12958] Test decision errors discard local holdout events * [AI-FSSDK] [FSSDK-12958] Re-run CI * Empty commit to trigger FSC --------- Co-authored-by: esrakartalOpt <esra.kartal@optimizely.com>
1 parent a0c792b commit e1c94f7

5 files changed

Lines changed: 238 additions & 36 deletions

File tree

‎lib/optimizely.rb‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# frozen_string_literal: true
22

33
#
4-
# Copyright 2016-2023, Optimizely and contributors
4+
# Copyright 2016-2023, 2026, Optimizely and contributors
55
#
66
# Licensed under the Apache License, Version 2.0 (the "License");
77
# you may not use this file except in compliance with the License.
@@ -225,21 +225,25 @@ def create_optimizely_decision(user_context, flag_key, decision, reasons, decide
225225
decision_event_dispatched = true
226226
end
227227

228-
if holdout_decision && !decide_options.include?(OptimizelyDecideOption::DISABLE_DECISION_EVENT) && decision_source != Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT']
229-
holdout_experiment = holdout_decision.experiment
230-
holdout_variation = holdout_decision.variation
231-
send_impression(
232-
config,
233-
holdout_experiment,
234-
holdout_variation ? holdout_variation['key'] : '',
235-
flag_key,
236-
holdout_experiment ? holdout_experiment['key'] : '',
237-
holdout_variation ? holdout_variation['featureEnabled'] : false,
238-
Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'],
239-
user_id,
240-
attributes
241-
)
242-
decision_event_dispatched = true
228+
# Accepts a single Decision or an Array; Kernel#Array would splat the Decision struct.
229+
holdout_decisions = holdout_decision.is_a?(Array) ? holdout_decision : [holdout_decision].compact
230+
if !decide_options.include?(OptimizelyDecideOption::DISABLE_DECISION_EVENT) && decision_source != Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT']
231+
holdout_decisions.each do |local_holdout_decision|
232+
holdout_experiment = local_holdout_decision.experiment
233+
holdout_variation = local_holdout_decision.variation
234+
send_impression(
235+
config,
236+
holdout_experiment,
237+
holdout_variation ? holdout_variation['key'] : '',
238+
flag_key,
239+
holdout_experiment ? holdout_experiment['key'] : '',
240+
holdout_variation ? holdout_variation['featureEnabled'] : false,
241+
Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'],
242+
user_id,
243+
attributes
244+
)
245+
decision_event_dispatched = true
246+
end
243247
end
244248

245249
# Generate all variables map if decide options doesn't include excludeVariables
@@ -408,8 +412,7 @@ def decide_for_keys(user_context, keys, decide_options = [], ignore_default_opti
408412
next
409413
end
410414
flag_decisions[flag_key] = decision
411-
holdout_decision = decision_list[i].holdout_decision
412-
holdout_decisions[flag_key] = holdout_decision if holdout_decision
415+
holdout_decisions[flag_key] = decision_list[i].holdout_decisions || [decision_list[i].holdout_decision].compact
413416
decision_reasons_dict[flag_key] ||= []
414417
decision_reasons_dict[flag_key].push(*reasons)
415418
end

‎lib/optimizely/decision_service.rb‎

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ class DecisionService
4141
Decision = Struct.new(:experiment, :variation, :source, :cmab_uuid)
4242
CmabDecisionResult = Struct.new(:error, :result, :reasons)
4343
VariationResult = Struct.new(:cmab_uuid, :error, :reasons, :variation_id, :holdout_decision)
44-
DecisionResult = Struct.new(:decision, :error, :reasons, :holdout_decision)
44+
# holdout_decision is deprecated; it holds the first entry of holdout_decisions.
45+
DecisionResult = Struct.new(:decision, :error, :reasons, :holdout_decision, :holdout_decisions)
4546

4647
DECISION_SOURCES = {
4748
'EXPERIMENT' => 'experiment',
@@ -199,6 +200,7 @@ def get_decision_for_flag(feature_flag, user_context, project_config, decide_opt
199200
# Check global holdouts first (flag level) — these apply to all rules across all flags
200201
holdouts = project_config.global_holdouts
201202
exclude_td_holdout_decision = nil
203+
holdout_decisions = []
202204

203205
holdouts.each do |holdout|
204206
holdout_decision = get_variation_for_holdout(holdout, user_context, project_config)
@@ -211,6 +213,7 @@ def get_decision_for_flag(feature_flag, user_context, project_config, decide_opt
211213
@logger.log(Logger::INFO, message)
212214
reasons.push(message)
213215
exclude_td_holdout_decision = holdout_decision.decision
216+
holdout_decisions << exclude_td_holdout_decision
214217
break
215218
else
216219
message = "The user '#{user_id}' is bucketed into holdout '#{holdout['key']}' for feature flag '#{feature_flag['key']}'."
@@ -225,16 +228,18 @@ def get_decision_for_flag(feature_flag, user_context, project_config, decide_opt
225228
unless exclude_td_holdout_decision
226229
experiment_decision = get_variation_for_feature_experiment(project_config, feature_flag, user_context, user_profile_tracker, decide_options)
227230
reasons.push(*experiment_decision.reasons)
231+
holdout_decisions.concat(experiment_decision.holdout_decisions || [])
228232

229-
return DecisionResult.new(experiment_decision.decision, experiment_decision.error, reasons) if experiment_decision.decision
233+
return DecisionResult.new(experiment_decision.decision, experiment_decision.error, reasons, holdout_decisions.first, holdout_decisions) if experiment_decision.decision
230234

231235
# If there's an error (e.g., CMAB error), return immediately without falling back to rollout
232-
return DecisionResult.new(nil, experiment_decision.error, reasons) if experiment_decision.error
236+
return DecisionResult.new(nil, experiment_decision.error, reasons, holdout_decisions.first, holdout_decisions) if experiment_decision.error
233237
end
234238

235239
# Check if the feature flag has a rollout and the user is bucketed into that rollout
236240
rollout_decision = get_variation_for_feature_rollout(project_config, feature_flag, user_context)
237241
reasons.push(*rollout_decision.reasons)
242+
holdout_decisions.concat(rollout_decision.holdout_decisions || [])
238243

239244
if rollout_decision.decision
240245
# Check if this was a forced decision (last reason contains "forced decision map")
@@ -247,11 +252,11 @@ def get_decision_for_flag(feature_flag, user_context, project_config, decide_opt
247252
reasons.push(message)
248253
end
249254

250-
DecisionResult.new(rollout_decision.decision, rollout_decision.error, reasons, exclude_td_holdout_decision)
255+
DecisionResult.new(rollout_decision.decision, rollout_decision.error, reasons, holdout_decisions.first, holdout_decisions)
251256
else
252257
message = "The user '#{user_id}' is not bucketed into a rollout for feature flag '#{feature_flag['key']}'."
253258
@logger.log(Logger::INFO, message)
254-
DecisionResult.new(nil, false, reasons, exclude_td_holdout_decision)
259+
DecisionResult.new(nil, false, reasons, holdout_decisions.first, holdout_decisions)
255260
end
256261
end
257262

@@ -358,6 +363,7 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_cont
358363
return DecisionResult.new(nil, false, decide_reasons)
359364
end
360365

366+
holdout_decisions = []
361367
# Evaluate each experiment and return the first bucketed experiment variation
362368
feature_flag['experimentIds'].each do |experiment_id|
363369
experiment = project_config.experiment_id_map[experiment_id]
@@ -377,25 +383,27 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_cont
377383
decide_reasons.push(*reasons_received)
378384

379385
# If there's an error, return immediately instead of falling back to next experiment
380-
return DecisionResult.new(nil, error, decide_reasons) if error
386+
return DecisionResult.new(nil, error, decide_reasons, holdout_decisions.first, holdout_decisions) if error
381387

382-
# If a local holdout decision was made, return it directly
383-
return DecisionResult.new(variation_result.holdout_decision, false, decide_reasons) if variation_result.holdout_decision
388+
if variation_result.holdout_decision
389+
holdout_decisions << variation_result.holdout_decision
390+
next
391+
end
384392

385393
next unless variation_id
386394

387395
variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
388396
variation = project_config.get_variation_from_flag(feature_flag['key'], variation_id, 'id') if variation.nil?
389397

390398
decision = Decision.new(experiment, variation, DECISION_SOURCES['FEATURE_TEST'], cmab_uuid)
391-
return DecisionResult.new(decision, error, decide_reasons)
399+
return DecisionResult.new(decision, error, decide_reasons, holdout_decisions.first, holdout_decisions)
392400
end
393401

394402
message = "The user '#{user_id}' is not bucketed into any of the experiments on the feature '#{feature_flag_key}'."
395403
@logger.log(Logger::INFO, message)
396404
decide_reasons.push(message)
397405

398-
DecisionResult.new(nil, false, decide_reasons)
406+
DecisionResult.new(nil, false, decide_reasons, holdout_decisions.first, holdout_decisions)
399407
end
400408

401409
def get_variation_for_feature_rollout(project_config, feature_flag, user_context)
@@ -431,22 +439,27 @@ def get_variation_for_feature_rollout(project_config, feature_flag, user_context
431439

432440
index = 0
433441
rollout_rules = rollout['experiments']
442+
holdout_decisions = []
434443
while index < rollout_rules.length
435444
holdout_decision, variation, skip_to_everyone_else, reasons_received = get_variation_from_delivery_rule(project_config, feature_flag_key, rollout_rules, index, user_context)
436445
decide_reasons.push(*reasons_received)
437446

438-
return DecisionResult.new(holdout_decision, false, decide_reasons) if holdout_decision
447+
if holdout_decision
448+
holdout_decisions << holdout_decision
449+
index += 1
450+
next
451+
end
439452

440453
if variation
441454
rule = rollout_rules[index]
442455
feature_decision = Decision.new(rule, variation, DECISION_SOURCES['ROLLOUT'], nil)
443-
return DecisionResult.new(feature_decision, false, decide_reasons)
456+
return DecisionResult.new(feature_decision, false, decide_reasons, holdout_decisions.first, holdout_decisions)
444457
end
445458

446459
index = skip_to_everyone_else ? (rollout_rules.length - 1) : (index + 1)
447460
end
448461

449-
DecisionResult.new(nil, false, decide_reasons)
462+
DecisionResult.new(nil, false, decide_reasons, holdout_decisions.first, holdout_decisions)
450463
end
451464

452465
def get_variation_from_experiment_rule(project_config, flag_key, rule, user, user_profile_tracker, options = [])

‎spec/decision_service_holdout_spec.rb‎

Lines changed: 186 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# frozen_string_literal: true
22

33
#
4-
# Copyright 2017-2020, 2023, Optimizely and contributors
4+
# Copyright 2017-2020, 2023, 2026, Optimizely and contributors
55
#
66
# Licensed under the Apache License, Version 2.0 (the "License");
77
# you may not use this file except in compliance with the License.
@@ -1386,4 +1386,189 @@
13861386
end
13871387
end
13881388
end
1389+
1390+
describe 'local holdout hit continues rule evaluation' do
1391+
let(:spy_event_processor) { spy('event_processor') }
1392+
let(:holdout_source) { Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'] }
1393+
let(:chrome_user) { {'browser_type' => 'chrome'} }
1394+
1395+
def local_holdout(id, rule_ids)
1396+
{
1397+
'id' => id,
1398+
'key' => id,
1399+
'status' => 'Running',
1400+
'audiences' => [],
1401+
'includedRules' => rule_ids,
1402+
'variations' => [{'id' => "#{id}_var", 'key' => 'holdout', 'featureEnabled' => false}],
1403+
'trafficAllocation' => [{'entityId' => "#{id}_var", 'endOfRange' => 10_000}]
1404+
}
1405+
end
1406+
1407+
# Global holdouts in the shared fixture bucket every user, so they are removed here.
1408+
def datafile(local_holdouts: nil, send_flag_decisions: true)
1409+
body = OptimizelySpec.deep_clone(OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS)
1410+
body['holdouts'] = []
1411+
body['localHoldouts'] = local_holdouts if local_holdouts
1412+
body['sendFlagDecisions'] = send_flag_decisions
1413+
body
1414+
end
1415+
1416+
def build_project(body, user_profile_service: nil, cmab_service: nil)
1417+
project = Optimizely::Project.new(
1418+
datafile: JSON.dump(body),
1419+
logger: spy_logger,
1420+
error_handler: error_handler,
1421+
event_processor: spy_event_processor,
1422+
user_profile_service: user_profile_service,
1423+
cmab_service: cmab_service
1424+
)
1425+
allow(project).to receive(:send_impression).and_wrap_original do |original, *args|
1426+
@impressions << args
1427+
original.call(*args)
1428+
end
1429+
@projects << project
1430+
project
1431+
end
1432+
1433+
def holdout_impressions
1434+
@impressions.select { |args| args[6] == holdout_source }.map { |args| args[1]['id'] }
1435+
end
1436+
1437+
before do
1438+
@projects = []
1439+
@impressions = []
1440+
end
1441+
after { @projects.each(&:close) }
1442+
1443+
it 'serves a later delivery rule after a holdout hit without jumping to Everyone Else' do
1444+
project = build_project(datafile(local_holdouts: [local_holdout('lh_a', ['177770'])]))
1445+
decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature')
1446+
1447+
expect(decision.rule_key).to eq('177772')
1448+
expect(decision.variation_key).to eq('177773')
1449+
expect(holdout_impressions).to eq(['lh_a'])
1450+
end
1451+
1452+
it 'sends one holdout event per held-out rule' do
1453+
project = build_project(datafile(local_holdouts: [local_holdout('lh_a', ['177770']), local_holdout('lh_b', ['177772'])]))
1454+
decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature')
1455+
1456+
expect(decision.rule_key).to eq('177776')
1457+
expect(holdout_impressions).to eq(%w[lh_a lh_b])
1458+
expect(project).to have_received(:send_impression)
1459+
.with(anything, anything, '177778', 'boolean_single_variable_feature', '177776', true, 'rollout', 'user', anything, anything)
1460+
end
1461+
1462+
it 'sends holdout events when no rule serves' do
1463+
project = build_project(datafile(local_holdouts: [local_holdout('lh_all', %w[177770 177772 177776])]))
1464+
decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature')
1465+
1466+
expect(decision.variation_key).to be_nil
1467+
expect(decision.enabled).to be false
1468+
expect(holdout_impressions).to eq(%w[lh_all lh_all lh_all])
1469+
end
1470+
1471+
it 'sends two holdout events when one local holdout targets two hit rules' do
1472+
project = build_project(datafile(local_holdouts: [local_holdout('lh_two', %w[177770 177772])]))
1473+
decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature')
1474+
1475+
expect(decision.rule_key).to eq('177776')
1476+
expect(holdout_impressions).to eq(%w[lh_two lh_two])
1477+
end
1478+
1479+
it 'sends the holdout event for a delivery rule even when sendFlagDecisions is off' do
1480+
project = build_project(datafile(local_holdouts: [local_holdout('lh_a', ['177770'])], send_flag_decisions: false))
1481+
decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature')
1482+
1483+
expect(decision.rule_key).to eq('177772')
1484+
expect(@impressions.length).to eq(1)
1485+
expect(holdout_impressions).to eq(['lh_a'])
1486+
end
1487+
1488+
it 'suppresses local holdout events with DISABLE_DECISION_EVENT' do
1489+
project = build_project(datafile(local_holdouts: [local_holdout('lh_a', ['177770']), local_holdout('lh_b', ['177772'])]))
1490+
decision = project.create_user_context('user', chrome_user).decide(
1491+
'boolean_single_variable_feature',
1492+
[Optimizely::Decide::OptimizelyDecideOption::DISABLE_DECISION_EVENT]
1493+
)
1494+
1495+
expect(decision.rule_key).to eq('177776')
1496+
expect(@impressions).to be_empty
1497+
end
1498+
1499+
it 'serves the second experiment rule after being held out of the first' do
1500+
body = datafile
1501+
body['featureFlags'].find { |f| f['key'] == 'boolean_feature' }['experimentIds'] = %w[122227 111127]
1502+
project = build_project(body)
1503+
1504+
decision = project.create_user_context('user', {}).decide('boolean_feature')
1505+
1506+
expect(decision.rule_key).to eq('test_experiment')
1507+
expect(holdout_impressions).to eq(['holdout_local_1'])
1508+
expect(project).to have_received(:send_impression)
1509+
.with(anything, anything, decision.variation_key, 'boolean_feature', 'test_experiment', true, 'feature-test', 'user', anything, anything)
1510+
1511+
config = project.config_manager.config
1512+
result = Optimizely::DecisionService.new(spy_logger, spy_cmab_service).get_decision_for_flag(
1513+
config.feature_flag_key_map['boolean_feature'],
1514+
project.create_user_context('user', {}),
1515+
config
1516+
)
1517+
expect(result.decision.experiment['id']).to eq('111127')
1518+
expect(result.holdout_decisions.map { |d| d.experiment['id'] }).to eq(['holdout_local_1'])
1519+
expect(result.holdout_decision).to eq(result.holdout_decisions.first)
1520+
end
1521+
1522+
it 'discards earlier local holdout hits when a later rule ends in a decision error' do
1523+
body = datafile
1524+
body['featureFlags'].find { |f| f['key'] == 'boolean_feature' }['experimentIds'] = %w[122227 111127]
1525+
body['experiments'].find { |e| e['id'] == '111127' }['cmab'] = {'attributeIds' => [], 'trafficAllocation' => 10_000}
1526+
failing_cmab = double('cmab_service')
1527+
allow(failing_cmab).to receive(:get_decision).and_raise(StandardError, 'CMAB request failed')
1528+
project = build_project(body, cmab_service: failing_cmab)
1529+
1530+
decisions = project.create_user_context('user', chrome_user).decide_for_keys(%w[boolean_feature boolean_single_variable_feature])
1531+
1532+
config = project.config_manager.config
1533+
result = Optimizely::DecisionService.new(spy_logger, failing_cmab).get_decision_for_flag(
1534+
config.feature_flag_key_map['boolean_feature'],
1535+
project.create_user_context('user', chrome_user),
1536+
config
1537+
)
1538+
expect(result.error).to be true
1539+
expect(result.holdout_decisions.map { |d| d.experiment['id'] }).to eq(['holdout_local_1'])
1540+
1541+
expect(decisions['boolean_feature'].variation_key).to be_nil
1542+
expect(decisions['boolean_feature'].reasons).to include('Failed to fetch CMAB data for experiment test_experiment.')
1543+
expect(@impressions.map { |args| args[3] }).not_to include('boolean_feature')
1544+
expect(holdout_impressions).to be_empty
1545+
expect(decisions['boolean_single_variable_feature'].variation_key).not_to be_nil
1546+
expect(@impressions.map { |args| args[3] }).to include('boolean_single_variable_feature')
1547+
end
1548+
1549+
it 'applies the local holdout over a stored sticky variation' do
1550+
ups = double('user_profile_service', save: nil)
1551+
allow(ups).to receive(:lookup).with('user').and_return(
1552+
user_id: 'user',
1553+
experiment_bucket_map: {'122227' => {variation_id: '122228'}}
1554+
)
1555+
project = build_project(datafile, user_profile_service: ups)
1556+
1557+
decision = project.create_user_context('user', {}).decide('boolean_feature')
1558+
1559+
expect(decision.variation_key).not_to eq('control_with_audience')
1560+
expect(decision.rule_key).not_to eq('test_experiment_with_audience')
1561+
expect(decision.variation_key).to be_nil
1562+
expect(decision.rule_key).to be_nil
1563+
expect(decision.enabled).to be false
1564+
expect(holdout_impressions).to eq(['holdout_local_1'])
1565+
end
1566+
1567+
it 'skips a held-out rule without a holdout event in legacy APIs' do
1568+
project = build_project(datafile(local_holdouts: [local_holdout('lh_a', %w[177770 177772])]))
1569+
1570+
expect(project.is_feature_enabled('boolean_single_variable_feature', 'user', chrome_user)).to be true
1571+
expect(holdout_impressions).to eq([])
1572+
end
1573+
end
13891574
end

0 commit comments

Comments
 (0)