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

ozayr-zaviar · 2021-07-12T14:32:31Z

Summary

Fixed an issue related to duplicate experiment_key.
While trying to get variation from the variation_key_map, it was unable to find because the latest experimentKey was overriding the previous one.

TestPlan

Run with FSC
https://travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/jobs/523753247

coveralls · 2021-07-12T15:04:53Z

Coverage decreased (-0.01%) to 99.477% when pulling 326b771 on uzair/uniqueexperimentkey into c6e659e on master.

msohailhussain

please review the logic, you can extract experiment_key_map from the experiment_id_map, other way will cause issues.

msohailhussain · 2021-07-12T18:56:43Z

lib/optimizely/config/datafile_project_config.rb

@@ -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|


don't do it using experiment_map, do it using experiment_id

msohailhussain · 2021-07-12T18:58:11Z

lib/optimizely/config/datafile_project_config.rb

@@ -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'))


Add all of your logics based on experiment_ids, not on experiment_keys.

msohailhussain · 2021-07-12T19:00:11Z

spec/config/datafile_project_config_spec.rb

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


why this change is needed?

mnoman09

Overall LGTM, Just update headers.

jaeopt

This fixes the delivery rule-key conflict. I still see potential issue with experiment key conflicts. I believe they'll covered by new FSC test cases.

mnoman09 · 2021-07-19T12:50:59Z

lib/optimizely/config/datafile_project_config.rb

@@ -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


fix this comment

mnoman09 · 2021-07-19T12:57:56Z

lib/optimizely/decision_service.rb

@@ -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


What is the reason of this comment?

jaeopt

Looks good to change experiment-key to -id based.
For log and decision messages, please keep all 'experiment-key'as is (instead of using 'experiment-id' which is not helpful to users).

jaeopt · 2021-07-19T16:26:21Z

lib/optimizely.rb

@@ -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}'.")


keep experiment_key in log

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

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

jaeopt · 2021-07-19T16:27:20Z

lib/optimizely/decision_service.rb

      unless project_config.experiment_running?(experiment)
-        message = "Experiment '#{experiment_key}' is not running."
+        message = "Experiment '#{experiment_id}' is not running."


keep experiment_key for log and message

Or Keep both.

jaeopt · 2021-07-19T16:32:58Z

lib/optimizely/decision_service.rb

@@ -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}'."


keep experiment_key in all messages here and others

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

zashraf1985 · 2021-07-20T17:23:25Z

lib/optimizely.rb

@@ -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}'.")


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

zashraf1985 · 2021-07-20T17:37:13Z

lib/optimizely/config/datafile_project_config.rb

+        @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')
+        @variation_key_map_by_experiment_id[id] = generate_key_map(variations, 'key')


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.

let's talk offline.

zashraf1985 · 2021-07-20T17:40:28Z

lib/optimizely/config/datafile_project_config.rb

@@ -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)


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?

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)

zashraf1985 · 2021-07-20T17:41:08Z

lib/optimizely/config/datafile_project_config.rb

+      nil
+    end
+
+    def get_variation_id_from_key_by_experiment_id(experiment_id, variation_key)


This makes sense because variation_key can probably duplicate across experiments.

zashraf1985 · 2021-07-20T17:41:44Z

lib/optimizely/config/datafile_project_config.rb

      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."


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

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.

zashraf1985 · 2021-07-20T17:42:10Z

lib/optimizely/decision_service.rb

      unless project_config.experiment_running?(experiment)
-        message = "Experiment '#{experiment_key}' is not running."
+        message = "Experiment '#{experiment_id}' is not running."


Or Keep both.

zashraf1985 · 2021-07-20T17:43:05Z

lib/optimizely/decision_service.rb

@@ -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}'."


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

zashraf1985 · 2021-07-20T17:43:58Z

lib/optimizely/decision_service.rb

        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]


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

zashraf1985 · 2021-07-20T17:47:44Z

lib/optimizely/config/datafile_project_config.rb

-        @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')


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?

jaeopt

I do not see the last reviews addressed yet. Added some more comments.

jaeopt · 2021-07-19T17:01:59Z

lib/optimizely/config/datafile_project_config.rb

+      #
+      # Returns the variation or nil if not found
+
+      variation_key_map_by_experiment_id = @variation_key_map_by_experiment_id[experiment_id]


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

jaeopt · 2021-07-19T17:02:12Z

lib/optimizely/config/datafile_project_config.rb

+
+      variation_key_map_by_experiment_id = @variation_key_map_by_experiment_id[experiment_id]
+      if variation_key_map_by_experiment_id
+        variation = variation_key_map_by_experiment_id[variation_key]


Suggested change

variation = variation_key_map_by_experiment_id[variation_key]

variation = variation_key_map[variation_key]

jaeopt · 2021-07-22T17:56:19Z

lib/optimizely.rb

@@ -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}'.")


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

jaeopt

LGTM

msohailhussain

lgtm

init

a7c6212

optimizely-codereview-bot assigned ozayr-zaviar Jul 12, 2021

ozayr-zaviar changed the title ~~init~~ feat: Duplicate experiment key issue with multi feature flag Jul 12, 2021

fix key fetch

20a58a0

ozayr-zaviar unassigned ozayr-zaviar Jul 12, 2021

testcase fixed

0134661

msohailhussain reviewed Jul 12, 2021

View reviewed changes

ozayr-zaviar added 4 commits July 14, 2021 02:12

testcases added and variable name fixed

a5b8636

logging tests added

a3645bd

lint fixes

ba3a009

testcase fixes

16a2dce

mnoman09 suggested changes Jul 14, 2021

View reviewed changes

msohailhussain closed this Jul 14, 2021

msohailhussain reopened this Jul 14, 2021

optimizely-codereview-bot assigned ozayr-zaviar Jul 14, 2021

jaeopt requested changes Jul 15, 2021

View reviewed changes

ozayr-zaviar added 9 commits July 15, 2021 10:47

key created from all experiments

009a86e

correction

450fded

experiment_key_map intialized

eecae46

revert

729a70b

replaced all experiment keys with ids

6ed69a1

All FSC test cases fixed

cc2af3f

testcases fixed

8bc63dd

coverall improved

2aca129

extra line removed

697e13d

mnoman09 suggested changes Jul 19, 2021

View reviewed changes

jaeopt requested changes Jul 19, 2021

View reviewed changes

zashraf1985 suggested changes Jul 20, 2021

View reviewed changes

msohailhussain marked this pull request as ready for review July 21, 2021 23:31

msohailhussain requested a review from a team as a code owner July 21, 2021 23:31

jaeopt requested changes Jul 22, 2021

View reviewed changes

comments addressed

326b771

jaeopt approved these changes Jul 26, 2021

View reviewed changes

zashraf1985 approved these changes Jul 26, 2021

View reviewed changes

msohailhussain approved these changes Jul 26, 2021

View reviewed changes

jaeopt merged commit 6f844ca into master Jul 26, 2021

jaeopt deleted the uzair/uniqueexperimentkey branch July 26, 2021 23:40

zashraf1985 pushed a commit that referenced this pull request Aug 2, 2021

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

c6129f7

	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]

	variation = variation_key_map_by_experiment_id[variation_key]
	variation = variation_key_map[variation_key]

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

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

Conversation

ozayr-zaviar commented Jul 12, 2021 • edited Loading

Summary

TestPlan

coveralls commented Jul 12, 2021 • edited Loading

msohailhussain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnoman09 left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

msohailhussain left a comment

Choose a reason for hiding this comment

ozayr-zaviar commented Jul 12, 2021 •

edited

Loading

coveralls commented Jul 12, 2021 •

edited

Loading