prismicio-community / php-kit

Community maintained development kit for Prismic and the PHP language
https://prismic.io
Other
109 stars 83 forks source link

API data cache time #85

Closed tremby closed 9 years ago

tremby commented 9 years ago

At https://github.com/prismicio/php-kit/blob/179a83363ef6c2fa120f5e2c90e5e24045eb776b/src/Prismic/Api.php#L330 the API data is cached for 5 units.

At https://github.com/prismicio/php-kit/blob/179a83363ef6c2fa120f5e2c90e5e24045eb776b/src/Prismic/Cache/CacheInterface.php#L43 the documentation for $ttl doesn't specify what the unit is.

At https://github.com/prismicio/php-kit/blob/179a83363ef6c2fa120f5e2c90e5e24045eb776b/src/Prismic/Cache/DefaultCache.php#L43 the $ttl variable is used as is and passed to apc_store, which interprets it as seconds.

  1. Please document in the interface that it means seconds, if that is indeed intended, or fix the logic if minutes were intended.
  2. Is caching the API data for as little as 5 seconds really intended? The request to get the API data is currently taking ~1 second for me on a test repository, and the thought of every five seconds every request until the value is recached taking 1 second longer than usual is unacceptable.
  3. Whether 5 seconds was intended or not, can you make that a default and have it configurable rather than hardcoded?

(edit: change links to point to source code as it was when ticket was originally opened)

erwan commented 9 years ago

Yes, it is seconds.

The API data is cached for 5 seconds only because a change in the writing room can happen at any moment, but we want to limit the number of calls as it would be useless to do several requests per seconds. Typically if you have several users accessing your site concurrently, they will all share the same cache so only one request every 5 seconds among all users will have the performance hit of the API call.

Now, 1 seconds sounds a bit long for the call to the API. We're usually below 200ms. Before making it configurable I'd rather make sure that the delay you're observing can not be improved.

Could you send a ticket to Intercom (from the Prismic UI) with your repository URL so we work that out? If the delay is caused by the latency, you can improve it on any production repository by activating the CDN. That should provide a low latency no matter where your server is located.

tremby commented 9 years ago

I think you may misunderstand me. I want to increase the cache time, even make it indefinite for my particular application, since I can use a web hook to clear the cache when a change takes place in the writing room.

Point 1 above should still be addressed, for the sake of people like me writing cache implementations. Point 2 -- fine. Point 3 -- I'd still like to see this implemented. Users like me who do make use of web hooks to clear the cache would benefit from increasing the amount of time for which the API info is cached.

Typically if you have several users accessing your site concurrently, they will all share the same cache so only one request every 5 seconds among all users will have the performance hit of the API call.

Not true. If we have 10 requests per second and the API call takes 1 second, every 5 seconds when the cache expires all 10 of the requests in the next second will hit your API, each call taking 1 second. Only requests coming in after one of those API calls has finished (and so the cache is repopulated) will then skip the API call.

tremby commented 9 years ago

Could you send a ticket to Intercom (from the Prismic UI) with your repository URL so we work that out? If the delay is caused by the latency, you can improve it on any production repository by activating the CDN. That should provide a low latency no matter where your server is located.

I'm not sure that'll be necessary. Right now I'm testing on a clone of the pies repository, in open mode, without CDN switched on. The amount of latency isn't really the point of this, it's the fact that there's any latency at all -- I want the API cache to last a lot longer and eliminate it.

tomjowitt commented 9 years ago

The points made here by @tremby are exactly the same issues as we're facing. We've swapped out the APC cache and replaced it with a custom Redis backend and would like a lot more control over this without resorting to webhooks and lots of additional code.

tremby commented 9 years ago

I got tired of waiting a long while back and in my own implementation I actually just totally overrode the amount of cache time requested by the API calls. In other words, I ignore the $ttl value given in the CacheInterface#set call and decide for myself how long to cache.

erwan commented 9 years ago

tomjowitt: your problem is the 5 seconds TTL for the API object, or is there something else?

tomjowitt commented 9 years ago

@erwan Yep, it'd be nice to have custom TTLs so our content is mostly fetched from Redis rather than expiring every 5 seconds.

tremby commented 9 years ago

@erwan, if you look at my original post, I believe @tomjowitt is asking for point 3 to be addressed. I would additionally still like point 1 to be addressed, which it never was.

Tom, just to make sure we're on the same page: the API data is cached by default for 5 units (which we know now are seconds). This is things like the latest master ref. If that has not changed (no documents have been edited) the other cached content should still be seen as fresh. The cached documents are not invalidated after five seconds. They are set to cache for an amount of time given in the response header of the API call around https://github.com/prismicio/php-kit/blob/fb61fcdb6d232ced3395127d25a4547b5679767f/src/Prismic/SearchForm.php#L305 but I don't recall off hand what sort of values come back in this header (I at least hope it's more than five seconds!)

erwan commented 9 years ago

@tremby the other pages have a max-age corresponding to 1 year, which is basically the longest you can set until the browser consider the header as bogus and ignores it completely. Since the ref in included in the URL and the ref changes when any content changes, everything except the API object can basically be kept in cache forever.

I just updated the PHPdoc for the CacheInterface: https://github.com/prismicio/php-kit/blob/master/src/Prismic/Cache/CacheInterface.php#L54

I'll probably make the "5 seconds" for the API object configurable, does an environment variable look like something that would make sense to PHP developers?

tremby commented 9 years ago

I just updated the PHPdoc for the CacheInterface

Thank you.

I'll probably make the "5 seconds" for the API object configurable, does an environment variable look like something that would make sense to PHP developers?

It wouldn't be unreasonable. I'd personally prefer a configuration hashmap passed to the API at initialization time, but understand that this would likely be a breaking change since other existing arguments configuring Prismic would be best put into the same hashmap. Or a method on the API object which can be called to change the configuration -- this wouldn't have to be breaking. Could alternatively do a combination of approaches, for example to offer an API method to configure, which would override the environment variable if present, but that might be overcomplicating it.