Skip to content
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: replace weight calculation with a rounded ceil #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

samc
Copy link

@samc samc commented Nov 3, 2023

Looks like we stumbled across a bug in our production middleware that was resulting in some unwarranted internal service errors - looks to us like there's an overflow in the min / max value calculation for random mutually exclusive groups.

I'll use an example from our production code that was giving us trouble to explain the problem.

  1. We have a mutually exclusive group (random assignment) with exactly 6 tenants
  2. shortlistedCampaigns would then have their weights adjusted to 16 based on the following:

campaign.weight = Math.floor(100 / shortlistedCampaigns.length);

3) based on that weight, the following would adjust the lower and upper bounds of startVariationAllocation & endVariationAllocation respectively:

const startRange = Math.ceil(variationWeight * 100);

4) this all results in a maximum upper bound of 9600 when it comes time to decide on a variation:

if (bucketValue >= variation.startVariationAllocation && bucketValue <= variation.endVariationAllocation) {
return variation;
}

  1. which, looking at the algo from _generateBucketValue, looks to default to a max of 10000 based on Constants.MAX_TRAFFIC_PERCENT. This leaves a 4% chance that an assigned bucketed value falls outside the acceptable bounds of all active tests in a group and returns null - failing on this line:

campaignKey: winnerCampaign.key

Here's a snapshot of the overflow in action on our production site:

image

@samc samc marked this pull request as ready for review November 3, 2023 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant