Cloudflare-WordPress icon indicating copy to clipboard operation
Cloudflare-WordPress copied to clipboard

Feature purge on cron

Open midweste opened this issue 2 years ago • 22 comments

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

midweste avatar Feb 07 '23 22:02 midweste

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

lkraav avatar Feb 08 '23 12:02 lkraav

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:

  • there is no error catching when it goes to purge the URLs should it exceed 1200 urls in less than 5 minutes. It silently fails to purge more than 1200 and the user would be none the wiser.
  • there is no way to know in advance how many related urls that method will try to purge because related urls are computed at purge time

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

midweste avatar Feb 08 '23 21:02 midweste

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.

lkraav avatar Feb 09 '23 09:02 lkraav

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.

midweste avatar Feb 09 '23 17:02 midweste

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?

elliott-stocks avatar Mar 06 '23 22:03 elliott-stocks

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?

midweste avatar Mar 06 '23 23:03 midweste

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

elliott-stocks avatar Mar 07 '23 19:03 elliott-stocks

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.

github-actions[bot] avatar Sep 04 '23 01:09 github-actions[bot]

Any update on this?

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

ivptr avatar Sep 07 '23 07:09 ivptr

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.

github-actions[bot] avatar Mar 07 '24 01:03 github-actions[bot]

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

jordantrizz avatar Mar 07 '24 13:03 jordantrizz

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

ivptr avatar Mar 07 '24 13:03 ivptr

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

  • As noted above: "The method purgeCacheByRelevantURLs is problematic as written because: There is no error catching when it goes to purge the URLs should it exceed 1200 urls in less than 5 minutes. It silently fails to purge more than 1200 and the user would be none the wiser. There is no way to know in advance how many related urls that method will try to purge because related urls are computed at purge time. 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"
  • Without these methods being split, there's not really a very good way to make sure there is adherence to the purge limit. I'm not interested to put in work on these methods if they won't be addressed as no one seemed to indicate they agreed it was an issue.
  • Adding a requirement of a worker (and a paid plan) is not something that I think is a good direction for initially addressing this issue. It should be built on top of an already reliable purge method

Thanks!

midweste avatar Mar 07 '24 14:03 midweste

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.

ivptr avatar Mar 07 '24 14:03 ivptr

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

midweste avatar Mar 07 '24 14:03 midweste

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

ivptr avatar Mar 07 '24 15:03 ivptr

Just some thoughts.

  • Shouldn't there be a purge method set, either purge all or by URL?
  • Implement a max number of URLs if purge by URL is set and then automatically a purge all is completed?
  • If purge by URL purges 80% of the site from Cloudflare cache, then shouldn't purge all be done?
  • Perhaps this is where tags can come in handy, pages cached by Cloudflare should have a unique tag for relatedURL's?

jordantrizz avatar Mar 07 '24 16:03 jordantrizz

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 avatar Mar 07 '24 20:03 midweste

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

midweste avatar Mar 07 '24 20:03 midweste

@aseure please advise on this.

ivptr avatar Mar 08 '24 08:03 ivptr

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.

jordantrizz avatar Mar 08 '24 14:03 jordantrizz

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

midweste avatar Apr 12 '24 20:04 midweste