-
Notifications
You must be signed in to change notification settings - Fork 27
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 9 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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -139,7 +139,7 @@ def initialize(datafile, logger, error_handler) | |||||
| @rollout_experiment_id_map = @rollout_experiment_id_map.merge(generate_key_map(exps, 'id')) | ||||||
| end | ||||||
| @all_experiments = @experiment_id_map.merge(@rollout_experiment_id_map) | ||||||
| @all_experiments.each do |key, exp| | ||||||
| @all_experiments.each do |id, exp| | ||||||
| variations = exp.fetch('variations') | ||||||
| variations.each do |variation| | ||||||
| variation_id = variation['id'] | ||||||
|
|
@@ -151,8 +151,8 @@ def initialize(datafile, logger, error_handler) | |||||
| end | ||||||
| @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') | ||||||
| @variation_id_map_by_experiment_id[id] = generate_key_map(variations, 'id') | ||||||
|
Contributor
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 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') | ||||||
| end | ||||||
| @feature_flag_key_map = generate_key_map(@feature_flags, 'key') | ||||||
| @experiment_feature_map = {} | ||||||
|
|
@@ -219,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. | ||||||
| # | ||||||
|
|
@@ -310,7 +325,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) | ||||||
|
Contributor
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. This makes sense because |
||||||
| # Get variation given experiment ID and variation key | ||||||
| # | ||||||
| # experiment_id - ID representing parent experiment of variation | ||||||
|
|
@@ -321,7 +336,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] | ||||||
|
||||||
| 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
Outdated
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.
| variation = variation_key_map_by_experiment_id[variation_key] | |
| variation = variation_key_map[variation_key] |
Outdated
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.
fix this comment
Outdated
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.
It will be helpful to keep both experiment_id and experiment_key in the log
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.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| # | ||
| # Copyright 2017-2020, Optimizely and contributors | ||
| # Copyright 2017-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. | ||
|
|
@@ -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 | ||
| # | ||
|
|
@@ -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." | ||
|
||
| @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['key'], 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 | ||
|
|
||
|
|
@@ -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 | ||
|
|
@@ -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}'." | ||
|
||
| @logger.log(Logger::INFO, message) | ||
| decide_reasons.push(message) | ||
| return nil, decide_reasons | ||
|
|
@@ -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 | ||
|
|
@@ -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] | ||
|
||
|
|
||
| return Decision.new(experiment, variation, DECISION_SOURCES['FEATURE_TEST']), decide_reasons | ||
| end | ||
|
|
@@ -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 | ||
|
|
@@ -334,7 +333,7 @@ def get_forced_variation(project_config, experiment_key, user_id) | |
| # Gets the forced variation for the given user and experiment. | ||
| # | ||
| # project_config - Instance of ProjectConfig | ||
| # experiment_key - String Key for experiment | ||
| # experiment_key - String key for experiment | ||
| # user_id - String ID for user | ||
| # | ||
| # Returns Variation The variation which the given user and experiment should be forced into | ||
|
|
@@ -347,29 +346,30 @@ def get_forced_variation(project_config, experiment_key, user_id) | |
| end | ||
|
|
||
| experiment_to_variation_map = @forced_variation_map[user_id] | ||
| # to be corrected | ||
|
||
| 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) | ||
|
|
||
|
|
@@ -378,7 +378,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 | ||
|
|
@@ -387,23 +387,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] | ||
|
|
||
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.
keep experiment_key in log
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.
I agree with @jaeopt on this. Also adding experiment_id can actually help too
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.
Having both will be helpful, but for consistency with all other SDKs, let's stick to "key" only in logs and messages