Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Rely on Flagsmith Engine for segment evaluation #2865

Merged
merged 3 commits into from
Nov 21, 2023
Merged

feat: Rely on Flagsmith Engine for segment evaluation #2865

merged 3 commits into from
Nov 21, 2023

Conversation

khvn26
Copy link
Member

@khvn26 khvn26 commented Oct 19, 2023 •
edited
Loading

Thanks for submitting a PR! Please check the boxes below:

  • I have run pre-commit to check linting
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

This PR removes legacy code responsible for segment rule evaluation and replaces it with Flagsmith engine implementation.

How did you test this code?

  1. Ran unit and integration tests.
  2. Evaluated identities locally.

@khvn26 khvn26 requested a review from matthewelwell October 19, 2023 12:57
@vercel
Copy link

vercel bot commented Oct 19, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 21, 2023 11:07am
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 21, 2023 11:07am
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 21, 2023 11:07am

@github-actions github-actions bot added the api Issue related to the REST API label Oct 19, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 19, 2023 •
edited
Loading

Uffizzi Preview deployment-41173 was deleted.

matthewelwell
matthewelwell reviewed Oct 19, 2023
View reviewed changes
api/segments/models.py Outdated
Comment on lines 118 to 95
segment_model = map_segment_to_engine(self)
identity_model = map_identity_to_engine(identity)
trait_models = map_traits_to_trait_models(traits) if traits else None
Copy link
Contributor

Choose a reason for hiding this comment

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

My concern here is that it's a bit hidden as to what additional database interactions are happening. I think, to plugin the engine, we need to do it much earlier in the flow and use e.g. engine.get_identity_feature_states.

Copy link
Member Author

Choose a reason for hiding this comment

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

additional database interactions

These are well covered by the currently failing assertNumQueries assertion in test_get_segments. IMO if we manage to keep the same amount (or less) by prefetching the data, we're good.

do it much earlier in the flow and use e.g. engine.get_identity_feature_states.

I'll think of it but my primary focus here is to replace the segment matching logic first.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO if we manage to keep the same amount (or less) by prefetching the data, we're good.

Yep, fair enough.

@matthewelwell matthewelwell mentioned this pull request Nov 2, 2023
Open
@khvn26 khvn26 self-assigned this Nov 20, 2023
@khvn26 khvn26 force-pushed the feat/engine-segment-eval branch from 403c5dd to 5a908b0 Compare November 20, 2023 23:45
@khvn26 khvn26 marked this pull request as ready for review November 20, 2023 23:47
@khvn26 khvn26 changed the title feat: engine segment evaluation feat: Rely on Flagsmith Engine for segment evaluation Nov 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bd1f373) 95.60% compared to head (3b11d53) 95.75%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2865      +/-   ##
==========================================
+ Coverage   95.60%   95.75%   +0.15%     
==========================================
  Files        1022     1021       -1     
  Lines       29854    29676     -178     
==========================================
- Hits        28541    28417     -124     
+ Misses       1313     1259      -54

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@khvn26 khvn26 force-pushed the feat/engine-segment-eval branch from 3bd2564 to 3b11d53 Compare November 21, 2023 10:52
@khvn26 khvn26 requested a review from matthewelwell November 21, 2023 11:24
matthewelwell
matthewelwell approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@matthewelwell matthewelwell left a comment

Choose a reason for hiding this comment

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

Approved with one minor comment. So goo d being able to delete all this code. I'm assuming as well that, since there are no test failures, we haven't had to introduce any new queries here.

api/environments/identities/tests/test_views.py
@@ -370,7 +370,7 @@ def test_identities_endpoint_returns_traits(self, mock_amplitude_wrapper):
trait = Trait.objects.create(
identity=self.identity,
trait_key="trait_key",
value_type="STRING",
value_type=STRING,
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to think this is because those tests were wrong (and we were lucky that the default type is string) but it's worth noting that this is a slight change "STRING" -> "unicode".

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. The engine mapper relies on Trait's trait_value property which doesn't employ the default string fallback.

@khvn26 khvn26 added this pull request to the merge queue Nov 21, 2023
Merged via the queue into main with commit 322eb08 Nov 21, 2023
@khvn26 khvn26 deleted the feat/engine-segment-eval branch November 21, 2023 11:59
@dabeeeenster dabeeeenster mentioned this pull request Nov 21, 2023
Merged
khvn26 added a commit that referenced this pull request Nov 27, 2023
@khvn26
Revert "feat: Rely on Flagsmith Engine for segment evaluation (#2865)"
915653b 
This reverts commit 322eb08.
@khvn26 khvn26 mentioned this pull request Nov 27, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewelwell matthewelwell matthewelwell approved these changes

Assignees

@khvn26 khvn26

Labels
api Issue related to the REST API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@khvn26 @codecov-commenter @matthewelwell