Skip to content

Conversation

gagantrivedi
Copy link
Member

No description provided.

Copy link
Member

@khvn26 khvn26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great minimal implementation for context values. As it builds on the existing data model, it doesn't require many changes to the code.

It does seem fairly extensible; we can gradually add more dependencies to the engine and expand the inner map with new root keys to introduce additional evaluation data.

However, the environment document exposes a bit too much internal data. I don't think users will be comfortable with fields such as django_id and featurestate_uuid when building their evaluation rules. It also uses arrays to present collections of potentially interesting rule properties, which makes context value JSON paths slightly harder to construct and allows for ambiguous index-based paths.

However, this is a familiar data structure to customers that they can easily access via the API or CLI tool.

One drawback is that we will have to synchronise the inner structure accurately across SDK implementations without a single source of truth. This could become a significant concern as we add more external data in the future. However, we'll probably be able to use a common schema for that data. This will result in the engine being informed by three data schemas, which makes me a little sad.

Finally, it's important to note that, to make this work with current segments containing rules with context values, we'll need to introduce an FE change and perform a data migration in the Core API.


data := map[string]interface{}{
"environment": env,
"identity": identity,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"identity": identity implies that jp works on structs, nice 👌

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.

2 participants