fix: N dynamodb queries in feature list endpoint#6758
fix: N dynamodb queries in feature list endpoint#6758matthewelwell wants to merge 13 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
matthewelwell
changed the title
matthewelwell
requested review from
gagantrivedi
and removed request for
a team
Docker builds report
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6758 +/- ##
=======================================
Coverage 98.27% 98.27%
=======================================
Files 1328 1328
Lines 48984 48938 -46
=======================================
- Hits 48138 48094 -44
+ Misses 846 844 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # api/edge_api/identities/edge_identity_service.py
api/features/features_service.py
Outdated
| ) | ||
| get_overrides_data_future = executor.submit( | ||
| get_edge_identity_overrides_for_feature_ids, | ||
| get_edge_identity_overrides, |
There was a problem hiding this comment.
We were discussing just getting the document_key to get the counts. Is the network gain not worth in the end? It looks like with a dedicated method we would also save on Pydantic overhead
https://github.com/Flagsmith/flagsmith/pull/6758/changes#diff-9bbfb0e97f54f08b1dec94a5e2f7233a9b9f86b4729795d089b20f88713f488bR23-R26
But I can understand it's neglictable unless tons of overrides
There was a problem hiding this comment.
Good shout, sorry, I forgot about that piece! Let me see what I can do...
# Conflicts: # api/tests/unit/features/test_unit_features_views.py
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Mostly the e2e failing to fix but that's gonna be a good improvement overall
api/environments/dynamodb/utils.py
Outdated
|
|
||
|
|
||
| def get_feature_id_from_identity_override_document_key(document_key: str) -> int: | ||
| return int(document_key.split(".")[1]) |
There was a problem hiding this comment.
I believe that's the origin of the e2e failing
| return int(document_key.split(".")[1]) | |
| return int(document_key.split(":")[1]) |
| assert second_feature_metadata_after[0]["field_value"] == "200" | ||
|
|
||
|
|
||
| def test_create_feature__required_metadata_on_other_project__returns_201( |
There was a problem hiding this comment.
Weird that this got included
| @@ -34,7 +33,6 @@ | |||
| @dataclass | |||
| class IdentityOverridesQueryResponse: | |||
There was a problem hiding this comment.
NIT: Just checked and it looks safe to be removed
matthewelwell
force-pushed
the
fix/features-list-dynamodb-query-count
branch
from
7ca4c59 to
e7dd086
Compare
2 participants
Changes
This PR refactors the logic for retrieving the number of identity overrides when retrieving the list of features within the context of an environment such that, instead of making a query to dynamodb for every feature in the page of features that are returned, it gets all of the overrides for the environment upfront and uses that information to populate the data.
The reason for doing this is to improve performance of the features list endpoint which is currently making multiple 'parallel' (depending on thread resources) calls to dynamodb when in 99% of cases, it only needs to make a single call.
How did you test this code?
Added a new test to verify the N queries issue is resolved
I actually wanted to also add another test to verify the logic using moto for dynamodb, but the issue is that (as far as I can tell), the test database, and moto do not support multithreading, and since the requests are executed via the ThreadPoolExecutor, we end up with bad results.