Free Listings + Paid Ads: Extend product statistics API to return the number of syncable products #1667

ianlin · 2022-09-06T08:30:47Z

Changes proposed in this Pull Request:

This PR implements the backend task 1 📌 Extends mc/product-statistics API to return # of syncable products from the epic issue #1610. Here are what have changed:

Extend an existing API mc/product-statistics and return a new data syncable_products in the response.
To calculate syncable_products, we use an existing method find_sync_ready_products in src/Product/ProductRepository.php.

Google account connected and Merchant Centre setup complete

Previously when calling mc/product-statistics API:

If Google account is not connected, respond 401 Google account is not connected.
- By checking the option gla_google_connected.
Or, if merchant centre setup is not complete, respond 400 Merchant Center account is not set up.
- By checking the option gla_mc_setup_completed_at
- This option will be set after calling the API mc/settings/sync.

In the step 4 of new onboarding flow (fqR0EHi63lWahRcVTKCcba-fi-449%3A80562), we haven't called the API mc/settings/sync so the option gla_mc_setup_completed_at is not being set. If we call mc/product-statistics API at this stage it will respond 400 Merchant Center account is not set up.

This PR also modifies the the above behaviour - if the merchant centre account is not set up (i.e. the option gla_mc_setup_completed_at is not being set), the API will still respond 200 with the data with syncable_products and the empty statistics:

{
    "timestamp": null,
    "statistics": null,
    "scheduled_sync": 0,
    "syncable_products": 29
}

Detailed test instructions:

Disconnect all of the accounts from the settings page: wp-admin/admin.php?page=wc-admin&path=/google/settings.
Use Postman, make a GET request to mc/product-statistics
- The response should be 401 Google account is not connected.
Go through the onboarding flow, stop at the step 4 Complete your campaign with paid ads.

Use Postman, make a GET request to mc/product-statistics, the response should be something like this:

{
    "timestamp": null,
    "statistics": null,
    "scheduled_sync": 0,
    "syncable_products": 29
}

Skip the ads setup or complete it, you should be redirected to the GLA home page

Make a GET request to mc/product-statistics again, the response should be something like this:

{
    "timestamp": 1662449383,
    "statistics": {
        "active": 0,
        "expiring": 0,
        "pending": 0,
        "disapproved": 23,
        "not_synced": 10
    },
    "scheduled_sync": 0,
    "syncable_products": 29
}

Bonus test

Repeat the same tests as above on a site with a large amount of products 500+. Can use Smooth Generator to generate a set of test products. Note that this might generate some unsyncable products so the total syncable product count might vary.

The expected result is for the request to take longer, however it should still end up with an accurate count for the syncable products. It's stored in a transient, so the second time round should be faster.

The UI is going to be adjusted to show a message like "calculating product count" so it's not an issue if the request takes a little longer to complete.

finished onboarding flow

mikkamp

Thanks for the PR, it's working and I'm getting the results as expected. However I have 500 products on my site and I'm already seeing slow responses, I added some suggestions for faster product queries.

I was expecting some more unit tests to break with these changes, but it seems we don't have unit tests for ProductStatisticsController or ProductSyncStats. Since we want to keep this moving I've logged a separate issue to get that done. See #1678

src/API/Site/Controllers/MerchantCenter/ProductStatisticsController.php

mikkamp · 2022-09-15T14:42:32Z

src/Jobs/ProductSyncStats.php

+	 * @return int
+	 */
+	public function get_syncable_products_count(): int {
+		$products = $this->product_repository->find_sync_ready_products()->get();


If all we are interested in is the count, then maybe we need to make a new helper function find_sync_ready_product_ids. Otherwise by default it will return objects, which means one DB query for the set of products and many more per product to get the full product data.

An even better way would be to get the create a helper function specifically for returning the total count. See the pagination section here: https://github.com/woocommerce/woocommerce/wiki/wc_get_products-and-WC_Product_Query#parameters

This is accomplished by sending the following args to the product query:

$args = [ 'return' => 'ids', 'paginate' => true, 'limit' => 1, ];

And then getting the total from $results->total. That prevents it from having to assemble a large collection of ID's and then having to count them all.

On further inspection this is more of an issue than I expected. The initial DB query is a rough estimate, we then apply filters on the product to allow extensions and special conditions to be excluded as well.

So we'll have to find a balance between either returning a rough estimate (without filtering), or switch to retrieving the products in batches (so we can apply filtering per batch). Might need some caching of the total so we don't do this too often.

Thanks for catching this performance problem and provided the solutions for it.

src/MerchantCenter/MerchantStatuses.php

puntope · 2022-09-16T15:53:38Z

❓ I get this results

{
    "timestamp": 1662449383,
    "statistics": {
        "active": 0,
        "expiring": 0,
        "pending": 0,
        "disapproved": 20,
        "not_synced": 1
    },
    "scheduled_sync": 0,
    "syncable_products": 22
}

How come syncable products are 22 and disapproved + not synced are 21??

puntope · 2022-09-16T16:02:13Z

I saw the expected result for this PR after the changes. So It looks good for me:

I was not able to test with a huge amount of products.

So I was finally able to test with 1000 products

When I run the call in step 4 I got a fatal error after 10-15 secs wait

PHP message: PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 12288 bytes)

