Skip to content

Commit 75fc548

Browse files
authored
refactor: Changed the way reasons are being returned from decide APIs (#279)
## Summary Reasons were previously being collected by sending a reasons object down the stack to all the related functions which would mutate it by pushing their own reasons. This is now changed. Every related function now returns the reasons array along with the actual value. ## Test plan - Manually tested thoroughly. - Fixed all the affected unit test. - FSC passing
1 parent f718a18 commit 75fc548

File tree

8 files changed

+498
-278
lines changed

8 files changed

+498
-278
lines changed

lib/optimizely.rb

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,8 @@ def decide(user_context, key, decide_options = [])
199199
experiment = nil
200200
decision_source = Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT']
201201

202-
decision = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes, decide_options, reasons)
202+
decision, reasons_received = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes, decide_options)
203+
reasons.push(*reasons_received)
203204

204205
# Send impression event if Decision came from a feature test and decide options doesn't include disableDecisionEvent
205206
if decision.is_a?(Optimizely::DecisionService::Decision)
@@ -405,7 +406,7 @@ def get_forced_variation(experiment_key, user_id)
405406
config = project_config
406407

407408
forced_variation_key = nil
408-
forced_variation = @decision_service.get_forced_variation(config, experiment_key, user_id)
409+
forced_variation, = @decision_service.get_forced_variation(config, experiment_key, user_id)
409410
forced_variation_key = forced_variation['key'] if forced_variation
410411

411412
forced_variation_key
@@ -489,7 +490,7 @@ def is_feature_enabled(feature_flag_key, user_id, attributes = nil)
489490
return false
490491
end
491492

492-
decision = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes)
493+
decision, = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes)
493494

494495
feature_enabled = false
495496
source_string = Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT']
@@ -738,7 +739,7 @@ def get_all_feature_variables(feature_flag_key, user_id, attributes = nil)
738739
return nil
739740
end
740741

741-
decision = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes)
742+
decision, = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes)
742743
variation = decision ? decision['variation'] : nil
743744
feature_enabled = variation ? variation['featureEnabled'] : false
744745
all_variables = {}
@@ -878,7 +879,7 @@ def get_variation_with_config(experiment_key, user_id, attributes, config)
878879

879880
return nil unless user_inputs_valid?(attributes)
880881

881-
variation_id = @decision_service.get_variation(config, experiment_key, user_id, attributes)
882+
variation_id, = @decision_service.get_variation(config, experiment_key, user_id, attributes)
882883
variation = config.get_variation_from_id(experiment_key, variation_id) unless variation_id.nil?
883884
variation_key = variation['key'] if variation
884885
decision_notification_type = if config.feature_experiment?(experiment['id'])
@@ -944,7 +945,7 @@ def get_feature_variable_for_type(feature_flag_key, variable_key, variable_type,
944945
return nil
945946
end
946947

947-
decision = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes)
948+
decision, = @decision_service.get_variation_for_feature(config, feature_flag, user_id, attributes)
948949
variation = decision ? decision['variation'] : nil
949950
feature_enabled = variation ? variation['featureEnabled'] : false
950951

lib/optimizely/audience.rb

