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
jaeopt 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 16 commits
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
18 changes: 10 additions & 8 deletions lib/optimizely.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

#
# Copyright 2016-2020, Optimizely and contributors
# Copyright 2016-2021, 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.
Expand Down Expand Up @@ -877,12 +877,14 @@ def get_variation_with_config(experiment_key, user_id, attributes, config)
experiment = config.get_experiment_from_key(experiment_key)
return nil if experiment.nil?

experiment_id = experiment['id']

return nil unless user_inputs_valid?(attributes)

variation_id, = @decision_service.get_variation(config, experiment_key, user_id, attributes)
variation_id, = @decision_service.get_variation(config, experiment_id, user_id, attributes)
variation = config.get_variation_from_id(experiment_key, variation_id) unless variation_id.nil?
variation_key = variation['key'] if variation
decision_notification_type = if config.feature_experiment?(experiment['id'])
decision_notification_type = if config.feature_experiment?(experiment_id)
Helpers::Constants::DECISION_NOTIFICATION_TYPES['FEATURE_TEST']
else
Helpers::Constants::DECISION_NOTIFICATION_TYPES['AB_TEST']
Expand Down Expand Up @@ -1078,10 +1080,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 +1097,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

@jaeopt jaeopt Jul 19, 2021

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

@zashraf1985 zashraf1985 Jul 20, 2021

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

@jaeopt jaeopt Jul 22, 2021

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
4 changes: 2 additions & 2 deletions lib/optimizely/bucketer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

#
# Copyright 2016-2017, 2019-2020 Optimizely and contributors
# Copyright 2016-2017, 2019-2021 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.
Expand Down Expand Up @@ -88,7 +88,7 @@ def bucket(project_config, experiment, bucketing_id, user_id)
decide_reasons.push(*find_bucket_reasons)

if variation_id && variation_id != ''
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)
return variation, decide_reasons
end

Expand Down
89 changes: 78 additions & 11 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 @@ -54,10 +54,12 @@ class DatafileProjectConfig < ProjectConfig
attr_reader :feature_variable_key_map
attr_reader :group_id_map
attr_reader :rollout_id_map
attr_reader :rollout_experiment_key_map
attr_reader :rollout_experiment_id_map
attr_reader :variation_id_map
attr_reader :variation_id_to_variable_usage_map
attr_reader :variation_key_map
attr_reader :variation_id_map_by_experiment_id
attr_reader :variation_key_map_by_experiment_id

def initialize(datafile, logger, error_handler)
# ProjectConfig init method to fetch and set project config data
Expand Down Expand Up @@ -117,9 +119,11 @@ def initialize(datafile, logger, error_handler)
@audience_id_map = @audience_id_map.merge(generate_key_map(@typed_audiences, 'id')) unless @typed_audiences.empty?
@variation_id_map = {}
@variation_key_map = {}
@variation_id_map_by_experiment_id = {}
@variation_key_map_by_experiment_id = {}
@variation_id_to_variable_usage_map = {}
@variation_id_to_experiment_map = {}
@experiment_key_map.each_value do |exp|
@experiment_id_map.each_value do |exp|
# Excludes experiments from rollouts
variations = exp.fetch('variations')
variations.each do |variation|
Expand All @@ -129,13 +133,13 @@ def initialize(datafile, logger, error_handler)
end
@rollout_id_map = generate_key_map(@rollouts, 'id')
# split out the experiment key map for rollouts
@rollout_experiment_key_map = {}
@rollout_experiment_id_map = {}
@rollout_id_map.each_value do |rollout|
exps = rollout.fetch('experiments')
@rollout_experiment_key_map = @rollout_experiment_key_map.merge(generate_key_map(exps, 'key'))
@rollout_experiment_id_map = @rollout_experiment_id_map.merge(generate_key_map(exps, 'id'))
end
@all_experiments = @experiment_key_map.merge(@rollout_experiment_key_map)
@all_experiments.each do |key, exp|
@all_experiments = @experiment_id_map.merge(@rollout_experiment_id_map)
@all_experiments.each do |id, exp|
variations = exp.fetch('variations')
variations.each do |variation|
variation_id = variation['id']
Expand All @@ -145,8 +149,10 @@ def initialize(datafile, logger, error_handler)

