fix: change seed for variantutils to ensure fair distribution

Unleash · Oct 26, 2023 · 40aa62d · 40aa62d
1 parent 4193824
commit 40aa62d
Show file tree

Hide file tree

Showing 8 changed files with 56 additions and 13 deletions.
diff --git a/src/main/java/io/getunleash/strategy/FlexibleRolloutStrategy.java b/src/main/java/io/getunleash/strategy/FlexibleRolloutStrategy.java
@@ -55,7 +55,7 @@ public boolean isEnabled(Map<String, String> parameters, UnleashContext unleashC
         final String groupId = parameters.getOrDefault(GROUP_ID, "");
 
         return stickinessId
-                .map(stick -> StrategyUtils.getNormalizedNumber(stick, groupId))
+                .map(stick -> StrategyUtils.getNormalizedNumber(stick, groupId, 0))
                 .map(norm -> percentage > 0 && norm <= percentage)
                 .orElse(false);
     }

diff --git a/src/main/java/io/getunleash/strategy/GradualRolloutSessionIdStrategy.java b/src/main/java/io/getunleash/strategy/GradualRolloutSessionIdStrategy.java
@@ -42,7 +42,7 @@ public boolean isEnabled(final Map<String, String> parameters, UnleashContext un
         final int percentage = StrategyUtils.getPercentage(parameters.get(PERCENTAGE));
         final String groupId = parameters.getOrDefault(GROUP_ID, "");
 
-        final int normalizedSessionId = StrategyUtils.getNormalizedNumber(sessionId.get(), groupId);
+        final int normalizedSessionId = StrategyUtils.getNormalizedNumber(sessionId.get(), groupId, 0);
 
         return percentage > 0 && normalizedSessionId <= percentage;
     }

diff --git a/src/main/java/io/getunleash/strategy/GradualRolloutUserIdStrategy.java b/src/main/java/io/getunleash/strategy/GradualRolloutUserIdStrategy.java
@@ -42,7 +42,7 @@ public boolean isEnabled(final Map<String, String> parameters, UnleashContext un
         final int percentage = StrategyUtils.getPercentage(parameters.get(PERCENTAGE));
         final String groupId = parameters.getOrDefault(GROUP_ID, "");
 
-        final int normalizedUserId = StrategyUtils.getNormalizedNumber(userId.get(), groupId);
+        final int normalizedUserId = StrategyUtils.getNormalizedNumber(userId.get(), groupId, 0);
 
         return percentage > 0 && normalizedUserId <= percentage;
     }

