cloudflare / Cloudflare-WordPress

A Cloudflare plugin for WordPress
https://www.cloudflare.com/wordpress/
BSD 3-Clause "New" or "Revised" License
215 stars 84 forks source link

Feature purge on cron #504

Open midweste opened 1 year ago

midweste commented 1 year ago

With the limitations and long run time of clearing related urls, and given the delay of clearing the edge caches, I've added the ability to purge aggregated entries on cron

By default, the existing behavior is maintained if the 'cloudflare_purge_on_cron' is not returning true

lkraav commented 1 year ago

All commented out code should probably be removed before ready for review?

midweste commented 1 year ago

All commented out code should probably be removed before ready for review?

Will remove, sorry about that.

In regard to a separate pull request that addresses the 1200 urls per 5 minutes:

The method purgeCacheByRelevantURLs is problematic as written because:

What do you think about splitting that method into two - one method that gets related urls, and another method that actually does the zonePurgeFiles? This would allow us to implement adherence to the API limit in the purging function

lkraav commented 1 year ago

In regard to a separate pull request that addresses the 1200 urls per 5 minutes:

Which PR is this? I haven't paid attention maybe, but I didn't even know there was some kind of 1200 / 5min limit.

midweste commented 1 year ago

In regard to a separate pull request that addresses the 1200 urls per 5 minutes:

Which PR is this? I haven't paid attention maybe, but I didn't even know there was some kind of 1200 / 5min limit.

PR doesn't yet exist, mainly asking for guidance in regard to direction before actually doing the work and testing.

Here is the general API rate limit documentation: https://developers.cloudflare.com/fundamentals/api/reference/limits/ Here is the api page for purge by url: https://api.cloudflare.com/#zone-purge-files-by-url Free tier is limited to 1000 urls per minute, but paid tier limits are not specified.

I'll see if I can get verification from support about a rate limit regarding paid purge by url.

elliott-stocks commented 1 year ago

I think this is a good use case for leveraging Cloudflare Queues to handle purging of the cache. WordPress would send the URLs to a queue which would be responsible for purging the cache via a Worker.

Since it's a queue it would reduce the risk of hitting any API limits.

This would introduce additional setup steps for users, so having it as optional would be great. It would be really useful for enterprise sites to offload the heavy lifting to Queues and Workers.

Thoughts?

midweste commented 1 year ago

I think this is a good use case for leveraging Cloudflare Queues to handle purging of the cache. WordPress would send the URLs to a queue which would be responsible for purging the cache via a Worker.

Since it's a queue it would reduce the risk of hitting any API limits.

This would introduce additional setup steps for users, so having it as optional would be great. It would be really useful for enterprise sites to offload the heavy lifting to Queues and Workers.

Thoughts?

I like the idea. I'll look more at the queue documentation as I'm not familiar.

I see that it requires a worker addition to the plan which is fine for most cases, but maybe the cron rate limited version is still valuable for free plan users.

The documentation has since changed and I can't seem to find the rate limit I had seen previously. Can you confirm there is a rate limit for purge by URL requests?

elliott-stocks commented 1 year ago

The documentation has since changed and I can't seem to find the rate limit I had seen previously. Can you confirm there is a rate limit for purge by URL requests?

@midweste On this page - https://developers.cloudflare.com/cache/how-to/purge-cache/ I see the following mentioned -

The single-file purge rate limit for the Free subscription is 1000 urls/min. The rate limit is subject to change

github-actions[bot] commented 1 year ago

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ivptr commented 1 year ago

Any update on this?

Saving any post takes 2-3 seconds due to CF\WordPress\Hooks::purgeCacheOnPostStatusChange function. This is very frustrating.

github-actions[bot] commented 7 months ago

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jordantrizz commented 7 months ago

Has this been addressed somewhere else or should it stay open?

ivptr commented 7 months ago

It should stay open, it's not addressed anywhere.

midweste commented 7 months ago

Has this been addressed somewhere else or should it stay open?

The PR as is works as expected, however, to improve on it I was looking for guidance in regard to the existing method purgeCacheByRelevantURLs https://github.com/cloudflare/Cloudflare-WordPress/blob/dd13e1509194ee0a15c4f737082d39cdc226ad71/src/WordPress/Hooks.php#L140

Thanks!

ivptr commented 7 months ago

Maybe cronPurgeQueue() function can take care of this, so it purges max 1000 URLs of an array and leave the rest for the next cron job?

Current limit: The single-file purge rate limit for the Free subscription is 1,000 URLs/minute. The rate limit is subject to change.: https://developers.cloudflare.com/cache/how-to/purge-cache/purge-by-single-file/#:~:text=The%20single%2Dfile%20purge%20rate%20limit%20for%20the%20Free%20subscription%20is%201%2C000%20URLs/minute.%20The%20rate%20limit%20is%20subject%20to%20change.

midweste commented 7 months ago

Maybe cronPurgeQueue() function can take care of this, so it purges max 1000 URLs of an array and leave the rest for the next cron job?

Current limit: The single-file purge rate limit for the Free subscription is 1,000 URLs/minute. The rate limit is subject to change.: https://developers.cloudflare.com/cache/how-to/purge-cache/purge-by-single-file/#:~:text=The%20single%2Dfile%20purge%20rate%20limit%20for%20the%20Free%20subscription%20is%201%2C000%20URLs/minute.%20The%20rate%20limit%20is%20subject%20to%20change.

The cloudflare_related_urls in cronPurgeQueue are just post_ids that have yet to determine their related urls, therefore you unfortunately cannot determine how many related urls are going to be added for every post_id you feed the purge method because they compute at run time. That's the main issue with the method both computing the related urls AND purging in one step. Also, there is no proper return so you can't determine even how many were purged in the previous request

In the current state of this method, you could feed a single post_id that has 2000 related urls and you would never know that the remaining 1000 urls were not purged

ivptr commented 7 months ago

So actually there is no other way but to split purgeCacheByRelevantURLs?

jordantrizz commented 7 months ago

Just some thoughts.

midweste commented 7 months ago

Just some thoughts.

  • Shouldn't there be a purge method set, either purge all or by URL? There is a purge all IIRC, and also API call for zonePurgeFiles

  • Implement a max number of URLs if purge by URL is set and then automatically a purge all is completed? Good idea, this could vary per customer based on how many things they have cached

  • If purge by URL purges 80% of the site from Cloudflare cache, then shouldn't purge all be done? Indeed, but for us to know that, we'd need to know how many files in cache total, and have the relatedUrls broken out so we knew before we tried to purge via purgeCacheByRelevantURLs if we met this threshold

  • Perhaps this is where tags can come in handy, pages cached by Cloudflare should have a unique tag for relatedURL's? This I'm not sure of, if for instance we changed a taxonomy slug, that would need to purge everything. Worth looking into, but maybe a bit beyond the scope of "next steps"

We can at least agree it seems that purgeCacheByRelevantURLs should break out the part that calculated related urls

midweste commented 7 months ago

All this said though, I don't see any reason why this PR can't be merged in. All the things we are talking about are different PRs

ivptr commented 7 months ago

@aseure please advise on this.

jordantrizz commented 7 months ago

We can at least agree it seems that purgeCacheByRelevantURLs should break out the part that calculated related urls

Yes.

All this said though, I don't see any reason why this PR can't be merged in. All the things we are talking about are different PRs

Yes, and ultimately this should be discussed in an issue and from there a PR should be created. So I'll stop posting here.

midweste commented 5 months ago

Would be glad to put more pull requests in, but not if maintainers do not respond or are not interested in these improvements