Experiment Allocation Changes (#150)

ld-kyee · pellyg-ld · ld-kyee · commit 0452cd97ac3e · 2021-06-03T09:10:03.000-07:00
* WIP - from sam's pairing session

* starting sdk changes

* adding tests and making sure everything works

* adding more tests

* removing the singleton for fallthrough

* Revert "removing the singleton for fallthrough"

This reverts commit dff7adbb809ecc63118d0fbff9742a88a039c679.

* taking a different approach to keep things immutable

* adding tests for untracked

* remove unnecessary comment

* making sure to return two values in all code paths

Co-authored-by: pellyg-ld &lt;gpelly@launchdarkly.com&gt;
diff --git a/lib/ldclient-rb/evaluation_detail.rb b/lib/ldclient-rb/evaluation_detail.rb
@@ -120,6 +120,9 @@ class EvaluationReason
     # or deleted. If {#kind} is not {#RULE_MATCH}, this will be `nil`.
     attr_reader :rule_id
 
+    # A boolean or nil value representing if the rule or fallthrough has an experiment rollout.
+    attr_reader :in_experiment
+
     # The key of the prerequisite flag that did not return the desired variation. If {#kind} is not
     # {#PREREQUISITE_FAILED}, this will be `nil`.
     attr_reader :prerequisite_key
@@ -136,8 +139,12 @@ def self.off
 
     # Returns an instance whose {#kind} is {#FALLTHROUGH}.
     # @return [EvaluationReason]
-    def self.fallthrough
-      @@fallthrough
+    def self.fallthrough(in_experiment=false)
+      if in_experiment
+        @@fallthrough_with_experiment
+      else
+        @@fallthrough
+      end
     end
 
     # Returns an instance whose {#kind} is {#TARGET_MATCH}.
@@ -153,10 +160,16 @@ def self.target_match
     # @param rule_id [String] unique string identifier for the matched rule
     # @return [EvaluationReason]
     # @raise [ArgumentError] if `rule_index` is not a number or `rule_id` is not a string
-    def self.rule_match(rule_index, rule_id)
+    def self.rule_match(rule_index, rule_id, in_experiment=false)
       raise ArgumentError.new("rule_index must be a number") if !(rule_index.is_a? Numeric)
       raise ArgumentError.new("rule_id must be a string") if !rule_id.nil? && !(rule_id.is_a? String) # in test data, ID could be nil
-      new(:RULE_MATCH, rule_index, rule_id, nil, nil)
+      
+      if in_experiment
+        er = new(:RULE_MATCH, rule_index, rule_id, nil, nil, true)
+      else
+        er = new(:RULE_MATCH, rule_index, rule_id, nil, nil)
+      end
+      er
     end
 
     # Returns an instance whose {#kind} is {#PREREQUISITE_FAILED}.
@@ -204,11 +217,17 @@ def to_s
     def inspect
       case @kind
       when :RULE_MATCH
-        "RULE_MATCH(#{@rule_index},#{@rule_id})"
+        if @in_experiment
+          "RULE_MATCH(#{@rule_index},#{@rule_id},#{@in_experiment})"
+        else
+          "RULE_MATCH(#{@rule_index},#{@rule_id})"
+        end
       when :PREREQUISITE_FAILED
         "PREREQUISITE_FAILED(#{@prerequisite_key})"
       when :ERROR
         "ERROR(#{@error_kind})"
+      when :FALLTHROUGH
+        @in_experiment ? "FALLTHROUGH(#{@in_experiment})" : @kind.to_s
       else
         @kind.to_s
       end
@@ -225,11 +244,21 @@ def as_json(*) # parameter is unused, but may be passed if we're using the json
       # as_json and then modify the result.
       case @kind
       when :RULE_MATCH
-        { kind: @kind, ruleIndex: @rule_index, ruleId: @rule_id }
+        if @in_experiment
+          { kind: @kind, ruleIndex: @rule_index, ruleId: @rule_id, in_experiment: @in_experiment }
+        else
+          { kind: @kind, ruleIndex: @rule_index, ruleId: @rule_id }
+        end
       when :PREREQUISITE_FAILED
         { kind: @kind, prerequisiteKey: @prerequisite_key }
       when :ERROR
         { kind: @kind, errorKind: @error_kind }
+      when :FALLTHROUGH
+        if @in_experiment
+          { kind: @kind, in_experiment: @in_experiment }
+        else
+          { kind: @kind }
+        end
       else
         { kind: @kind }
       end
@@ -263,14 +292,15 @@ def [](key)
 
     private
 
-    def initialize(kind, rule_index, rule_id, prerequisite_key, error_kind)
+    def initialize(kind, rule_index, rule_id, prerequisite_key, error_kind, in_experiment=nil)
       @kind = kind.to_sym
       @rule_index = rule_index
       @rule_id = rule_id
       @rule_id.freeze if !rule_id.nil?
       @prerequisite_key = prerequisite_key
       @prerequisite_key.freeze if !prerequisite_key.nil?
       @error_kind = error_kind
+      @in_experiment = in_experiment
     end
 
     private_class_method :new
@@ -279,6 +309,7 @@ def self.make_error(error_kind)
       new(:ERROR, nil, nil, nil, error_kind)
     end
 
+    @@fallthrough_with_experiment = new(:FALLTHROUGH, nil, nil, nil, nil, true)
     @@fallthrough = new(:FALLTHROUGH, nil, nil, nil, nil)
     @@off = new(:OFF, nil, nil, nil, nil)
     @@target_match = new(:TARGET_MATCH, nil, nil, nil, nil)
diff --git a/lib/ldclient-rb/impl/evaluator.rb b/lib/ldclient-rb/impl/evaluator.rb
@@ -190,7 +190,7 @@ def segment_rule_match_user(rule, user, segment_key, salt)
         return true if !rule[:weight]
         
         # All of the clauses are met. See if the user buckets in
-        bucket = EvaluatorBucketing.bucket_user(user, segment_key, rule[:bucketBy].nil? ? "key" : rule[:bucketBy], salt)
+        bucket = EvaluatorBucketing.bucket_user(user, segment_key, rule[:bucketBy].nil? ? "key" : rule[:bucketBy], salt, nil)
         weight = rule[:weight].to_f / 100000.0
         return bucket < weight
       end
@@ -213,7 +213,13 @@ def get_off_value(flag, reason)
       end
 
       def get_value_for_variation_or_rollout(flag, vr, user, reason)
-        index = EvaluatorBucketing.variation_index_for_user(flag, vr, user)
+        index, in_experiment = EvaluatorBucketing.variation_index_for_user(flag, vr, user)
+        #if in experiment is true, set reason to a different reason instance/singleton with in_experiment set
+        if in_experiment && reason.kind == :FALLTHROUGH
+          reason = EvaluationReason::fallthrough(in_experiment)
+        elsif in_experiment && reason.kind == :RULE_MATCH
+          reason = EvaluationReason::rule_match(reason.rule_index, reason.rule_id, in_experiment)
+        end
         if index.nil?
           @logger.error("[LDClient] Data inconsistency in feature flag \"#{flag[:key]}\": variation/rollout object with no variation or rollout")
           return Evaluator.error_result(EvaluationReason::ERROR_MALFORMED_FLAG)
diff --git a/lib/ldclient-rb/impl/evaluator_bucketing.rb b/lib/ldclient-rb/impl/evaluator_bucketing.rb
@@ -10,30 +10,39 @@ module EvaluatorBucketing
       # @param user [Object] the user properties
       # @return [Number] the variation index, or nil if there is an error
       def self.variation_index_for_user(flag, rule, user)
+        in_experiment = nil
+
         variation = rule[:variation]
-        return variation if !variation.nil? # fixed variation
+        return variation, in_experiment if !variation.nil? # fixed variation
         rollout = rule[:rollout]
-        return nil if rollout.nil?
+        return nil, in_experiment if rollout.nil?
         variations = rollout[:variations]
         if !variations.nil? && variations.length > 0 # percentage rollout
           rollout = rule[:rollout]
           bucket_by = rollout[:bucketBy].nil? ? "key" : rollout[:bucketBy]
-          bucket = bucket_user(user, flag[:key], bucket_by, flag[:salt])
+
+          seed = rollout[:seed]
+          bucket = bucket_user(user, flag[:key], bucket_by, flag[:salt], seed) # may not be present
           sum = 0;
           variations.each do |variate|
+            if rule[:rollout][:kind] == "experiment" && !variate[:untracked]
+              in_experiment = true
+            end
+
             sum += variate[:weight].to_f / 100000.0
+
             if bucket < sum
-              return variate[:variation]
+              return variate[:variation], in_experiment
             end
           end
           # The user's bucket value was greater than or equal to the end of the last bucket. This could happen due
           # to a rounding error, or due to the fact that we are scaling to 100000 rather than 99999, or the flag
           # data could contain buckets that don't actually add up to 100000. Rather than returning an error in
           # this case (or changing the scaling, which would potentially change the results for *all* users), we
           # will simply put the user in the last bucket.
-          variations[-1][:variation]
+          [ variations[-1][:variation], in_experiment ]
         else # the rule isn't well-formed
-          nil
+          [ nil, in_experiment ]
         end
       end
 
@@ -44,7 +53,7 @@ def self.variation_index_for_user(flag, rule, user)
       # @param bucket_by [String|Symbol] the name of the user attribute to be used for bucketing
       # @param salt [String] the feature flag's or segment's salt value
       # @return [Number] the bucket value, from 0 inclusive to 1 exclusive
-      def self.bucket_user(user, key, bucket_by, salt)
+      def self.bucket_user(user, key, bucket_by, salt, seed)
         return nil unless user[:key]
 
         id_hash = bucketable_string_value(EvaluatorOperators.user_value(user, bucket_by))
@@ -56,7 +65,11 @@ def self.bucket_user(user, key, bucket_by, salt)
           id_hash += "." + user[:secondary].to_s
         end
 
-        hash_key = "%s.%s.%s" % [key, salt, id_hash]
+        if seed
+          hash_key = "%d.%s" % [seed, id_hash]
+        else
+          hash_key = "%s.%s.%s" % [key, salt, id_hash]
+        end
 
         hash_val = (Digest::SHA1.hexdigest(hash_key))[0..14]
         hash_val.to_i(16) / Float(0xFFFFFFFFFFFFFFF)
diff --git a/lib/ldclient-rb/impl/event_factory.rb b/lib/ldclient-rb/impl/event_factory.rb
@@ -103,6 +103,11 @@ def context_to_context_kind(user)
 
       def is_experiment(flag, reason)
         return false if !reason
+
+        if reason.in_experiment
+          return true
+        end
+
         case reason[:kind]
         when 'RULE_MATCH'
           index = reason[:ruleIndex]
@@ -115,6 +120,7 @@ def is_experiment(flag, reason)
         end
         false
       end
+
     end
   end
 end
diff --git a/spec/impl/evaluator_bucketing_spec.rb b/spec/impl/evaluator_bucketing_spec.rb
@@ -4,17 +4,43 @@
   subject { LaunchDarkly::Impl::EvaluatorBucketing }
 
   describe "bucket_user" do
+    describe "seed exists" do
+      let(:seed) { 61 }
+      it "gets the expected bucket values for seed" do
+        user = { key: "userKeyA" }
+        bucket = subject.bucket_user(user, "hashKey", "key", "saltyA", seed)
+        expect(bucket).to be_within(0.0000001).of(0.09801207);
+
+        user = { key: "userKeyB" }
+        bucket = subject.bucket_user(user, "hashKey", "key", "saltyA", seed)
+        expect(bucket).to be_within(0.0000001).of(0.14483777);
+
+        user = { key: "userKeyC" }
+        bucket = subject.bucket_user(user, "hashKey", "key", "saltyA", seed)
+        expect(bucket).to be_within(0.0000001).of(0.9242641);
+      end
+
+      it "should return the same bucket if the seed and user is the same" do
+        user = { key: "userKeyA" }
+        bucket1 = subject.bucket_user(user, "hashKey", "bucket_by", "saltyA", seed)
+        bucket2 = subject.bucket_user(user, "hashKey1", "bucket_by", "saltyB", seed)
+        bucket3 = subject.bucket_user(user, "hashKey2", "bucket_by", "saltyC", seed)
+        expect(bucket1).to eq(bucket2)
+        expect(bucket2).to eq(bucket3)
+      end
+    end
+
     it "gets expected bucket values for specific keys" do
       user = { key: "userKeyA" }
-      bucket = subject.bucket_user(user, "hashKey", "key", "saltyA")
+      bucket = subject.bucket_user(user, "hashKey", "key", "saltyA", nil)
       expect(bucket).to be_within(0.0000001).of(0.42157587);
 
       user = { key: "userKeyB" }
-      bucket = subject.bucket_user(user, "hashKey", "key", "saltyA")
+      bucket = subject.bucket_user(user, "hashKey", "key", "saltyA", nil)
       expect(bucket).to be_within(0.0000001).of(0.6708485);
 
       user = { key: "userKeyC" }
-      bucket = subject.bucket_user(user, "hashKey", "key", "saltyA")
+      bucket = subject.bucket_user(user, "hashKey", "key", "saltyA", nil)
       expect(bucket).to be_within(0.0000001).of(0.10343106);
     end
 
@@ -26,8 +52,8 @@
           intAttr: 33333
         }
       }
