Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Diagnostics] Refactor diagnostics track methods to handle background work automatically #4270

Merged
merged 2 commits into from
Sep 26, 2024
Merged

[Diagnostics] Refactor diagnostics track methods to handle background work automatically #4270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa876d4
Refactor diagnostics track methods to handle background work automati…
tonidero Sep 12, 2024
7217cb8
fix swiftlint
tonidero Sep 26, 2024
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
49 changes: 27 additions & 22 deletions Sources/Diagnostics/DiagnosticsTracker.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ import Foundation
protocol DiagnosticsTrackerType {

@available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
func track(_ event: DiagnosticsEvent) async
func track(_ event: DiagnosticsEvent)

@available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
func trackCustomerInfoVerificationResultIfNeeded(_ customerInfo: CustomerInfo) async
func trackCustomerInfoVerificationResultIfNeeded(_ customerInfo: CustomerInfo)

@available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
// swiftlint:disable:next function_parameter_count
Expand All @@ -28,7 +28,7 @@ protocol DiagnosticsTrackerType {
errorMessage: String?,
errorCode: Int?,
storeKitErrorDescription: String?,
responseTime: TimeInterval) async
responseTime: TimeInterval)

@available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
// swiftlint:disable:next function_parameter_count
Expand All @@ -38,37 +38,42 @@ protocol DiagnosticsTrackerType {
responseCode: Int,
backendErrorCode: Int?,
resultOrigin: HTTPResponseOrigin?,
verificationResult: VerificationResult) async
verificationResult: VerificationResult)

@available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
func trackPurchaseRequest(wasSuccessful: Bool,
storeKitVersion: StoreKitVersion,
errorMessage: String?,
errorCode: Int?,
storeKitErrorDescription: String?) async
storeKitErrorDescription: String?)

}

