10000 feat: Duplicate experiment key issue with multi feature flag by ozayr-zaviar · Pull Request #282 · optimizely/ruby-sdk · GitHub
[go: up one dir, main page]
Skip to content

feat: Duplicate experiment key issue with multi feature flag #282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Jul 26, 2021
Merged

feat: Duplicate experiment key issue with multi feature flag #282

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a7c6212
init
ozayr-zaviar Jul 12, 2021
20a58a0
fix key fetch
ozayr-zaviar Jul 12, 2021
0134661
testcase fixed
ozayr-zaviar Jul 12, 2021
a5b8636
testcases added and variable name fixed
ozayr-zaviar Jul 13, 2021
a3645bd
logging tests added
ozayr-zaviar Jul 13, 2021
ba3a009
lint fixes
ozayr-zaviar Jul 13, 2021
16a2dce
testcase fixes
ozayr-zaviar Jul 13, 2021
009a86e
key created from all experiments
ozayr-zaviar Jul 15, 2021
450fded
correction
ozayr-zaviar Jul 15, 2021
eecae46
experiment_key_map intialized
ozayr-zaviar Jul 15, 2021
729a70b
revert
ozayr-zaviar Jul 15, 2021
6ed69a1
replaced all experiment keys with ids
ozayr-zaviar Jul 15, 2021
cc2af3f
All FSC test cases fixed
ozayr-zaviar Jul 16, 2021
8bc63dd
testcases fixed
ozayr-zaviar Jul 18, 2021
2aca129
coverall improved
ozayr-zaviar Jul 18, 2021
697e13d
extra line removed
ozayr-zaviar Jul 18, 2021
326b771
comments addressed
ozayr-zaviar Jul 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
replaced all experiment keys with ids
  • Loading branch information
@ozayr-zaviar
ozayr-zaviar committed Jul 15, 2021
commit 6ed69a1f5928c1b8b5fdedfd231a23687bb72359
10 changes: 5 additions & 5 deletions lib/optimizely.rb
View file 10000
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,10 +1078,10 @@ def send_impression(config, experiment, variation_key, flag_key, rule_key, enabl
}
end

experiment_key = experiment['key']
experiment_id = experiment['id']

variation_id = ''
variation_id = config.get_variation_id_from_key(experiment_key, variation_key) if experiment_key != ''
variation_id = config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key) if experiment_id != ''

