Add hook to allow disable of full batch api sync (e.g. for large stores) to avoid potential performance issues #2033

haszari · 2021-06-22T02:14:19Z

Fixes #2022, #2035

Changes proposed in this Pull Request:

This PR provides a hook to allow developers (or hosting platforms) to disable specific full-catalog sync operations (via Batch API). See below Why? for more background and context.

Specifically these two flows are affected:

Onboarding & initial connection. The initial full catalog sync is only triggered if the filter allows it.
On-demand sync using button in Marketing > Facebook > Product sync. If full batch API sync is disabled via filter, an error will be shown.
AJAX full sync endpoint (used for button above) returns an error if site has large number of products.

The default behaviour is that the full batch-api background sync is allowed (consistent with previous releases).

Some example use-cases for the hook.

On shared hosting or other resource-limited environments, to disable full batch API sync for large-scale sites (5000+ products, or completely disable).
On hosts that have detailed database logging, to disable batch API sync more aggressively (e.g. 1000+ products).

Example use of hook:

function wpcomsh_disallow_fb_for_woo_full_batch_api_sync( $allow_full_sync, $product_count ) {
	// Disable only for sites with a large number of products.
	$max_products_for_safe_full_sync = 10;
	if ( $product_count > $max_products_for_safe_full_sync ) {
		return false;
	}

	return $allow_full_sync;
}
add_filter( 'facebook_for_woocommerce_allow_full_batch_api_sync', 'wpcomsh_disallow_fb_for_woo_full_batch_api_sync', 10, 2 );

Also in this PR the get_product_count() function has been optimised to use wp_count_posts. We're passing the total number of products to the hook. The previous query was loading all the IDs into memory, which is unnecessary and could impact performance for larger stores.

Why?

A full catalog sync using the batch API and the Products\Sync\Background task can cause issues.

The Background class uses the SkyVerge background job framework. This adds an option to the database representing the job status and progress, which includes all product IDs that are included in the sync job.

There are two risks with this:

The option is large, and grows with the number of products that need to be synched => large database size.
The option is re-written constantly => large database traffic/throughput. This can generate significant database logs (depending on config of course).

Long term, we may need to tweak or replace the Products\Sync\Background so we have a scalable, robust way to sync full catalog. This is a medium-term patch to prevent this sync mechanism causing issues for merchants and web hosts, and to give more control over whether full batch sync happens.

How does this impact stores, and the product sync feature?

Disabling the initial sync means that new stores (over the num products threshold) will not see their products in Facebook catalog by default. Products will incrementally sync as they are added or edited; or potentially sites can use feed-based sync.
The manual full sync button will show an error (see below). Should be low impact, as it's not intended to be used regularly; is intended as a workaround for sync issues.
Note: this will not affect any stores by default - a filter function needs to be added to prevent any syncs from occurring.

Does this solve the issue?

It may be still possible to trigger a similar problem via code or API (or UI!), by marking a large number of products as needing sync (e.g. make a trivial edit). This PR doesn't fix the root of the problem (Products\Sync\Background scalability). One potential workaround: #2034

How to test the changes in this Pull Request:

Add a filter for facebook_for_woocommerce_allow_full_batch_api_sync to disable full sync, e.g. for sites with > 5000 products.

Two main flows to test:

Full sync on-demand via admin button.
Full sync triggered automatically after onboarding.
Any other way of triggering a full batch API sync.

Test to ensure the filter disables full sync appropriately.

Changelog entry

Dev: Add facebook_for_woocommerce_allow_full_batch_api_sync filter to allow external developers to disable or enable full catalog sync (batch API)

- query loads all product IDs - which could be a lot - wp_count_posts uses a `COUNT()` sql statement which doesn't load anything - in my testing, WP_Query approach uses 40x more memory

…min tab" This reverts commit 3896e99. Reverting - will hide/adjust button server side

haszari · 2021-06-22T02:21:21Z

facebook-commerce.php

+	 * @var int Maximum number of products for full sync.
+	 * Used to disable full batch-API sync flows which may cause performance issues.
+	 **/
+	const MAX_PRODUCTS_FOR_FULL_SYNC = 500;


TBC - we need to determine what the appropriate threshold is. It may need to be smaller, but ideally we want this as large as we can safely get away with, to reduce the number of impacted stores.

I'm leaning towards 5000 or maybe 2500 here - the site that triggered the issue had 19k products.

I have 2.6.0 running with 2k-5k products and products have been synced, these sites are running on a shared account on Siteground.

That's useful info, thanks for setting these up @rcstr !

I'm thinking we expose a filter for max products for full sync, then hosts or sites can tweak this threshold to suit.

Let's bump this up to 5k - that's approx a quarter of the 19k from the problematic site, and host/site can use the filter if they need a smaller threshold.