@available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *)
final class DiagnosticsTracker: DiagnosticsTrackerType {
final class DiagnosticsTracker: DiagnosticsTrackerType, Sendable {

private let diagnosticsFileHandler: DiagnosticsFileHandlerType
private let diagnosticsDispatcher: OperationDispatcher
private let dateProvider: DateProvider

init(diagnosticsFileHandler: DiagnosticsFileHandlerType,
diagnosticsDispatcher: OperationDispatcher = .default,
dateProvider: DateProvider = DateProvider()) {
self.diagnosticsFileHandler = diagnosticsFileHandler
self.diagnosticsDispatcher = diagnosticsDispatcher
self.dateProvider = dateProvider
}

func track(_ event: DiagnosticsEvent) async {
await self.clearDiagnosticsFileIfTooBig()
await self.diagnosticsFileHandler.appendEvent(diagnosticsEvent: event)
func track(_ event: DiagnosticsEvent) {
self.diagnosticsDispatcher.dispatchOnWorkerThread {
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 is the main change, now dispatching to a background thread happens internally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need a diagnosticsDispatcher? You could just create a Task here,

func track(_ event: DiagnosticsEvent) {
    Task {
        await self.clearDiagnosticsFileIfTooBig()
        await self.diagnosticsFileHandler.appendEvent(diagnosticsEvent: event)
    }
}

However, with this change, events submitted to track() aren't guaranteed to run sequentially as they are with the OperationDispatcher. But this guarantee won't have existed before, so unless you want to make that change, I think using Task is the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 main reasons I decided to go with the OperationDispatcher:

  • First, as you said, this will make the operations run sequentially, which I believe would be safer, since there could be chances of changing contexts between tracking different events, and if we reach the file too big issue, we might be missing extra data. As you said, this problem would be present right now.
  • Second, I wanted to be able to test this class. If I used Task, I don't have a good way to determine when the tracking finished to do the assertions. With OperationDispatcher, we can pass a MockOperationDispatcher in the tests that runs things synchronously (mostly), allowing us to do the assertions immediately.

Lmk if you would like to talk more about it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, sounds good!

await self.clearDiagnosticsFileIfTooBig()
await self.diagnosticsFileHandler.appendEvent(diagnosticsEvent: event)
}
}

func trackCustomerInfoVerificationResultIfNeeded(
_ customerInfo: CustomerInfo
) async {
) {
let verificationResult = customerInfo.entitlements.verification
if verificationResult == .notRequested {
return
Expand All @@ -79,7 +84,7 @@ final class DiagnosticsTracker: DiagnosticsTrackerType {
properties: [.verificationResultKey: AnyEncodable(verificationResult.name)],
timestamp: self.dateProvider.now()
)
await track(event)
self.track(event)
}

// swiftlint:disable:next function_parameter_count
Expand All @@ -88,8 +93,8 @@ final class DiagnosticsTracker: DiagnosticsTrackerType {
errorMessage: String?,
errorCode: Int?,
storeKitErrorDescription: String?,
responseTime: TimeInterval) async {
await track(
responseTime: TimeInterval) {
self.track(
DiagnosticsEvent(eventType: .appleProductsRequest,
properties: [
.successfulKey: AnyEncodable(wasSuccessful),
Expand All @@ -110,8 +115,8 @@ final class DiagnosticsTracker: DiagnosticsTrackerType {
responseCode: Int,
backendErrorCode: Int?,
resultOrigin: HTTPResponseOrigin?,
verificationResult: VerificationResult) async {
await track(
verificationResult: VerificationResult) {
self.track(
DiagnosticsEvent(
eventType: DiagnosticsEvent.EventType.httpRequestPerformed,
properties: [
Expand All @@ -132,8 +137,8 @@ final class DiagnosticsTracker: DiagnosticsTrackerType {
storeKitVersion: StoreKitVersion,
errorMessage: String?,
errorCode: Int?,
storeKitErrorDescription: String?) async {
await track(
storeKitErrorDescription: String?) {
self.track(
DiagnosticsEvent(eventType: .applePurchaseAttempt,
properties: [
.successfulKey: AnyEncodable(wasSuccessful),
Expand All @@ -154,14 +159,14 @@ private extension DiagnosticsTracker {
func clearDiagnosticsFileIfTooBig() async {
if await self.diagnosticsFileHandler.isDiagnosticsFileTooBig() {
await self.diagnosticsFileHandler.emptyDiagnosticsFile()
await self.trackMaxEventsStoredLimitReached()
self.trackMaxEventsStoredLimitReached()
}
}

func trackMaxEventsStoredLimitReached() async {
await self.track(.init(eventType: .maxEventsStoredLimitReached,
properties: [:],
timestamp: self.dateProvider.now()))
func trackMaxEventsStoredLimitReached() {
self.track(.init(eventType: .maxEventsStoredLimitReached,
properties: [:],
timestamp: self.dateProvider.now()))
}

}
4 changes: 1 addition & 3 deletions Sources/Identity/CustomerInfoManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,7 @@ class CustomerInfoManager {

if #available(iOS 15.0, tvOS 15.0, macOS 12.0, watchOS 8.0, *) {
if let tracker = self.diagnosticsTracker, lastSentCustomerInfo != customerInfo {
Task(priority: .background) {
await tracker.trackCustomerInfoVerificationResultIfNeeded(customerInfo)
}
tracker.trackCustomerInfoVerificationResultIfNeeded(customerInfo)
}
}

Expand Down
50 changes: 24 additions & 26 deletions Sources/Networking/HTTPClient/HTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,33 +582,31 @@ private extension HTTPClient {
guard let diagnosticsTracker = self.diagnosticsTracker, let result else { return }
let responseTime = self.dateProvider.now().timeIntervalSince(requestStartTime)
let requestPathName = request.httpRequest.path.name
Task(priority: .background) {
switch result {
case let .success(response):
let httpStatusCode = response.httpStatusCode.rawValue
let verificationResult = response.verificationResult
await diagnosticsTracker.trackHttpRequestPerformed(endpointName: requestPathName,
responseTime: responseTime,
wasSuccessful: true,
responseCode: httpStatusCode,
backendErrorCode: nil,
resultOrigin: response.origin,
verificationResult: verificationResult)
case let .failure(error):
var responseCode = -1
var backendErrorCode: Int?
if case let .errorResponse(errorResponse, code, _) = error {
responseCode = code.rawValue
backendErrorCode = errorResponse.code.rawValue
}
await diagnosticsTracker.trackHttpRequestPerformed(endpointName: requestPathName,
responseTime: responseTime,
wasSuccessful: false,
responseCode: responseCode,
backendErrorCode: backendErrorCode,
resultOrigin: nil,
verificationResult: .notRequested)
switch result {
case let .success(response):
let httpStatusCode = response.httpStatusCode.rawValue
let verificationResult = response.verificationResult
diagnosticsTracker.trackHttpRequestPerformed(endpointName: requestPathName,
responseTime: responseTime,
wasSuccessful: true,
responseCode: httpStatusCode,
backendErrorCode: nil,
resultOrigin: response.origin,
verificationResult: verificationResult)
case let .failure(error):
var responseCode = -1
var backendErrorCode: Int?
if case let .errorResponse(errorResponse, code, _) = error {
responseCode = code.rawValue
backendErrorCode = errorResponse.code.rawValue
}
diagnosticsTracker.trackHttpRequestPerformed(endpointName: requestPathName,
responseTime: responseTime,
wasSuccessful: false,
responseCode: responseCode,
backendErrorCode: backendErrorCode,
resultOrigin: nil,
verificationResult: .notRequested)
}
}
}
Expand Down
14 changes: 6 additions & 8 deletions Sources/Purchasing/ProductsManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,12 @@ private extension ProductsManager {
?? error?.localizedDescription
let errorCode = error?.errorCode
let storeKitErrorDescription = StoreKitErrorUtils.extractStoreKitErrorDescription(from: error)
Task(priority: .background) {
await diagnosticsTracker.trackProductsRequest(wasSuccessful: error == nil,
storeKitVersion: storeKitVersion,
errorMessage: errorMessage,
errorCode: errorCode,
storeKitErrorDescription: storeKitErrorDescription,
responseTime: responseTime)
}
diagnosticsTracker.trackProductsRequest(wasSuccessful: error == nil,
storeKitVersion: storeKitVersion,
errorMessage: errorMessage,
errorCode: errorCode,
storeKitErrorDescription: storeKitErrorDescription,
responseTime: responseTime)
}
}

Expand Down
21 changes: 9 additions & 12 deletions Sources/Purchasing/Purchases/PurchasesOrchestrator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,18 +895,15 @@ private extension PurchasesOrchestrator {
error: PublicError?) {
if #available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *),
let diagnosticsTracker = self.diagnosticsTracker {
Task(priority: .background) {
let errorMessage =
(error?.userInfo[NSUnderlyingErrorKey] as? Error)?.localizedDescription ?? error?.localizedDescription
let errorCode = error?.code
let storeKitErrorDescription = StoreKitErrorUtils.extractStoreKitErrorDescription(from: error)

await diagnosticsTracker.trackPurchaseRequest(wasSuccessful: error == nil,
storeKitVersion: storeKitVersion,
errorMessage: errorMessage,
errorCode: errorCode,
storeKitErrorDescription: storeKitErrorDescription)
}
let errorMessage = (error?.userInfo[NSUnderlyingErrorKey] as? Error)?.localizedDescription
?? error?.localizedDescription
let errorCode = error?.code
let storeKitErrorDescription = StoreKitErrorUtils.extractStoreKitErrorDescription(from: error)
diagnosticsTracker.trackPurchaseRequest(wasSuccessful: error == nil,
storeKitVersion: storeKitVersion,
errorMessage: errorMessage,
errorCode: errorCode,
storeKitErrorDescription: storeKitErrorDescription)
}
}

Expand Down
Loading