feat: Implement dynamic targeting key #2218
Conversation
✅ Deploy Preview for go-feature-flag-doc-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Since rules don't have access to the parent flag, I had to pass the |
ricardobeat
changed the title
There was a problem hiding this comment.
Hey @ricardobeat, thanks a lot for creating this pull request! It helps me understand a bit better what we were discussing.
If I'm correct, you want the evaluation to fail and return to the default value provided in the evaluation context if the bucketingKey is not found.
This is an interesting idea and a simple way to avoid having to fall back on the targetingKey. However, I'm concerned about the new ErrorCode we're sending because it's not a known one from OpenFeature (see list here).
I've also reviewed the different documentation you shared in the Slack channel. It seems there's no consensus on whether to fall back to default behavior or fail.
Optimizely falls back to the targetingKey (see here).
If you don't pass a bucketing ID to the attributes parameter, users are bucketed by user IDs, which is the default method.
I haven't done a deep review of the code yet but it looks good, I am just not sure if falling is a good idea when the bucketingKey required by the flag is not set in the evaluation context 🤔.
I couldn't find Optimizely's reasoning for this. Here is my attempt at illustrating the problem: 
The user in org1 receives the This defeats the purpose of the feature test, and when we set up analytics for the A/B test, we are going to measure things like users in organization who interacted with feature X. Having users randomly assigned to different variants within the same organization means we will measure a mix of A and B, making the metric not useful, or worse, silently biased if this (missing key) happens only for a small % of users. Personally I don't see a logical reason to have a fallback, and would expect it to behave exactly as if |
There was a problem hiding this comment.
You are right, falling back to the default value is probably the best option here.
I've put some comments in the PR, especially about replacing the key in the context where I don't see the adding value.
If we want to move forward with this PR, I think we should address the documentation part because it will be necessary to explain well how it works.
If you are lost with the doc, I can always help you.
PS: For the CI falling on swagger, you can run make swagger in the root of the repo to update the swagger files.
internal/utils/serializer.go
Outdated
| @@ -14,6 +14,6 @@ func ContextToMap(ctx ffcontext.Context) map[string]interface{} { | |||
| userCopy[key] = value | |||
| } | |||
| userCopy["anonymous"] = ctx.IsAnonymous() | |||
| userCopy["key"] = ctx.GetKey() | |||
| userCopy["key"] = key | |||
There was a problem hiding this comment.
As mentioned in the issue, I'd prefer not to do this.
key and targetingKey are special fields in the evaluation context and we should keep them like that.
For this feature, the important part is to be sure that we split the users based on another field it should not have an impact on the way to write rules.
This part will change how you write your rules and may lead to misleading configuration of your flags.
Let's say you have this flag
my-flag:
variation:
enabled: true
disabled: false
defaultRule:
percentage:
enabled: 10
disabled: 90
bucketingKey: teamIdand this evaluation context:
{
"targetingKey": "f1f0d804-f23f-4c93-a935-6e04ce2cf696",
"teamId": 567,
"email": "[email protected]"
}If you have to add a rule inside the flag based on the teamId you can go with
# ...
targeting:
- query: teamId eq "567"
variation: enabled
# ...I don't see the adding value to also put teamId value as key.
|
@thomaspoignant I've done a bit of refactoring according to the comments above:
I also took a first pass at documentation. Let me know what you think. |
thomaspoignant
force-pushed
the
bucketing-key
branch
from
5cf04e8 to
3870812
Compare
There was a problem hiding this comment.
Hey @ricardobeat sorry I have been on vacation for some weeks this is why I was a bit quiet.
I think we are close to the solution with this PR, my only remarks is about the tests that are not testing what the change of key is affecting (in percentage-based evaluation).
As soon as we have a good test for that I think we will be ready to merge.
internal/flag/internal_flag_test.go
Outdated
| { | ||
| name: "Should use custom bucketing key when set", | ||
| flag: flag.InternalFlag{ | ||
| Variations: &map[string]*interface{}{ | ||
| "variation_A": testconvert.Interface("value_A"), | ||
| "variation_B": testconvert.Interface("value_B"), | ||
| "variation_C": testconvert.Interface("value_C"), | ||
| }, | ||
| BucketingKey: "teamId", | ||
| Rules: &[]flag.Rule{ | ||
| { | ||
| Query: testconvert.String("key eq \"team-123\""), | ||
| VariationResult: testconvert.String("variation_B"), | ||
| }, | ||
| { | ||
| Query: testconvert.String("key eq \"user-key\""), | ||
| VariationResult: testconvert.String("variation_C"), | ||
| }, | ||
| }, | ||
| DefaultRule: &flag.Rule{ | ||
| VariationResult: testconvert.String("variation_A"), | ||
| }, | ||
| Metadata: &map[string]interface{}{ | ||
| "teamId": "team-123", | ||
| }, | ||
| }, | ||
| args: args{ | ||
| flagName: "my-flag", | ||
| user: ffcontext.NewEvaluationContextBuilder("user-key").AddCustom("teamId", "team-123").Build(), | ||
| flagContext: flag.Context{ | ||
| DefaultSdkValue: "value_default", | ||
| }, | ||
| }, | ||
| want: "value_B", | ||
| want1: flag.ResolutionDetails{ | ||
| Variant: "variation_B", | ||
| Reason: flag.ReasonTargetingMatch, | ||
| RuleIndex: testconvert.Int(0), | ||
| Cacheable: true, | ||
| Metadata: map[string]interface{}{ | ||
| "teamId": "team-123", | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "Should not use user key when bucketing key is set", | ||
| flag: flag.InternalFlag{ | ||
| Variations: &map[string]*interface{}{ | ||
| "variation_A": testconvert.Interface("value_A"), | ||
| "variation_B": testconvert.Interface("value_B"), | ||
| }, | ||
| BucketingKey: "region", | ||
| Rules: &[]flag.Rule{ | ||
| { | ||
| Query: testconvert.String("key eq \"user-key\""), | ||
| VariationResult: testconvert.String("variation_B"), | ||
| }, | ||
| }, | ||
| DefaultRule: &flag.Rule{ | ||
| VariationResult: testconvert.String("variation_A"), | ||
| }, | ||
| Metadata: &map[string]interface{}{ | ||
| "region": "nl-nh", | ||
| }, | ||
| }, | ||
| args: args{ | ||
| flagName: "my-flag", | ||
| user: ffcontext.NewEvaluationContextBuilder("user-key").AddCustom("region", "nl-nh").Build(), | ||
| flagContext: flag.Context{ | ||
| DefaultSdkValue: "value_default", | ||
| }, | ||
| }, | ||
| want: "value_A", | ||
| want1: flag.ResolutionDetails{ | ||
| Variant: "variation_A", | ||
| Reason: flag.ReasonDefault, | ||
| Cacheable: true, | ||
| Metadata: map[string]interface{}{ | ||
| "region": "nl-nh", | ||
| }, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
I am not sure those tests are relevant because they do not testing the percentage split based on the bucketing key.
A good test here could be to have a percentage-based evaluation, set a targetingKey that affect to 1 variant and a teamId that affect to another variant.
The test should check that when the bucketingKey is set we are serving the good variation.
|
@ricardobeat do you need help finishing this PR? |
|
@thomaspoignant sorry, I was also on holiday the week before and just got around to reviewing this. Need some help with figuring out the tests indeed, commented above 🙏 |
Signed-off-by: Thomas Poignant <[email protected]>
Signed-off-by: Thomas Poignant <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2218 +/- ##
==========================================
+ Coverage 86.02% 86.07% +0.05%
==========================================
Files 102 102
Lines 3743 3771 +28
==========================================
+ Hits 3220 3246 +26
- Misses 399 400 +1
- Partials 124 125 +1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Thomas Poignant <[email protected]>
|
Description
Introduces the
bucketingKeyfield in flag configuration. It allows the targeting key to be dynamically overwritten at evaluation time by another value coming from the context. See related issue for more info.This is a non-breaking change.
Closes issue(s)
Resolve #2135
Checklist
README.mdand/website/docs)