Lines changed: 18 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -38,33 +38,23 @@ def user_meets_audience_conditions?(config, experiment, attributes, logger, logg
3838
# This defaults to experiment['key'].
3939
#
4040
# Returns boolean representing if user satisfies audience conditions for the audiences or not.
41+
decide_reasons = []
4142
logging_hash ||= 'EXPERIMENT_AUDIENCE_EVALUATION_LOGS'
4243
logging_key ||= experiment['key']
4344

4445
logs_hash = Object.const_get "Optimizely::Helpers::Constants::#{logging_hash}"
4546

4647
audience_conditions = experiment['audienceConditions'] || experiment['audienceIds']
4748

48-
logger.log(
49-
Logger::DEBUG,
50-
format(
51-
logs_hash['EVALUATING_AUDIENCES_COMBINED'],
52-
logging_key,
53-
audience_conditions
54-
)
55-
)
49+
message = format(logs_hash['EVALUATING_AUDIENCES_COMBINED'], logging_key, audience_conditions)
50+
logger.log(Logger::DEBUG, message)
5651

5752
# Return true if there are no audiences
5853
if audience_conditions.empty?
59-
logger.log(
60-
Logger::INFO,
61-
format(
62-
logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'],
63-
logging_key,
64-
'TRUE'
65-
)
66-
)
67-
return true
54+
message = format(logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'], logging_key, 'TRUE')
55+
logger.log(Logger::INFO, message)
56+
decide_reasons.push(message)
57+
return true, decide_reasons
6858
end
6959

7060
attributes ||= {}
@@ -80,39 +70,28 @@ def user_meets_audience_conditions?(config, experiment, attributes, logger, logg
8070
return nil unless audience
8171

8272
audience_conditions = audience['conditions']
83-
logger.log(
84-
Logger::DEBUG,
85-
format(
86-
logs_hash['EVALUATING_AUDIENCE'],
87-
audience_id,
88-
audience_conditions
89-
)
90-
)
73+
message = format(logs_hash['EVALUATING_AUDIENCE'], audience_id, audience_conditions)
74+
logger.log(Logger::DEBUG, message)
75+
decide_reasons.push(message)
9176

9277
audience_conditions = JSON.parse(audience_conditions) if audience_conditions.is_a?(String)
9378
result = ConditionTreeEvaluator.evaluate(audience_conditions, evaluate_custom_attr)
9479
result_str = result.nil? ? 'UNKNOWN' : result.to_s.upcase
95-
logger.log(
96-
Logger::DEBUG,
97-
format(logs_hash['AUDIENCE_EVALUATION_RESULT'], audience_id, result_str)
98-
)
80+
message = format(logs_hash['AUDIENCE_EVALUATION_RESULT'], audience_id, result_str)
81+
logger.log(Logger::DEBUG, message)
82+
decide_reasons.push(message)
83+
9984
result
10085
end
10186

10287
eval_result = ConditionTreeEvaluator.evaluate(audience_conditions, evaluate_audience)
103-
10488
eval_result ||= false
10589

106-
logger.log(
107-
Logger::INFO,
108-
format(
109-
logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'],
110-
logging_key,
111-
eval_result.to_s.upcase
112-
)
113-
)
90+
message = format(logs_hash['AUDIENCE_EVALUATION_RESULT_COMBINED'], logging_key, eval_result.to_s.upcase)
91+
logger.log(Logger::INFO, message)
92+
decide_reasons.push(message)
11493

115-
eval_result
94+
[eval_result, decide_reasons]
11695
end
11796
end
11897
end

lib/optimizely/bucketer.rb

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ def initialize(logger)
3535
@bucket_seed = HASH_SEED
3636
end
3737

38-
def bucket(project_config, experiment, bucketing_id, user_id, decide_reasons = nil)
38+
def bucket(project_config, experiment, bucketing_id, user_id)
3939
# Determines ID of variation to be shown for a given experiment key and user ID.
4040
#
4141
# project_config - Instance of ProjectConfig
@@ -44,7 +44,9 @@ def bucket(project_config, experiment, bucketing_id, user_id, decide_reasons = n
4444
# user_id - String ID for user.
4545
#
4646
# Returns variation in which visitor with ID user_id has been placed. Nil if no variation.
47-
return nil if experiment.nil?
47+
return nil, [] if experiment.nil?
48+
49+
decide_reasons = []
4850

4951
# check if experiment is in a group; if so, check if user is bucketed into specified experiment
5052
# this will not affect evaluation of rollout rules.
@@ -55,72 +57,77 @@ def bucket(project_config, experiment, bucketing_id, user_id, decide_reasons = n
5557
group = project_config.group_id_map.fetch(group_id)
5658
if Helpers::Group.random_policy?(group)
5759
traffic_allocations = group.fetch('trafficAllocation')
58-
bucketed_experiment_id = find_bucket(bucketing_id, user_id, group_id, traffic_allocations)
60+
bucketed_experiment_id, find_bucket_reasons = find_bucket(bucketing_id, user_id, group_id, traffic_allocations)
61+
decide_reasons.push(*find_bucket_reasons)
62+
5963
# return if the user is not bucketed into any experiment
6064
unless bucketed_experiment_id
6165
message = "User '#{user_id}' is in no experiment."
6266
@logger.log(Logger::INFO, message)
63-
decide_reasons&.push(message)
64-
return nil
67+
decide_reasons.push(message)
68+
return nil, decide_reasons
6569
end
6670

6771
# return if the user is bucketed into a different experiment than the one specified
6872
if bucketed_experiment_id != experiment_id
6973
message = "User '#{user_id}' is not in experiment '#{experiment_key}' of group #{group_id}."
7074
@logger.log(Logger::INFO, message)
71-
decide_reasons&.push(message)
72-
return nil
75+
decide_reasons.push(message)
76+
return nil, decide_reasons
7377
end
7478

7579
# continue bucketing if the user is bucketed into the experiment specified
7680
message = "User '#{user_id}' is in experiment '#{experiment_key}' of group #{group_id}."
7781
@logger.log(Logger::INFO, message)
78-
decide_reasons&.push(message)
82+
decide_reasons.push(message)
7983
end
8084
end
8185

8286
traffic_allocations = experiment['trafficAllocation']
83-
variation_id = find_bucket(bucketing_id, user_id, experiment_id, traffic_allocations, decide_reasons)
87+
variation_id, find_bucket_reasons = find_bucket(bucketing_id, user_id, experiment_id, traffic_allocations)
88+
decide_reasons.push(*find_bucket_reasons)
89+
8490
if variation_id && variation_id != ''
8591
variation = project_config.get_variation_from_id(experiment_key, variation_id)
86-
return variation
92+
return variation, decide_reasons
8793
end
8894

8995
# Handle the case when the traffic range is empty due to sticky bucketing
9096
if variation_id == ''
9197
message = 'Bucketed into an empty traffic range. Returning nil.'
9298
@logger.log(Logger::DEBUG, message)
93-
decide_reasons&.push(message)
99+
decide_reasons.push(message)
94100
end
95101

96-
nil
102+
[nil, decide_reasons]
97103
end
98104

99-
def find_bucket(bucketing_id, user_id, parent_id, traffic_allocations, decide_reasons = nil)
105+
def find_bucket(bucketing_id, user_id, parent_id, traffic_allocations)
100106
# Helper function to find the matching entity ID for a given bucketing value in a list of traffic allocations.
101107
#
102108
# bucketing_id - String A customer-assigned value user to generate bucketing key
103109
# user_id - String ID for user
104110
# parent_id - String entity ID to use for bucketing ID
105111
# traffic_allocations - Array of traffic allocations
106112
#
107-
# Returns entity ID corresponding to the provided bucket value or nil if no match is found.
113+
# Returns and array of two values where first value is the entity ID corresponding to the provided bucket value
114+
# or nil if no match is found. The second value contains the array of reasons stating how the deicision was taken
115+
decide_reasons = []
108116
bucketing_key = format(BUCKETING_ID_TEMPLATE, bucketing_id: bucketing_id, entity_id: parent_id)
109117
bucket_value = generate_bucket_value(bucketing_key)
110118

111119
message = "Assigned bucket #{bucket_value} to user '#{user_id}' with bucketing ID: '#{bucketing_id}'."
112120
@logger.log(Logger::DEBUG, message)
113-
decide_reasons&.push(message)
114121

115122
traffic_allocations.each do |traffic_allocation|
116123
current_end_of_range = traffic_allocation['endOfRange']
117124
if bucket_value < current_end_of_range
118125
entity_id = traffic_allocation['entityId']
119-
return entity_id
126+
return entity_id, decide_reasons
120127
end
121128
end
122129

123-
nil
130+
[nil, decide_reasons]
124131
end
125132

126133
private

0 commit comments

Comments
 (0)