Use Elasticsearch to fetch applications for exports #34266

proteusvacuum · 2024-03-12T03:33:17Z

Product Description

Fetching all apps when there are a large number of deleted apps was extremely slow, and was timing out. This fetches apps from elasticsearch instead of re-writing the couch view, since I think that was going to require a reindex of the whole index (which I'm guessing is undesirable).

This is currently behind an internal feature flag, and also a domain setting. To enable it, first enable the EXPORTS_APPS_USE_ELASTICSEARCH feature flag, then on the domain settings screen enable the Use elasticsearch when fetching apps for exports setting.

The list of possible apps to export will be different than when pulling data from couch: It will not include forms from "Unknown Apps" (mostly because there are apps that are malformed in the DB currently and it was causing 500 errors when fetching these. For example: https://staging.commcarehq.org/a/qa-automation/apps/view/934af150f0746ff291335b7161d2c076/).

Technical Summary

https://dimagi.atlassian.net/browse/SAAS-15314
Pulling the list of exports on the qa-automation domain from couch was extremely slow, likely due to the volume of deleted apps on that domain. Since those are baked into the couch view, I opted to fetch the apps from elasticsearch instead of creating a new couch view, or modifying the existing one.

This has sped up the request from ~3-5 minutes to < 2 seconds.

Feature Flag

export_apps_use_elasticsearch

Safety Assurance

Safety story

This is only fetching data, and is behind a flag.

Automated test coverage

I've added a new unit test, along with updating existing tests to validate existing behaviour.

QA Plan

This is actually for the QA team to use during testing on staging, as their domain has a large number of deleted apps.

Rollback instructions

This PR can be reverted after deploy with no further considerations

Labels & Review

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change

millerdev

Seems like there is a fair amount of experimentation going on in the commit history of this PR. What do you think of cleaning that up with a rebase, removing things like f395ae1 and 45ea357 and other things that were added and then changed?

millerdev · 2024-06-19T17:21:59Z

corehq/apps/app_manager/fields.py

+    def __init__(self, domain, project, user, as_dict=True):
        self.domain = domain
+        self.domain_object = project


Would it work to only pass in the domain object?

Suggested change

def __init__(self, domain, project, user, as_dict=True):

self.domain = domain

self.domain_object = project

def __init__(self, project, user, as_dict=True):

self.domain = project.name

self.domain_object = project

self.domain could also be changed to a property that returns self.domain_object.name

millerdev · 2024-06-19T18:25:09Z

corehq/ex-submodules/couchforms/tests/test_analytics.py

@@ -111,6 +111,8 @@ def test_get_form_analytics_metadata__app(self):

    @flaky_slow
    def test_get_exports_by_form(self):
+        # Call this twice, since the couchdb `update_after` to force couchdb to return the right results
+        get_exports_by_form(self.domain)


Does calling it twice actually force it, or does it just take a bit more time, making the second call a bit more likely to return the correct results? In other words, is this possibly a flaky test?

I think it actually forces it. This is also a flaky test, but it was before I added this change (and is annotated as such)

millerdev · 2024-06-19T18:28:54Z

corehq/ex-submodules/couchforms/tests/test_analytics.py

+                'submissions': 1,
+            },
+            'key': ['exports_forms_analytics_domain', self.app_id_3,
+                    'my://crazy.xmlns/deleted-app']


This is confusing in the context of exclude_deleted_apps=True. Can you say a bit more about why this makes sense?

Edit: maybe this is irrelevant. Looks like it was later removed.

Yes, sorry about that - there was a lot of experimentation going on here

jingcheng16 · 2024-06-20T18:45:14Z

corehq/apps/app_manager/fields.py

            form = f['value']
            if form.get('app_deleted') and not form.get('submissions'):
                continue
            if 'app' in form:
                form['has_app'] = True
                forms.append(form)
-            else:
+            elif not self.domain_object.exports_use_elasticsearch:


Hi Farid, can you explain what does it mean when app is not in form? I would prefer not to exclude those forms, because when I run in a shell, your _get_export_forms_by_app_es
is already quick enough to solve the current issue. 500 error seems like another issue. Loading those either unknown or delete apps used to work without 500 error, at least until 2 months ago. If there is some changes that resulted in those 500 errors, we should fix that, instead of just exclude those forms.

(Offlining a response to this)

jingcheng16 · 2024-06-21T15:27:40Z

Btw not sure if you can open this, but this is the Sentry issue I found when access your example

Add show_deleted_apps_exports to domain model and settings

Add deleted app to test_get_exports_by_form Optionally get export forms through ES Make test not flaky Use flag to use elasticsearch to get fields Remove deleted apps when fetching from elasticsearch Revert "Remove deleted apps when fetching from elasticsearch" This reverts commit 7ae476e. Update toggle to be switching on using the elasticsearch getter Don't error when there are no modules / forms Only return no-copy-of apps

proteusvacuum · 2024-07-01T16:07:01Z

@millerdev I rebased and squashed out a bunch of the experimental commits. Sorry about that.

This reverts commit 5985875.

This reverts commit 2350424.

jingcheng16

Thank you!

proteusvacuum force-pushed the fr/deleted-apps-2 branch 4 times, most recently from 8118925 to 9cdc525 Compare March 12, 2024 12:27

proteusvacuum force-pushed the fr/deleted-apps-2 branch from f2df7ef to 3dad7a1 Compare March 24, 2024 19:46

proteusvacuum changed the title ~~Fr/deleted apps 2~~ Use Elasticsearch to fetch applications for exports Mar 24, 2024

proteusvacuum force-pushed the fr/deleted-apps-2 branch 2 times, most recently from 3dddf8a to 6c46862 Compare March 24, 2024 20:17

proteusvacuum force-pushed the fr/deleted-apps-2 branch from 5d635b9 to 5fd1eaa Compare June 4, 2024 21:19

proteusvacuum marked this pull request as ready for review June 19, 2024 13:15

proteusvacuum requested a review from orangejenny as a code owner June 19, 2024 13:15

jingcheng16 requested review from AmitPhulera, jingcheng16, millerdev and gherceg June 19, 2024 13:24

millerdev reviewed Jun 19, 2024

View reviewed changes

jingcheng16 requested changes Jun 20, 2024

View reviewed changes

proteusvacuum added 3 commits July 1, 2024 12:03

Add EXPORT_HIDE_DELETED_APPLICATIONS toggle

76c004a

Add show_deleted_apps_exports to domain model and settings

Ignore unknown forms for elasticsearch case

5985875

proteusvacuum force-pushed the fr/deleted-apps-2 branch from 7571fcf to 5985875 Compare July 1, 2024 16:05

proteusvacuum added 3 commits August 21, 2024 16:18

Merge remote-tracking branch 'origin/master' into fr/deleted-apps-2

e5a12fc

Revert "Ignore unknown forms for elasticsearch case"

2350424

This reverts commit 5985875.

Reapply "Ignore unknown forms for elasticsearch case"

caefff8

This reverts commit 2350424.

jingcheng16 approved these changes Sep 3, 2024

View reviewed changes

proteusvacuum force-pushed the fr/deleted-apps-2 branch 2 times, most recently from a5c3e79 to caefff8 Compare September 3, 2024 13:18

Merge branch 'master' into fr/deleted-apps-2

c8361e0

Merge branch 'master' into fr/deleted-apps-2

2ce01d0

proteusvacuum requested a review from jingcheng16 September 3, 2024 13:59

Merge branch 'master' into fr/deleted-apps-2

28e2d6b

jingcheng16 approved these changes Oct 13, 2024

View reviewed changes

jingcheng16 merged commit 266d4f6 into dimagi:master Oct 13, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Elasticsearch to fetch applications for exports #34266

Use Elasticsearch to fetch applications for exports #34266

proteusvacuum commented Mar 12, 2024 •

edited

Loading

millerdev left a comment

millerdev Jun 19, 2024

millerdev Jun 19, 2024

proteusvacuum Jul 1, 2024

millerdev Jun 19, 2024 •

edited

Loading

proteusvacuum Jul 1, 2024

jingcheng16 Jun 20, 2024

proteusvacuum Jul 1, 2024

jingcheng16 commented Jun 21, 2024

proteusvacuum commented Jul 1, 2024

jingcheng16 left a comment

Use Elasticsearch to fetch applications for exports #34266

Use Elasticsearch to fetch applications for exports #34266

Conversation

proteusvacuum commented Mar 12, 2024 • edited Loading

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Rollback instructions

Labels & Review

millerdev left a comment

Choose a reason for hiding this comment

millerdev Jun 19, 2024

Choose a reason for hiding this comment

millerdev Jun 19, 2024

Choose a reason for hiding this comment

proteusvacuum Jul 1, 2024

Choose a reason for hiding this comment

millerdev Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

proteusvacuum Jul 1, 2024

Choose a reason for hiding this comment

jingcheng16 Jun 20, 2024

Choose a reason for hiding this comment

proteusvacuum Jul 1, 2024

Choose a reason for hiding this comment

jingcheng16 commented Jun 21, 2024

proteusvacuum commented Jul 1, 2024

jingcheng16 left a comment

Choose a reason for hiding this comment

proteusvacuum commented Mar 12, 2024 •

edited

Loading

millerdev Jun 19, 2024 •

edited

Loading