-      stringResult = subject.bucket_user(user, "hashKey", "stringAttr", "saltyA")
-      intResult = subject.bucket_user(user, "hashKey", "intAttr", "saltyA")
+      stringResult = subject.bucket_user(user, "hashKey", "stringAttr", "saltyA", nil)
+      intResult = subject.bucket_user(user, "hashKey", "intAttr", "saltyA", nil)
 
       expect(intResult).to be_within(0.0000001).of(0.54771423)
       expect(intResult).to eq(stringResult)
@@ -40,7 +66,7 @@
           floatAttr: 33.5
         }
       }
-      result = subject.bucket_user(user, "hashKey", "floatAttr", "saltyA")
+      result = subject.bucket_user(user, "hashKey", "floatAttr", "saltyA", nil)
       expect(result).to eq(0.0)
     end
 
@@ -52,7 +78,7 @@
           boolAttr: true
         }
       }
-      result = subject.bucket_user(user, "hashKey", "boolAttr", "saltyA")
+      result = subject.bucket_user(user, "hashKey", "boolAttr", "saltyA", nil)
       expect(result).to eq(0.0)
     end
   end
@@ -65,7 +91,7 @@
 
       # First verify that with our test inputs, the bucket value will be greater than zero and less than 100000,
       # so we can construct a rollout whose second bucket just barely contains that value
