Flagsmith / flagsmith

Open Source Feature Flagging and Remote Config Service. Host on-prem or use our hosted version at https://flagsmith.com/
https://flagsmith.com/
BSD 3-Clause "New" or "Revised" License
4.9k stars 375 forks source link

Segment percentage split evaluation is not consistent between Core API and local evaluation #2186

Open matthewelwell opened 1 year ago

matthewelwell commented 1 year ago

Describe the bug

Identities are evaluated differently for segments containing a percentage split rule when evaluated against the Core API versus local evaluation mode in SDKs.

To Reproduce

Steps to reproduce the behavior:

  1. Run the Core API (e.g. self hosted)
  2. Create a segment with a percentage split rule of 50% and add a segment override to a basic feature in a given environment
  3. Instantiate two instances of a client (e.g. Node.js), one running in local evaluation mode and one in remote
  4. Evaluate the flag for multiple identities in both instances of the client and compare the results
  5. Notice that the state of the flag can differ for certain identities between local and remote evaluation

Expected behavior

The result is the same for both local and remote evaluation for all identities

Screenshots

N/a

How are you running Flagsmith?

Additional context

This will affect the following clients when running in local evaluation mode:

Note that python, ruby, rust are not affected as they will use the django id if it exists.

matthewelwell commented 1 year ago

To fix this, we have a couple of options:

  1. Change the SDKs listed above to use the django id if present
  2. Change the API to use the composite key and change those that are 'not affected' to always use the composite key as well

Option (1) is by far the simplest option but is slightly less clean than option 2. It also somewhat diverges from the logic that was implemented for MV V2 evaluations (details of which can be found here)

khvn26 commented 1 year ago

I'm in favour of 2. FWIW it's an API change + 3 SDKs vs 6 SDKs so simplicity of Option (1) isn't that apparent for me?

matthewelwell commented 1 year ago

The ability to major version the SDKs is much easier than the API, however and I would suggest that a change in behaviour like this would require a major version - or an approach similar to the one done for #1882 where users need to opt in to the consistent evaluation on the API.

dabeeeenster commented 1 year ago

If the some of the other languages are already doing option 1 I think lets stick with 1? This avoids API versioning and makes the SDKs consistent with each other?

matthewelwell commented 1 year ago

PHP PR: https://github.com/Flagsmith/flagsmith-php-client/pull/48

matthewelwell commented 1 year ago

Node.js PR: https://github.com/Flagsmith/flagsmith-nodejs-client/pull/119

khvn26 commented 1 year ago

Golang PR: https://github.com/Flagsmith/flagsmith-go-client/pull/88

khvn26 commented 1 year ago

.NET PR: https://github.com/Flagsmith/flagsmith-dotnet-client/pull/69

khvn26 commented 1 year ago

Java PR: https://github.com/Flagsmith/flagsmith-java-client/pull/122

matthewelwell commented 1 year ago

I've just realised that using the DjangoId attribute is not a valid solution to this issue since the identity information from the Core API is, in fact, never passed to the SDKs, particularly in local evaluation mode.

More research needed into the scope of the issue and a new solution.

matthewelwell commented 1 year ago

The only solution I can see here is to ensure that all points at which percentage split evaluations are carried out, we use the composite key since this is the only relevant data that all evaluation engines will always have.

To achieve this, we should also have a setting on the environment which changes to use the composite key across the board. Essentially replicating the logic we already have for use_mv_v2_evaluation.

matthewelwell commented 1 year ago

To confirm, we only need to make this change in the Core API. All the clients are now consistent that they use the Django ID if it exists, otherwise, they use the composite key.

matthewelwell commented 11 months ago

Latest update: percentage split is still prioritising the django_id in our Core API, even if using consistent hashing is enabled. Since v2.85.0 we are using the flagsmith engine for evaluating segments and hence will need to make the change in the engine to verify if the environment is configured to use consistent hashing.