-
Notifications
You must be signed in to change notification settings - Fork 71
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
fix: new variant hashing #533
Conversation
@@ -1,5 +1,23 @@ | |||
import * as murmurHash3 from 'murmurhash3js'; | |||
|
|||
export default function normalizedValue(id: string, groupId: string, normalizer = 100): number { | |||
return (murmurHash3.x86.hash32(`${groupId}:${id}`) % normalizer) + 1; | |||
function normalizedValue(id: string, |
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.
making this private since I want other code to use strategy or variant specific functions
const STRATEGY_SEED = 0; | ||
|
||
export function normalizedStrategyValue(id: string, groupId: string): number { | ||
return normalizedValue(id, groupId, 100, STRATEGY_SEED); |
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.
It looks like existing code always assumes 100 for strategies
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 believe we do, because the percentage slider for our gradualRollouts are 0-100 %
|
||
export function normalizedVariantValue(id: string, | ||
groupId: string, | ||
normalizer: number): number { |
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.
for variants we want to force the normalizer usage by the calling code
return normalizedValue(id, groupId, 100, STRATEGY_SEED); | ||
} | ||
|
||
const VARIANT_SEED = 86028157; |
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 deliberate don't want this to leak out to the calling code
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.
Nice.
What
Uses a new seed for ensuring a fair distribution for variants.
Background
After a customer reported that variant distribution seemed skewed we performed some testing and found that since we use the same hash string for both gradual rollout and variant allocation we'd reduced the set of groups we could get to whatever percentage our gradual rollout was set.
Example
Take a gradualRollout of 10%, this will select normalized hashes between 1 and 10, when we then again hash the same string that gave us between 1 and 10, but with modulo 1000 for variants, this will only give us 100 possible groups, instead of the expected 1000.
Fix
Force the normalization to accept a seed, and make sure to use a new seed when normalizing the variant distribution hash.
Worth noting
This will require release of a new major version, since we are changing how hashing works.