diff --git a/src/main/java/io/getunleash/strategy/StrategyUtils.java b/src/main/java/io/getunleash/strategy/StrategyUtils.java
@@ -35,13 +35,13 @@ public static boolean isNumeric(final CharSequence cs) {
      * @param groupId
      * @return
      */
-    public static int getNormalizedNumber(String identifier, String groupId) {
-        return getNormalizedNumber(identifier, groupId, ONE_HUNDRED);
+    public static int getNormalizedNumber(String identifier, String groupId, long seed) {
+        return getNormalizedNumber(identifier, groupId, ONE_HUNDRED, seed);
     }
 
-    public static int getNormalizedNumber(String identifier, String groupId, int normalizer) {
+    public static int getNormalizedNumber(String identifier, String groupId, int normalizer, long seed) {
         byte[] value = (groupId + ':' + identifier).getBytes();
-        long hash = Murmur3.hash_x86_32(value, value.length, 0);
+        long hash = Murmur3.hash_x86_32(value, value.length, seed);
         return (int) (hash % normalizer) + 1;
     }
 

diff --git a/src/main/java/io/getunleash/variant/VariantUtil.java b/src/main/java/io/getunleash/variant/VariantUtil.java
@@ -5,13 +5,17 @@
 import io.getunleash.Variant;
 import io.getunleash.lang.Nullable;
 import io.getunleash.strategy.StrategyUtils;
-import java.awt.*;
+
 import java.util.*;
 import java.util.List;
 import java.util.function.Predicate;
 
 public final class VariantUtil {
     static final String GROUP_ID_KEY = "groupId";
+    // To avoid using the same seed for gradual rollout and variant selection.
+    // This caused an unfortunate bias since we'd already excluded x % of the hash results.
+    // This is the 5.000.001st prime.
+    public static final Long VARIANT_NORMALIZATION_SEED = 86028157L;
 
     private VariantUtil() {}
 
@@ -115,7 +119,7 @@ public static Variant selectVariant(
                     StrategyUtils.getNormalizedNumber(
                             getSeed(context, customStickiness),
                             parameters.get(GROUP_ID_KEY),
-                            totalWeight);
+                            totalWeight, VARIANT_NORMALIZATION_SEED);
 
             int counter = 0;
             for (VariantDefinition variant : variants) {

diff --git a/src/test/java/io/getunleash/strategy/GradualRolloutSessionIdStrategyTest.java b/src/test/java/io/getunleash/strategy/GradualRolloutSessionIdStrategyTest.java
@@ -106,7 +106,7 @@ public void should_not_be_enabled_when_0percent_rollout() {
     public void should_be_enabled_above_minimum_percentage() {
         String sessionId = "1574576830";
         String groupId = "";
-        int minimumPercentage = StrategyUtils.getNormalizedNumber(sessionId, groupId);
+        int minimumPercentage = StrategyUtils.getNormalizedNumber(sessionId, groupId, 0);
 
         UnleashContext context = UnleashContext.builder().sessionId(sessionId).build();
 

diff --git a/src/test/java/io/getunleash/strategy/GradualRolloutUserIdStrategyTest.java b/src/test/java/io/getunleash/strategy/GradualRolloutUserIdStrategyTest.java
@@ -98,7 +98,7 @@ public void should_not_be_enabled_when_0percent_rollout() {
     public void should_be_enabled_above_minimum_percentage() {
         String userId = "1574576830";
         String groupId = "";
-        int minimumPercentage = StrategyUtils.getNormalizedNumber(userId, groupId);
+        int minimumPercentage = StrategyUtils.getNormalizedNumber(userId, groupId, 0);
 
         UnleashContext context = UnleashContext.builder().userId(userId).build();
 

diff --git a/src/test/java/io/getunleash/strategy/StrategyUtilsTest.java b/src/test/java/io/getunleash/strategy/StrategyUtilsTest.java
@@ -1,14 +1,53 @@
 package io.getunleash.strategy;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import com.google.errorprone.annotations.Var;
+import io.getunleash.variant.VariantUtil;
+import org.assertj.core.data.Offset;
 import org.junit.jupiter.api.Test;
 
+import java.util.HashMap;
+import java.util.Map;
+import java.util.UUID;
+
 public class StrategyUtilsTest {
 
     @Test
     public void normalized_values_are_the_same_across_node_java_and_go_clients() {
-        assertEquals(73, StrategyUtils.getNormalizedNumber("123", "gr1"));
-        assertEquals(25, StrategyUtils.getNormalizedNumber("999", "groupX"));
+        assertEquals(73, StrategyUtils.getNormalizedNumber("123", "gr1", 0));
+        assertEquals(25, StrategyUtils.getNormalizedNumber("999", "groupX", 0));
+    }
+
+    @Test
+    public void normalized_values_with_variant_seed_are_the_same_across_node_java() {
+        assertThat(StrategyUtils.getNormalizedNumber("123", "gr1", VariantUtil.VARIANT_NORMALIZATION_SEED)).isEqualTo(96);
+        assertThat(StrategyUtils.getNormalizedNumber("999", "groupX", VariantUtil.VARIANT_NORMALIZATION_SEED)).isEqualTo(60);
+    }
+
+    @Test
+    public void selecting_ten_percent_of_users_and_then_finding_variants_should_still_have_variants_evenly_distributed() {
+        int ones = 0, twos = 0, threes = 0, loopSize = 500000, selectionSize = 0;
+        Map<Integer, Integer> variantCounts = new HashMap<>();
+        for (int i = 0; i < loopSize; i++) {
+            String id = UUID.randomUUID().toString();
+            int featureRollout = StrategyUtils.getNormalizedNumber(id, "feature.name.that.is.quite.long", 0);
+            if (featureRollout < 11) {
+                int variantGroup = StrategyUtils.getNormalizedNumber(id, "feature.name.that.is.quite.long", 1000, VariantUtil.VARIANT_NORMALIZATION_SEED);
+                variantCounts.compute(variantGroup, (k, v) -> { if (v == null) { return 1; } else { return v+1; }});
+                if(variantGroup <= 333) {
+                    ones++;
+                } else if (variantGroup <= 666) {
+                    twos++;
+                } else if (variantGroup <= 1000) {
+                    threes++;
+                }
+                selectionSize++;
+            }
+        }
+        assertThat(ones / (double) (selectionSize)).isCloseTo(0.33, Offset.offset(0.01));
+        assertThat(twos / (double) (selectionSize)).isCloseTo(0.33, Offset.offset(0.01));
+        assertThat(threes / (double) (selectionSize)).isCloseTo(0.33, Offset.offset(0.01));
     }
 }