-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Local Catalog] Add Better Analytics to Background Task Refresh System #16011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think – 23.1, or put it in the 23.0 beta? I'm happy either way, but have done 23.1 for now.
I'd say yes to 23.0 beta, mostly because I'm the release DRI 😆 . Otherwise, I feel like this isn't a very urgent change. It'd be the first time I'm generating a new beta, is the process to change the PR base to the release 23.0 branch and then I'll generate a new beta from the release dashboard?
I put the app on an iPad device to the background for more than an hour and didn't see any logs when I came back. I updated the scheduled time to 5 minutes, and didn't see an event until maybe an hour later. The first event below was naturally triggered, and the second was triggered from the console.
2025/08/15 06:38:34:448 🔵 Tracked background_data_synced, properties: [battery_level: 0.949999988079071, network_type: wifi, is_low_power_mode: false, is_low_data_mode: false, is_powered: true, blog_id: -1, background_time_granted: 119, is_expensive_connection: false, time_taken: 2, ...]
2025/08/15 06:47:29:408 🔵 Tracked background_data_synced, properties: [is_powered: true, time_since_last_run: 534, is_low_data_mode: false, network_type: wifi, is_low_power_mode: false, blog_id: -1, is_expensive_connection: false, battery_level: 0.949999988079071, time_taken: 1, ...]
Even though I only saw these two events (the app log file included the last 3 hours), the battery settings showed that the app spent 27 minutes in the background 🤔
| // Launch all refresh tasks in parallel. | ||
| let refreshTasks = Task { | ||
| do { | ||
| let systemInfo = await BackgroundTaskSystemInfo() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: how about initializing the system info async when it's ready to track the event later? just in case it takes a bit of time (maybe it's close to zero) that can affect the time for the main tasks below. Or is it here to capture the device status at the beginning of the refresh task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't delay the tasks below – let await doesn't block further execution until the variable is accessed, after the tasks run.
You're right though, the reason it's here is to capture the status at the beginning of the task, because that will give us the accurate background time granted value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, no, I should use async let for that. I'll try it.
| let timeTaken = round(Date.now.timeIntervalSince(startTime)) | ||
| ServiceLocator.analytics.track(event: .BackgroundUpdates.dataSynced(timeTaken: timeTaken)) | ||
| let timeSinceLastRun = lastRunTime?.timeIntervalSinceNow.magnitude |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: how about consolidating the format of these two durations? either rounded (maybe better for visualization in Tracks event view) or double (probably better).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since timeTaken is preexisting, I don't really want to change that to keep the data consistent. I'll round it for both.
| WooAnalyticsEvent(statName: .backgroundDataSynced, properties: [Keys.timeTaken: timeTaken]) | ||
| } | ||
|
|
||
| static func dataSyncedDetailed( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't comment on an unchanged line, meant to comment on L20. as in the CI lint error, we can delete the dataSynced(timeTaken:) as it's not used anymore. let's also make sure to update the new event properties in the existing Tracks event.
|
|
||
| // Background Task Refresh | ||
| case latestBackgroundOrderSyncDate | ||
| case lastBackgroundRefreshTime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe this could indicate that it's the "completion" time to be more exact.
| case lastBackgroundRefreshTime | |
| case lastBackgroundRefreshCompletionTime |
|
|
||
| private struct BackgroundTaskSystemInfo { | ||
| let backgroundTimeGranted: TimeInterval? | ||
| let networkInfo: NetworkInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this could be private
| let device = UIDevice.current | ||
| device.isBatteryMonitoringEnabled = true | ||
|
|
||
| self.isPowered = device.batteryState == .charging || device.batteryState == .full |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: how about just logging the battery state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that will just log an int, not a useful string. Also during analysis, we'd have to check for both values which indicate that it's plugged in.
| static let batteryLevel = "battery_level" | ||
| static let isLowPowerMode = "is_low_power_mode" | ||
| static let timeSinceLastRun = "time_since_last_run" | ||
| static let completionStatus = "completion_status" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this isn't used, also noted in the CI lint error
| let refreshTasks = Task { | ||
| do { | ||
| let systemInfo = await BackgroundTaskSystemInfo() | ||
| let lastRunTime = UserDefaults.standard[.lastBackgroundRefreshTime] as? Date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: this could probably be moved to where the event is tracked?
I'd say 23.0 beta, so we have more data when we needed. That said it's not urgent, so I leave the decision to @jaclync as the release manager. |
OK, let's do it. I've made a new branch, from I've made a new PR for it: #16014, and I've closed this one in favour of that. You should be able to add a new release in the release tool for the 23.0 version. Let me know if you have any trouble though! |
Yea, if keeping the same PR, it'd require deleting commits from trunk manually by rebasing children of the branch iteratively. New PR looks good. |


Description
This PR adds detailed analytics tracking to the existing background task refresh system to help us understand
how background syncing works in real-world conditions. This will inform our choices for POS local catalog sync, whether we can use the same App Refresh task or would need a background processing task.
@toupper @jaclync What do you think – 23.1, or put it in the 23.0 beta? I'm happy either way, but have done 23.1 for now.
Also, if you think there's anything else we should try to track, let me know.
Changes
Analytics Updates
System Info Collection
Analytics Properties
Steps to reproduce
- Turn Low Data Mode on/off: Settings → Cellular → Cellular Data Options → Low Data Mode
- Turn Low Power Mode on/off: Settings → Battery → Low Power Mode
- Try different networks: WiFi, Cellular, WiFi hotspot
- Run from Xcode, adding a breakpoint after the task is scheduled (at the end of AppDelegate.applicationDidEnterBackground(_:))
- In Xcode debug console, run this command:
-
e -l objc -- (void)[[BGTaskScheduler sharedScheduler] _simulateLaunchForTaskWithIdentifier:@"com.automattic.woocommerce.refresh"]- Allow the app to run on
- Look for Tracked background_data_synced in the logs
- Make sure all the properties show up and look correct:
network_typeshould show your current connectionis_low_data_modeshould match what you set in Settingsis_low_power_modeshould match what you set in Settingsbattery_levelshould show current battery percentageis_poweredshould show if you're chargingbackground_time_grantedwon't be logged, because the app is essentially in the foreground even when you background it.- WiFi with/without Low Data Mode
- Cellular with/without roaming (if you can)
- Different battery levels and charging states
- Run while app is in background vs foreground
Expected Results
Analytics should show detailed info like:
Testing information
I've tested this on an iPhone running iOS 18, with both simulated and real BGAppRefreshTasks. Screenshot below shows a real task's logged result.
Screenshots
RELEASE-NOTES.txtif necessary.