metadata = {
flag_key: flag_key,
Expand All @@ -1095,11 +1095,11 @@ def send_impression(config, experiment, variation_key, flag_key, rule_key, enabl
@event_processor.process(user_event)
return unless @notification_center.notification_count(NotificationCenter::NOTIFICATION_TYPES[:ACTIVATE]).positive?

@logger.log(Logger::INFO, "Activating user '#{user_id}' in experiment '#{experiment_key}'.")
@logger.log(Logger::INFO, "Activating user '#{user_id}' in experiment '#{experiment_id}'.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep experiment_key in log

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @jaeopt on this. Also adding experiment_id can actually help too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having both will be helpful, but for consistency with all other SDKs, let's stick to "key" only in logs and messages


experiment = nil if experiment_key == ''
experiment = nil if experiment_id == ''
variation = nil
variation = config.get_variation_from_id(experiment_key, variation_id) unless experiment.nil?
variation = config.get_variation_from_id_by_experiment_id(experiment_id, variation_id) unless experiment.nil?
log_event = EventFactory.create_log_event(user_event, @logger)
@notification_center.send_notifications(
NotificationCenter::NOTIFICATION_TYPES[:ACTIVATE],
Expand Down
29 changes: 22 additions & 7 deletions lib/optimizely/config/datafile_project_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ def initialize(datafile, logger, error_handler)
@experiments.push(exp.merge('groupId' => key))
end
end
@experiment_key_map = generate_key_map(@experiments, 'key')
@experiment_key_map = {}
@experiment_id_map = generate_key_map(@experiments, 'id')
@audience_id_map = generate_key_map(@audiences, 'id')
@audience_id_map = @audience_id_map.merge(generate_key_map(@typed_audiences, 'id')) unless @typed_audiences.empty?
Expand All @@ -140,8 +140,8 @@ def initialize(datafile, logger, error_handler)
end
@all_experiments = @experiment_id_map.merge(@rollout_experiment_id_map)
@all_experiments.each do |id, exp|
@experiment_key_map[exp['key']] = exp
variations = exp.fetch('variations')
@experiment_key_map[exp.key] = exp
variations.each do |variation|
variation_id = variation['id']
variation['featureEnabled'] = variation['featureEnabled'] == true
Expand Down Expand Up @@ -220,6 +220,21 @@ def get_experiment_from_key(experiment_key)
nil
end

def get_experiment_from_id(experiment_id)
# Retrieves experiment ID for a given key
#
# experiment_id - String id representing the experiment
#
# Returns Experiment or nil if not found

experiment = @experiment_id_map[experiment_id]
return experiment if experiment

@logger.log Logger::ERROR, "Experiment id '#{experiment_id}' is not in datafile."
@error_handler.handle_error InvalidExperimentError
nil
end

def get_experiment_key(experiment_id)
# Retrieves experiment key for a given ID.
#
Expand Down Expand Up @@ -311,7 +326,7 @@ def get_variation_from_id_by_experiment_id(experiment_id, variation_id)
nil
end

def get_variation_from_key_by_experiment_id(experiment_id, variation_key)
def get_variation_id_from_key_by_experiment_id(experiment_id, variation_key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense because variation_key can probably duplicate across experiments.

# Get variation given experiment ID and variation key
#
# experiment_id - ID representing parent experiment of variation
Expand All @@ -322,7 +337,7 @@ def get_variation_from_key_by_experiment_id(experiment_id, variation_key)
variation_key_map_by_experiment_id = @variation_key_map_by_experiment_id[experiment_id]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
variation_key_map_by_experiment_id = @variation_key_map_by_experiment_id[experiment_id]
variation_key_map = @variation_key_map_by_experiment_id[experiment_id]

same name as the map will be confusing

if variation_key_map_by_experiment_id
variation = variation_key_map_by_experiment_id[variation_key]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
variation = variation_key_map_by_experiment_id[variation_key]
variation = variation_key_map[variation_key]

return variation if variation
return variation['id'] if variation

@logger.log Logger::ERROR, "Variation key '#{variation_key}' is not in datafile."
@error_handler.handle_error InvalidVariationError
Expand Down Expand Up @@ -357,17 +372,17 @@ def get_variation_id_from_key(experiment_key, variation_key)
nil
end

def get_whitelisted_variations(experiment_key)
def get_whitelisted_variations(experiment_id)
# Retrieves whitelisted variations for a given experiment Key
#
# experiment_key - String Key representing the experiment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix this comment

#
# Returns whitelisted variations for the experiment or nil

experiment = @experiment_key_map[experiment_key]
experiment = @experiment_id_map[experiment_id]
return experiment['forcedVariations'] if experiment

@logger.log Logger::ERROR, "Experiment key '#{experiment_key}' is not in datafile."
@logger.log Logger::ERROR, "Experiment key '#{experiment_id}' is not in datafile."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be helpful to keep both experiment_id and experiment_key in the log

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant use experiment key here as we are no longer sending key in get_whitelisted_variations instead we are sending the ID. If the given ID is wrong we cannot get an experiment and hence we can not get the key.

@error_handler.handle_error InvalidExperimentError
end

Expand Down
47 changes: 22 additions & 25 deletions lib/optimizely/decision_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,11 @@ def initialize(logger, user_profile_service = nil)
@forced_variation_map = {}
end

def get_variation(project_config, experiment_key, user_id, attributes = nil, decide_options = [])
def get_variation(project_config, experiment_id, user_id, attributes = nil, decide_options = [])
# Determines variation into which user will be bucketed.
#
# project_config - project_config - Instance of ProjectConfig
# experiment_key - Experiment for which visitor variation needs to be determined
# experiment_id - Experiment for which visitor variation needs to be determined
# user_id - String ID for user
# attributes - Hash representing user attributes
#
Expand All @@ -68,24 +68,23 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec
bucketing_id, bucketing_id_reasons = get_bucketing_id(user_id, attributes)
decide_reasons.push(*bucketing_id_reasons)
# Check to make sure experiment is active
experiment = project_config.get_experiment_from_key(experiment_key)
experiment = project_config.get_experiment_from_id(experiment_id)
return nil, decide_reasons if experiment.nil?

experiment_id = experiment['id']
unless project_config.experiment_running?(experiment)
message = "Experiment '#{experiment_key}' is not running."
message = "Experiment '#{experiment_id}' is not running."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep experiment_key for log and message

Copy link
Contributor

Or Keep both.

@logger.log(Logger::INFO, message)
decide_reasons.push(message)
return nil, decide_reasons
end

# Check if a forced variation is set for the user
forced_variation, reasons_received = get_forced_variation(project_config, experiment_key, user_id)
forced_variation, reasons_received = get_forced_variation(project_config, experiment_id, user_id)
decide_reasons.push(*reasons_received)
return forced_variation['id'], decide_reasons if forced_variation

# Check if user is in a white-listed variation
whitelisted_variation_id, reasons_received = get_whitelisted_variation_id(project_config, experiment_key, user_id)
whitelisted_variation_id, reasons_received = get_whitelisted_variation_id(project_config, experiment_id, user_id)
decide_reasons.push(*reasons_received)
return whitelisted_variation_id, decide_reasons if whitelisted_variation_id

Expand All @@ -97,7 +96,7 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec
saved_variation_id, reasons_received = get_saved_variation_id(project_config, experiment_id, user_profile)
decide_reasons.push(*reasons_received)
if saved_variation_id
message = "Returning previously activated variation ID #{saved_variation_id} of experiment '#{experiment_key}' for user '#{user_id}' from user profile."
message = "Returning previously activated variation ID #{saved_variation_id} of experiment '#{experiment_id}' for user '#{user_id}' from user profile."
@logger.log(Logger::INFO, message)
decide_reasons.push(message)
return saved_variation_id, decide_reasons
Expand All @@ -108,7 +107,7 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec
user_meets_audience_conditions, reasons_received = Audience.user_meets_audience_conditions?(project_config, experiment, attributes, @logger)
decide_reasons.push(*reasons_received)
unless user_meets_audience_conditions
message = "User '#{user_id}' does not meet the conditions to be in experiment '#{experiment_key}'."
message = "User '#{user_id}' does not meet the conditions to be in experiment '#{experiment_id}'."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep experiment_key in all messages here and others

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Agree! An i think you should keep both id and key in the messages

@logger.log(Logger::INFO, message)
decide_reasons.push(message)
return nil, decide_reasons
Expand All @@ -122,7 +121,7 @@ def get_variation(project_config, experiment_key, user_id, attributes = nil, dec
message = ''
if variation_id
variation_key = variation['key']
message = "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_key}'."
message = "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_id}'."
else
message = "User '#{user_id}' is in no variation."
end
Expand Down Expand Up @@ -186,13 +185,13 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_id,
return nil, decide_reasons
end

experiment_key = experiment['key']
variation_id, reasons_received = get_variation(project_config, experiment_key, user_id, attributes, decide_options)
experiment_id = experiment['id']
variation_id, reasons_received = get_variation(project_config, experiment_id, user_id, attributes, decide_options)
decide_reasons.push(*reasons_received)

next unless variation_id

variation = project_config.variation_id_map[experiment_key][variation_id]
variation = project_config.variation_id_map_by_experiment_id[experiment_id][variation_id]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why accessing it directly from the map when you have an accessor method?


return Decision.new(experiment, variation, DECISION_SOURCES['FEATURE_TEST']), decide_reasons
end
Expand Down Expand Up @@ -315,7 +314,7 @@ def set_forced_variation(project_config, experiment_key, user_id, variation_key)
return true
end

variation_id = project_config.get_variation_id_from_key(experiment_key, variation_key)
variation_id = project_config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key)

# check if the variation exists in the datafile
unless variation_id
Expand All @@ -330,11 +329,11 @@ def set_forced_variation(project_config, experiment_key, user_id, variation_key)
true
end

def get_forced_variation(project_config, experiment_key, user_id)
def get_forced_variation(project_config, experiment_id, user_id)
# Gets the forced variation for the given user and experiment.
#
# project_config - Instance of ProjectConfig
# experiment_key - String Key for experiment
# experiment_id - String id for experiment
# user_id - String ID for user
#
# Returns Variation The variation which the given user and experiment should be forced into
Expand All @@ -347,29 +346,27 @@ def get_forced_variation(project_config, experiment_key, user_id)
end

experiment_to_variation_map = @forced_variation_map[user_id]
experiment = project_config.get_experiment_from_key(experiment_key)
experiment_id = experiment['id'] if experiment
# check for nil and empty string experiment ID
# this case is logged in get_experiment_from_key
return nil, decide_reasons if experiment_id.nil? || experiment_id.empty?

unless experiment_to_variation_map.key? experiment_id
message = "No experiment '#{experiment_key}' mapped to user '#{user_id}' in the forced variation map."
message = "No experiment '#{experiment_id}' mapped to user '#{user_id}' in the forced variation map."
@logger.log(Logger::DEBUG, message)
decide_reasons.push(message)
return nil, decide_reasons
end

variation_id = experiment_to_variation_map[experiment_id]
variation_key = ''
variation = project_config.get_variation_from_id(experiment_key, variation_id)
variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
variation_key = variation['key'] if variation

# check if the variation exists in the datafile
# this case is logged in get_variation_from_id
return nil, decide_reasons if variation_key.empty?

message = "Variation '#{variation_key}' is mapped to experiment '#{experiment_key}' and user '#{user_id}' in the forced variation map"
message = "Variation '#{variation_key}' is mapped to experiment '#{experiment_id}' and user '#{user_id}' in the forced variation map"
@logger.log(Logger::DEBUG, message)
decide_reasons.push(message)

Expand All @@ -378,7 +375,7 @@ def get_forced_variation(project_config, experiment_key, user_id)

private

def get_whitelisted_variation_id(project_config, experiment_key, user_id)
def get_whitelisted_variation_id(project_config, experiment_id, user_id)
# Determine if a user is whitelisted into a variation for the given experiment and return the ID of that variation
#
# project_config - project_config - Instance of ProjectConfig
Expand All @@ -387,23 +384,23 @@ def get_whitelisted_variation_id(project_config, experiment_key, user_id)
#
# Returns variation ID into which user_id is whitelisted (nil if no variation)

whitelisted_variations = project_config.get_whitelisted_variations(experiment_key)
whitelisted_variations = project_config.get_whitelisted_variations(experiment_id)

return nil, nil unless whitelisted_variations

whitelisted_variation_key = whitelisted_variations[user_id]

return nil, nil unless whitelisted_variation_key

whitelisted_variation_id = project_config.get_variation_id_from_key(experiment_key, whitelisted_variation_key)
whitelisted_variation_id = project_config.get_variation_id_from_key_by_experiment_id(experiment_id, whitelisted_variation_key)

unless whitelisted_variation_id
message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}', which is not in the datafile."
@logger.log(Logger::INFO, message)
return nil, message
end

message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}' of experiment '#{experiment_key}'."
message = "User '#{user_id}' is whitelisted into variation '#{whitelisted_variation_key}' of experiment '#{experiment_id}'."
@logger.log(Logger::INFO, message)

[whitelisted_variation_id, message]
Expand Down
4 changes: 3 additions & 1 deletion lib/optimizely/project_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ def experiment_running?(experiment); end

def get_experiment_from_key(experiment_key); end

def get_experiment_from_id(experiment_id); end

def get_experiment_key(experiment_id); end

def get_event_from_key(event_key); end
Expand All @@ -72,7 +74,7 @@ def get_variation_from_key_by_experiment_id(experiment_id, variation_key); end

def get_variation_id_from_key(experiment_key, variation_key); end

def get_whitelisted_variations(experiment_key); end
def get_whitelisted_variations(experiment_id); end

def get_attribute_id(attribute_key); end

Expand Down
6 changes: 3 additions & 3 deletions spec/config/datafile_project_config_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,14 +868,14 @@
end
end

describe 'get_variation_from_key_by_experiment_id' do
describe 'get_variation_id_from_key_by_experiment_id' do
it 'should log a message when provided experiment id is invalid' do
config.get_variation_from_key_by_experiment_id('invalid_id', 'some_variation')
config.get_variation_id_from_key_by_experiment_id('invalid_id', 'some_variation')
expect(spy_logger).to have_received(:log).with(Logger::ERROR,
"Experiment id 'invalid_id' is not in datafile.")
end
it 'should return nil when provided variation key is invalid' do
expect(config.get_variation_from_key_by_experiment_id('111127', 'invalid_variation')).to eq(nil)
expect(config.get_variation_id_from_key_by_experiment_id('111127', 'invalid_variation')).to eq(nil)
expect(spy_logger).to have_received(:log).with(Logger::ERROR,
"Variation key 'invalid_variation' is not in datafile.")
end
Expand Down
0