Remote Inbox: Performance Tweaks

[timmyc]

Describe the bug Related: #6181

When looking into the Remote Inbox bug ( see related issue above ) I noticed that the hook in StoredStateSetupForProducts is I believe inadvertently calling RemoteInboxNotificationsEngine::run(); anytime an already published product is being updated.

I think the intention of this hook is to only run when a product is transitioning from draft -> published but cc @becdetat for clarification.

Suggested Actions

• [ ] Add stronger checks to the run_on_transition_post_status method to ensure the logic only runs when a product transitions to publish
• [ ] Investigate if we could maybe use some woo specific hooks here instead, maybe woocommerce_new_product, I feel like Bec looked into these but they didn't work as expected.
• [ ] I would also like us to consider not triggering a download of remote inbox specs via RemoteInboxNotificationsEngine::run() could we instead run the engine using persisted note rules if they exist? Sites should have a cached copy of remote inbox specs from the prior day's cron job, so seems like this should be okay.
• [ ] If a full run of the remote inbox is required, I think we should consider placing the execution of that code path in an actionscheduler job so we aren't doing a wc_remote_get during the product update request.

cc @ilyasfoo @moon0326 for any further thoughts since you both spent some time on this.

[becdetat]

@timmyc

I think the intention of this hook is to only run when a product is transitioning from draft -> published but cc @becdetat for clarification.

I left it open to run when products were updated/deleted as well, to handle a broader range of trigger rules.

Investigate if we could maybe use some woo specific hooks here instead, maybe woocommerce_new_product, I feel like Bec looked into these but they didn't work as expected.

I wasn't actually aware of the woocommerce_new_product hook, any example code I found elsewhere in the system (for example built-in notes that trigger on adding a product) used transition_post_status. There is some special handling for the product count during that hook, because the product count is incorrect (n-1) during a product add, so any change to the hooks would need to be tested with the product count rule.

I would also like us to consider not triggering a download of remote inbox specs via RemoteInboxNotificationsEngine::run() could we instead run the engine using persisted note rules if they exist? Sites should have a cached copy of remote inbox specs from the prior day's cron job, so seems like this should be okay.

My idea was that if the engine was triggered before the first download then it should download before running, otherwise the first product added test might not trigger a note like "Your first product" until the day after. The case when there isn't a cached copy of specs should only be on the first run of the engine.

If a full run of the remote inbox is required, I think we should consider placing the execution of that code path in an actionscheduler job so we aren't doing a wc_remote_get during the product update request.

As long as the run happens very close to the product update this should be ok.

[ilyasfoo]

As long as the run happens very close to the product update this should be ok.

@becdetat We can use as_enqueue_async_action to register our job and spawn_cron to immediately trigger cron, although I don't see this widely used anywhere, and I don't know if there would be any side effects of running it ad-hoc.

@timmyc I'm a little unsure if this is within this issue's scope, but I think another good addition to the tasks would be to add the new woocommerce_activated_plugin proxy hook that we've introduced in WC 5.0 so that WC could run the spec download immediately after plugin activation. Previously this was not the case if WCA running as embedded package, background of the issue is detailed here.

I'd be glad to edit the issue description if you're ok with adding that in!

[timmyc]

We can use as_enqueue_async_action to register our job and spawn_cron to immediately trigger cron, although I don't see this widely used anywhere, and I don't know if there would be any side effects of running it ad-hoc.

I would be okay with keeping the full run, if we enqueued a job to do it instead. As for calling spawn_cron ( not really too savvy about that ) but we do need to consider that some sites might be using wp_cli to process action scheduler queues, so not sure if we should trigger a cron. I'd personally be okay with just waiting for the job to process.

would be to add the new woocommerce_activated_plugin proxy hook that we've introduced in WC 5.0 so that WC could run the spec download immediately after plugin activation.

I really like this approach too... I think if we download, or enqueue a job to download the inbox specs on plugin activation, then all notes tied to lifecycle events around onboarding should be good to go.

[ilyasfoo]

To summarize, this is what I think needs to be done:

Remove DataSourcePoller::read_specs_from_data_sources() from RemoteInboxNotificationsEngine::run() to detach from remote dependency. In place of this, we'll want to make sure that we have a copy of the specs locally by running read_specs_from_data_sources after plugin activation using as_enqueue_async_action. I don't know if this is a 100% guarantee that we have specs in before running, but I can't think of any case that a typical user will not have specs downloaded after activation, since WP Cron should run immediately after the next request.

@becdetat Do you think this is acceptable change?

[becdetat]

@ilyasfoo the specs sill need to be refreshed occasionally, so they need to be on a 24 hour schedule as well. Otherwise this sounds fine.

[becdetat]

Remove DataSourcePoller::read_specs_from_data_sources() from RemoteInboxNotificationsEngine::run() to detach from remote dependency

It only reads from the data sources in run() if the specs cached in the wc_remote_inbox_notifications_specs option is empty (which should only be the case the first time the engine is run). Reading from data sources also happens in a wc_admin_daily cron task handler, but I don't see a problem with that as the specs need to be updated from the feeds regularly.

So the only actual performance issue that I can see is running the engine when a product is added, which is handled in #6995 by offloading the engine run using action-scheduler. I'll close this issue once that PR is merged, unless anyone has any objections @timmyc @ilyasfoo

[ilyasfoo]

@becdetat I'm thinking initial we could also make the initial run during plugin activation async as well, since looking at how #6995 works it seems like the async action is pretty quick (AFAIK at most it's around 1 minute delay, I don't think a general user would create a product after installing our plugin that quickly).

[becdetat]

Thanks @ilyasfoo

could also make the initial run during plugin activation async as well

I've created a follow-up ticket. I think we're getting into the area of diminishing returns though. Running the engine at this point only involves one to three database accesses for the options plus whatever the rules require, the feeds aren't polled so there's no out of network access. Database access is very cheap compared to hitting a web server.

Note that action-scheduler (and WP Cron) don't actually make things happen asynchronously, they just put it off until the next request (or the first request after the scheduled timestamp). So you offload the engine run until the next request, making the current request a bit faster at the expense of the next request.

[ilyasfoo]

Note that action-scheduler (and WP Cron) don't actually make things happen asynchronously, they just put it off until the next request (or the first request after the scheduled timestamp). So you offload the engine run until the next request, making the current request a bit faster at the expense of the next request.

Ah, I didn't know about this, I thought it ran as a background process.

I've created a follow-up ticket. I think we're getting into the area of diminishing returns though. Running the engine at this point only involves one to three database accesses for the options plus whatever the rules require, the feeds aren't polled so there's no out of network access. Database access is very cheap compared to hitting a web server.

You do make a good point. I think the performance improvements from making this change might not be as great as I thought it would be, at least not worth in added complexity you mentioned.