mikkamp · 2022-09-19T07:19:52Z

How come syncable products are 22 and disapproved + not synced are 21?

Good question, I think @ianlin also identified some discrepancies with the amount of counted products. It could be helpful if you check which product is not being included to see if we can track this down further. Probably best to move it to a separate issue.

When I run the call in step 4 I got a fatal error after 10-15 secs wait
PHP message: PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 12288 bytes)

Based on that error message your allowed memory size is set at 128MB, which is below the minimum recommendations. I initially didn't want to make the batch size too small, but can you try changing it to something like 200 here. We might need to keep it at a bit more conservative number in case there are servers with limited resources.

ianlin · 2022-09-19T09:20:07Z

Good question, I think @ianlin also identified some discrepancies with the amount of counted products. It could be helpful if you check which product is not being included to see if we can track this down further. Probably best to move it to a separate issue.

Yeah, this problem was listed as a sub-task in the backend task 1 of the epic issue here.

puntope

✅ LGTM

Seems for me that is is ready to go after all the tweaks.

Thanks @mikkamp and @ianlin

mikkamp · 2022-09-19T12:21:24Z

Seems for me that is is ready to go after all the tweaks.

Can you just confirm if changing the batch size worked for you to clear up the out of memory errors?
I still want to lower it but just wondering what amount works for you.

mikkamp · 2022-09-20T11:14:25Z

I was doing some additional testing and was able to reproduce the out of memory error by reducing the memory limit to 128MB and increasing the amount of products I have. Using a smaller batch size does not fix the issue. Apparently somewhere along the line it's not clearing up the previous set of products which I think has to do with populating the caches, but it would need some further digging.

I also noticed that the find_sync_ready_products finds both single products and variations whereas when we calculate the product statistics we get from the Merchant Center we count each variable product as just one item. So there is going to be a discrepancy there with product count. If we need to group the variations to achieve a proper count then the query would need to be completely adjusted and the filters won't work the same.

ianlin · 2022-09-21T07:31:17Z

I did some tests and here's my result:

Memory limit: 128M
Batch size: 500
Total products: 1190

The only case I encountered OOM problem is right after I created 100 products at once using wc-smooth-generator, and calling the API without the cache. I think it's because when products were created the GMC product sync jobs were scheduled as well, so calling the product statistics API at this point would cause OOM. If I manually added the cache to my local DB, the OOM issue didn't occur and the API respond correctly. After all the GMC product sync jobs were completed, the OOM issue didn't occur even when the cache didn't exist. As a result I think it'd be acceptable for setting 500 as batch size.

ianlin · 2022-09-21T09:10:42Z

I just found a weird issue that I got 4075 syncable_products while the total products I've got in WC is 1190. I will keep looking into this.

mikkamp · 2022-09-21T09:28:37Z

If I manually added the cache to my local DB, the OOM issue didn't occur and the API respond correctly.

Can you clarify what you mean by adding the cache to your local DB? I traced the memory usage and it seems to be a combination of the WP meta cache as well as the WC_Cache_Helper class which caches the product data.

Here is an overview of memory usage (logging offset, count, peak memory usage, current memory usage)

[20-Sep-2022 16:46:14 UTC] 500 99 58403720 57229432
[20-Sep-2022 16:46:16 UTC] 1000 301 75588288 75512832
[20-Sep-2022 16:46:18 UTC] 1500 782 99822976 99768328
[20-Sep-2022 16:46:19 UTC] 2000 1282 121776120 121731080
[20-Sep-2022 16:46:19 UTC] PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 20480 bytes) in wp-includes/meta.php on line 1189

I can reduce the memory usage by calling wp_cache_flush() within the loop but it still uses a substantial amount:

[20-Sep-2022 16:45:42 UTC] 500 99 58403720 57229432
[20-Sep-2022 16:45:44 UTC] 1000 301 66749288 66519048
[20-Sep-2022 16:45:46 UTC] 1500 782 80233296 80178648
[20-Sep-2022 16:45:48 UTC] 2000 1282 92011224 91966184
[20-Sep-2022 16:45:48 UTC] 2279 1561 100752624 94368768

The WC cache does it per product, so I can't find one function to clear all caching. We could try clearing it in a loop but that would slow down the call a bit more.

I just found a weird issue that I got 4075 syncable_products while the total products I've got in WC is 1190. I will keep looking into this

The query retrieves the product types simple and variation (not variable) so with a lot of product variation this is expected to deviate quite a bit (which is what I mentioned here)

ianlin · 2022-09-21T10:42:30Z

Can you clarify what you mean by adding the cache to your local DB?

