Skip to content
New issue

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

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

Already on GitHub? # to your account

fix-slow-family-endpoint-in-the-admin-service #285

Conversation

odrakes-cpr
Copy link
Contributor

@odrakes-cpr odrakes-cpr commented Jan 27, 2025

Description

The search endpoint api/v1/families/?q= for families was running quite slow in the admin service, taking over 6 seconds to return a response. Notably on the cclw user admin account which has the most amount of documents and families, we suspected that this had something to do with the multiple geographies changes that we made.

To resolve this we have gone with using a raw sql query and instead executing that.

  • The get endpoint works fine and does not seem to be an intensive process so we will leave that be for now.
  • This update relates to the search endpoint even though the title says all endpoint, the endpoint of concern is the search
  • We have tried to replicate the query as best as we can, retrieving the family slugs, family documents, events etc in the main query rather than running a separate query in the serialization process (_family_to_dto......)
  • The main query should replicate the same joins etc and where values are not present in the join, such as document ids which would be easier accessed through the orm, running subqueries to get those values, i.e slugs/geos
  • Where some of the properties required for the front are actually hybrid properties and therefore not accessible in a sql query as they sit on the orm, we have tried to match the calculation of those values see family_status|last_updated_date|published_date in the family models.

These are represented in the sql query as such

        CASE
            WHEN EXISTS (
                SELECT 1
                FROM family_document fd
                WHERE fd.family_import_id = f.import_id
                AND fd.document_status = 'PUBLISHED'
            ) THEN 'PUBLISHED'
            WHEN EXISTS (
                SELECT 1
                FROM family_document fd
                WHERE fd.family_import_id = f.import_id
                AND fd.document_status = 'CREATED'
            ) THEN 'CREATED'
            ELSE 'DELETED'
        END AS family_status,
        (
            SELECT MAX(fe.date)
            FROM family_event fe
            WHERE fe.family_import_id = f.import_id
        ) AS last_updated_date,
        (
            SELECT MIN(fe.date)
            FROM family_event fe
            WHERE fe.family_import_id = f.import_id
            AND EXISTS (
                    SELECT 1
                    FROM jsonb_array_elements_text(fe.valid_metadata::jsonb->'datetime_event_name') AS datetime_event_name
                    WHERE datetime_event_name = fe.event_type_name
                )
        ) AS published_date
  • Finally for the query, we have passed in variables/params into the main query rather than the actual values to prevent against sql injection or anything going awry with user inputs.
  • Running the raw sql query and the dto mapping now takes and average of 1.5seconds as illustrated in the docker logs below, which solves the problem of a slow endpoint as the query is coming back quicker.
Screenshot 2025-01-29 at 12 40 45

Proposed version

Please select the option below that is most relevant from the list below. This
will be used to generate the next tag version name during auto-tagging.

  • Skip auto-tagging
  • Patch
  • Minor version
  • Major version

Visit the Semver website to understand the
difference between MAJOR, MINOR, and PATCH versions.

Notes:

  • If none of these options are selected, auto-tagging will fail
  • Where multiple options are selected, the most senior option ticked will be
    used -- e.g. Major > Minor > Patch
  • If you are selecting the version in the list above using the textbox, make
    sure your selected option is marked [x] with no spaces in between the
    brackets and the x

Type of change

Please select the option(s) below that are most relevant:

  • Bug fix
  • New feature
  • Breaking change
  • GitHub workflow update
  • Documentation update
  • Remove legacy code
  • Dependency update

How Has This Been Tested?

Please describe the tests that you added to verify your changes.

Reviewer Checklist

  • DB_CLIENT DEPENDENCY IS ON THE LATEST VERSION
  • The PR represents a single feature (small drive-by fixes are also ok)
  • The PR includes tests that are sufficient for the level of risk
  • The code is sufficiently commented, particularly in hard-to-understand areas
  • Any required documentation updates have been made
  • Any TODOs added are captured in future tickets
  • No FIXMEs remain

Osneil Drakes and others added 5 commits January 13, 2025 17:10
@odrakes-cpr odrakes-cpr requested a review from a team as a code owner January 27, 2025 09:53
@odrakes-cpr odrakes-cpr marked this pull request as draft January 27, 2025 09:54
Osneil Drakes added 7 commits January 28, 2025 13:27
- handle filters, pass in where conditions appropriately
- order by last modified
- will pass query params into the raw sql query itself to prevent
  against sql injection
- family status is not a column in the db but a property on the
  model/orm, so we can't access it directly in the query - have tried to mimic it in the main query and filter
- move the query for slugs into the main query, via a left sided join
- add in back limit results by max results, we know this will be passed
  as a query param or use the default that will be set
- todo access teh published date and last updated date
@odrakes-cpr
Copy link
Contributor Author

app/repository/family.py Outdated Show resolved Hide resolved
Copy link
Contributor

@annaCPR annaCPR left a comment

Choose a reason for hiding this comment

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

suggestion: Would you mind documenting in the PR description why and how this solution fixes the problem we had with the endpoint? Just so that I don't have to try to figure it out from the SQL :-P

@odrakes-cpr
Copy link
Contributor Author

Resulting query -
Screenshot 2025-01-29 at 12 27 44

@odrakes-cpr odrakes-cpr changed the title Fix family all endpoint running slow due to an intensive query fix-slow-family-endpoint-in-the-admin-service Jan 29, 2025
Osneil Drakes added 2 commits January 29, 2025 15:24
- this was a gnarly bug to find and fix, thank god for our tests
- with the initial inner joins when we would have null values for event
  ids or document ids the query would ignore those records when
  returning the results.
- adding a left join means that we still get those values back, for
  instances where a family is created but does not yet have an allocated
  document or event
- the readdto does not allow for nonetypes so have converted this to an
  empty array as expected.
@odrakes-cpr
Copy link
Contributor Author

suggestion: Would you mind documenting in the PR description why and how this solution fixes the problem we had with the endpoint? Just so that I don't have to try to figure it out from the SQL :-P

Sure hopefully that has helped.

@odrakes-cpr odrakes-cpr marked this pull request as ready for review January 29, 2025 19:25
Copy link
Contributor

@jamesgorrie jamesgorrie left a comment

Choose a reason for hiding this comment

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

Really nice investigative work - I wish we had some metrics to show the improvement in real time.

Might be worth getting current local metrics and compare them so we can see the improvement.

app/repository/family.py Outdated Show resolved Hide resolved
app/repository/family.py Outdated Show resolved Hide resolved
app/repository/family.py Show resolved Hide resolved
app/repository/helpers.py Show resolved Hide resolved
@odrakes-cpr odrakes-cpr merged commit 547e615 into main Feb 3, 2025
10 checks passed
@odrakes-cpr odrakes-cpr deleted the fix-family-all-endpoint-running-slow-due-to-an-intensive-query branch February 3, 2025 13:03
@katybaulch katybaulch restored the fix-family-all-endpoint-running-slow-due-to-an-intensive-query branch February 3, 2025 13:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants