feat: Add fractional evaluation for segments#45
Conversation
| <ItemGroup> | ||
| <None Include="..\..\specification\Fixtures\*.json" Link="Fixtures\%(RecursiveDir)%(Filename)%(Extension)"> | ||
| <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> | ||
| <CopyToOutputDirectory>Always</CopyToOutputDirectory> |
There was a problem hiding this comment.
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.
Fine with me 👍
There was a problem hiding this comment.
Pull request overview
Adds client-side fractional rollout evaluation for feature toggles (based on targeting key hashing) and updates the toggle evaluations API/fixtures accordingly.
Changes:
- Implement fractional (percentage-based) client-side evaluation using MurmurHash-derived bucketing.
- Update feature toggle evaluation models and switch API endpoint to
/api/toggles/evaluations/v3/. - Update specification fixtures/submodule handling to ensure the latest fixtures are used during tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Octopus.OpenFeature.Provider/OctopusFeatureContext.cs | Adds rollout percentage + targeting key hashing logic for evaluation. |
| src/Octopus.OpenFeature.Provider/OctopusFeatureClient.cs | Updates evaluation DTOs and switches to new evaluations API route. |
| src/Octopus.OpenFeature.Provider/Octopus.OpenFeature.Provider.csproj | Adds MurmurHash dependency for rollout bucketing. |
| src/Octopus.OpenFeature.Provider/IsExternalInit.cs | Adds init-only support shim for records on older target frameworks. |
| src/Octopus.OpenFeature.Provider.Tests/OctopusFeatureContextTests.cs | Updates unit tests to new evaluation DTO shape. |
| src/Octopus.OpenFeature.Provider.Tests/OctopusFeatureContextProviderTests.cs | Updates provider tests to new evaluation DTO shape. |
| src/Octopus.OpenFeature.Provider.SpecificationTests/Server.cs | Updates WireMock route to the new evaluations endpoint. |
| src/Octopus.OpenFeature.Provider.SpecificationTests/Octopus.OpenFeature.Provider.SpecificationTests.csproj | Forces fixture copy for spec tests to always use latest JSON fixtures. |
| specification | Bumps spec submodule pointer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /// <summary> | ||
| /// Produces a normalized number between 1 and 100 for a given TargetingKey, with less than 1% variance |
There was a problem hiding this comment.
The docstring is inaccurate/misleading as written: the method returns an integer bucket in the inclusive range 1..100, and modulo-based mapping introduces a very small bias (not 'less than 1% variance'). Update the comment to describe the bucketing approach and range precisely (and, if needed, note that any bias is negligible).
| /// Produces a normalized number between 1 and 100 for a given TargetingKey, with less than 1% variance | |
| /// Computes a deterministic integer bucket in the inclusive range 1–100 for the given evaluation and targeting keys. | |
| /// The bucket is derived by hashing "evaluationKey:targetingKey" with Murmur32 and mapping the result via modulo 100; | |
| /// this yields an approximately uniform distribution across buckets with a very small, negligible bias. |
There was a problem hiding this comment.
What do you think, @dylanlerch? I'm copying your comment on the same function in the OctoToggle code.
There was a problem hiding this comment.
I think it's probably a more accurate description of it.
There was a problem hiding this comment.
The first line probably will do though.
| using var algorithm = MurmurHash.Create32(); | ||
| var hash = algorithm.ComputeHash(bytes); |
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.
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?
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 ThreadLocal?
There was a problem hiding this comment.
@liamhughes Does this look like what you would expect?
842a037
There was a problem hiding this comment.
Yep.
Don't think you need the null suppression !?
|
|
||
| using var algorithm = MurmurHash.Create32(); | ||
| var hash = algorithm.ComputeHash(bytes); | ||
| var value = BitConverter.ToUInt32(hash, 0); |
There was a problem hiding this comment.
BitConverter.ToUInt32 is endianness-dependent. While most current .NET runtimes are little-endian, using an explicit endianness read (e.g., via System.Buffers.Binary.BinaryPrimitives) makes rollout bucketing deterministic across any platform/runtime and avoids hard-to-debug rollout inconsistencies.
There was a problem hiding this comment.
Great point. I've changed this to do an explicitly little-endian read. We should repeat this across all the client libraries.
| return false; // return false if hash number is larger than rollout percentage | ||
| } | ||
| } | ||
|
|
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 👇
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.
41ca6f4 to
964a3cf
Compare
| { | ||
| return false; | ||
| } | ||
| if (evaluation.EvaluationKey == null) |
There was a problem hiding this comment.
This is checked in MissingRequiredPropertiesForClientSideEvaluation, but I suppose the compiler might not be smart enough to track that through to here?
| using var algorithm = MurmurHash.Create32(); | ||
| var hash = algorithm.ComputeHash(bytes); |
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 ThreadLocal?
src/Octopus.OpenFeature.Provider.Tests/OctopusFeatureContextTests.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| var targetingKey = context?.TargetingKey; | ||
| if (targetingKey == null || targetingKey == "") |
There was a problem hiding this comment.
I swapped that out for this as I was getting a null reference warning on targetingKey in the else block below.
3ce6f2a to
1f16d35
Compare
1f16d35 to
7faef02
Compare
src/Octopus.OpenFeature.Provider.Tests/OctopusFeatureContextTests.cs
Outdated
Show resolved
Hide resolved
| partial class OctopusFeatureContext(FeatureToggles toggles, ILoggerFactory loggerFactory) | ||
| { | ||
| public byte[] ContentHash => toggles.ContentHash; | ||
| static readonly ThreadLocal<HashAlgorithm> RolloutHashAlgorithm = new(() => MurmurHash.Create32()); |
There was a problem hiding this comment.
I'm curious about this. Is there any more information on why this is necessary? The need for the ! in RolloutHashAlgorithm.Value!.ComputeHash(bytes); makes me a little suspect.
There was a problem hiding this comment.
FWIW, I don't think the null forgiving operator ! is actually required.
| public void WhenTargetingKeyFallsWithinRolloutPercentage_AndSegmentValueDoesNotMatchRequiredSegment_EvaluatesToFalse() | ||
| { | ||
| var featureToggles = new FeatureToggles([ | ||
| new FeatureToggleEvaluation("test-feature", true, "evaluation-key", [new("license", "enterprise")], 13) |
There was a problem hiding this comment.
Could go with something higher (like 99) as the percentage here to make sure it is really because of the segment (but still also evaluating the percentage).
dylanlerch
left a comment
There was a problem hiding this comment.
Looks good. I've tested this against live toggle service and all works as expected.

👥 This PR is a team effort combo move by @liamhughes and myself.
Background 🌇
OctoToggle allows customers to rollout to a given percentage of allowed Tenants. To do this, we compare the rollout percentage to an integer from 0 to 100 generated from each tenant's ID and evaluation key.
Our OpenFeature provider implementations allow customers to rollout to a list of allowed Segments. We would now like to add the functionality to rollout to a given percentage of allowed Segments following the same logic used for Tenant rollout in OctoToggle.
As Segment rollout is happening in the provider, rather than OctoToggle, it is important that each provider gives a consistent toggle status for each Segment.
What's this? 🌵
The primary purpose of this PR is to enable us to rollout to a given percentage of Segments.
To set up for this we updated the
FeatureToggleEvaluationclass to contain anEvaluationKeyfield andClientRolloutPercentagefield.We are determining whether a Segment is within the rollout percentage by:
〰️ Creating a string from the
EvaluationKeyand the evaluation context'sTargetingKey〰️ Computing a hash from that string
〰️ Converting the hash to an unsigned integer
〰️ Converting the integer to a value between 1 and 100
〰️ Comparing that value to the rollout percentage specified on the feature toggle.
This follows the same logic that we use for tenant rollout evaluation in OctoToggle.
If a segment is within the rollout percentage, we continue on with the existing logic which compares it against the segment key-values required by the toggle.
What else is in here? 🎈
As I was adding Segment rollout, Liam was also adding a new evaluations endpoint to OctoToggle. This PR integrates those changes:
〰️ We now call
/api/toggles/evaluations/v3/in place of/api/toggles/evaluations/v3/〰️ The
FeatureToggleEvaluationclass has been updated to match the new endpoint's response shape. I.e. theNamefield is removed, theEvaluationKeyandClientRolloutPercentagefields are added, and a nullableSegmentfield can now be handled.How to review? 🔍
☑️ Apologies that this PR has become a bit tricky to follow! We thought that adding the two sets of changes together would be simpler than rebasing my changes onto Liam's.
💬 Check out the comments on the code.
🧪You can run the accompanying tests by pointing the specifications submodule to the latest commit on this PR.
Part of DEVEX-79
Part of DEVEX-105