feat: Add fractional evaluation for segments #45
There are no files selected for viewing
liamhughes marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| namespace System.Runtime.CompilerServices; | ||
|
|
||
| internal static class IsExternalInit; | ||
liamhughes marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| using System.Buffers.Binary; | ||
| using System.Text; | ||
| using System.Text.RegularExpressions; | ||
| using Microsoft.Extensions.Logging; | ||
| using Murmur; | ||
| using OpenFeature.Constant; | ||
| using OpenFeature.Model; | ||
|
|
||
|
|
@@ -41,6 +44,12 @@ | |
| "The slug provided did not match any of your Octopus Feature Toggles. Please double check your slug and try again."); | ||
| } | ||
|
|
||
| if (MissingRequiredPropertiesForClientSideEvaluation(feature)) | ||
| { | ||
| return new ResolutionDetails<bool>(slug, defaultValue, ErrorType.ParseError, | ||
| $"Feature toggle {slug} is missing necessary information for client-side evaluation."); | ||
| } | ||
|
|
||
| return new ResolutionDetails<bool>(slug, Evaluate(feature, context)); | ||
| } | ||
|
|
||
|
|
@@ -63,7 +72,52 @@ | |
|
|
||
| bool Evaluate(FeatureToggleEvaluation evaluation, EvaluationContext? context = null) | ||
| { | ||
| if (!evaluation.IsEnabled) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| var targetingKey = context?.TargetingKey; | ||
| if (targetingKey == null || targetingKey == "") | ||
|
Contributor
Author
There was a problem hiding this comment. I swapped that out for this as I was getting a null reference warning on |
||
| { | ||
| if (evaluation.ClientRolloutPercentage < 100) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| if (GetNormalizedNumber(evaluation.EvaluationKey, targetingKey) > evaluation.ClientRolloutPercentage) | ||
|
Check warning on line 90 in src/Octopus.OpenFeature.Provider/OctopusFeatureContext.cs
|
||
| { | ||
| return false; // return false if hash number is larger than rollout percentage | ||
| } | ||
| } | ||
dylanlerch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. I considered placing the rollout percentage evaluation logic below the call to MatchesSegment, but I thought that MatchesSegment might actually be the most time intensive of the two 👇
Contributor
There was a problem hiding this comment. We may end up switching this around once we add reasons. For example, we need the reason that it does (or doesn't) get a toggle to be consistent between all libraries. For now we don't see this, so it's fine. |
||
| return evaluation.Segments.Length == 0 || MatchesSegment(context, evaluation.Segments); | ||
|
Check warning on line 96 in src/Octopus.OpenFeature.Provider/OctopusFeatureContext.cs
|
||
| } | ||
|
|
||
| /// <summary> | ||
| /// Computes a deterministic integer bucket in the inclusive range 1–100 for the given evaluation and targeting keys. | ||
| /// </summary> | ||
| private int GetNormalizedNumber(string evaluationKey, string targetingKey) | ||
| { | ||
| var bytes = Encoding.UTF8.GetBytes(string.Concat(evaluationKey, ":", targetingKey)); | ||
|
|
||
| using var algorithm = MurmurHash.Create32(); | ||
| var hash = algorithm.ComputeHash(bytes); | ||
|
Comment on lines
+111
to
+112
There was a problem hiding this comment. Creating and disposing a hash algorithm instance on every evaluation can add avoidable overhead if flags are evaluated frequently. Consider using a static/thread-local approach (if the MurmurHash implementation is not thread-safe) or a reusable API provided by the library to compute the 32-bit hash without per-call allocations.
Contributor
Author
There was a problem hiding this comment. Is this really a problem? I don't think that creating the hash algorithm instance will take a lot of time, right?
Contributor
There was a problem hiding this comment. Quick poke around the Murmur code and it doesn't look like it allocates a lot, but also it couldn't hurt to make it a
Contributor
Author
There was a problem hiding this comment. @liamhughes Does this look like what you would expect?
Contributor
There was a problem hiding this comment. Yep. Don't think you need the |
||
| // Explicitly little-endian to ensure consistent int values across all client libraries. | ||
| var value = BinaryPrimitives.ReadUInt32LittleEndian(hash); | ||
| return (int)(value % 100 + 1); | ||
| } | ||
|
|
||
|
|
||
| private static bool MissingRequiredPropertiesForClientSideEvaluation(FeatureToggleEvaluation evaluation) | ||
| { | ||
| if (!evaluation.IsEnabled) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return evaluation.ClientRolloutPercentage is null || evaluation.EvaluationKey is null || evaluation.Segments is null; | ||
| } | ||
| } | ||
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 was having trouble running the tests locally, but that may have been because Caitlyn and I were editing the submodule files at the same time?
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.
Fine with me 👍