@variation_id_to_variable_usage_map[variation_id] = generate_key_map(variation_variables, 'id')
end
@variation_id_map[key] = generate_key_map(variations, 'id')
@variation_key_map[key] = generate_key_map(variations, 'key')
@variation_id_map[exp['key']] = generate_key_map(variations, 'id')
@variation_key_map[exp['key']] = generate_key_map(variations, 'key')
@variation_id_map_by_experiment_id[id] = generate_key_map(variations, 'id')
Copy link
Contributor

@zashraf1985 zashraf1985 Jul 20, 2021

Choose a reason for hiding this comment

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

why do we need it by experiment id? i assume variation ids are unique? if yes, then a simple map with variation ids as keys and variations as values would suffice? It was probably required in case of variation keys because keys can be reused across the experiments and you needed to specify which experiment to look into, but for variation id, i am not sure if experiment info is needed at all?

@variation_key_map_by_experiment_id[id] = generate_key_map(variations, 'key')
Comment on lines +152 to +155
Copy link
Contributor

@zashraf1985 zashraf1985 Jul 20, 2021

Choose a reason for hiding this comment

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

These variable names are really confusing. Looking at variation_id_map, it looks like its a map of variations with variation ids as keys but looks like the key is the experiment id which contains all the variations for that experiment. We might want to consider refactoring it later for better and more intuitive naming. Or if possible and if it makes sense to you , can you rename these variables to reflect more of what they actually are. variation_id_map can probably be variation_map_by_experiment_id etc. You can certainly think of a better variable name or can also leave it like that to refactor it later, i wont block the review on this.

Copy link
Contributor

@msohailhussain msohailhussain Jul 26, 2021

Choose a reason for hiding this comment

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

let's talk offline.

end
@feature_flag_key_map = generate_key_map(@feature_flags, 'key')
@experiment_feature_map = {}
Expand Down Expand Up @@ -213,6 +219,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 @@ -281,6 +302,52 @@ def get_variation_from_id(experiment_key, variation_id)
nil
end

def get_variation_from_id_by_experiment_id(experiment_id, variation_id)
Copy link
Contributor

@zashraf1985 zashraf1985 Jul 20, 2021

Choose a reason for hiding this comment

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

Can variation ids duplicate across experiments? why do we need experiment_id if we have variation_id. shouldn't variation_id be enought to uniquely identify and fetch a variation?

Copy link
Contributor Author

@ozayr-zaviar ozayr-zaviar Jul 26, 2021

Choose a reason for hiding this comment

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

Yes as far as I know variation Ids are unique but I was following this method def get_variation_from_id(experiment_key, variation_id)

# Get variation given experiment ID and variation ID
#
# experiment_id - ID representing parent experiment of variation
# variation_id - ID of the variation
#
# Returns the variation or nil if not found

variation_id_map_by_experiment_id = @variation_id_map_by_experiment_id[experiment_id]
if variation_id_map_by_experiment_id
variation = variation_id_map_by_experiment_id[variation_id]
return variation if variation

@logger.log Logger::ERROR, "Variation id '#{variation_id}' is not in datafile."
@error_handler.handle_error InvalidVariationError
return nil
end

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

def get_variation_id_from_key_by_experiment_id(experiment_id, variation_key)
Copy link
Contributor

@zashraf1985 zashraf1985 Jul 20, 2021

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
# variation_key - Key of the variation
#
# Returns the variation or nil if not found

variation_key_map_by_experiment_id = @variation_key_map_by_experiment_id[experiment_id]
Copy link
Contributor

@jaeopt jaeopt Jul 19, 2021

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

@jaeopt jaeopt Jul 19, 2021

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['id'] if variation

@logger.log Logger::ERROR, "Variation key '#{variation_key}' is not in datafile."
@error_handler.handle_error InvalidVariationError
return nil
end

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

def get_variation_id_from_key(experiment_key, variation_key)
# Get variation ID given experiment key and variation key
#
Expand All @@ -304,17 +371,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

@mnoman09 mnoman09 Jul 19, 2021

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

@zashraf1985 zashraf1985 Jul 20, 2021

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

@ozayr-zaviar ozayr-zaviar Jul 26, 2021

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
Loading