fix: Speed up identity overrides #4840

zachaysan · 2024-11-18T14:17:31Z

Changes

This changes the way that identity overrides are requested for the features page in two ways. First, instead of the entire document getting paginated and returned to the client, only one page per feature is returned. Second, instead of every feature being returned in the network call, we only serve up the features for the current page.

How did you test this code?

I've written one new test, although I'm awaiting codecov to let me know where else needs testing, since I'm sure some are missing.

vercel · 2024-11-18T14:17:36Z

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

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
docs	⬜️ Ignored (Inspect)	Visit Preview	Nov 19, 2024 7:34pm
flagsmith-frontend-preview	⬜️ Ignored (Inspect)	Visit Preview	Nov 19, 2024 7:34pm
flagsmith-frontend-staging	⬜️ Ignored (Inspect)	Visit Preview	Nov 19, 2024 7:34pm

github-actions · 2024-11-18T14:17:57Z

Docker builds report

Image	Build Status	Security report
`ghcr.io/flagsmith/flagsmith-api-test:pr-4840`	Finished ✅	Skipped
`ghcr.io/flagsmith/flagsmith-frontend:pr-4840`	Finished ✅	Results ✅
`ghcr.io/flagsmith/flagsmith-api:pr-4840`	Finished ✅	Results ✅
`ghcr.io/flagsmith/flagsmith:pr-4840`	Finished ✅	Results ✅
`ghcr.io/flagsmith/flagsmith-private-cloud:pr-4840`	Finished ✅	Results ✅

github-actions · 2024-11-18T14:23:12Z

Uffizzi Ephemeral Environment `deployment-58332`

☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/4840

📄 View Application Logs etc.

What is Uffizzi? Learn more!

khvn26

Added a bunch of questions + not sure if the original issue is actually solved by this.

khvn26 · 2024-11-18T14:48:32Z

api/environments/dynamodb/wrappers/environment_wrapper.py

+        feature_ids: None | list[int] = None,
+        limit_feature_identities_to_one_page: bool = False,
    ) -> typing.List[dict[str, Any]]:
        try:
-            return list(
-                self.query_get_all_items(
-                    KeyConditionExpression=Key(ENVIRONMENTS_V2_PARTITION_KEY).eq(
-                        str(environment_id),
+            if not limit_feature_identities_to_one_page:
+                if feature_ids is not None:
+                    raise NotImplementedError(
+                        "Multiple feature ids is currently not supported "
+                        "when not limiting to a single page of features"


It seems like feature_id, feature_ids and limit_feature_identities_to_one_page could be compressed to a single feature_ids argument. This would significantly simplify the logic too.

We need to keep the distinction between feature_id and feature_ids to support existing logic, but I was able to remove the limit_feature_identities_to_one_page and rely on feature_ids for the pre-existing logic.

khvn26 · 2024-11-18T14:50:09Z

api/environments/dynamodb/wrappers/environment_wrapper.py

+                            executor.submit(
+                                self.get_page_of_feature_identities,
+                                environment_id,
+                                feature_id,
+                            )


This certainly looks like a nice solution — I haven't thought of running several queries in parallel when thinking of solving the issue.

Have you considered cases when one feature has >1MB of identity overrides data? The original issue mentioned introducing approximate counters for those (e.g. "1000+") but it doesn't seem to be implemented here.

I've added a new field to mark identity overrides as being over the limit but this will need to be supported by the frontend with a new field.

khvn26 · 2024-11-18T14:52:14Z

api/environments/dynamodb/wrappers/environment_wrapper.py

        except KeyError as e:
            raise ObjectDoesNotExist() from e

+    def get_page_of_feature_identities(


Not a fan of introducing feature_identities name while having established identity_overrides. Also, not a fan of using prepositions in function names.

Suggested change

def get_page_of_feature_identities(

def get_identity_overrides_page(

Good idea. Updated.

khvn26 · 2024-11-18T14:53:52Z

api/tests/unit/features/test_unit_features_views.py

+def test_list_features_calls_get_overrides_data_with_feature_ids(
+    dynamo_enabled_project: Project,
+    dynamo_enabled_project_environment_one: Environment,
+    admin_client_new: APIClient,
+    mocker: MockerFixture,
+) -> None:
+    # Given
+    feature = Feature.objects.create(
+        name="test_feature", project=dynamo_enabled_project
+    )
+    url = "%s?environment=%d" % (
+        reverse(
+            "api-v1:projects:project-features-list", args=[dynamo_enabled_project.id]
+        ),
+        dynamo_enabled_project_environment_one.id,
+    )
+    mock_get_overrides_data = mocker.patch("features.views.get_overrides_data")
+    mock_get_overrides_data.return_value = {
+        feature.id: EnvironmentFeatureOverridesData()
+    }
+
+    # When
+    response = admin_client_new.get(url)
+
+    # Then
+    assert response.status_code == status.HTTP_200_OK
+    mock_get_overrides_data.assert_called_once_with(
+        dynamo_enabled_project_environment_one,
+        [feature.id],
+    )


This test seems to fail in CI. Also, in addition to a mocked unit test, I would expect to see a functional test that places actual data in fixtured Dynamo tables.

I've created a functional test with the Dynamo fixtured tables.

khvn26 · 2024-11-18T14:55:44Z

api/environments/dynamodb/wrappers/environment_wrapper.py

+            KeyConditionExpression=Key(ENVIRONMENTS_V2_PARTITION_KEY).eq(
+                str(environment_id),
+            )
+            & Key(ENVIRONMENTS_V2_SORT_KEY).begins_with(
+                get_environments_v2_identity_override_document_key(
+                    feature_id=feature_id,
+                ),
+            )
+        )


nit: The query is identical to the one used in get_identity_overrides_by_environment_id method — maybe it makes sense to DRY this out.

Sure, sounds good. Updated.

khvn26 · 2024-11-18T14:59:02Z

api/environments/dynamodb/wrappers/environment_wrapper.py

+                        )
+
+                results = []
+                for future in as_completed(futures):


I don't think I understand the need to use as_completed here.

It's just to process results as they become available rather than waiting for all tasks to finish before proceeding.

This doesn't do anything as all the submitted tasks are completed by the time of this call.

The tasks are done by implicitly calling ThreadPoolExecutor.__exit__ as you exit from the with ThreadPoolExecutor() block.

…h/flagsmith into fix/speed_up_identity_overrides

codecov · 2024-11-19T14:14:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.35%. Comparing base (9bbfdf0) to head (3e2c959).
Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4840   +/-   ##
=======================================
  Coverage   97.35%   97.35%           
=======================================
  Files        1182     1183    +1     
  Lines       41272    41344   +72     
=======================================
+ Hits        40179    40251   +72     
  Misses       1093     1093

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

zachaysan added 4 commits November 18, 2024 14:08

Query only one page of identity overrides for features

49142b0

Pass feature ids to get_edge_identity_overrides

9e8d767

Paginate the view to get the feature ids for the context

8d864f2

Add a test that get_overrides_data gets passed the feature ids

5a5e7e4

zachaysan requested a review from a team as a code owner November 18, 2024 14:17

zachaysan requested review from khvn26 and removed request for a team November 18, 2024 14:17

github-actions bot added api Issue related to the REST API fix labels Nov 18, 2024

khvn26 requested changes Nov 18, 2024

View reviewed changes

simplify caching logic

80f6588

github-actions bot added fix and removed fix labels Nov 18, 2024

zachaysan added 5 commits November 18, 2024 17:36

Update test to pass

678ddd4

Merge branch 'fix/speed_up_identity_overrides' of github.com:Flagsmit…

79d46d5

…h/flagsmith into fix/speed_up_identity_overrides

Add warning docstring

33814e5

Update mock assertion

6d0c046

Limit based on whether feature ids have been passed in

cdf378d

github-actions bot added fix and removed fix labels Nov 19, 2024

zachaysan added 6 commits November 19, 2024 16:43

Remove obsolete argument

1a8393b

Add more identity overrides boolean

415cb51

Remove obsolete arg and set more identity overrides

9044edb

Set more identity overrides boolean

a470918

Add get_identity_overrides_key_condition_expression and remove argument

4fc6b50

Add more identity overrides

d9b52b5

Remove obsolete argument

9854471

github-actions bot added fix and removed fix labels Nov 19, 2024

Add test for searching based on feature ids

3e2c959

github-actions bot added fix and removed fix labels Nov 19, 2024

zachaysan requested a review from khvn26 November 19, 2024 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Speed up identity overrides #4840

fix: Speed up identity overrides #4840

zachaysan commented Nov 18, 2024

vercel bot commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

khvn26 left a comment

khvn26 Nov 18, 2024

zachaysan Nov 19, 2024

khvn26 Nov 18, 2024

zachaysan Nov 19, 2024

khvn26 Nov 18, 2024

zachaysan Nov 19, 2024

khvn26 Nov 18, 2024

zachaysan Nov 19, 2024

khvn26 Nov 18, 2024

zachaysan Nov 19, 2024

khvn26 Nov 18, 2024

zachaysan Nov 19, 2024

khvn26 Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading

	def get_page_of_feature_identities(
	def get_identity_overrides_page(

fix: Speed up identity overrides #4840

Are you sure you want to change the base?

fix: Speed up identity overrides #4840

Conversation

zachaysan commented Nov 18, 2024

Changes

How did you test this code?

vercel bot commented Nov 18, 2024 • edited Loading

github-actions bot commented Nov 18, 2024 • edited Loading

Docker builds report

github-actions bot commented Nov 18, 2024 • edited Loading

Uffizzi Ephemeral Environment deployment-58332

khvn26 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khvn26 Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 19, 2024 • edited Loading

Codecov Report

vercel bot commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

Uffizzi Ephemeral Environment `deployment-58332`

khvn26 Nov 19, 2024 •

edited

Loading

codecov bot commented Nov 19, 2024 •

edited

Loading