[FEATURE] Specify custom seed for fractional operator

[colebaileygit]

Requirements

Currently, the fractional implementation defaults to using $flagd.flagKey as prefix for the hash key to seed the hash. This is a good default for running independent experiments across multiple feature flags at the same time.

We would also like to be able to correlate multiple feature flags / systems to a single experiment, meaning the same user should be allocated to the same variant group across multiple flags and hence the same seed must be used. For this reason we would not want to use flagKey as a seed, but some unique experimentId injected into the flag targeting.

My proposal to make this possible while keeping backwards compatibility would be to allow an optional custom seed as second argument to fractional, e.g.:

// Custom static seed
"targeting": {
  "fractional": [
    { "var": "user.name" },
    "experiment-123",
    [ "clubs", 25 ],
    [ "diamonds", 25 ],
    [ "hearts", 25 ],
    [ "spades", 25 ]
  ]
}

// Custom seed from context
"targeting": {
  "fractional": [
    { "var": "user.name" },
    { "var": "populationSeed" },
    [ "clubs", 25 ],
    [ "diamonds", 25 ],
    [ "hearts", 25 ],
    [ "spades", 25 ]
  ]
}

// Status quo: custom key with default flagKey seed
"targeting": {
  "fractional": [
    { "var": "user.name" },
    [ "clubs", 25 ],
    [ "diamonds", 25 ],
    [ "hearts", 25 ],
    [ "spades", 25 ]
  ]
}

// Status quo: default targetingKey with default flagKey seed
"targeting": {
  "fractional": [
    [ "clubs", 25 ],
    [ "diamonds", 25 ],
    [ "hearts", 25 ],
    [ "spades", 25 ]
  ]
}

One downside to using positional arguments like this is there would be no way to provide a custom seed but also use the default targetingKey since the 1st argument will be interpreted as a hash key. However this has the simple workaround of explicitly giving the default value:

// default targetingKey with custom seed
"targeting": {
  "fractional": [
    { "var": "targetingKey" },
    "experiment-123",
    [ "clubs", 25 ],
    [ "diamonds", 25 ],
    [ "hearts", 25 ],
    [ "spades", 25 ]
  ]
}

Tasks

Beta Give feedback

1. feat!: allow custom seed when using targetingKey override for fractional op #1266
   +0
2. feat: allow custom seed in fractional op flagd-schemas#136
   +0
3. feat: test multiple fractional flags with shared seed flagd-testbed#115
   +0
4. feat!: allow overrides for fractional seed java-sdk-contrib#737
   +0
   

   
5. feat: in-process offline flagd resolver python-sdk-contrib#74
   +0
6. feat!: allow overrides for fractional seed js-sdk-contrib#870
   +0
   

[colebaileygit]

I took a stab at making this possible in flagd core -- see reference PRs above

This would also require implementing same behavior in each language's in-memory provider which should be pretty straightforward. (And of course updating more docs 😃) Happy to proceed further if this proposal makes sense

[toddbaert]

This sounds like a reasonable idea, and I appreciate your pr! I'll give it a proper look tomorrow.

[beeme1mr]

Hey @colebaileygit, this is great. We had discussions about this in the past and I actually thought it was already possible 😅. Our idea back in the day was basically the same as what you're proposing. If @toddbaert agrees, perhaps we start by updating flagd and the in-process implementations before updating the schema and docs.

[toddbaert]

@colebaileygit

I think you've highlighted a legitimate use-case and an existing shortcoming in flagd. Your proposal also seems solid to me.

That said, I'd like to propose something similar that will, perhaps, afford a better user-experience in the end...

Proposal

Instead of adding a second optional arg as you've proposed, what if we change the existing implementation so that it does NOT automatically append the flag key. Essentially, this line would change to simply return bucketBy, feDistributions, nil. This would mean that users would now fully control the "seed". If they wanted to use a seed they would have to implement it in their bucking expression.

This would require no schema changes, and I think be a better user experience.

Downsides

Of course, this would be a behaviorally breaking change in terms of the variant that will be returned give a particular bucking setup and evaluation context... however, we are still sub 1.0, and I really doubt many people are counting on assigned variants not changing with new versions, generally (though we want to be careful of this post 1.0). Additionally, we could publish some very obvious guidance for how to work around the behavioral change to retain the previous assignment (simply append the flag-key to your bucketing value manually).

Also, we'd want to implement this in the in-process providers within a relatively short time frame to maintain behavioral parity.

What do you think?

cc @beeme1mr @Kavindu-Dodan

