Redis is giving us a big performance boost (it’s roughly halved the
median request time on the admin app).
Once we’re confident that it’s working properly[1] we can eke out a bit
more performance from it by keeping the caches alive for longer. As
far as I can tell we’re still using Redis in a very low-volume way[2],
so increasing the number of things we’re storing shouldn’t start taxing
our Redis server at all. But reducing the number of times we have to
hit the API to refresh the cache _should_ result in some performance
increase.
---
1. ie we’re not seeing instances of stale caches not being invalidated
2. We have 2.5G of available space in Redis. Here is our current usage:
```
used_memory:7728960
used_memory_human:7.37M
used_memory_rss:7728960
used_memory_peak:16563776
used_memory_peak_human:15.79M
used_memory_lua:37888
```
This is easier to read than having to understand the arguments 1…n of
the cache decorator are ‘magic’, and gives us more flexibility about
how the cache keys are formatted, eg being able to add words in the
middle of them.
Also changes the key format for all templates to be
`service-{service_id}-templates` instead of `templates-{service_id}`
because then it’s clearer what the ID represents.
A lot of the frequently-used pages in the admin app rely on the API to
get templates.
So this commit adds three new caches:
- a single template version (including a key without a version number,
which is the current version)
- all the templates for a service
- all versions of a template
The first will be the most crucial for performance, but there’s not much
cost to adding the other two.
`@cache.delete('user', 'user_id')` is easier to read and understand than
`@cache.delete('user', key_from_args=[1])`. This will become even more
apparent if we have to start doing stuff like `key_from_args=[1, 5]`,
which is a lot more opaque than just saying
`'service_id', 'template_id'`.
It does make the implementation a bit more complex, but I’m not too
worried about that because:
- the tests are solid
- it’s nicely encapsulated
This `*params` argument seems to be copy/pasted boilerplate. It’s not
used by any consumers of this client, and makes it harder to write a
decorator for this function.
Accepting an invite changes:
- the `user_to_service` list of users returned by `GET /service/<id>`
- the `services` list return by `GET /user/<id>`
The latter change is causing the functional tests to fail.
Changed the Python link on the Documentation page to link to the new
Python docs (docs.notifications.service.gov.uk/python.html) instead of
the Python client GitHub repo.
In the same way, and for the same reasons that we’re caching the service
object.
Here’s a sample of the data returned by the API – so we should make sure
that any changes to this data invalidate the cache.
If we ever change a user’s phone number (for example) directly in the
database, then we will need to invalidate this cache manually.
```python
{
'data':{
'organisations':[
'4c707b81-4c6d-4d33-9376-17f0de6e0405'
],
'logged_in_at':'2018-04-10T11:41:03.781990Z',
'id':'2c45486e-177e-40b8-997d-5f4f81a461ca',
'email_address':'test@example.gov.uk',
'platform_admin':False,
'password_changed_at':'2018-01-01 10:10:10.100000',
'permissions':{
'42a9d4f2-1444-4e22-9133-52d9e406213f':[
'manage_api_keys',
'send_letters',
'manage_users',
'manage_templates',
'view_activity',
'send_texts',
'send_emails',
'manage_settings'
],
'a928eef8-0f25-41ca-b480-0447f29b2c20':[
'manage_users',
'manage_templates',
'manage_settings',
'send_texts',
'send_emails',
'send_letters',
'manage_api_keys',
'view_activity'
],
},
'state':'active',
'mobile_number':'07700900123',
'failed_login_count':0,
'name':'Example',
'services':[
'6078a8c0-52f5-4c4f-b724-d7d1ff2d3884',
'6afe3c1c-7fda-4d8d-aa8d-769c4bdf7803',
],
'current_session_id':'fea2ade1-db0a-4c90-93e7-c64a877ce83e',
'auth_type':'sms_auth'
}
}
```
Some time between version 6.32 and 6.34 of the Cloudfoundry CLI the
ability to redirect the output of a command into `cf push -f` was
broken.
The only alternative we can think of is writing the file to disk, doing
the deploy, and then deleting it.
We’re careful to write to a directory outside the current repo to avoid:
- including secrets in the deployed package
- accidentally checking the secrets into source control
`/tmp/` seems to be a good place to put it, since, even if the delete
doesn’t run, it will get cleaned up eventually (probably when the
machine next boots).
Right now this only applies to people deploying from their local
machines. At some point it will affect Jenkins too, but isn’t now. So
this commit only fixes the problem for the commands that developers run
locally.
fixup! Write manifests to disk instead of redirecting
Most of the time spent by the admin app to generate a page is spent
waiting for the API. This is slow for three reasons:
1. Talking to the API means going out to the internet, then through
nginx, the Flask app, SQLAlchemy, down to the database, and then
serialising the result to JSON and making it into a HTTP response
2. Each call to the API is synchronous, therefore if a page needs 3 API
calls to render then the second API call won’t be made until the
first has finished, and the third won’t start until the second has
finished
3. Every request for a service page in the admin app makes a minimum
of two requests to the API (`GET /service/…` and `GET /user/…`)
Hitting the database will always be the slowest part of an app like
Notify. But this slowness is exacerbated by 2. and 3. Conversely every
speedup made to 1. is multiplied by 2. and 3.
So this pull request aims to make 1. a _lot_ faster by taking nginx,
Flask, SQLAlchemy and the database out of the equation. It replaces them
with Redis, which as an in-memory key/value store is a lot faster than
Postgres. There is still the overhead of going across the network to
talk to Redis, but the net improvement is vast.
This commit only caches the `GET /service` response, but is written in
such a way that we can easily expand to caching other responses down the
line.
The tradeoff here is that our code is more complex, and we risk
introducing edge cases where a cache becomes stale. The mitigations
against this are:
- invalidating all caches after 24h so a stale cache doesn’t remain
around indefinitely
- being careful when we add new stuff to the service response
---
Some indicative numbers, based on:
- `GET http://localhost:6012/services/<service_id>/template/<template_id>`
- with the admin app running locally
- talking to Redis running locally
- also talking to the API running locally, itself talking to a local
Postgres instance
- times measured with Chrome web inspector, average of 10 requests
╲ | No cache | Cache service | Cache service and user | Cache service, user and template
-- | -- | -- | -- | --
**Request time** | 136ms | 97ms | 73ms | 37ms
**Improvement** | 0% | 41% | 88% | 265%
---
Estimates of how much storage this requires:
- Services: 1,942 on production × 2kb = 4Mb
- Users: 4,534 on production × 2kb = 9Mb
- Templates: 7,079 on production × 4kb = 28Mb
* Juror Central Summoning Bureau - Online – HM Courts & Tribunals Service
* Richmond and Wandsworth Council – Richmond and Wandsworth Council
* ESFA feedback – Department for Education
* PINS Digital Service – Planning Inspectorate
* HMPO Belfast – Home Office
* food.gov.uk – Food Standards Agency
* NCC Mobile Team – Newcastle City Council
* Cambs CC Emergency planning – Cambridgeshire County Council
* HMPO Durham – Home Office
* HMPO London – Home Office
* HMPO Glasgow – Home Office
* HMPO Peterborough – Home Office
* HMPO Southport – Home Office
* Student Loans Company. Password Reset. – Student Loans Company
* DigITS – Crown Commercial Service
* Ofsted Reports Beta – Ofsted
* Revenue Accounts – Ministry of Housing, Communities & Local Government
* Luton Council - Electoral Registration – Luton Council
* Cael eich Pensiwn y Wladwriaeth – Department for Work and Pensions
* Warwick Crown Court – Ministry of Justice
* ProLive - Housing Repairs – Pembrokeshire County Council
* Wirral Council – Wirral Council
* DfT Bus Open Data – Department for Transport
* Paris File Watcher – Pembrokeshire County Council
* WFDC Electoral Services – Wyre Forest District Council
Adding a ‘testing’ template it not enough. It needs to have some real
looking content, so that we can:
- work out what a service is doing
- assess whether that’s a reasonable (ie meeting the terms of use) thing
to be doing with Notify
At the moment we’re having to go back to services quite a lot when they
request to go live and ask them for this stuff.
