Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Use the Rails cache for everything #183

Merged
merged 1 commit into from
Feb 22, 2017
Merged

Use the Rails cache for everything #183

merged 1 commit into from
Feb 22, 2017

Conversation

tijmenb
Copy link
Contributor

@tijmenb tijmenb commented Feb 21, 2017

TLDR: This replaces the homegrown cache with the Rails cache. It turns out that the Cache class doesn't do anything, causing hundreds thousands of unnecessary requests to fetch locales, templates & components.

The story starts when @issyl0 and I were investigating high 404's on origin last week. We noticed that there we're a lot of requests to static for /templates/locales/en-GB, which returns 404:

screen shot 2017-02-21 at 18 44 57

By sampling a few requests (using the govuk_request_id) we figured out that most of those requests were coming from smart-answers. Moreover, each page request was sometimes responsible for 40 to 120 subsequent API requests:

screen shot 2017-02-21 at 18 48 50

The root cause of the 404's is that static doesn't have a en-GB locale, which is smart-answers default_locale. This was temporarily fixed in https://github.com/alphagov/static/tree/hack-locales-test.

But this didn't fix the issue, we just saw the 404 being replaced by 200:

Before: lots of 404s
screen shot 2017-02-21 at 18 53 24

After: lots of 200s
screen shot 2017-02-21 at 18 53 05

This meant that somehow, slimmer's caching was broken. To investigate further, we added a bunch of statsd instrumentation to slimmer, which sends stats for each cache hit/miss. The branch was deployed in smart-answers to staging. After a while we got this graph from grafana:

screen shot 2017-02-21 at 18 55 32

It shows that there were actually no cache hits in slimmer, just a lot of misses. We can only conclude that the Cache class (which uses Singleton from the standard library) isn't actually working. We haven't been able to figure out why.

The solution is to replace the homegrown solution with Rails.cache.

After deploying this in smart-answers, we see the following in kibana:

screen shot 2017-02-21 at 19 00 54

@fields.status:200 AND @fields.application:"static" AND @fields.request: "/templates/locales/en-GB"

Which is exactly what we expect for a cache configured with expires_in: 5.minutes (one request per process).

After deploying the gem branch to smart-answers, collections, multipage-frontend and collections, we see that traffic drops significantly:

screen shot 2017-02-21 at 19 04 16

My guess is that when deployed to all frontend applications we'll see just a few hundred requests per hour for the locales, govuk_components and templates.

https://trello.com/c/D9HmkJwI

@@ -28,10 +28,7 @@ def initialize(app, *args, &block)
options[:asset_host] = Plek.current.find("static")
end

cache = Cache.instance
cache.use_cache = options[:use_cache] if options[:use_cache]
Copy link

@Davidslv Davidslv Feb 22, 2017

Choose a reason for hiding this comment

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

