-
Notifications
You must be signed in to change notification settings - Fork 108
feat!: fractional bucketing improvements #1909
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,23 +9,27 @@ | |
| "github.com/twmb/murmur3" | ||
| ) | ||
|
|
||
| const maxWeightSum = math.MaxInt32 // 2,147,483,647 | ||
|
|
||
| const FractionEvaluationName = "fractional" | ||
|
|
||
| type Fractional struct { | ||
| Logger *logger.Logger | ||
| } | ||
|
|
||
| type fractionalEvaluationDistribution struct { | ||
| totalWeight int | ||
| totalWeight int32 | ||
| weightedVariants []fractionalEvaluationVariant | ||
| data any | ||
| logger *logger.Logger | ||
| } | ||
|
|
||
| type fractionalEvaluationVariant struct { | ||
| variant string | ||
| weight int | ||
| variant any // string, bool, number or nil | ||
| weight int32 | ||
| } | ||
|
|
||
| func (v fractionalEvaluationVariant) getPercentage(totalWeight int) float64 { | ||
| func (v fractionalEvaluationVariant) getPercentage(totalWeight int32) float64 { | ||
| if totalWeight == 0 { | ||
| return 0 | ||
| } | ||
|
|
@@ -38,16 +42,17 @@ | |
| } | ||
|
|
||
| func (fe *Fractional) Evaluate(values, data any) any { | ||
| valueToDistribute, feDistributions, err := parseFractionalEvaluationData(values, data) | ||
| valueToDistribute, feDistributions, err := parseFractionalEvaluationData(values, data, fe.Logger) | ||
| if err != nil { | ||
| fe.Logger.Warn(fmt.Sprintf("parse fractional evaluation data: %v", err)) | ||
| return nil | ||
| } | ||
|
|
||
| return distributeValue(valueToDistribute, feDistributions) | ||
| hashValue := uint32(murmur3.StringSum32(valueToDistribute)) | ||
| return distributeValue(hashValue, feDistributions) | ||
|
Comment on lines
+51
to
+52
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| func parseFractionalEvaluationData(values, data any) (string, *fractionalEvaluationDistribution, error) { | ||
| func parseFractionalEvaluationData(values, data any, logger *logger.Logger) (string, *fractionalEvaluationDistribution, error) { | ||
| valuesArray, ok := values.([]any) | ||
| if !ok { | ||
| return "", nil, errors.New("fractional evaluation data is not an array") | ||
|
|
@@ -61,9 +66,8 @@ | |
| return "", nil, errors.New("data isn't of type map[string]any") | ||
| } | ||
|
|
||
| // Ignore the error as we can't really do anything if the properties are | ||
| // somehow missing. | ||
| properties, _ := getFlagdProperties(dataMap) | ||
| flagKey := properties.FlagKey | ||
|
|
||
| bucketBy, ok := valuesArray[0].(string) | ||
| if ok { | ||
|
|
@@ -76,73 +80,123 @@ | |
|
|
||
| targetingKey, ok := dataMap[targetingKeyKey].(string) | ||
| if !ok { | ||
| return "", nil, errors.New("bucketing value not supplied and no targetingKey in context") | ||
| return "", nil, fmt.Errorf("flag %q: bucketing value not supplied and no targetingKey in context", flagKey) | ||
| } | ||
|
|
||
| bucketBy = fmt.Sprintf("%s%s", properties.FlagKey, targetingKey) | ||
| } | ||
|
|
||
| feDistributions, err := parseFractionalEvaluationDistributions(valuesArray) | ||
| feDistributions, err := parseFractionalEvaluationDistributions(valuesArray, data, logger, flagKey) | ||
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
|
|
||
| return bucketBy, feDistributions, nil | ||
| } | ||
|
|
||
| func parseFractionalEvaluationDistributions(values []any) (*fractionalEvaluationDistribution, error) { | ||
| func parseFractionalEvaluationDistributions(values []any, data any, logger *logger.Logger, flagKey string) (*fractionalEvaluationDistribution, error) { | ||
|
Check failure on line 97 in core/pkg/evaluator/fractional.go
|
||
| feDistributions := &fractionalEvaluationDistribution{ | ||
| totalWeight: 0, | ||
| weightedVariants: make([]fractionalEvaluationVariant, len(values)), | ||
| data: data, | ||
| logger: logger, | ||
| } | ||
|
|
||
| // parse all weights first to validate the sum | ||
| var totalWeightInt64 int64 = 0 | ||
|
|
||
| for i := 0; i < len(values); i++ { | ||
| distributionArray, ok := values[i].([]any) | ||
| if !ok { | ||
| return nil, errors.New("distribution elements aren't of type []any. " + | ||
| "please check your rule in flag definition") | ||
| return nil, fmt.Errorf("flag %q: distribution elements aren't of type []any. "+ | ||
| "please check your rule in flag definition", flagKey) | ||
| } | ||
|
|
||
| if len(distributionArray) == 0 { | ||
| return nil, errors.New("distribution element needs at least one element") | ||
| return nil, fmt.Errorf("flag %q: distribution element needs at least one element", flagKey) | ||
| } | ||
|
|
||
| variant, ok := distributionArray[0].(string) | ||
| if !ok { | ||
| return nil, errors.New("first element of distribution element isn't string") | ||
| // JSONLogic pre-evaluates all arguments before they reach fractional. | ||
| // Pre-evaluated operators become primitive values (strings, numbers, etc.), never map[string]any nodes. | ||
| var variant any | ||
| switch v := distributionArray[0].(type) { | ||
| case string: | ||
| variant = v | ||
| case bool: | ||
| variant = v | ||
| case float64: | ||
| variant = v | ||
| case nil: | ||
| variant = nil | ||
| default: | ||
| return nil, fmt.Errorf("flag %q: first element of distribution element must be a string, bool, number, or nil", flagKey) | ||
| } | ||
|
|
||
| weight := 1.0 | ||
| weight := int64(1) | ||
| if len(distributionArray) >= 2 { | ||
| // parse as float64 first since that's what JSON gives us | ||
| distributionWeight, ok := distributionArray[1].(float64) | ||
| if !ok && distributionArray[1] != nil { | ||
toddbaert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return nil, fmt.Errorf("flag %q: weight must be a number", flagKey) | ||
| } | ||
| if ok { | ||
| // default the weight to 1 if not specified explicitly | ||
| weight = distributionWeight | ||
| weight = int64(distributionWeight) | ||
| } | ||
| } | ||
|
|
||
| // validate weight is a whole number | ||
| if len(distributionArray) >= 2 { | ||
| distributionWeight, ok := distributionArray[1].(float64) | ||
| if ok && distributionWeight != float64(int64(distributionWeight)) { | ||
| return nil, fmt.Errorf("flag %q: weights must be integers", flagKey) | ||
| } | ||
| } | ||
|
|
||
| feDistributions.totalWeight += int(weight) | ||
| // validate individual weight doesn't exceed int32 | ||
| if weight > math.MaxInt32 { | ||
| return nil, fmt.Errorf("flag %q: weight %d exceeds maximum allowed value %d", flagKey, weight, math.MaxInt32) | ||
| } | ||
|
|
||
| // clamp negative weights to 0 | ||
| if weight < 0 { | ||
| // negative weights can be the result of rollout calculations, so we log and clamp to 0 rather than returning an error | ||
| logger.Debug(fmt.Sprintf("flag %q: negative weight %d clamped to 0", flagKey, weight)) | ||
| weight = 0 | ||
| } | ||
|
|
||
| totalWeightInt64 += weight | ||
| feDistributions.weightedVariants[i] = fractionalEvaluationVariant{ | ||
| variant: variant, | ||
| weight: int(weight), | ||
| weight: int32(weight), | ||
| } | ||
| } | ||
|
|
||
| // validate total weight doesn't exceed MaxInt32 | ||
| if totalWeightInt64 > int64(maxWeightSum) { | ||
| return nil, fmt.Errorf("flag %q: sum of all weights (%d) exceeds maximum allowed value (%d)", flagKey, totalWeightInt64, maxWeightSum) | ||
| } | ||
|
|
||
| feDistributions.totalWeight = int32(totalWeightInt64) | ||
| return feDistributions, nil | ||
| } | ||
|
|
||
| // distributeValue calculate hash for given hash key and find the bucket distributions belongs to | ||
| func distributeValue(value string, feDistribution *fractionalEvaluationDistribution) string { | ||
| hashValue := int32(murmur3.StringSum32(value)) | ||
| hashRatio := math.Abs(float64(hashValue)) / math.MaxInt32 | ||
| bucket := hashRatio * 100 // in range [0, 100] | ||
| // distributeValue accepts a pre-computed 32-bit hash value and distributes it to a variant using high-precision integer arithmetic. | ||
| // It maps a 32-bit hash to the range [0, totalWeight) and finds the variant bucket that contains that value. | ||
| func distributeValue(hashValue uint32, feDistribution *fractionalEvaluationDistribution) any { | ||
| if feDistribution.totalWeight == 0 { | ||
| return "" | ||
| } | ||
|
|
||
| bucket := (uint64(hashValue) * uint64(feDistribution.totalWeight)) >> 32 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weird that gemini seems to just be making a lot of these observational comment lately. |
||
|
|
||
| rangeEnd := float64(0) | ||
| for _, weightedVariant := range feDistribution.weightedVariants { | ||
| rangeEnd += weightedVariant.getPercentage(feDistribution.totalWeight) | ||
| var rangeEnd uint64 = 0 | ||
| for _, variant := range feDistribution.weightedVariants { | ||
| rangeEnd += uint64(variant.weight) | ||
| if bucket < rangeEnd { | ||
| return weightedVariant.variant | ||
| return variant.variant | ||
| } | ||
| } | ||
|
|
||
| // unreachable given validation | ||
| return "" | ||
| } | ||
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.
The
fe.Loggeris passed as an argument toparseFractionalEvaluationData. This is good for testability and dependency injection.