-
Notifications
You must be signed in to change notification settings - Fork 28
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.
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
Changes from 3 commits
a7c6212
20a58a0
0134661
a5b8636
a3645bd
ba3a009
16a2dce
009a86e
450fded
eecae46
729a70b
6ed69a1
cc2af3f
8bc63dd
2aca129
697e13d
326b771
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -58,6 +58,8 @@ class DatafileProjectConfig < ProjectConfig | |||||
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 | ||||||
|
@@ -117,6 +119,8 @@ 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| | ||||||
|
@@ -132,9 +136,9 @@ def initialize(datafile, logger, error_handler) | |||||
@rollout_experiment_key_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_key_map = @rollout_experiment_key_map.merge(generate_key_map(exps, 'id')) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add all of your logics based on experiment_ids, not on experiment_keys. |
||||||
end | ||||||
@all_experiments = @experiment_key_map.merge(@rollout_experiment_key_map) | ||||||
@all_experiments = @experiment_id_map.merge(@rollout_experiment_key_map) | ||||||
@all_experiments.each do |key, exp| | ||||||
variations = exp.fetch('variations') | ||||||
variations.each do |variation| | ||||||
|
@@ -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[key] = generate_key_map(variations, 'id') | ||||||
@variation_key_map_by_experiment_id[key] = generate_key_map(variations, 'key') | ||||||
end | ||||||
@feature_flag_key_map = generate_key_map(@feature_flags, 'key') | ||||||
@experiment_feature_map = {} | ||||||
|
@@ -281,6 +287,52 @@ def get_variation_from_id(experiment_key, variation_id) | |||||
nil | ||||||
end | ||||||
|
||||||
def get_variation_from_id_by_experiment_id(experiment_id, variation_id) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can variation ids duplicate across experiments? why do we need There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
# 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_from_key_by_experiment_id(experiment_id, variation_key) | ||||||
# 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] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
same name as the map will be confusing |
||||||
if variation_key_map_by_experiment_id | ||||||
variation = variation_key_map_by_experiment_id[variation_key] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return variation 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 | ||||||
# | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -736,7 +736,7 @@ | |
'177776' => config_body['rollouts'][0]['experiments'][2], | ||
'177774' => config_body['rollouts'][1]['experiments'][0], | ||
'177779' => config_body['rollouts'][1]['experiments'][1], | ||
'rollout_exp_with_diff_id_and_key' => config_body['rollouts'][1]['experiments'][2] | ||
'177780' => config_body['rollouts'][1]['experiments'][2] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this change is needed? |
||
} | ||
|
||
expect(project_config.attribute_key_map).to eq(expected_attribute_key_map) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't do it using experiment_map, do it using experiment_id