Add server-side Redis caching layer to Relay store backend #5684
Conversation
damassi
force-pushed
the
caching-sketch
branch
3 times, most recently
from
27c6ec9
to
92d5379
Compare
| export interface CacheConfig { | ||
| size: number | ||
| ttl: number | ||
| } |
There was a problem hiding this comment.
Here's where these values are set: https://github.com/artsy/force/blob/master/src/v2/Artsy/Relay/createRelaySSREnvironment.ts#L98-L99
damassi
force-pushed
the
caching-sketch
branch
3 times, most recently
from
c897224
to
add2d48
Compare
| const client = redis.createClient() | ||
|
|
||
| this.redisCache = { | ||
| del: promisify(client.del).bind(client), |
There was a problem hiding this comment.
There was a problem hiding this comment.
Two other notes:
- @joeyAghion => [Performance] Cache unauthenticated pages, configurable via ENV #5047 incorporated A/B test assignments into cache keys to avoid serving incorrect markup. Something similar might be necessary here.
- Only render one breakpoint for mobile googlebot reaction#3085 serves only one breakpoint to Googlebot based on user-agent. If it's still in place (in this repo), we might need to handle that and any other special cases by either updating the caching logic or encoding them in cache keys.
damassi
force-pushed
the
caching-sketch
branch
2 times, most recently
from
8cbd06d
to
ceff36d
Compare
damassi
force-pushed
the
caching-sketch
branch
2 times, most recently
from
c6dda7f
to
0fcdccf
Compare
There was a problem hiding this comment.
- We shouldn't merge this until the datadog tracing is restored since that will be necessary to evaluate its impact.
- I can't tell if this is scoped to any particular requests (like for the artist page). I think it should be, at least initially. There are too many state- or time-dependent pages that would need special care otherwise.
@icirellik and I discussed some possibilities here but not quite sure what next steps are or even if there's a straightforward path forward. We currently are tracing all requests, but they happen to be forwarded to
Right now it applies to all routes that do not have Can you think of some other state or time dependent pages where we would want this turned off when logged out? The only apps that this covers are those located here. Most stateful behavior requires a logged-in state. But either way, we can configure this however. |
|
Regarding which apps might not be OK to cache, I'm thinking of things like viewing rooms, shows, features, auctions, and fairs, which all have time dependent states and treatments. I agree that this approach might work in most cases but I'd rather scope it more narrowly to start. Not a blocker if you feel strongly that this is a better default though. If an app does want to opt out, would it just set |
Aside from viewing rooms and our new features, the other apps are all in our old system and don't apply. Features don't have any real state, and viewing rooms have an expiration (but the exipiration +- 15m seems like an ok tradeoff). With our cache expiry currently set to 15 minutes I'm not too worried about things when logged out, but should return to this once it's out and we get a better look at the numbers. Lets see how it goes for a minute, we don't have much state in the new AppShell other than the Artwork page.
All config is co-located on the route so we can tune this however we see fit, eg: // Simple version
path: '/artist/:id',
Component: ArtistApp,
cacheConfig: {
force: true
}
// or
path: '/artist/:id',
Component: ArtistApp,
getCacheConfig: (route, { context }) => {
if (route.pathname === 'foo' && context.user) {
return {
force: true
}
}
} |
|
(Handing this off to @mzikherman - ty!) |
| }) | ||
|
|
||
| describe("server", () => { | ||
| // it("does not set cache if enableServerSideCaching=false", async () => { |
There was a problem hiding this comment.
These are good specs, just need to figure out how to mock the new cacheClient properly (and keep these specs/assertions).
Wanted to comment out to see if there was other test issues/flakiness in the global suite on CI before revisiting this known one.
|
Ok, i think this is good to go (🤞 🍏 ). Since the Datadog monitoring has been fixed, I think we can merge this in, and check on staging + merge to production? We can monitor latency by endpoint (we might expect artist page latency to increase, since we'll be disabling the page cache, but the other non-artwork V2 apps should have their latency improve). We can also check to see how the cache is working by doing Papertrail searches (can see the writes/reads, similar to page caching. The log lines are prefixed with "[RelayCache]", etc.). |
With the new AppShell we run into a fundamental incompatibility around full-page caching. A number of the issues have been outlined here. (See original PR: #5047.)
A better approach is to utilize the isomorphic relay caching middleware infra we already have in place (currently only used on the client), and do response caching backed by Redis on the server.
The flow:
/artist/id(or whatever route ATM, I haven't scoped things in this POC)artistID)I achieved this by taking existing
cacheMiddlewareand writing a light wrapper around it that communicates with Redis. Might be something to think about PR'ing upstream once the idea is fleshed out, some kind of callback hook that taps into the middleware's get / set lifecycle for a secondary backing datastore.Stats
Some preliminary stats running from my local machine (I imagine it will be faster in prod):
Artist
No cache
Cache
Comparison with full page caching
See example implementation here: d69d4a0
Artwork
No cache
Cache
Collection
No cache
Cache