Skip to content

Conversation

bacherfl
Copy link
Contributor

This PR adds support for relative weights in the fractional evaluator. This new (non breaking) feature has been proposed in open-feature/flagd#1282 (The related PR to implement this in flagd is this one: open-feature/flagd#1313).
In addition to supporting relative weights instead of percentages, the weight value for a distribution item can be omitted. In this case, a default weight of 1 will be applied.

Leaving this in draft until the related PR in flagd has been merged

@github-actions github-actions bot requested a review from toddbaert May 23, 2024 11:05
@toddbaert toddbaert force-pushed the feat/fractional-op-relative-weights branch from 506724a to 7ebf869 Compare June 27, 2024 16:44
@toddbaert toddbaert marked this pull request as ready for review June 27, 2024 18:53
@toddbaert toddbaert requested review from a team as code owners June 27, 2024 18:53
@toddbaert
Copy link
Member

@aepfli Not sure if .NET is your thing, but since you did a few others, could you quickly skim this?

Copy link
Member

@aepfli aepfli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good from my side, i do have a little comment regarding the tests, but this is more like a little nit or improvement for the future


[Theory]
[MemberData(nameof(FractionalEvaluationTestData.FractionalEvaluationContext), MemberType = typeof(FractionalEvaluationTestData))]
public void EvaluateUsingRelativeWeights(string email, string flagKey, string expected)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should evaluate if it makes sense to move the json tests of the java contrib sdk from https://github.com/open-feature/java-sdk-contrib/tree/main/providers/flagd/src/test/resources/fractional to the harness, and use them as test for all our implementation of the fractionals. maybe we are still missing some data, which is part of the test code, but i think this would allow us to be more stable throughout all the packages. Normally there should be a way in each of our contrib sdks to load json files and to parameterize tests in some sort of way (assumption for dotnet)

@toddbaert toddbaert force-pushed the feat/fractional-op-relative-weights branch from 7ebf869 to 3b61919 Compare July 3, 2024 17:36
@toddbaert
Copy link
Member

I found a couple small issues with this when adding some additional e2e tests, working on some fixes.

@toddbaert toddbaert self-requested a review July 3, 2024 19:45
@toddbaert toddbaert force-pushed the feat/fractional-op-relative-weights branch from 9d6d572 to edd04f0 Compare July 4, 2024 01:42
@toddbaert
Copy link
Member

toddbaert commented Jul 4, 2024

@bacherfl I made a couple minor tweaks to the algorithm; we had a slight deviation when no bucking value was supplied. You can see them here. Also, this was fun.

I also added the e2e tests for this new functionality.

Thanks so much!

@toddbaert toddbaert force-pushed the feat/fractional-op-relative-weights branch from a8b55a0 to 132ab9d Compare July 4, 2024 02:20
{
//object value;
if (from is Dictionary<string, object> dict)
if (from is IDictionary<string, object> dict)
Copy link
Member

@toddbaert toddbaert Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stumped me for a moment. In prod, from here implements IDictionary, but is not actually an instance of Dictionary, so this evaluated to false, resulting in no injected props. This fixes it!

@toddbaert toddbaert changed the title feat: add support for relative weights in fractional evaluator feat: relative weights in fractional, fix injected props Jul 4, 2024
@toddbaert toddbaert merged commit 7cccc8d into main Jul 4, 2024
@askpt askpt deleted the feat/fractional-op-relative-weights branch April 21, 2025 20:45
weyert pushed a commit to weyert/dotnet-sdk-contrib that referenced this pull request Jun 19, 2025
…e#208)

Signed-off-by: Florian Bacher <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Todd Baert <[email protected]>
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.

3 participants