Improvements to fractional custom evalutor

[toddbaert]

The fractional evaluation currently uses xxh3 algo as part of it's bucketing algorithm (see here). In order to support in-memory implementations in other languages, we should use a more popular algorithm, such as murmur3 (this is popular with other feature flag SDKs as well). Additionally, the algorithm doesn't currently take into account the flag key. This means that, for different flags, bucketing will have a predictable ordinality. The algo should be updated to use both the selected context property AND the flag key as factors to the hash. We also should update the evaluator to be able to use nested context props (not just top level props).

Definition of done:

• fractional evaluator updated to use murmur3 or md5, or other more popular hash than xxh3 (take into account support in various other langues, such as Java, JS, etc)
• flag key added to context and used as input to hash (implemented feat: add flag key to hash in fractional evaluation #847 (review))
• ability to use nested props as key
  • see: docs: add specification for in-process flagd provider implementations #848 (comment)
    • allow the use a JSON logic expression to get the bucket value (ie: {"var":"some.nested.prop"})
    • if no value is provided, use {"cat": [{"var":"$flagd.flagKey"}, {"var":"targetingKey"}]} (basically what we have now, but expressed as a JSON logic expression)

[craigpastro]

👋 @toddbaert. I took a quick look at this. The first step is easy. For part 2, flag key also used as input to hash, one way I imagine this could be accomplished is by adding the flag key to the context/data when calling the functional evaluator. Is that what you were thinking too? Happy to make a PR for this if so.

I didn't understand what step 3 is asking: "ability to use nested props as key (user.email)".

[toddbaert]

Hey @craigpastro ! If you look at the fractional evaluation doc, the first argument is the property we use for the deterministic bucketing (in addition to the key, which we will add, as you mention). The problem is, it seems to me this property MUST be a top level property in the map.

It would be great if we could express the desire to use a nested property, something like user.email if it exists in the context. JSON logic allows for the declaration and extractions of variables, so we may be able to leverage that.

However, if this proves too difficult, I'm fine with opening a new issue for that feature, or just avoiding it. The more important things for the time being are using a different (more popular) hash algo, and also involving the key in the bucketing.

cc @beeme1mr @bacherfl

[craigpastro]

Thanks for your comment, @toddbaert ! I'll create a draft PR for the first two and read a bit more about jsonlogic to see how to handle 3.

I am still wondering how to approach two though. At this point I only have access to the targeting body and the context body. No flag key. Here I go have access to the flag key and can add it to the targeting bytes or the context, so that we can access it in in here, but I am not sure that that is the right approach. WDYT?

[toddbaert]

Thanks for your comment, @toddbaert ! I'll create a draft PR for the first two and read a bit more about jsonlogic to see how to handle 3.

I am still wondering how to approach two though. At this point I only have access to the targeting body and the context body. No flag key. Here I go have access to the flag key and can add it to the targeting bytes or the context, so that we can access it in in here, but I am not sure that that is the right approach. WDYT?

I think the way the JSON logic API works, the best approach might be to pass a new "higher level" map to the JSON logic's apply function, which would contain 2 top level properties: a flag key, and the context. This will avoid us having to add our own flag key prop to the context, which otherwise might trample over a user-added value.

I think @bacherfl was playing around with this problem as well, so he might have some ideas.

[craigpastro]

Ah, yeah, that is a better idea. I am not sure how this can work with jsonlogic though as it is pushing the data down a level so that vars will also need to account for that?

In any case, I'll make a PR for the first part 😄

[toddbaert]

Ah, yeah, that is a better idea. I am not sure how this can work with jsonlogic though as it is pushing the data down a level so that vars will also need to account for that?

In any case, I'll make a PR for the first part 😄

Ya thinking about this more, I'm not sure what I suggested before is feasible. Maybe we could inject the key into the targeting bytes. We do something similar at a different point to allow reuse of shared rules. I'll look into it a bit tomorrow.

If we can't make that work nicely, perhaps we can just create some namespacing convention for properties we inject to the context, and document that, to reduce the risk of overwriting stuff from clients. In addition to the flag key, values like the current utc time, etc could be useful, so we should come up with some solution for this.

[toddbaert]

Ah, yeah, that is a better idea. I am not sure how this can work with jsonlogic though as it is pushing the data down a level so that vars will also need to account for that?
In any case, I'll make a PR for the first part 😄

Ya thinking about this more, I'm not sure what I suggested before is feasible. Maybe we could inject the key into the targeting bytes. We do something similar at a different point to allow reuse of shared rules. I'll look into it a bit tomorrow.

If we can't make that work nicely, perhaps we can just create some namespacing convention for properties we inject to the context, and document that, to reduce the risk of overwriting stuff from clients. In addition to the flag key, values like the current utc time, etc could be useful, so we should come up with some solution for this.

The more I think about it, the more I think that adding some attributes directly into the context under some well-defined name is a good idea. Flag key would be one example, but also potentially:

• current epoch time
• flag type
• arbitrary key/values passed at flagd startup

I see these as out-of-scope for this issue, but potentially valuable. I think the thing we need to decide is how to store them. I think perhaps we could put them under a property, perhaps called __ambient__ or something like that. So then they could be accessed like __ambient__.timestamp. Or else we could just put them all under different properties, for example __timestamp__ and __flag_type__.

Once we decide, we can implement the first of these for the flag_key.

[craigpastro]

I was playing around with this last night and it seems to work fine. I think you are right, namespacing and documenting would certainly help. That the risk remains is a bit of a bummer though. There should be a nicer way, but I just don't see it.

Recently it seems that a lot of projects are using https://github.com/google/cel-spec for user input custom logic. Maybe this is also something to take a look at? I am not too familiar with it, but have just seen it quite a bit.

I can throw together a draft PR using __flag_key__ in the context so you can see what that looks like.

[toddbaert]

I can throw together a draft PR using flag_key in the context so you can see what that looks like.

Yes, I'd be interested in that. A number of contributors are away because there was some holidays this week in EU, but if you open a draft I'm sure people will comment over the next week.

[beeme1mr]

I like the idea of reserving a namespace like $flagd that we could use to automatically set useful context values. If it's always set by flagd and well documented, users could leverage this context in their rulesets.

[toddbaert]

I've updated the issue description with our progress so far (thanks again @craigpastro and @bacherfl ) and refined the remaining task.

[craigpastro]

Shouldn't 2 and 3 be:

• flag key added to context and used as input to hash (implemented feat: add flag key to hash in fractional evaluation #847 (review))
• ability to use nested props as key

[toddbaert]

Shouldn't 2 and 3 be:

• flag key added to context and used as input to hash (implemented feat: add flag key to hash in fractional evaluation #847 (review))
• ability to use nested props as key

Yes, sorry about that!

[toddbaert]

@craigpastro do you mind if I take on part 3? I have some time to do it... though I'm more than happy to let you do it if you'd like to (or you've already started).

[craigpastro]

Hi @toddbaert. Sorry, I was out most of last week. I do have time to look at it today. I am not sure I completely understand it, but I can give it a shot.

[toddbaert]

Hi @toddbaert. Sorry, I was out most of last week. I do have time to look at it today. I am not sure I completely understand it, but I can give it a shot.

@bacherfl has fully specified the desired behavior of the evaluator here (important because it will be implemented elsewhere as well). The implementation you've worked on here is quite close already. The main difference is that we want to try to use a JSON-logic expression to extract the bucketing value. See some background discussion here.