search

woocommerce / woocommerce-ios

WooCommerce iOS app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
298 stars 110 forks source link

[Dashboard] Include Blaze campaign syncing in waiting time tracker for app launch #11969

Closed rachelmcr closed 3 weeks ago

rachelmcr commented 7 months ago

We currently use AppStartupWaitingTimeTracker to track the perceived time it takes for the app to launch, including loading content in the dashboard. To keep the tracking simple, the tracker includes actions that contribute to loading content on the dashboard screen, and tracks when they end (rather than tracking every launch action).

As part of an audit of API requests made on app launch (https://github.com/woocommerce/woocommerce-ios/issues/11592) we found that the tracker was missing the action that syncs Blaze campaigns, and that was added in https://github.com/woocommerce/woocommerce-ios/pull/11606. However, that revealed an issue with the tracking not being thread safe, causing a crash (https://github.com/woocommerce/woocommerce-ios/issues/11962). The tracking change was reverted in https://github.com/woocommerce/woocommerce-ios/pull/11968.

We should try again to update the tracker to include Blaze campaign syncing, but this time guarding against that crash.

peril-woocommerce[bot] commented 7 months ago
Fails
:no_entry_sign: Please add a feature label to this issue. e.g. 'feature: stats'

Generated by :no_entry_sign: dangerJS

rachelmcr commented 7 months ago

There are two things I think we can address here:

  1. We should cancel the tracker (with end()) if the Blaze request fails due to a network connection error. That way, we're not trying to end syncBlazeCampaigns when the request fails. (I forgot to include this in the original change in #11606.)
  2. We should consider using something like NSLock to make sure we aren't attempting to modify startupActionsPending concurrently. Making the first change will likely prevent that crash from recurring in the same way, but this should guard against it happening if we add to the startup actions we're tracking in the future.

I started making these changes but ran out of time, so I'll work on that for the next release.

Ecarrion commented 3 weeks ago

@rachelmcr The dashboard cards now have their own way of measuring their startup time. I think this is no longer relevant. But feel free to re-open if it is.