-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: create a Rust sample plugin for A/B testing #140
Conversation
Signed-off-by: pweiber <[email protected]>
Signed-off-by: pweiber <[email protected]>
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
# Conflicts: # plugins/samples/ab_testing/BUILD
Signed-off-by: pweiber <[email protected]>
fn should_use_v2(user: &str) -> bool { | ||
// Calculate a deterministic hash from user. | ||
// Sum the ASCII values of the characters and then offset by length. | ||
let sum: u64 = user.bytes().map(u64::from).sum(); |
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.
Is there a reason why we aren't using the default hashing implementation for slices in Rust? https://doc.rust-lang.org/stable/std/hash/trait.Hash.html#method.hash_slice
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 did this way because the tests were based on the c++ code, for one of them if I used the default hashing I was getting different values
for the "WithHashAboveThresholdKeepTheSame"
C++ computes std::hash("userAAA") % 100 = 63 (63 > 50 → no redirect).
Rust’s DefaultHasher produces a different value (e.g., hash % 100 ≤ 50 → redirect).
Using like this I'm getting
// For "userAAA": 642 + 21 = 663 → 63%100
Let me know if it is fine to keep it like this or if we should adjust for both
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.
Sounds good. Maybe add a comment saying why were are hashing by hand instead of using the rust standard library.
fn should_use_v2(user: &str) -> bool { | ||
// Calculate a deterministic hash from user. | ||
// Sum the ASCII values of the characters and then offset by length. | ||
let sum: u64 = user.bytes().map(u64::from).sum(); |
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.
Sounds good. Maybe add a comment saying why were are hashing by hand instead of using the rust standard library.
@dallen68 Let me know if the comment is fine like that or if we should change something |
Rust version for the A/B testing.
If the hash value is less than or equal to the percentile, the request is served by v2 file. Otherwise, it is served by its original file.