ISOM-1006: Optimize verifySiteMember()

[timotheeg]

Problem

verifySiteMember() is a potential cause for the DB load we see. It is very costly for what it does, and we run it all the time as part of the auth flow.

verifySiteMember() is a potential cause for the DB load we see

Some flaws exhibited

• Multiple consecutive query on the isomeradmin tables for the same users
• Fetching more data than needed when we actually care for a exists() boolean output
• getRole() issues an extremely costly query, involving multiple full table inner joins and json processing.

Sample trace

The 3 queries are the most costly operations IsomerCMS runs, both in term of number of queries and in term of time spent:

Solution

This PR will be fully backward compatible but aims to reduce the cost of verifySiteMember().

Expected outcomes

• No more duplicate queries for the same isomeradmin user
• Faster middleware processing for verifySiteMember() (because query in getRole will be 4x+ faster)
• Reduced DB CPU load
• Less time spent overall by the app over these queries, visible in this view in DataDog

Note:

• Through this PR, I noticed that the CollaboratorsService is instantiated twice in the codebase (in server.ts and in common/index.ts), which seems wrong, I have opened a separate linear ticket to have that cleaned up)
• When actual isomer admins visit a site they are not a member of, there will still be 2 duplicated query. Further refactor and optimization work could be undertaken:
  • Implement local cache for site memberships
  • Add local cache for getUserId()
  • Refactor code: is it right that getRole() for a site can return isomerAdmin? And if the role is admin, perhaps further processing in verifySiteMember()

Verified on staging:
Sample trace here

Reference:

• Linear ticket

[linear]

ISOM-1006 Optimize verifySiteMember

[timotheeg]

I tried 2 possible query optimizations (see details in this commit for details)

Results are as below in this sample trace

original: 15.8ms
optimization option 1: 3.49ms
optimization option 2: 2.07ms

Optimization 1 retains the check for soft deletion of users, repos, and sites, while optimization 2 makes an efficient query on just repos and site_members, and assumes both the site and the user are not soft deleted (which may or may not cause trouble).

Optimization 1 is still 4x more performant than the original, so we could start by releasing that and we can keep optimization 2 as a potential improvement for the future.

@harishv7 any comment on the continuous check for soft deletions in every query?

[timotheeg]

Query plan reports from postgres on the 3 queries:

original:

optimization 1:

optimization 2:

[timotheeg]

Also for reference, below is an example of the actual query sequelize sent to the DB (on staging)
(It's easier to read than the normalized version in the DD traces)

SELECT 
    "Site".*,
    "site_members"."id" AS "site_members.id",
    "site_members"."email" AS "site_members.email",
    "site_members"."github_id" AS "site_members.githubId",
    "site_members"."contact_number" AS "site_members.contactNumber",
    "site_members"."last_logged_in" AS "site_members.lastLoggedIn",
    "site_members"."created_at" AS "site_members.createdAt",
    "site_members"."updated_at" AS "site_members.updatedAt",
    "site_members"."deleted_at" AS "site_members.deletedAt",
    "site_members->SiteMember"."id" AS "site_members.SiteMember.id",
    "site_members->SiteMember"."user_id" AS "site_members.SiteMember.userId",
    "site_members->SiteMember"."site_id" AS "site_members.SiteMember.siteId",
    "site_members->SiteMember"."role" AS "site_members.SiteMember.role",
    "site_members->SiteMember"."created_at" AS "site_members.SiteMember.createdAt",
    "site_members->SiteMember"."updated_at" AS "site_members.SiteMember.updatedAt"
FROM
(
    SELECT
        "Site"."id",
        "Site"."name",
        "Site"."site_status" AS "siteStatus",
        "Site"."job_status" AS "jobStatus",
        "Site"."is_private" AS "isPrivate",
        "Site"."created_at" AS "createdAt",
        "Site"."updated_at" AS "updatedAt",
        "Site"."deleted_at" AS "deletedAt",
        "Site"."creator_id" AS "creatorId",
        "repo"."id" AS "repo.id",
        "repo"."name" AS "repo.name",
        "repo"."url" AS "repo.url",
        "repo"."created_at" AS "repo.createdAt",
        "repo"."updated_at" AS "repo.updatedAt",
        "repo"."deleted_at" AS "repo.deletedAt",
        "repo"."site_id" AS "repo.siteId"
    
    FROM
        "sites" AS "Site" INNER JOIN "repos" AS "repo"
            ON
                "Site"."id" = "repo"."site_id"
                AND ("repo"."deleted_at" IS NULL AND "repo"."name" = 'kishore-test')
            
    WHERE
        ("Site"."deleted_at" IS NULL) 
        AND ( 
            SELECT
                "SiteMember"."id"

            FROM "site_members" AS "SiteMember" INNER JOIN "users" AS "User"
                ON 
                    "SiteMember"."user_id" = "User"."id"
                    AND ("User"."deleted_at" IS NULL AND "User"."id" = '119')

            WHERE ("Site"."id" = "SiteMember"."site_id")
            LIMIT 1
        ) IS NOT NULL
    LIMIT 1

)
AS "Site" INNER JOIN
(
    "site_members" AS "site_members->SiteMember" INNER JOIN "users" AS "site_members"
        ON 
            "site_members"."id" = "site_members->SiteMember"."user_id"
)
ON 
    "Site"."id" = "site_members->SiteMember"."site_id"
    AND ("site_members"."deleted_at" IS NULL AND "site_members"."id" = '119');

[kishore03109]

nit: can either limit 1 or can just add a comment stating that the if statement is done defensively, practically we only expect to see one or 0 elements in the array

[timotheeg]

Hey @kishore03109 btw, I'm a litttle confused with something: the original query had a check for site_members.deleted_at, but I do not see that field in prod DB.. How can this be? 🤔

Never mind! OMG sequelize does this INNER JOIN "users" AS "site_members"
which is a very confusing alias 😅

Oh I also note from looking at the query plans that we are missing an index for repos.name. I opened a ticket to add it. @alexanderleegs FYI

[timotheeg]

I updated the IsomerCMS dashboard graph to show the DB CPU load this week versus last week, so we can see at glance whether we're doing better or worse.

Direct link to widget here