Perf: Better response times from api/public/v1/courses

[inulty-dfe]

Context

Counting the number of courses in a recruitment cycle is very expensive. We do this on every request to the courses endpoint.

This is an "agonising" endpoint in Skylight

Loadtesting the change

The tests were run with Siege.

Siege documentation

All tests were run against review apps. The review apps all had 1 replica with 512MiB memory.

Before changes

inulty-dfe ~/Code/publish-teacher-training ( perf-api-providers) $ siege -c 10 -t 1m -if app/controllers/api/loadtest-urls-other-branch.txt

{       "transactions":                           91,
        "availability":                       100.00,
        "elapsed_time":                        59.50,
        "data_transferred":                    19.95,
        "response_time":                        5.97,
        "transaction_rate":                     1.53,
        "throughput":                           0.34,
        "concurrency":                          9.13,
        "successful_transactions":                31,
        "failed_transactions":                     0,
        "longest_transaction":                 19.52,
        "shortest_transaction":                 0.97
}71
}

With just loaded? changes

inulty-dfe ~/Code/publish-teacher-training ( perf-api-providers) $ siege -c 10 -t 1m -if app/controllers/api/loadtest-urls-no-cache.txt

{       "transactions":                           75,
        "availability":                       100.00,
        "elapsed_time":                        59.49,
        "data_transferred":                    26.39,
        "response_time":                        7.16,
        "transaction_rate":                     1.26,
        "throughput":                           0.44,
        "concurrency":                          9.03,
        "successful_transactions":                45,
        "failed_transactions":                     0,
        "longest_transaction":                 19.76,
        "shortest_transaction":                 1.11
}

With count query cached and loaded? changes

inulty-dfe ~/Code/publish-teacher-training ( perf-api-providers) $ siege -c 10 -t 1m -if app/controllers/api/loadtest-urls.txt

{       "transactions":                           94,
        "availability":                       100.00,
        "elapsed_time":                        59.86,
        "data_transferred":                    51.11,
        "response_time":                        6.07,
        "transaction_rate":                     1.57,
        "throughput":                           0.85,
        "concurrency":                          9.53,
        "successful_transactions":                84,
        "failed_transactions":                     0,
        "longest_transaction":                 13.18,
        "shortest_transaction":                 1.90
}

Result

Operation	Original	No cache changes	Cache changes
throughput	0.34	0.44	0.85
successful_transactions	31	45	84
data_transferred	19.95	26.39	51.11

The URLs

The URLs used were variations on this list.
I added no_cache for the "no cache" test run.
I changed the PR number for the "original" test run.

Changes proposed in this pull request

• Use loaded? to avoid making requests to the database when we already have records in memory.
• Cache the course record count per recruitment cycle for a period of time.
• The filter field is not used in the cache key because it is ignored in the query. The API does not filter courses.

Guidance to review

• How long should the course count be cached? I would suggest 1 day.

Things to check

• This code does not rely on migrations in the same Pull Request
• If this code includes a migration adding or changing columns, it also backfills existing records for consistency
• If this code adds a column to the DB, decide whether it needs to be in analytics yml file or analytics blocklist
• API release notes have been updated if necessary
• Required environment variables have been updated added to the Azure KeyVault
• Inform data insights team due to database changes
• Make sure all information from the Trello card is in here
• Rebased main
• Cleaned commit history
• Tested by running locally
• Attach PR to Trello card

[gms-gs]

Niceee!! <3

[Nitemaeric]

This looks like it can be generalised for all associations.

Have you considered looking into ActiveRecord/Associations/CollectionProxy? A #maximum method there might be able to address this issue across the entire app with a single change.

[inulty-dfe]

Yeah that looks tempting. but I would be careful about doing that. It's something we should discuss though, it seems like a useful thing to do.

[Nitemaeric]

Below might be something for another PR.

It might be more conventional to read the Cache-Control request header and return cached vs non-cached results.

I haven't quite dug into if that's indeed the right header, or what the header value should be (I think no-cache?), and also if my assumption is considered standard practice, but it might be worth looking into.

[inulty-dfe]

I'm happy to remove this, I don't think it should be merged. Undocumented api params probably aren't a good idea.

As for the cache headers, I believe those (response headers) are for instructing how the client should cache, not how the server should respond.

[Nitemaeric]

I think that this section is indicating that there are request header standards too, but yeah, I haven't seen it in practice yet. I wonder how the browser implements the "Disable cache" checkbox.

But yeah, I agree. Let's remove the no_cache for now.

[tomas-stefano]

Let's have a discussion about this expiration time?

[tomas-stefano]

Everything looks good. Great initiative. ✌️

Let's have a separate discussion about the cache & expiration.