From 4cacd376a536e950ea44e9cd162f73afcbf2f183 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 30 Oct 2023 14:43:11 +0100 Subject: [PATCH 01/10] Fix: compare variant values when doing variant-based spec tests The old version only tested whether a flag was enabled or not; not whether the payload matches --- tests/clientspec.rs | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/tests/clientspec.rs b/tests/clientspec.rs index a70483d..b3ca47b 100644 --- a/tests/clientspec.rs +++ b/tests/clientspec.rs @@ -38,6 +38,21 @@ mod tests { enabled: bool, } + impl PartialEq for VariantResult { + fn eq(&self, other: &client::Variant) -> bool { + let payload_matches = match &self._payload { + Some(payload) => match (other.payload.get("type"), other.payload.get("value")) { + (Some(_type), Some(value)) => { + &payload._type == _type && &payload._value == value + } + _ => false, + }, + None => other.payload.get("type").is_none() && other.payload.get("value").is_none(), + }; + self.enabled == other.enabled && self._name == other.name && payload_matches + } + } + #[derive(Debug, Deserialize)] struct VariantTest { description: String, @@ -134,11 +149,11 @@ mod tests { } Tests::VariantTests { variant_tests } => { for test in variant_tests { - let result = - c.is_enabled_str(&test.toggle_name, Some(&test.context), false); + let result = c.get_variant_str(&test.toggle_name, &test.context); + assert_eq!( - test.expected_result.enabled, result, - "Test '{}' failed: got {} instead of {:?}", + test.expected_result, result, + "Test '{}' failed: got {:?} instead of {:?}", test.description, result, test.expected_result ); } From b772c51dd12bd8a9544b94a0ae483f90af4bebd4 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Mon, 30 Oct 2023 15:52:41 +0100 Subject: [PATCH 02/10] Chore: add tests for normalizing function --- src/strategy.rs | 44 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/src/strategy.rs b/src/strategy.rs index b8d82ca..1a306d4 100644 --- a/src/strategy.rs +++ b/src/strategy.rs @@ -115,6 +115,8 @@ pub fn partial_rollout(group: &str, variable: Option<&String>, rollout: u32) -> } } +const VARIANT_NORMALIZATION_SEED: u32 = 86028157; + /// Calculates a hash in the standard way expected for Unleash clients. Not /// required for extension strategies, but reusing this is probably a good idea /// for consistency across implementations. @@ -124,8 +126,30 @@ pub fn normalised_hash(group: &str, identifier: &str, modulus: u32) -> std::io:: // benchmarking would be useful - small datasizes here could make the best // path non-obvious) - but until murmur3 is fixed, we need to provide it // with a single string no matter what. + normalised_hash_internal(group, identifier, modulus, 0) +} + +pub fn normalised_variant_hash( + group: &str, + identifier: &str, + modulus: u32, +) -> std::io::Result { + // See https://github.com/stusmall/murmur3/pull/16 : .chain may avoid + // copying in the general case, and may be faster (though perhaps + // benchmarking would be useful - small datasizes here could make the best + // path non-obvious) - but until murmur3 is fixed, we need to provide it + // with a single string no matter what. + normalised_hash_internal(group, identifier, modulus, VARIANT_NORMALIZATION_SEED) +} + +fn normalised_hash_internal( + group: &str, + identifier: &str, + modulus: u32, + seed: u32, +) -> std::io::Result { let mut reader = Cursor::new(format!("{}:{}", &group, &identifier)); - murmur3_32(&mut reader, 0).map(|hash_result| hash_result % modulus) + murmur3_32(&mut reader, seed).map(|hash_result| hash_result % modulus) } // Build a closure to handle session id rollouts, parameterised by groupId and a @@ -861,4 +885,22 @@ mod tests { fn normalised_hash() { assert!(50 > super::normalised_hash("AB12A", "122", 100).unwrap()); } + + #[test] + fn test_normalized_hash() { + assert_eq!(73, super::normalised_hash("123", "gr1", 100).unwrap()); + assert_eq!(25, super::normalised_hash("999", "groupX", 100).unwrap()); + } + + #[test] + fn test_normalised_variant_hash() { + assert_eq!( + 96, + super::normalised_variant_hash("123", "gr1", 100).unwrap() + ); + assert_eq!( + 60, + super::normalised_variant_hash("999", "groupX", 100).unwrap() + ); + } } From 5f800bbd580a563388b4ab464c13460ec319b70c Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 31 Oct 2023 08:44:06 +0100 Subject: [PATCH 03/10] Fix: use variant hashing when getting variants. --- src/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client.rs b/src/client.rs index 07e01e7..3939908 100644 --- a/src/client.rs +++ b/src/client.rs @@ -591,7 +591,7 @@ where } let identifier = identifier.unwrap(); let total_weight = feature.variants.iter().map(|v| v.value.weight as u32).sum(); - strategy::normalised_hash(&group, identifier, total_weight) + strategy::normalised_variant_hash(&group, identifier, total_weight) .map(|selected_weight| { let mut counter: u32 = 0; for variant in feature.variants.iter().as_ref() { From b8b0cbea1524b926d9e91deede65a0e2d57f124c Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 31 Oct 2023 08:55:10 +0100 Subject: [PATCH 04/10] Update client spec to version with variant fixes --- client-specification | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client-specification b/client-specification index 3d91a8f..9acd706 160000 --- a/client-specification +++ b/client-specification @@ -1 +1 @@ -Subproject commit 3d91a8f18e1679994d308e5dde47954ae2def56d +Subproject commit 9acd706a71e27f78a76f74519176d3dd0444e06e From 755e645f24f3f74a47230c9302adf422a85df9f7 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 31 Oct 2023 10:05:51 +0100 Subject: [PATCH 05/10] Fix: argument order in tests --- src/strategy.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/strategy.rs b/src/strategy.rs index 1a306d4..c6a88e6 100644 --- a/src/strategy.rs +++ b/src/strategy.rs @@ -115,8 +115,6 @@ pub fn partial_rollout(group: &str, variable: Option<&String>, rollout: u32) -> } } -const VARIANT_NORMALIZATION_SEED: u32 = 86028157; - /// Calculates a hash in the standard way expected for Unleash clients. Not /// required for extension strategies, but reusing this is probably a good idea /// for consistency across implementations. @@ -129,16 +127,13 @@ pub fn normalised_hash(group: &str, identifier: &str, modulus: u32) -> std::io:: normalised_hash_internal(group, identifier, modulus, 0) } +const VARIANT_NORMALIZATION_SEED: u32 = 86028157; + pub fn normalised_variant_hash( group: &str, identifier: &str, modulus: u32, ) -> std::io::Result { - // See https://github.com/stusmall/murmur3/pull/16 : .chain may avoid - // copying in the general case, and may be faster (though perhaps - // benchmarking would be useful - small datasizes here could make the best - // path non-obvious) - but until murmur3 is fixed, we need to provide it - // with a single string no matter what. normalised_hash_internal(group, identifier, modulus, VARIANT_NORMALIZATION_SEED) } @@ -149,7 +144,7 @@ fn normalised_hash_internal( seed: u32, ) -> std::io::Result { let mut reader = Cursor::new(format!("{}:{}", &group, &identifier)); - murmur3_32(&mut reader, seed).map(|hash_result| hash_result % modulus) + murmur3_32(&mut reader, seed).map(|hash_result| (hash_result % modulus) + 1) } // Build a closure to handle session id rollouts, parameterised by groupId and a @@ -888,19 +883,19 @@ mod tests { #[test] fn test_normalized_hash() { - assert_eq!(73, super::normalised_hash("123", "gr1", 100).unwrap()); - assert_eq!(25, super::normalised_hash("999", "groupX", 100).unwrap()); + assert_eq!(73, super::normalised_hash("gr1", "123", 100).unwrap()); + assert_eq!(25, super::normalised_hash("groupX", "999", 100).unwrap()); } #[test] fn test_normalised_variant_hash() { assert_eq!( 96, - super::normalised_variant_hash("123", "gr1", 100).unwrap() + super::normalised_variant_hash("gr1", "123", 100).unwrap() ); assert_eq!( 60, - super::normalised_variant_hash("999", "groupX", 100).unwrap() + super::normalised_variant_hash("groupX", "999", 100).unwrap() ); } } From 8cff7823db551697d674aa7a16391e91372f7d89 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 31 Oct 2023 10:07:27 +0100 Subject: [PATCH 06/10] Fix: update existing tests with changes from new variant hashing --- src/client.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client.rs b/src/client.rs index 3939908..3021683 100644 --- a/src/client.rs +++ b/src/client.rs @@ -1320,7 +1320,7 @@ mod tests { ], enabled: true, }; - assert_eq!(variant2, c.get_variant(UserFeatures::two, &uid1)); + assert_eq!(variant1, c.get_variant(UserFeatures::two, &uid1)); assert_eq!(variant2, c.get_variant(UserFeatures::two, &session1)); assert_eq!(variant1, c.get_variant(UserFeatures::two, &host1)); } @@ -1395,7 +1395,7 @@ mod tests { ], enabled: true, }; - assert_eq!(variant2, c.get_variant_str("two", &uid1)); + assert_eq!(variant1, c.get_variant_str("two", &uid1)); assert_eq!(variant2, c.get_variant_str("two", &session1)); assert_eq!(variant1, c.get_variant_str("two", &host1)); } From a0b4e7833647d7800a3291ce84be7f57a1e75acd Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 31 Oct 2023 12:49:57 +0100 Subject: [PATCH 07/10] Chore: move some comments around --- src/strategy.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/strategy.rs b/src/strategy.rs index c6a88e6..e9a8b73 100644 --- a/src/strategy.rs +++ b/src/strategy.rs @@ -119,16 +119,15 @@ pub fn partial_rollout(group: &str, variable: Option<&String>, rollout: u32) -> /// required for extension strategies, but reusing this is probably a good idea /// for consistency across implementations. pub fn normalised_hash(group: &str, identifier: &str, modulus: u32) -> std::io::Result { - // See https://github.com/stusmall/murmur3/pull/16 : .chain may avoid - // copying in the general case, and may be faster (though perhaps - // benchmarking would be useful - small datasizes here could make the best - // path non-obvious) - but until murmur3 is fixed, we need to provide it - // with a single string no matter what. normalised_hash_internal(group, identifier, modulus, 0) } const VARIANT_NORMALIZATION_SEED: u32 = 86028157; +/// Calculates a hash for **variant distribution** in the standard way +/// expected for Unleash clients. This differs from the +/// [`normalised_hash`] function in that it uses a different seed to +/// ensure a fair distribution. pub fn normalised_variant_hash( group: &str, identifier: &str, @@ -143,6 +142,11 @@ fn normalised_hash_internal( modulus: u32, seed: u32, ) -> std::io::Result { + // See https://github.com/stusmall/murmur3/pull/16 : .chain may avoid + // copying in the general case, and may be faster (though perhaps + // benchmarking would be useful - small datasizes here could make the best + // path non-obvious) - but until murmur3 is fixed, we need to provide it + // with a single string no matter what. let mut reader = Cursor::new(format!("{}:{}", &group, &identifier)); murmur3_32(&mut reader, seed).map(|hash_result| (hash_result % modulus) + 1) } From 865f8b40fde51e2b119d693b5df14666e0b7ba16 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 31 Oct 2023 13:24:14 +0100 Subject: [PATCH 08/10] Fix: variant selection algorithm --- src/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client.rs b/src/client.rs index 3021683..ce508cc 100644 --- a/src/client.rs +++ b/src/client.rs @@ -596,7 +596,7 @@ where let mut counter: u32 = 0; for variant in feature.variants.iter().as_ref() { counter += variant.value.weight as u32; - if counter > selected_weight { + if counter >= selected_weight { variant.count.fetch_add(1, Ordering::Relaxed); return variant.into(); } From b0213b2b665dd2d437f97832fd2203060401b643 Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 31 Oct 2023 13:51:15 +0100 Subject: [PATCH 09/10] Fix: client tests --- client-specification | 2 +- src/strategy.rs | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/client-specification b/client-specification index 9acd706..207acbe 160000 --- a/client-specification +++ b/client-specification @@ -1 +1 @@ -Subproject commit 9acd706a71e27f78a76f74519176d3dd0444e06e +Subproject commit 207acbe3e62c1a8132baf57ab7330cfb88218081 diff --git a/src/strategy.rs b/src/strategy.rs index e9a8b73..4be992c 100644 --- a/src/strategy.rs +++ b/src/strategy.rs @@ -119,7 +119,7 @@ pub fn partial_rollout(group: &str, variable: Option<&String>, rollout: u32) -> /// required for extension strategies, but reusing this is probably a good idea /// for consistency across implementations. pub fn normalised_hash(group: &str, identifier: &str, modulus: u32) -> std::io::Result { - normalised_hash_internal(group, identifier, modulus, 0) + normalised_hash_internal(group, identifier, modulus, 0, 0) } const VARIANT_NORMALIZATION_SEED: u32 = 86028157; @@ -133,7 +133,7 @@ pub fn normalised_variant_hash( identifier: &str, modulus: u32, ) -> std::io::Result { - normalised_hash_internal(group, identifier, modulus, VARIANT_NORMALIZATION_SEED) + normalised_hash_internal(group, identifier, modulus, VARIANT_NORMALIZATION_SEED, 1) } fn normalised_hash_internal( @@ -141,6 +141,7 @@ fn normalised_hash_internal( identifier: &str, modulus: u32, seed: u32, + additional: u32, ) -> std::io::Result { // See https://github.com/stusmall/murmur3/pull/16 : .chain may avoid // copying in the general case, and may be faster (though perhaps @@ -148,7 +149,7 @@ fn normalised_hash_internal( // path non-obvious) - but until murmur3 is fixed, we need to provide it // with a single string no matter what. let mut reader = Cursor::new(format!("{}:{}", &group, &identifier)); - murmur3_32(&mut reader, seed).map(|hash_result| (hash_result % modulus) + 1) + murmur3_32(&mut reader, seed).map(|hash_result| (hash_result % modulus) + additional) } // Build a closure to handle session id rollouts, parameterised by groupId and a From 1e24d77f928ea0f6e92395427dd1e85475a183ff Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 31 Oct 2023 13:57:02 +0100 Subject: [PATCH 10/10] Fix: gradual rollout --- src/strategy.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/strategy.rs b/src/strategy.rs index 4be992c..dcbf269 100644 --- a/src/strategy.rs +++ b/src/strategy.rs @@ -107,7 +107,7 @@ pub fn partial_rollout(group: &str, variable: Option<&String>, rollout: u32) -> 100 => true, rollout => { if let Ok(normalised) = normalised_hash(group, variable, 100) { - rollout > normalised + rollout >= normalised } else { false } @@ -119,7 +119,7 @@ pub fn partial_rollout(group: &str, variable: Option<&String>, rollout: u32) -> /// required for extension strategies, but reusing this is probably a good idea /// for consistency across implementations. pub fn normalised_hash(group: &str, identifier: &str, modulus: u32) -> std::io::Result { - normalised_hash_internal(group, identifier, modulus, 0, 0) + normalised_hash_internal(group, identifier, modulus, 0) } const VARIANT_NORMALIZATION_SEED: u32 = 86028157; @@ -133,7 +133,7 @@ pub fn normalised_variant_hash( identifier: &str, modulus: u32, ) -> std::io::Result { - normalised_hash_internal(group, identifier, modulus, VARIANT_NORMALIZATION_SEED, 1) + normalised_hash_internal(group, identifier, modulus, VARIANT_NORMALIZATION_SEED) } fn normalised_hash_internal( @@ -141,7 +141,6 @@ fn normalised_hash_internal( identifier: &str, modulus: u32, seed: u32, - additional: u32, ) -> std::io::Result { // See https://github.com/stusmall/murmur3/pull/16 : .chain may avoid // copying in the general case, and may be faster (though perhaps @@ -149,7 +148,7 @@ fn normalised_hash_internal( // path non-obvious) - but until murmur3 is fixed, we need to provide it // with a single string no matter what. let mut reader = Cursor::new(format!("{}:{}", &group, &identifier)); - murmur3_32(&mut reader, seed).map(|hash_result| (hash_result % modulus) + additional) + murmur3_32(&mut reader, seed).map(|hash_result| hash_result % modulus + 1) } // Build a closure to handle session id rollouts, parameterised by groupId and a