[colebaileygit]

In that case, the example configurations would look like this then?

"targeting": {
  "fractional": [
    { "cat": ["experiment-123", { "var": "user.name" }] },
    [ "clubs", 25 ],
    [ "diamonds", 25 ],
    [ "hearts", 25 ],
    [ "spades", 25 ]
  ]
}

"targeting": {
  "fractional": [
    { "cat": [{ "var": "populationSeed": }, { "var": "user.name" }] },
    [ "clubs", 25 ],
    [ "diamonds", 25 ],
    [ "hearts", 25 ],
    [ "spades", 25 ]
  ]
}

This was actually my first attempt at getting this to work, so that does seem more intuitive and less restrictive since any nested jsonlogic could be used here including nested ifs etc (assuming we also resolve #1265). I could imagine wild scenarios where you end up duplicating if statements twice if you for some reason want both the seed and key to be dynamic within a fractional invocation using my proposal, but I'd hope those are not the norm. I like this approach too 👍

One downside is the "base" case becomes more verbose with { "cat": [{ "var": "$flagd.flagKey" }, { "var": "targetingKey" }] } but given the shorthand op fills both in for you that should be fine and ensures there are no hidden variables. I wonder though if allowing "seedless" randomisations could become a foot-gun if people naively put in { "var": "user.email" } like the docs indicate now since it is a good practice to ensure each flag is randomised independently from each other statistically speaking.

[beeme1mr]

I wonder though if allowing "seedless" randomisations could become a foot-gun if people naively put in { "var": "user.email" } like the docs indicate now since it is a good practice to ensure each flag is randomized independently from each other statistically speaking.

This is my main concern, but since it's a relatively advanced use case I think we can address most of the concern through good docs. I would prefer to go with this approach since it's more intuitive and avoids overloading the operation with more arguments. It would have to be marked as a breaking change but the impact would be minimal and consuming the change would be straightforward.

[toddbaert]

I wonder though if allowing "seedless" randomisations could become a foot-gun

The only foot gun here is that with multiple flags, users will get bucketted similarly, (ie: session123 will consistently get bucket 3 across multiple flags, but I THINK this is OK if we communicate it with doc, since as you note, the shorthand handles it for you.

@colebaileygit @beeme1mr If you both agree, I think I'm comfortable suggesting that we go ahead with this implementation. @colebaileygit would you be interested in getting started on that approach? You might have issues with the e2e suite expectations not matching the new behavior, but I can help you with that if you get stuck there.

I'd also be happy to update the docs accordingly.

[colebaileygit]

Sure thing! Will update the PRs above to the new approach and then look into the other memory providers that I know of

[Kavindu-Dodan]

+1 from my end. I also think this to be a good UX improvement.

[colebaileygit]

I've updated the linked PRs above, but wasn't able to quickly onboard the build / test process for js-sdk-contrib or the e2e suite for java-sdk-contrib, maybe someone more familiar would be faster there. Also I think we'll need to update the go-sdk once the new go-core version is available.

What's the order of operations to get all the PRs in sync with each other and versioned? Schemas > testbed > core > providers? I especially want to make sure I'm not messing anything up with the submodules and fork branches

[beeme1mr]

I'll try and knock out the js-sdk-contrib update. Perhaps @toddbaert or @Kavindu-Dodan could handle Java when they have a moment.

[Kavindu-Dodan]

What's the order of operations to get all the PRs in sync with each other and versioned? Schemas > testbed > core > providers?

Given there's a breaking change (test incompatibility), you could follow this order

flagd-testbed (updated tests) + new release -> flagd + new release

flagd go provider should wait for flagd release as it uses the core module. The java provider can be updated independently as soon as flagd-testbed is ready with updated tests.

[colebaileygit]

Java PR is ready for review: open-feature/java-sdk-contrib#737

Small issue still needs to be resolved with the test-harness since the RPC evaluator needs to be updated once the flagd release is ready. I could also imagine moving the docker-compose helper I added out of java repo and into test-harness might make more sense here, but maybe others have a better approach.

[beeme1mr]

@colebaileygit Updates the JS can be tracked here.

[toddbaert]

@colebaileygit @beeme1mr I think this cane be closed with the merge of the JS implementation? Or is there something else we need as well?

[colebaileygit]

I think so! Did we need to update golang sdk with latest flagd core?

[Kavindu-Dodan]

@colebaileygit flagd go provider is already updated - https://github.com/open-feature/go-sdk-contrib/releases/tag/providers%2Fflagd%2Fv0.2.0 :)