diff --git a/lib/optimizely.rb b/lib/optimizely.rb index b0c8035b..a41f6f1f 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # -# Copyright 2016-2023, Optimizely and contributors +# Copyright 2016-2023, 2026, Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # 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 decision_event_dispatched = true end - if holdout_decision && !decide_options.include?(OptimizelyDecideOption::DISABLE_DECISION_EVENT) && decision_source != Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'] - holdout_experiment = holdout_decision.experiment - holdout_variation = holdout_decision.variation - send_impression( - config, - holdout_experiment, - holdout_variation ? holdout_variation['key'] : '', - flag_key, - holdout_experiment ? holdout_experiment['key'] : '', - holdout_variation ? holdout_variation['featureEnabled'] : false, - Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'], - user_id, - attributes - ) - decision_event_dispatched = true + # Accepts a single Decision or an Array; Kernel#Array would splat the Decision struct. + holdout_decisions = holdout_decision.is_a?(Array) ? holdout_decision : [holdout_decision].compact + if !decide_options.include?(OptimizelyDecideOption::DISABLE_DECISION_EVENT) && decision_source != Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'] + holdout_decisions.each do |local_holdout_decision| + holdout_experiment = local_holdout_decision.experiment + holdout_variation = local_holdout_decision.variation + send_impression( + config, + holdout_experiment, + holdout_variation ? holdout_variation['key'] : '', + flag_key, + holdout_experiment ? holdout_experiment['key'] : '', + holdout_variation ? holdout_variation['featureEnabled'] : false, + Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'], + user_id, + attributes + ) + decision_event_dispatched = true + end end # 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 next end flag_decisions[flag_key] = decision - holdout_decision = decision_list[i].holdout_decision - holdout_decisions[flag_key] = holdout_decision if holdout_decision + holdout_decisions[flag_key] = decision_list[i].holdout_decisions || [decision_list[i].holdout_decision].compact decision_reasons_dict[flag_key] ||= [] decision_reasons_dict[flag_key].push(*reasons) end diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index ece3ee44..19fbfc25 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -41,7 +41,8 @@ class DecisionService Decision = Struct.new(:experiment, :variation, :source, :cmab_uuid) CmabDecisionResult = Struct.new(:error, :result, :reasons) VariationResult = Struct.new(:cmab_uuid, :error, :reasons, :variation_id, :holdout_decision) - DecisionResult = Struct.new(:decision, :error, :reasons, :holdout_decision) + # holdout_decision is deprecated; it holds the first entry of holdout_decisions. + DecisionResult = Struct.new(:decision, :error, :reasons, :holdout_decision, :holdout_decisions) DECISION_SOURCES = { 'EXPERIMENT' => 'experiment', @@ -199,6 +200,7 @@ def get_decision_for_flag(feature_flag, user_context, project_config, decide_opt # Check global holdouts first (flag level) — these apply to all rules across all flags holdouts = project_config.global_holdouts exclude_td_holdout_decision = nil + holdout_decisions = [] holdouts.each do |holdout| 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 @logger.log(Logger::INFO, message) reasons.push(message) exclude_td_holdout_decision = holdout_decision.decision + holdout_decisions << exclude_td_holdout_decision break else 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 unless exclude_td_holdout_decision experiment_decision = get_variation_for_feature_experiment(project_config, feature_flag, user_context, user_profile_tracker, decide_options) reasons.push(*experiment_decision.reasons) + holdout_decisions.concat(experiment_decision.holdout_decisions || []) - return DecisionResult.new(experiment_decision.decision, experiment_decision.error, reasons) if experiment_decision.decision + return DecisionResult.new(experiment_decision.decision, experiment_decision.error, reasons, holdout_decisions.first, holdout_decisions) if experiment_decision.decision # If there's an error (e.g., CMAB error), return immediately without falling back to rollout - return DecisionResult.new(nil, experiment_decision.error, reasons) if experiment_decision.error + return DecisionResult.new(nil, experiment_decision.error, reasons, holdout_decisions.first, holdout_decisions) if experiment_decision.error end # Check if the feature flag has a rollout and the user is bucketed into that rollout rollout_decision = get_variation_for_feature_rollout(project_config, feature_flag, user_context) reasons.push(*rollout_decision.reasons) + holdout_decisions.concat(rollout_decision.holdout_decisions || []) if rollout_decision.decision # 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 reasons.push(message) end - DecisionResult.new(rollout_decision.decision, rollout_decision.error, reasons, exclude_td_holdout_decision) + DecisionResult.new(rollout_decision.decision, rollout_decision.error, reasons, holdout_decisions.first, holdout_decisions) else message = "The user '#{user_id}' is not bucketed into a rollout for feature flag '#{feature_flag['key']}'." @logger.log(Logger::INFO, message) - DecisionResult.new(nil, false, reasons, exclude_td_holdout_decision) + DecisionResult.new(nil, false, reasons, holdout_decisions.first, holdout_decisions) end end @@ -358,6 +363,7 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_cont return DecisionResult.new(nil, false, decide_reasons) end + holdout_decisions = [] # Evaluate each experiment and return the first bucketed experiment variation feature_flag['experimentIds'].each do |experiment_id| experiment = project_config.experiment_id_map[experiment_id] @@ -377,10 +383,12 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_cont decide_reasons.push(*reasons_received) # If there's an error, return immediately instead of falling back to next experiment - return DecisionResult.new(nil, error, decide_reasons) if error + return DecisionResult.new(nil, error, decide_reasons, holdout_decisions.first, holdout_decisions) if error - # If a local holdout decision was made, return it directly - return DecisionResult.new(variation_result.holdout_decision, false, decide_reasons) if variation_result.holdout_decision + if variation_result.holdout_decision + holdout_decisions << variation_result.holdout_decision + next + end next unless variation_id @@ -388,14 +396,14 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_cont variation = project_config.get_variation_from_flag(feature_flag['key'], variation_id, 'id') if variation.nil? decision = Decision.new(experiment, variation, DECISION_SOURCES['FEATURE_TEST'], cmab_uuid) - return DecisionResult.new(decision, error, decide_reasons) + return DecisionResult.new(decision, error, decide_reasons, holdout_decisions.first, holdout_decisions) end message = "The user '#{user_id}' is not bucketed into any of the experiments on the feature '#{feature_flag_key}'." @logger.log(Logger::INFO, message) decide_reasons.push(message) - DecisionResult.new(nil, false, decide_reasons) + DecisionResult.new(nil, false, decide_reasons, holdout_decisions.first, holdout_decisions) end 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 index = 0 rollout_rules = rollout['experiments'] + holdout_decisions = [] while index < rollout_rules.length holdout_decision, variation, skip_to_everyone_else, reasons_received = get_variation_from_delivery_rule(project_config, feature_flag_key, rollout_rules, index, user_context) decide_reasons.push(*reasons_received) - return DecisionResult.new(holdout_decision, false, decide_reasons) if holdout_decision + if holdout_decision + holdout_decisions << holdout_decision + index += 1 + next + end if variation rule = rollout_rules[index] feature_decision = Decision.new(rule, variation, DECISION_SOURCES['ROLLOUT'], nil) - return DecisionResult.new(feature_decision, false, decide_reasons) + return DecisionResult.new(feature_decision, false, decide_reasons, holdout_decisions.first, holdout_decisions) end index = skip_to_everyone_else ? (rollout_rules.length - 1) : (index + 1) end - DecisionResult.new(nil, false, decide_reasons) + DecisionResult.new(nil, false, decide_reasons, holdout_decisions.first, holdout_decisions) end def get_variation_from_experiment_rule(project_config, flag_key, rule, user, user_profile_tracker, options = []) diff --git a/spec/decision_service_holdout_spec.rb b/spec/decision_service_holdout_spec.rb index e9846271..33e0d491 100644 --- a/spec/decision_service_holdout_spec.rb +++ b/spec/decision_service_holdout_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # -# Copyright 2017-2020, 2023, Optimizely and contributors +# Copyright 2017-2020, 2023, 2026, Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -1386,4 +1386,189 @@ end end end + + describe 'local holdout hit continues rule evaluation' do + let(:spy_event_processor) { spy('event_processor') } + let(:holdout_source) { Optimizely::DecisionService::DECISION_SOURCES['HOLDOUT'] } + let(:chrome_user) { {'browser_type' => 'chrome'} } + + def local_holdout(id, rule_ids) + { + 'id' => id, + 'key' => id, + 'status' => 'Running', + 'audiences' => [], + 'includedRules' => rule_ids, + 'variations' => [{'id' => "#{id}_var", 'key' => 'holdout', 'featureEnabled' => false}], + 'trafficAllocation' => [{'entityId' => "#{id}_var", 'endOfRange' => 10_000}] + } + end + + # Global holdouts in the shared fixture bucket every user, so they are removed here. + def datafile(local_holdouts: nil, send_flag_decisions: true) + body = OptimizelySpec.deep_clone(OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS) + body['holdouts'] = [] + body['localHoldouts'] = local_holdouts if local_holdouts + body['sendFlagDecisions'] = send_flag_decisions + body + end + + def build_project(body, user_profile_service: nil, cmab_service: nil) + project = Optimizely::Project.new( + datafile: JSON.dump(body), + logger: spy_logger, + error_handler: error_handler, + event_processor: spy_event_processor, + user_profile_service: user_profile_service, + cmab_service: cmab_service + ) + allow(project).to receive(:send_impression).and_wrap_original do |original, *args| + @impressions << args + original.call(*args) + end + @projects << project + project + end + + def holdout_impressions + @impressions.select { |args| args[6] == holdout_source }.map { |args| args[1]['id'] } + end + + before do + @projects = [] + @impressions = [] + end + after { @projects.each(&:close) } + + it 'serves a later delivery rule after a holdout hit without jumping to Everyone Else' do + project = build_project(datafile(local_holdouts: [local_holdout('lh_a', ['177770'])])) + decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature') + + expect(decision.rule_key).to eq('177772') + expect(decision.variation_key).to eq('177773') + expect(holdout_impressions).to eq(['lh_a']) + end + + it 'sends one holdout event per held-out rule' do + project = build_project(datafile(local_holdouts: [local_holdout('lh_a', ['177770']), local_holdout('lh_b', ['177772'])])) + decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature') + + expect(decision.rule_key).to eq('177776') + expect(holdout_impressions).to eq(%w[lh_a lh_b]) + expect(project).to have_received(:send_impression) + .with(anything, anything, '177778', 'boolean_single_variable_feature', '177776', true, 'rollout', 'user', anything, anything) + end + + it 'sends holdout events when no rule serves' do + project = build_project(datafile(local_holdouts: [local_holdout('lh_all', %w[177770 177772 177776])])) + decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature') + + expect(decision.variation_key).to be_nil + expect(decision.enabled).to be false + expect(holdout_impressions).to eq(%w[lh_all lh_all lh_all]) + end + + it 'sends two holdout events when one local holdout targets two hit rules' do + project = build_project(datafile(local_holdouts: [local_holdout('lh_two', %w[177770 177772])])) + decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature') + + expect(decision.rule_key).to eq('177776') + expect(holdout_impressions).to eq(%w[lh_two lh_two]) + end + + it 'sends the holdout event for a delivery rule even when sendFlagDecisions is off' do + project = build_project(datafile(local_holdouts: [local_holdout('lh_a', ['177770'])], send_flag_decisions: false)) + decision = project.create_user_context('user', chrome_user).decide('boolean_single_variable_feature') + + expect(decision.rule_key).to eq('177772') + expect(@impressions.length).to eq(1) + expect(holdout_impressions).to eq(['lh_a']) + end + + it 'suppresses local holdout events with DISABLE_DECISION_EVENT' do + project = build_project(datafile(local_holdouts: [local_holdout('lh_a', ['177770']), local_holdout('lh_b', ['177772'])])) + decision = project.create_user_context('user', chrome_user).decide( + 'boolean_single_variable_feature', + [Optimizely::Decide::OptimizelyDecideOption::DISABLE_DECISION_EVENT] + ) + + expect(decision.rule_key).to eq('177776') + expect(@impressions).to be_empty + end + + it 'serves the second experiment rule after being held out of the first' do + body = datafile + body['featureFlags'].find { |f| f['key'] == 'boolean_feature' }['experimentIds'] = %w[122227 111127] + project = build_project(body) + + decision = project.create_user_context('user', {}).decide('boolean_feature') + + expect(decision.rule_key).to eq('test_experiment') + expect(holdout_impressions).to eq(['holdout_local_1']) + expect(project).to have_received(:send_impression) + .with(anything, anything, decision.variation_key, 'boolean_feature', 'test_experiment', true, 'feature-test', 'user', anything, anything) + + config = project.config_manager.config + result = Optimizely::DecisionService.new(spy_logger, spy_cmab_service).get_decision_for_flag( + config.feature_flag_key_map['boolean_feature'], + project.create_user_context('user', {}), + config + ) + expect(result.decision.experiment['id']).to eq('111127') + expect(result.holdout_decisions.map { |d| d.experiment['id'] }).to eq(['holdout_local_1']) + expect(result.holdout_decision).to eq(result.holdout_decisions.first) + end + + it 'discards earlier local holdout hits when a later rule ends in a decision error' do + body = datafile + body['featureFlags'].find { |f| f['key'] == 'boolean_feature' }['experimentIds'] = %w[122227 111127] + body['experiments'].find { |e| e['id'] == '111127' }['cmab'] = {'attributeIds' => [], 'trafficAllocation' => 10_000} + failing_cmab = double('cmab_service') + allow(failing_cmab).to receive(:get_decision).and_raise(StandardError, 'CMAB request failed') + project = build_project(body, cmab_service: failing_cmab) + + decisions = project.create_user_context('user', chrome_user).decide_for_keys(%w[boolean_feature boolean_single_variable_feature]) + + config = project.config_manager.config + result = Optimizely::DecisionService.new(spy_logger, failing_cmab).get_decision_for_flag( + config.feature_flag_key_map['boolean_feature'], + project.create_user_context('user', chrome_user), + config + ) + expect(result.error).to be true + expect(result.holdout_decisions.map { |d| d.experiment['id'] }).to eq(['holdout_local_1']) + + expect(decisions['boolean_feature'].variation_key).to be_nil + expect(decisions['boolean_feature'].reasons).to include('Failed to fetch CMAB data for experiment test_experiment.') + expect(@impressions.map { |args| args[3] }).not_to include('boolean_feature') + expect(holdout_impressions).to be_empty + expect(decisions['boolean_single_variable_feature'].variation_key).not_to be_nil + expect(@impressions.map { |args| args[3] }).to include('boolean_single_variable_feature') + end + + it 'applies the local holdout over a stored sticky variation' do + ups = double('user_profile_service', save: nil) + allow(ups).to receive(:lookup).with('user').and_return( + user_id: 'user', + experiment_bucket_map: {'122227' => {variation_id: '122228'}} + ) + project = build_project(datafile, user_profile_service: ups) + + decision = project.create_user_context('user', {}).decide('boolean_feature') + + expect(decision.variation_key).not_to eq('control_with_audience') + expect(decision.rule_key).not_to eq('test_experiment_with_audience') + expect(decision.variation_key).to be_nil + expect(decision.rule_key).to be_nil + expect(decision.enabled).to be false + expect(holdout_impressions).to eq(['holdout_local_1']) + end + + it 'skips a held-out rule without a holdout event in legacy APIs' do + project = build_project(datafile(local_holdouts: [local_holdout('lh_a', %w[177770 177772])])) + + expect(project.is_feature_enabled('boolean_single_variable_feature', 'user', chrome_user)).to be true + expect(holdout_impressions).to eq([]) + end + end end diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb index eb70a9c9..766d7e64 100644 --- a/spec/decision_service_spec.rb +++ b/spec/decision_service_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # -# Copyright 2017-2020, 2023, Optimizely and contributors +# Copyright 2017-2020, 2023, 2026, Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -479,6 +479,8 @@ expected_decision_result = Optimizely::DecisionService::DecisionResult.new( expected_decision, false, + [], + nil, [] ) user_profile_tracker = Optimizely::UserProfileTracker.new(user_context.user_id) diff --git a/spec/optimizely_user_context_spec.rb b/spec/optimizely_user_context_spec.rb index 14360c23..72188170 100644 --- a/spec/optimizely_user_context_spec.rb +++ b/spec/optimizely_user_context_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # -# Copyright 2020, 2022-2023, Optimizely and contributors +# Copyright 2020, 2022-2023, 2026, Optimizely and contributors # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -743,7 +743,6 @@ user_id = 'tester' original_attributes = {} threads = [] - user_clone = nil user_context_obj = Optimizely::OptimizelyUserContext.new(forced_decision_project_instance, user_id, original_attributes) allow(user_context_obj).to receive(:clone) allow(user_context_obj).to receive(:set_forced_decision) @@ -760,7 +759,7 @@ # clone threads << Thread.new do 100.times do - user_clone = user_context_obj.clone + user_context_obj.clone end end