(And let's test it - how big is the option with 5000 products?)

facebook-commerce.php

rcstr

Changes look good, haven't fully tested yet. I think this will be a good place to define wc_facebook_enable_product_sync to be sure sync doesn't run

includes/Handlers/Connection.php

haszari · 2021-06-24T03:29:07Z

includes/Admin/Settings_Screens/Product_Sync.php

 		?>

 		<h2>

 			<?php esc_html_e( 'Product sync', 'facebook-for-woocommerce' ); ?>

-			<?php if ( facebook_for_woocommerce()->get_connection_handler()->is_connected() ) : ?>
+			<?php if ( $show_sync_button ) : ?>


Hmm, maybe we should not hide the button, so it's clear to merchant why it's disabled! 🤔

rcstr · 2021-06-24T04:37:06Z

@haszari - I have applied the proposed changes in #2036 (review)

Added facebook_for_woocommerce_allow_full_batch_api_sync filter cd58075
Updated Sync products button to be shown if site is connected, but show an error 70731bb

haszari

Looks great to me - I think we can close the other PRs and do some more testing & get feedback on this approach & then release 🎉

Keen to get data on what the impact of the threshold is on the database option size - can you run some tests and add the info to this PR? E.g. for 500 products, 1000, 2500, 5000.

Also please refresh the PR description to match the latest changes (or remind me – I can do tomorrow).

facebook-commerce.php

haszari · 2021-06-24T04:46:06Z

facebook-commerce.php

+	 * @var int Maximum number of products for full sync.
+	 * Used to disable full batch-API sync flows which may cause performance issues.
+	 **/
+	const MAX_PRODUCTS_FOR_FULL_SYNC = 500;


Let's bump this up to 5k - that's approx a quarter of the 19k from the problematic site, and host/site can use the filter if they need a smaller threshold.

haszari · 2021-06-24T04:46:43Z

facebook-commerce.php

+	 * @var int Maximum number of products for full sync.
+	 * Used to disable full batch-API sync flows which may cause performance issues.
+	 **/
+	const MAX_PRODUCTS_FOR_FULL_SYNC = 500;


(And let's test it - how big is the option with 5000 products?)

haszari · 2021-06-24T04:48:17Z

includes/AJAX.php

@@ -214,6 +214,11 @@ public function admin_complete_order() {
 	 * @since 2.0.0
 	 */
 	public function sync_products() {
+		// Inhibit on-demand full batch-api sync if the store has large product count.


Let's tweak these comments, as this could be disabled based on various factors now that it's an opt-out hook.

E.g. Allow opt-out of full batch-API sync, for example if store has a large number of products.

haszari · 2021-06-24T04:48:45Z

includes/Handlers/Connection.php

@@ -301,7 +301,10 @@ public function handle_connect() {
 			$this->update_system_user_id( $system_user_id );
 			$this->update_installation_data();

-			facebook_for_woocommerce()->get_products_sync_handler()->create_or_update_all_products();
+			// Only trigger initial full batch-api sync if the store doesn't have large product count.


Likewise for this comment 👯

…-large-stores * release-2.6.1: add 2.6.1 changelog entries run phpcs npm script silently, so xml output isn't broken by command echo add phpcs reminder to PR template get phpcs passing in GitHub action: - add explicit pr command targeting only php files (was hitting js files) - use in action yml script un-ignore phpcs and fix phpcs issues add phpcs:ignoreFile comment to all unhappy files

haszari

This is looking splendid - thanks @rcstr . I've added some more comments and also updated the description. Thanks for proactively sorting out the changelog!

I'm still testing (in conjunction with a PR using the filter), but 🤞 this PR is ready to go.

- the reported issue may not affect all platforms / merchants - this reduces the chance of the disable impacting sites that rely on full sync

haszari · 2021-06-24T23:22:38Z

Update: I've reworked this so it just provides the filter. By default, no behaviour will change.

Explaining the rationale for this:

We've only had reports of this from a specific hosting platform; we don't know that the issue is affecting merchants on other platforms.
Full sync, especially after onboarding, is an important feature. If we silently break it for some (large) sites that could be confusing and cause more support requests.
The hook allows all merchants, platforms and developers to adjust the behaviour as needed.
It's a nice clear simple story.

- no longer opting out by default - add rationale & context to hook changelog

haszari · 2021-06-28T01:14:50Z

Update: tweaked the error message for the AJAX full sync - now just says the sync was disabled via filter (may not be due to product count).

haszari · 2021-06-28T01:49:17Z

I've tested this with and without a custom filter and confirmed it allows disabling of the two relevant full sync operations. I've added some more logging and tweaked the messages so it's clear that this was disabled via a customization.

Merging - and proceeding to release 2.6.1!

Merge pull request #2033 from woocommerce/fix/disable-full-batch-api-sync-for-large-stores Add hook to allow disable of full batch api sync (e.g. for large stores) to avoid potential performance issues

haszari added 6 commits June 22, 2021 13:26

optimise get_product_count() - use wp_count_posts instead of WP_Query:

5a48528

- query loads all product IDs - which could be a lot - wp_count_posts uses a `COUNT()` sql statement which doesn't load anything - in my testing, WP_Query approach uses 40x more memory

render total product count as script data for product sync admin tab

3896e99

Revert "render total product count as script data for product sync ad…

cd18e4d

…min tab" This reverts commit 3896e99. Reverting - will hide/adjust button server side

hide sync button if > 500 published products

34dc4b9

disable AJAX full batch API sync if > 500 products

5e83953

disable full sync after onboarding if store has many products

c30677f

haszari self-assigned this Jun 22, 2021

haszari added feature: onboarding Related to onboarding, set up and initial connection to Facebook. feature: product sync Relating to syncing product data to Facebook. labels Jun 22, 2021

haszari added this to the 2.6.1 milestone Jun 22, 2021

haszari added the GH Review: review-needed label Jun 22, 2021

haszari commented Jun 22, 2021

View reviewed changes

facebook-commerce.php Show resolved Hide resolved

haszari requested a review from rcstr June 22, 2021 02:50

rcstr reviewed Jun 22, 2021

View reviewed changes

includes/Handlers/Connection.php Outdated Show resolved Hide resolved

rcstr approved these changes Jun 23, 2021

View reviewed changes

rcstr changed the base branch from master to release-2.6.1 June 23, 2021 17:48

rcstr mentioned this pull request Jun 23, 2021

Allow full batch api sync to be enable or disabled #2036

Closed

1 task

haszari mentioned this pull request Jun 23, 2021

Add feature flag that disables full background sync #2035

Closed

haszari commented Jun 24, 2021

View reviewed changes

rcstr added 5 commits June 23, 2021 22:14

add version and remove extra line

21411f4

add facebook_for_woocommerce_allow_full_batch_api_sync filter

cd58075

show Sync products button if site is connected

70731bb

fix function reference

6c52c0c

fix conditional

ebd3fd4

haszari assigned rcstr Jun 24, 2021

haszari commented Jun 24, 2021

View reviewed changes

haszari mentioned this pull request Jun 24, 2021

Disable full batch API sync on Atomic env #2037

Closed

1 task

bump MAX_PRODUCTS_FOR_FULL_SYNC to 5k

d27a5a9

rcstr and others added 5 commits June 24, 2021 13:16

extract variable and update filter description

211dd13

remove unused variable

4352711

update changelog

a5c0ef7

Tweak & enhance various comments for extra clarity of intention 📠

7999df6

haszari commented Jun 24, 2021

View reviewed changes

remove the opt-out of full batch-api sync - allow by default:

437e6df

- the reported issue may not affect all platforms / merchants - this reduces the chance of the disable impacting sites that rely on full sync

haszari changed the title ~~Disable full batch api sync for large stores to avoid potential performance issues~~ Add hook to allow disable of full batch api sync (e.g. for large stores) to avoid potential performance issues Jun 24, 2021

haszari added 2 commits June 25, 2021 11:26

update changelog:

8dff997

- no longer opting out by default - add rationale & context to hook changelog

adjust error message when AJAX full sync disabled - disabled via hook

0a9d148

add log message when initial sync is disabled by hook

a013e3b

haszari merged commit ea29aaf into release-2.6.1 Jun 28, 2021

haszari deleted the fix/disable-full-batch-api-sync-for-large-stores branch June 28, 2021 01:49

haszari mentioned this pull request Jun 28, 2021

Release 2.6.1 #2038

Closed

This was referenced Jun 28, 2021

Release 2.6.1 #2039

Merged

Disabling product sync globally #2022

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hook to allow disable of full batch api sync (e.g. for large stores) to avoid potential performance issues #2033

Add hook to allow disable of full batch api sync (e.g. for large stores) to avoid potential performance issues #2033

haszari commented Jun 22, 2021 •

edited

haszari Jun 22, 2021

haszari Jun 23, 2021

rcstr Jun 23, 2021

haszari Jun 23, 2021

haszari Jun 24, 2021

haszari Jun 24, 2021

haszari Jun 24, 2021

rcstr left a comment

haszari Jun 24, 2021

rcstr commented Jun 24, 2021

haszari left a comment

haszari Jun 24, 2021

haszari Jun 24, 2021

haszari Jun 24, 2021

haszari Jun 24, 2021

haszari left a comment

haszari commented Jun 24, 2021 •

edited

haszari commented Jun 28, 2021

haszari commented Jun 28, 2021

Add hook to allow disable of full batch api sync (e.g. for large stores) to avoid potential performance issues #2033

Add hook to allow disable of full batch api sync (e.g. for large stores) to avoid potential performance issues #2033

Conversation

haszari commented Jun 22, 2021 • edited

Changes proposed in this Pull Request:

Why?

How does this impact stores, and the product sync feature?

Does this solve the issue?

How to test the changes in this Pull Request:

Changelog entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcstr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcstr commented Jun 24, 2021

haszari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haszari left a comment

Choose a reason for hiding this comment

haszari commented Jun 24, 2021 • edited

haszari commented Jun 28, 2021

haszari commented Jun 28, 2021

haszari commented Jun 22, 2021 •

edited

haszari commented Jun 24, 2021 •

edited