-      bucket_value = (subject.bucket_user(user, flag_key, "key", salt) * 100000).truncate()
+      bucket_value = (subject.bucket_user(user, flag_key, "key", salt, nil) * 100000).truncate()
       expect(bucket_value).to be > 0
       expect(bucket_value).to be < 100000
 
@@ -83,7 +109,7 @@
       }
       flag = { key: flag_key, salt: salt }
 
-      result_variation = subject.variation_index_for_user(flag, rule, user)
+      result_variation, _ = subject.variation_index_for_user(flag, rule, user)
       expect(result_variation).to be matched_variation
     end
 
@@ -92,7 +118,7 @@
       flag_key = "flagkey"
       salt = "salt"
 
-      bucket_value = (subject.bucket_user(user, flag_key, "key", salt) * 100000).truncate()
+      bucket_value = (subject.bucket_user(user, flag_key, "key", salt, nil) * 100000).truncate()
 
       # We'll construct a list of variations that stops right at the target bucket value
       rule = {
@@ -104,7 +130,7 @@
       }
       flag = { key: flag_key, salt: salt }
 
-      result_variation = subject.variation_index_for_user(flag, rule, user)
+      result_variation, _ = subject.variation_index_for_user(flag, rule, user)
       expect(result_variation).to be 0
     end
   end