Sorry for the confusion, what I really meant is the transient (_transient_gla_syncable_products_count and _transient_timeout_gla_syncable_products_count) we stored in the wp_options table.

Based on discussion during the dev call, we decided to close this PR and:

Create a new endpoint to retrieve the count of syncable products.
Create a separate job to calculate the number of syncable products, and trigger it when the user just start the onboarding flow.

For the discrepancies with the amount of counted products, per Mik's finding here (#1667 (comment)), we need to group the product with type variations to get the correct count, but shouldn't affect how many products that are really synced to GMC. This will be a separate PR from the above new endpoint.

mikkamp · 2022-09-21T11:35:59Z

we need to group the product with type variations to get the correct count, but shouldn't affect how many products that are really synced to GMC

Since we want to run the same filter the grouping needs to be done after the filter. So my thoughts would be for the Job storing the count to store (unique) included ID's, instead of storing a count as it runs through the batches. This is because of batch one could contain 5 variations of the same product and batch 2 could contain another 5. So if we store the parent ID as counted it won't count it twice.

ianlin added 4 commits September 6, 2022 12:48

Add a method to return number of syncable products in ProductSyncStats

3f84d4e

Return syncable_products in API mc/product-statistics

e6d4e71

Return empty statistics if Google account is connected but haven't

8bf4153

finished onboarding flow

Add test for returning null statistics in MerchantStatuses

6b6dd6c

ianlin added php Pull requests that update Php code changelog: none Skip changelog entry for this PR labels Sep 6, 2022

ianlin requested a review from a team September 6, 2022 08:30

ianlin self-assigned this Sep 6, 2022

github-actions bot added the changelog: update Big changes to something that wasn't broken. label Sep 6, 2022

eason9487 requested review from a team and removed request for a team September 15, 2022 12:24

mikkamp mentioned this pull request Sep 15, 2022

Unit tests for ProductStatisticsController or ProductSyncStats #1678

Open

mikkamp reviewed Sep 15, 2022

View reviewed changes

mikkamp added 4 commits September 16, 2022 08:02

Update schema description for syncable_products

f2a8427

Use isset to confirm statistics has been set

3e0519a

Retrieve product count in batches

2642416

Use a transient for storing syncable product count

0013f75

mikkamp requested a review from a team September 16, 2022 13:40

puntope reviewed Sep 16, 2022

View reviewed changes

src/MerchantCenter/MerchantStatuses.php Show resolved Hide resolved

puntope approved these changes Sep 19, 2022

View reviewed changes

ianlin mentioned this pull request Sep 21, 2022

Free Listings + Paid Ads: Update product feed API to return the bestselling product #1681

Merged

ianlin closed this Sep 22, 2022

eason9487 deleted the update/1610-add-api-to-get-sync-ready-products branch September 22, 2022 02:11

ianlin mentioned this pull request Sep 22, 2022

Free Listings + Paid Ads: Return null statistics when onboarding is not finished #1690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Free Listings + Paid Ads: Extend product statistics API to return the number of syncable products #1667

Free Listings + Paid Ads: Extend product statistics API to return the number of syncable products #1667

ianlin commented Sep 6, 2022 •

edited by mikkamp

Loading

mikkamp left a comment

mikkamp Sep 15, 2022

mikkamp Sep 16, 2022

ianlin Sep 19, 2022

puntope commented Sep 16, 2022

puntope commented Sep 16, 2022

mikkamp commented Sep 19, 2022

ianlin commented Sep 19, 2022

puntope left a comment

mikkamp commented Sep 19, 2022

mikkamp commented Sep 20, 2022

ianlin commented Sep 21, 2022

ianlin commented Sep 21, 2022

mikkamp commented Sep 21, 2022

ianlin commented Sep 21, 2022

mikkamp commented Sep 21, 2022

Free Listings + Paid Ads: Extend product statistics API to return the number of syncable products #1667

Free Listings + Paid Ads: Extend product statistics API to return the number of syncable products #1667

Conversation

ianlin commented Sep 6, 2022 • edited by mikkamp Loading

Changes proposed in this Pull Request:

Google account connected and Merchant Centre setup complete

Detailed test instructions:

Bonus test

mikkamp left a comment

Choose a reason for hiding this comment

mikkamp Sep 15, 2022

Choose a reason for hiding this comment

mikkamp Sep 16, 2022

Choose a reason for hiding this comment

ianlin Sep 19, 2022

Choose a reason for hiding this comment

puntope commented Sep 16, 2022

puntope commented Sep 16, 2022

mikkamp commented Sep 19, 2022

ianlin commented Sep 19, 2022

puntope left a comment

Choose a reason for hiding this comment

mikkamp commented Sep 19, 2022

mikkamp commented Sep 20, 2022

ianlin commented Sep 21, 2022

ianlin commented Sep 21, 2022

mikkamp commented Sep 21, 2022

ianlin commented Sep 21, 2022

mikkamp commented Sep 21, 2022

ianlin commented Sep 6, 2022 •

edited by mikkamp

Loading