Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,16 +1,58 @@
import Foundation
import WooFoundation

extension WooAnalyticsEvent {
enum BackgroundUpdates {

private enum Keys {
static let timeTaken = "time_taken"
static let backgroundTimeGranted = "background_time_granted"
static let networkType = "network_type"
static let isExpensiveConnection = "is_expensive_connection"
static let isLowDataMode = "is_low_data_mode"
static let isPowered = "is_powered"
static let batteryLevel = "battery_level"
static let isLowPowerMode = "is_low_power_mode"
static let timeSinceLastRun = "time_since_last_run"
static let completionStatus = "completion_status"
Copy link
Contributor

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

}

static func dataSynced(timeTaken: TimeInterval) -> WooAnalyticsEvent {
WooAnalyticsEvent(statName: .backgroundDataSynced, properties: [Keys.timeTaken: timeTaken])
}

static func dataSyncedDetailed(
Copy link
Contributor

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.

timeTaken: TimeInterval,
backgroundTimeGranted: TimeInterval?,
networkType: String,
isExpensiveConnection: Bool,
isLowDataMode: Bool,
isPowered: Bool,
batteryLevel: Float,
isLowPowerMode: Bool,
timeSinceLastRun: TimeInterval?
) -> WooAnalyticsEvent {
var properties: [String: WooAnalyticsEventPropertyType] = [
Keys.timeTaken: Int64(timeTaken),
Keys.networkType: networkType,
Keys.isExpensiveConnection: isExpensiveConnection,
Keys.isLowDataMode: isLowDataMode,
Keys.isPowered: isPowered,
Keys.batteryLevel: Float64(batteryLevel),
Keys.isLowPowerMode: isLowPowerMode
]

if let backgroundTimeGranted = backgroundTimeGranted {
properties[Keys.backgroundTimeGranted] = Int64(backgroundTimeGranted)
}

if let timeSinceLastRun = timeSinceLastRun {
properties[Keys.timeSinceLastRun] = Int64(timeSinceLastRun)
}

return WooAnalyticsEvent(statName: .backgroundDataSynced, properties: properties)
}

static func dataSyncError(_ error: Error) -> WooAnalyticsEvent {
WooAnalyticsEvent(statName: .backgroundDataSyncError, properties: [:], error: error)
}
Expand Down
1 change: 1 addition & 0 deletions WooCommerce/Classes/Extensions/UserDefaults+Woo.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ extension UserDefaults {

// Background Task Refresh
case latestBackgroundOrderSyncDate
case lastBackgroundRefreshTime
Copy link
Contributor

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.

Suggested change
case lastBackgroundRefreshTime
case lastBackgroundRefreshCompletionTime


// Blaze Local notification
case blazeNoCampaignReminderOpened
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import UIKit
import Foundation
import BackgroundTasks
import Network

final class BackgroundTaskRefreshDispatcher {

Expand Down Expand Up @@ -60,6 +61,8 @@ final class BackgroundTaskRefreshDispatcher {
// Launch all refresh tasks in parallel.
let refreshTasks = Task {
do {
let systemInfo = await BackgroundTaskSystemInfo()
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 lastRunTime = UserDefaults.standard[.lastBackgroundRefreshTime] as? Date
Copy link
Contributor

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?


let startTime = Date.now

Expand All @@ -78,7 +81,23 @@ final class BackgroundTaskRefreshDispatcher {
}

let timeTaken = round(Date.now.timeIntervalSince(startTime))
ServiceLocator.analytics.track(event: .BackgroundUpdates.dataSynced(timeTaken: timeTaken))
let timeSinceLastRun = lastRunTime?.timeIntervalSinceNow.magnitude
Copy link
Contributor

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).

Copy link
Contributor Author

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.


ServiceLocator.analytics.track(event: .BackgroundUpdates.dataSyncedDetailed(
timeTaken: timeTaken,
backgroundTimeGranted: systemInfo.backgroundTimeGranted,
networkType: systemInfo.networkType,
isExpensiveConnection: systemInfo.isExpensiveConnection,
isLowDataMode: systemInfo.isLowDataMode,
isPowered: systemInfo.isPowered,
batteryLevel: systemInfo.batteryLevel,
isLowPowerMode: systemInfo.isLowPowerMode,
timeSinceLastRun: timeSinceLastRun
))

// Save date, for use in analytics next time we refresh
UserDefaults.standard[.lastBackgroundRefreshTime] = Date.now

backgroundTask.setTaskCompleted(success: true)

} catch {
Expand All @@ -93,7 +112,7 @@ final class BackgroundTaskRefreshDispatcher {
ServiceLocator.analytics.track(event: .BackgroundUpdates.dataSyncError(BackgroundError.expired))
refreshTasks.cancel()
}
}
}
}

private extension BackgroundTaskRefreshDispatcher {
Expand All @@ -109,3 +128,87 @@ extension BackgroundTaskRefreshDispatcher {
case expired
}
}

// MARK: - System Information Helper

private struct NetworkInfo {
let type: String
let isExpensive: Bool
let isLowDataMode: Bool
}

private struct BackgroundTaskSystemInfo {
let backgroundTimeGranted: TimeInterval?
let networkInfo: NetworkInfo
Copy link
Contributor

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 isPowered: Bool
let batteryLevel: Float
let isLowPowerMode: Bool

// Computed properties for clean external access
var networkType: String { networkInfo.type }
var isExpensiveConnection: Bool { networkInfo.isExpensive }
var isLowDataMode: Bool { networkInfo.isLowDataMode }

@MainActor
init() async {
// Background time granted (nil if foreground/unlimited)
let backgroundTime = UIApplication.shared.backgroundTimeRemaining
self.backgroundTimeGranted = backgroundTime < Double.greatestFiniteMagnitude ? backgroundTime : nil

// Network info
self.networkInfo = await Self.getNetworkInfo()

// Power and battery info
let device = UIDevice.current
device.isBatteryMonitoringEnabled = true

self.isPowered = device.batteryState == .charging || device.batteryState == .full
Copy link
Contributor

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?

Copy link
Contributor Author

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.

self.batteryLevel = device.batteryLevel
self.isLowPowerMode = ProcessInfo.processInfo.isLowPowerModeEnabled

device.isBatteryMonitoringEnabled = false
}

private static func getNetworkInfo() async -> NetworkInfo {
return await withCheckedContinuation { continuation in
let monitor = NWPathMonitor()

monitor.pathUpdateHandler = { path in
continuation.resume(returning: NetworkInfo(path: path))
monitor.cancel()
}

let queue = DispatchQueue(label: "network.monitor.queue")
monitor.start(queue: queue)
}
}
}

private extension NetworkInfo {
init(path: NWPath) {
guard path.status == .satisfied else {
self.type = "no_connection"
self.isExpensive = false
self.isLowDataMode = false
return
}

self.type = Self.networkType(from: path)
self.isExpensive = path.isExpensive
self.isLowDataMode = path.isConstrained
}

private static func networkType(from path: NWPath) -> String {
if path.usesInterfaceType(.wifi) {
return "wifi"
} else if path.usesInterfaceType(.cellular) {
return "cellular"
} else if path.usesInterfaceType(.wiredEthernet) {
return "ethernet"
} else if path.usesInterfaceType(.loopback) {
return "loopback"
} else {
return "other"
}
}
}