diff --git a/spec/impl/evaluator_rule_spec.rb b/spec/impl/evaluator_rule_spec.rb
@@ -91,6 +91,38 @@ module Impl
         result = basic_evaluator.evaluate(flag, user, factory)
         expect(result.detail.reason).to eq(EvaluationReason::rule_match(0, 'ruleid'))
       end
+
+      describe "experiment rollout behavior" do
+        it "sets the in_experiment value if rollout kind is experiment " do
+          rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }],
+            rollout: { kind: 'experiment', variations: [ { weight: 100000, variation: 1, untracked: false } ] } }
+          flag = boolean_flag_with_rules([rule])
+          user = { key: "userkey", secondary: 999 }
+          result = basic_evaluator.evaluate(flag, user, factory)
+          expect(result.detail.reason.to_json).to include('"in_experiment":true')
+          expect(result.detail.reason.in_experiment).to eq(true)
+        end
+
+        it "does not set the in_experiment value if rollout kind is not experiment " do
+          rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }],
+            rollout: { kind: 'rollout', variations: [ { weight: 100000, variation: 1, untracked: false } ] } }
+          flag = boolean_flag_with_rules([rule])
+          user = { key: "userkey", secondary: 999 }
+          result = basic_evaluator.evaluate(flag, user, factory)
+          expect(result.detail.reason.to_json).to_not include('"in_experiment":true')
+          expect(result.detail.reason.in_experiment).to eq(nil)
+        end
+
+        it "does not set the in_experiment value if rollout kind is experiment and untracked is true" do
+          rule = { id: 'ruleid', clauses: [{ attribute: 'key', op: 'in', values: ['userkey'] }],
+            rollout: { kind: 'experiment', variations: [ { weight: 100000, variation: 1, untracked: true } ] } }
+          flag = boolean_flag_with_rules([rule])
+          user = { key: "userkey", secondary: 999 }
+          result = basic_evaluator.evaluate(flag, user, factory)
+          expect(result.detail.reason.to_json).to_not include('"in_experiment":true')
+          expect(result.detail.reason.in_experiment).to eq(nil)
+        end
+      end
     end
   end
 end
diff --git a/spec/impl/evaluator_spec.rb b/spec/impl/evaluator_spec.rb
diff --git a/spec/impl/event_factory_spec.rb b/spec/impl/event_factory_spec.rb
