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

feat: relative weights in fractional, fix injected props #208

Merged
merged 3 commits into from
Jul 4, 2024

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
@@ -17,15 +17,15 @@ internal class FlagdProperties
internal FlagdProperties(object from)
{
//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
9 checks passed
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