I might be wrong but this looks like the cause of the cache not be working, I'm not sure where options[:use_cache] is coming from but it's likely that it got forgotten to be set, so the default (false) will be use (https://github.com/alphagov/slimmer/pull/183/files#diff-3edabcdd528cf7a4390669a093f8f185L12)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, use_cache is definitely false in the smartanswers:

tijmenbrommet@production-calculators-frontend-1:~$ govuk_app_console smartanswers
irb(main):001:0> Slimmer::Cache.instance
=> #<Slimmer::Cache:0x007fb87ee53150 @cache_last_reset=2017-02-22 09:15:09 +0000, @use_cache=false, @cache_ttl=300, @data_store={}>

But I don't understand why since it is looks like its turned on in production.

The same is true for collections:

irb(main):002:0> Slimmer::Cache.instance
=> #<Slimmer::Cache:0x007ff393677e68 @cache_last_reset=2017-02-22 09:21:01 +0000, @use_cache=false, @cache_ttl=300, @data_store={}>

But not government-frontend:

irb(main):004:0> Slimmer::Cache.instance
=> #<Slimmer::Cache:0x007f54a340cde8 @cache_last_reset=2017-02-22 09:21:42 +0000, @use_cache=true, @cache_ttl=300, @data_store={}>

😕

@boffbowsh
Copy link
Contributor

Is the Rails cache always configured? For whitehall frontend there's a memcached instance on every box, but not for "normal" frontend. But we should configure this.

@tijmenb
Copy link
Contributor Author

tijmenb commented Feb 22, 2017

Good point. Rails.cache defaults to ActiveSupport::Cache::FileStore, which is maybe fine for this usage? Memcached won't grow indefinitely and will allow the processes to share the cache, so definitely better.

@NickColley
Copy link
Contributor

Nice work! 😍

Would this also speed up local development of our frontend applications? We've had a small investigation into this to try and avoid the constant calls to static.

@tijmenb
Copy link
Contributor Author

tijmenb commented Feb 22, 2017

@NickColley yeah, I think that would prevent those calls in dev for most applications.

This replaces the home-grown cache with `Rails.cache`.

It looks like some frontend apps are misconfigured are
not actually using the caching mechanism. This causes
a large number of needless requests to `static`.
@tijmenb tijmenb merged commit 52c17a5 into master Feb 22, 2017
@tijmenb tijmenb deleted the use-rails-cache branch February 22, 2017 13:21
tijmenb added a commit to alphagov/smart-answers that referenced this pull request Feb 22, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183
tijmenb added a commit that referenced this pull request Feb 22, 2017
This was introduced in #183
while copy-pasting the caching statements. Adds a test to prevent
re-occurance.
@tijmenb tijmenb mentioned this pull request Feb 22, 2017
tijmenb added a commit that referenced this pull request Feb 22, 2017
This was introduced in #183
while copy-pasting the caching statements. Adds a test to prevent
re-occurance.
tijmenb added a commit that referenced this pull request Feb 22, 2017
This was introduced in #183
while copy-pasting the caching statements. Adds a test to prevent
re-occurance.
tijmenb added a commit to alphagov/collections that referenced this pull request Feb 22, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to
`static`.
tijmenb added a commit to alphagov/smart-answers that referenced this pull request Feb 22, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183
tijmenb added a commit to alphagov/calculators that referenced this pull request Feb 22, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to `static`.

https://trello.com/c/D9HmkJwI
tijmenb added a commit to alphagov/calculators that referenced this pull request Feb 22, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to `static`.

https://trello.com/c/D9HmkJwI
tijmenb added a commit to alphagov/calendars that referenced this pull request Feb 22, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to `static`.

https://trello.com/c/D9HmkJwI
@fofr
Copy link
Contributor

fofr commented Feb 22, 2017

Awesome PR 💯 🎆
Good work @tijmenb and @issyl0

tijmenb added a commit to alphagov/smart-answers that referenced this pull request Feb 22, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183
tijmenb added a commit to alphagov/calendars that referenced this pull request Feb 22, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to `static`.

https://trello.com/c/D9HmkJwI
tijmenb added a commit to alphagov/multipage-frontend that referenced this pull request Feb 23, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to `static`.

https://trello.com/c/D9HmkJwI
tijmenb added a commit to alphagov/licence-finder that referenced this pull request Feb 23, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to `static`.

https://trello.com/c/D9HmkJwI
tijmenb added a commit to alphagov/licence-finder that referenced this pull request Feb 23, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to `static`.

https://trello.com/c/D9HmkJwI
tijmenb added a commit to alphagov/multipage-frontend that referenced this pull request Feb 23, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to `static`.

https://trello.com/c/D9HmkJwI
tijmenb added a commit to alphagov/feedback that referenced this pull request Feb 23, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will prevent this app from making many needless requests to `static`.

https://trello.com/c/D9HmkJwI
tijmenb added a commit to alphagov/frontend that referenced this pull request Feb 23, 2017
This bumps slimmer to make it use the rails cache and send a user agent:

- alphagov/slimmer#184
- alphagov/slimmer#183

It will probably not have much effect on this app, because the cache is
configured correctly here.

https://trello.com/c/D9HmkJwI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants