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.

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
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 1 addition & 1 deletion lib/optimizely/bucketer.rb
Original file line number Diff line number Diff line change
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
60 changes: 56 additions & 4 deletions lib/optimizely/config/datafile_project_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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|
Copy link
Contributor

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

Expand All @@ -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'))
Copy link
Contributor

Choose a reason for hiding this comment

The 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|
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[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 = {}
Expand Down Expand Up @@ -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)
Copy link
Contributor

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

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_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]
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

@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 Down
4 changes: 4 additions & 0 deletions lib/optimizely/project_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ def get_audience_from_id(audience_id); end

def get_variation_from_id(experiment_key, variation_id); end

def get_variation_from_id_by_experiment_id(experiment_id, variation_id); end

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
Expand Down
2 changes: 1 addition & 1 deletion spec/config/datafile_project_config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Expand Down