From 1da9f99a39205b956e772e6ced16e00e8400fe1e Mon Sep 17 00:00:00 2001 From: kenji Date: Fri, 1 Sep 2023 16:22:06 +0700 Subject: [PATCH] fix: catch network error & add the current timeout setting in the TimeoutErrorMetricsEvent --- .../Internal/Event/EventInteractor.swift | 131 +++++++++++------- .../Internal/Remote/ApiClientImpl.swift | 16 ++- Bucketeer/Sources/Public/BKTError.swift | 56 ++++++-- BucketeerTests/BKTClientTests.swift | 19 ++- BucketeerTests/BKTErrorTests.swift | 102 +++++++++++++- BucketeerTests/EventInteractorTests.swift | 6 +- BucketeerTests/Mock/MockEvents.swift | 2 +- 7 files changed, 254 insertions(+), 78 deletions(-) diff --git a/Bucketeer/Sources/Internal/Event/EventInteractor.swift b/Bucketeer/Sources/Internal/Event/EventInteractor.swift index 36fc0e8e..dc7b756d 100644 --- a/Bucketeer/Sources/Internal/Event/EventInteractor.swift +++ b/Bucketeer/Sources/Internal/Event/EventInteractor.swift @@ -178,11 +178,17 @@ final class EventInteractorImpl: EventInteractor { } func trackFetchEvaluationsFailure(featureTag: String, error: BKTError) throws { - let metrics = metricsEvent(apiId: .getEvaluations, labels: ["tag": featureTag], error: error) + let eventData = error.toMetricsEventData( + apiId: .getEvaluations, + labels: ["tag": featureTag], + currentTimeSeconds: clock.currentTimeSeconds, + sdkVersion: sdkVersion, + metadata: metadata + ) try trackMetricsEvent(events: [ .init( id: idGenerator.id(), - event: metrics, + event: eventData, type: .metrics ) ]) @@ -190,11 +196,17 @@ final class EventInteractorImpl: EventInteractor { func trackRegisterEventsFailure(error: BKTError) throws { // note: using the same tag in BKConfig.featureTag - let metrics = metricsEvent(apiId: .registerEvents, labels: ["tag": featureTag], error: error) + let eventData = error.toMetricsEventData( + apiId: .registerEvents, + labels: ["tag": featureTag], + currentTimeSeconds: clock.currentTimeSeconds, + sdkVersion: sdkVersion, + metadata: metadata + ) try trackMetricsEvent(events: [ .init( id: idGenerator.id(), - event: metrics, + event: eventData, type: .metrics ) ]) @@ -271,54 +283,6 @@ final class EventInteractorImpl: EventInteractor { } } - private func metricsEvent(apiId: ApiId, labels: [String: String], error: BKTError) -> EventData { - let metricsEventData: MetricsEventData - let metricsEventType: MetricsEventType - switch error { - case .timeout: - metricsEventData = .timeoutError(.init(apiId: apiId, labels: labels)) - metricsEventType = .timeoutError - case .network: - metricsEventData = .networkError(.init(apiId: apiId, labels: labels)) - metricsEventType = .networkError - case .badRequest: - metricsEventData = .badRequestError(.init(apiId: apiId, labels: labels)) - metricsEventType = .badRequestError - case .unauthorized: - metricsEventData = .unauthorizedError(.init(apiId: apiId, labels: labels)) - metricsEventType = .unauthorizedError - case .forbidden: - metricsEventData = .forbiddenError(.init(apiId: apiId, labels: labels)) - metricsEventType = .forbiddenError - case .notFound: - metricsEventData = .notFoundError(.init(apiId: apiId, labels: labels)) - metricsEventType = .notFoundError - case .clientClosed: - metricsEventData = .clientClosedError(.init(apiId: apiId, labels: labels)) - metricsEventType = .clientClosedError - case .unavailable: - metricsEventData = .unavailableError(.init(apiId: apiId, labels: labels)) - metricsEventType = .unavailableError - case .apiServer: - metricsEventData = .internalServerError(.init(apiId: apiId, labels: labels)) - metricsEventType = .internalServerError - case .unknownServer: - metricsEventData = .unknownError(.init(apiId: apiId, labels: labels)) - metricsEventType = .unknownError - default: - metricsEventData = .internalSdkError(.init(apiId: apiId, labels: labels)) - metricsEventType = .internalError - } - return .metrics(.init( - timestamp: clock.currentTimeSeconds, - event: metricsEventData, - type: metricsEventType, - sourceId: .ios, - sdk_version: sdkVersion, - metadata: metadata - )) - } - private func updateEventsAndNotify() { do { let events = try eventDao.getEvents() @@ -369,3 +333,66 @@ extension Event { } } } + +extension BKTError { + func toMetricsEventData(apiId: ApiId, labels: [String: String], currentTimeSeconds: Int64, sdkVersion: String, metadata: [String: String]?) -> + EventData { + let error = self + let metricsEventData: MetricsEventData + let metricsEventType: MetricsEventType + switch error { + case .timeout(_, _, let timeoutMillis): + // https://github.com/bucketeer-io/ios-client-sdk/issues/16 + // Pass the current timeout setting in seconds via labels. + let timeoutSecs : Double = Double(timeoutMillis)/1000 + metricsEventData = .timeoutError( + .init( + apiId: apiId, + labels: labels.merging( + ["timeout":"\(timeoutSecs)"] + , uniquingKeysWith: { (first, _) in first } + ) + ) + ) + metricsEventType = .timeoutError + case .network: + metricsEventData = .networkError(.init(apiId: apiId, labels: labels)) + metricsEventType = .networkError + case .badRequest: + metricsEventData = .badRequestError(.init(apiId: apiId, labels: labels)) + metricsEventType = .badRequestError + case .unauthorized: + metricsEventData = .unauthorizedError(.init(apiId: apiId, labels: labels)) + metricsEventType = .unauthorizedError + case .forbidden: + metricsEventData = .forbiddenError(.init(apiId: apiId, labels: labels)) + metricsEventType = .forbiddenError + case .notFound: + metricsEventData = .notFoundError(.init(apiId: apiId, labels: labels)) + metricsEventType = .notFoundError + case .clientClosed: + metricsEventData = .clientClosedError(.init(apiId: apiId, labels: labels)) + metricsEventType = .clientClosedError + case .unavailable: + metricsEventData = .unavailableError(.init(apiId: apiId, labels: labels)) + metricsEventType = .unavailableError + case .apiServer: + metricsEventData = .internalServerError(.init(apiId: apiId, labels: labels)) + metricsEventType = .internalServerError + case .illegalArgument, .illegalState: + metricsEventData = .internalSdkError(.init(apiId: apiId, labels: labels)) + metricsEventType = .internalError + case .unknownServer, .unknown: + metricsEventData = .unknownError(.init(apiId: apiId, labels: labels)) + metricsEventType = .unknownError + } + return .metrics(.init( + timestamp: currentTimeSeconds, + event: metricsEventData, + type: metricsEventType, + sourceId: .ios, + sdk_version: sdkVersion, + metadata: metadata + )) + } +} diff --git a/Bucketeer/Sources/Internal/Remote/ApiClientImpl.swift b/Bucketeer/Sources/Internal/Remote/ApiClientImpl.swift index 6e223d86..d34f509a 100644 --- a/Bucketeer/Sources/Internal/Remote/ApiClientImpl.swift +++ b/Bucketeer/Sources/Internal/Remote/ApiClientImpl.swift @@ -45,11 +45,12 @@ final class ApiClientImpl: ApiClient { sourceId: .ios ) let featureTag = self.featureTag + let timeoutMillisValue = timeoutMillis ?? defaultRequestTimeoutMills logger?.debug(message: "[API] Fetch Evaluation: \(requestBody)") send( requestBody: requestBody, path: "get_evaluations", - timeoutMillis: timeoutMillis ?? defaultRequestTimeoutMills, + timeoutMillis: timeoutMillisValue, completion: { (result: Result<(GetEvaluationsResponse, URLResponse), Error>) in switch result { case .success((var response, let urlResponse)): @@ -61,7 +62,7 @@ final class ApiClientImpl: ApiClient { response.featureTag = featureTag completion?(.success(response)) case .failure(let error): - completion?(.failure(error: .init(error: error), featureTag: featureTag)) + completion?(.failure(error: .init(error: error).copyWith(timeoutMillis: timeoutMillisValue), featureTag: featureTag)) } } ) @@ -214,3 +215,14 @@ fileprivate extension UnixTimestamp { return Date(timeIntervalSince1970: TimeInterval(self / 1_000)) // must take a millisecond-precise Unix timestamp } } + +extension BKTError { + func copyWith(timeoutMillis: Int64) -> BKTError { + switch self { + case .timeout(let m, let e, _): + return .timeout(message: m, error: e, timeoutMillis: timeoutMillis) + default: + return self + } + } +} diff --git a/Bucketeer/Sources/Public/BKTError.swift b/Bucketeer/Sources/Public/BKTError.swift index 6cc1944a..86af79d2 100644 --- a/Bucketeer/Sources/Public/BKTError.swift +++ b/Bucketeer/Sources/Public/BKTError.swift @@ -10,7 +10,7 @@ public enum BKTError: Error, Equatable { case apiServer(message: String) // network errors - case timeout(message: String, error: Error) + case timeout(message: String, error: Error, timeoutMillis: Int64) case network(message: String, error: Error) // sdk errors @@ -33,8 +33,9 @@ public enum BKTError: Error, Equatable { (.illegalArgument(let m1), .illegalArgument(let m2)), (.illegalState(let m1), .illegalState(let m2)): return m1 == m2 - case (.timeout(let m1, _), .timeout(let m2, _)), - (.network(let m1, _), .network(let m2, _)), + case (.timeout(let m1, _, let t1), .timeout(let m2, _, let t2)): + return t1 == t2 && m1 == m2 + case (.network(let m1, _), .network(let m2, _)), (.unknownServer(let m1, _), .unknownServer(let m2, _)), (.unknown(let m1, _), .unknown(let m2, _)): return m1 == m2 @@ -45,6 +46,7 @@ public enum BKTError: Error, Equatable { } extension BKTError : LocalizedError { + internal init(error: Error) { if let bktError = error as? BKTError { self = bktError @@ -87,9 +89,15 @@ extension BKTError : LocalizedError { } let nsError = error as NSError - if nsError.domain == NSURLErrorDomain, - nsError.code == NSURLErrorTimedOut { - self = .timeout(message: "Request timeout error: \(error)", error: error) + if nsError.domain == NSURLErrorDomain { + let nsErrorCode = nsError.code + if BKTError.networkErrorCodes.contains(nsErrorCode) { + self = .network(message: "Network connection error: \(error)", error: error) + } else if nsErrorCode == NSURLErrorTimedOut { + self = .timeout(message: "Request timeout error: \(error)", error: error, timeoutMillis: 0) + } else { + self = .unknown(message: "Unknown error: \(error)", error: error) + } } else { self = .unknown(message: "Unknown error: \(error)", error: error) } @@ -113,7 +121,7 @@ extension BKTError : LocalizedError { return message case .apiServer(message: let message): return message - case .timeout(message: let message, _): + case .timeout(message: let message, _, _): return message case .network(message: let message, _): return message @@ -143,7 +151,7 @@ extension BKTError : LocalizedError { .illegalState: return nil - case .timeout(message: _, error: let error): + case .timeout(message: _, error: let error, _): // note: create description for unknown error type return "\(error)" @@ -158,3 +166,35 @@ extension BKTError : LocalizedError { } } } + +extension BKTError { + // full list of NSURLError https://developer.apple.com/documentation/foundation/nserror/1448136-nserror_codes#3139076 + static let networkErrorCodes = [ + NSURLErrorBadURL, + NSURLErrorUnsupportedURL, + NSURLErrorNotConnectedToInternet, + NSURLErrorNetworkConnectionLost, + NSURLErrorCannotFindHost, + NSURLErrorCannotConnectToHost, + NSURLErrorDNSLookupFailed, + // Router, gateway error + NSURLErrorHTTPTooManyRedirects, + NSURLErrorRedirectToNonExistentLocation, + // SSL error + NSURLErrorAppTransportSecurityRequiresSecureConnection, + NSURLErrorSecureConnectionFailed, + NSURLErrorServerCertificateHasBadDate, + NSURLErrorServerCertificateUntrusted, + NSURLErrorServerCertificateHasUnknownRoot, + NSURLErrorServerCertificateNotYetValid, + NSURLErrorClientCertificateRejected, + NSURLErrorClientCertificateRequired, + // Data network errors 3G,4G... + NSURLErrorResourceUnavailable, + NSURLErrorCannotLoadFromNetwork, + NSURLErrorInternationalRoamingOff, + NSURLErrorCallIsActive, + NSURLErrorDataNotAllowed, + NSURLErrorRequestBodyStreamExhausted + ] +} diff --git a/BucketeerTests/BKTClientTests.swift b/BucketeerTests/BKTClientTests.swift index db1d60ab..53854257 100644 --- a/BucketeerTests/BKTClientTests.swift +++ b/BucketeerTests/BKTClientTests.swift @@ -184,13 +184,14 @@ final class BKTClientTests: XCTestCase { let expectation = self.expectation(description: "") expectation.expectedFulfillmentCount = 3 var count = 0 + let expectedTimeoutMillis: Int64 = 3500 let dataModule = MockDataModule( userHolder: .init(user: .mock1), apiClient: MockApiClient(getEvaluationsHandler: { (user, userEvaluationsId, timeoutMillis, handler) in XCTAssertEqual(user, .mock1) XCTAssertEqual(userEvaluationsId, "") - XCTAssertEqual(timeoutMillis, nil) - handler?(.failure(error: .timeout(message: "timeout", error: NSError()), featureTag: "feature")) + XCTAssertEqual(timeoutMillis, expectedTimeoutMillis) + handler?(.failure(error: .timeout(message: "timeout", error: NSError(), timeoutMillis: timeoutMillis ?? 0), featureTag: "feature")) expectation.fulfill() }), eventDao: MockEventDao(addEventsHandler: { events in @@ -199,7 +200,15 @@ final class BKTClientTests: XCTestCase { id: "mock1", event: .metrics(.init( timestamp: 1, - event: .timeoutError(.init(apiId: .getEvaluations, labels: ["tag": "feature"])), + event: .timeoutError( + .init( + apiId: .getEvaluations, + labels: [ + "tag": "feature", + "timeout":"\(3.5)" + ] + ) + ), type: .timeoutError, sourceId: .ios, sdk_version: "0.0.2", @@ -222,8 +231,8 @@ final class BKTClientTests: XCTestCase { clock: MockClock(timestamp: 1) ) let client = BKTClient(dataModule: dataModule, dispatchQueue: .global()) - client.fetchEvaluations(timeoutMillis: nil) { error in - XCTAssertEqual(error, .timeout(message: "timeout", error: NSError())) + client.fetchEvaluations(timeoutMillis: expectedTimeoutMillis) { error in + XCTAssertEqual(error, .timeout(message: "timeout", error: NSError(), timeoutMillis: expectedTimeoutMillis)) expectation.fulfill() } wait(for: [expectation], timeout: 0.1) diff --git a/BucketeerTests/BKTErrorTests.swift b/BucketeerTests/BKTErrorTests.swift index 1d41c3b3..6ec30882 100644 --- a/BucketeerTests/BKTErrorTests.swift +++ b/BucketeerTests/BKTErrorTests.swift @@ -24,8 +24,8 @@ class BKTErrorTests: XCTestCase { assertEqual(.unavailable(message: "1"), .unavailable(message: "1")) assertEqual(.apiServer(message: "1"), .apiServer(message: "1")) assertEqual( - .timeout(message: "1", error: SomeError.a), - .timeout(message: "1", error: SomeError.a) + .timeout(message: "1", error: SomeError.a, timeoutMillis: 1000), + .timeout(message: "1", error: SomeError.a, timeoutMillis: 1000) ) assertEqual( .network(message: "1", error: SomeError.a), @@ -44,8 +44,8 @@ class BKTErrorTests: XCTestCase { // equal with diffrent error assertEqual( - .timeout(message: "1", error: SomeError.a), - .timeout(message: "1", error: SomeError.b) + .timeout(message: "1", error: SomeError.a, timeoutMillis: 1000), + .timeout(message: "1", error: SomeError.b, timeoutMillis: 1000) ) assertEqual( .network(message: "1", error: SomeError.a), @@ -69,8 +69,8 @@ class BKTErrorTests: XCTestCase { assertNotEqual(.unavailable(message: "1"), .unavailable(message: "2")) assertNotEqual(.apiServer(message: "1"), .apiServer(message: "2")) assertNotEqual( - .timeout(message: "1", error: SomeError.a), - .timeout(message: "2", error: SomeError.a) + .timeout(message: "1", error: SomeError.a, timeoutMillis: 1000), + .timeout(message: "2", error: SomeError.a, timeoutMillis: 1000) ) assertNotEqual( .network(message: "1", error: SomeError.a), @@ -151,13 +151,101 @@ class BKTErrorTests: XCTestCase { let timeoutError = NSError(domain: NSURLErrorDomain, code: NSURLErrorTimedOut, userInfo: [:]) assertEqual( .init(error: timeoutError), - .timeout(message: "Request timeout error: \(timeoutError)", error: timeoutError) + .timeout(message: "Request timeout error: \(timeoutError)", error: timeoutError, timeoutMillis: 0) ) + for networkErrorCode in BKTError.networkErrorCodes { + let networkError = NSError(domain: NSURLErrorDomain, code: networkErrorCode, userInfo: [:]) + assertEqual( + .init(error: networkError), + .network(message: "Network connection error: \(networkError)", error: networkError) + ) + } + let unknownError = NSError(domain: "unknown", code: 3000, userInfo: [:]) assertEqual( .init(error: unknownError), .unknown(message: "Unknown error: \(unknownError)", error: unknownError) ) } + + func testToMetricsEventData() { + for errorCase in BKTError.allCases { + let apiId : ApiId = .getEvaluations + let labels = ["key":"value"] + let eventData = errorCase.toMetricsEventData(apiId: .getEvaluations, labels: ["key":"value"], currentTimeSeconds: 1000, sdkVersion: "1.0.0", metadata: ["key_metadata":"data"]) + let metricsEventData: MetricsEventData + let metricsEventType: MetricsEventType + switch errorCase { + case .timeout: + metricsEventData = .timeoutError(.init(apiId: apiId, labels: ["key":"value", "timeout": "4.5"])) + metricsEventType = .timeoutError + case .network: + metricsEventData = .networkError(.init(apiId: apiId, labels: labels)) + metricsEventType = .networkError + case .badRequest: + metricsEventData = .badRequestError(.init(apiId: apiId, labels: labels)) + metricsEventType = .badRequestError + case .unauthorized: + metricsEventData = .unauthorizedError(.init(apiId: apiId, labels: labels)) + metricsEventType = .unauthorizedError + case .forbidden: + metricsEventData = .forbiddenError(.init(apiId: apiId, labels: labels)) + metricsEventType = .forbiddenError + case .notFound: + metricsEventData = .notFoundError(.init(apiId: apiId, labels: labels)) + metricsEventType = .notFoundError + case .clientClosed: + metricsEventData = .clientClosedError(.init(apiId: apiId, labels: labels)) + metricsEventType = .clientClosedError + case .unavailable: + metricsEventData = .unavailableError(.init(apiId: apiId, labels: labels)) + metricsEventType = .unavailableError + case .apiServer: + metricsEventData = .internalServerError(.init(apiId: apiId, labels: labels)) + metricsEventType = .internalServerError + case .illegalArgument, .illegalState: + metricsEventData = .internalSdkError(.init(apiId: apiId, labels: labels)) + metricsEventType = .internalError + case .unknownServer, .unknown: + metricsEventData = .unknownError(.init(apiId: apiId, labels: labels)) + metricsEventType = .unknownError + } + let expectedEventData: EventData = .metrics(.init( + timestamp: 1000, + event: metricsEventData, + type: metricsEventType, + sourceId: .ios, + sdk_version: "1.0.0", + metadata: ["key_metadata":"data"] + )) + XCTAssertEqual(eventData, expectedEventData) + } + } +} + +extension BKTError: CaseIterable { + + public enum TestError: Error { + case timeout + case network + case unknown + case unknownServer + } + + public static var allCases: [BKTError] = [ + .badRequest(message: "badRequest"), + .unauthorized(message: "unauthorized"), + .forbidden(message: "forbidden"), + .notFound(message: "notFound"), + .clientClosed(message: "clientClosed"), + .unavailable(message: "unavailable"), + .apiServer(message: "apiServer"), + .timeout(message: "timeout", error: TestError.timeout, timeoutMillis: 4500), + .network(message: "network", error: TestError.network), + .illegalArgument(message: "illegalArgument"), + .illegalState(message: "illegalState"), + .unknownServer(message: "unknownServer", error: TestError.unknownServer), + .unknown(message: "unknown", error: TestError.unknown) + ] } diff --git a/BucketeerTests/EventInteractorTests.swift b/BucketeerTests/EventInteractorTests.swift index 7358e3bd..acb1614a 100644 --- a/BucketeerTests/EventInteractorTests.swift +++ b/BucketeerTests/EventInteractorTests.swift @@ -222,7 +222,7 @@ final class EventInteractorTests: XCTestCase { id: "id", event: .metrics(.init( timestamp: 1, - event: .timeoutError(.init(apiId: .getEvaluations, labels: ["tag": "featureTag1"])), + event: .timeoutError(.init(apiId: .getEvaluations, labels: ["tag": "featureTag1", "timeout": "3.0"])), type: .timeoutError, sourceId: .ios, sdk_version: "0.0.2", @@ -242,7 +242,7 @@ final class EventInteractorTests: XCTestCase { interactor.set(eventUpdateListener: listener) try interactor.trackFetchEvaluationsFailure( featureTag: "featureTag1", - error: .timeout(message: "timeout", error: SomeError.a) + error: .timeout(message: "timeout", error: SomeError.a, timeoutMillis: 3000) ) wait(for: [expectation], timeout: 1) } @@ -442,7 +442,7 @@ final class EventInteractorTests: XCTestCase { // Simulate track metrics events // Try continue send events but fail. Should tracked | id = 7 let timeoutError = NSError(domain: NSURLErrorDomain, code: NSURLErrorTimedOut, userInfo: [:]) - try interactor.trackFetchEvaluationsFailure(featureTag: "featureTag1", error: .timeout(message: "unknown", error: timeoutError)) + try interactor.trackFetchEvaluationsFailure(featureTag: "featureTag1", error: .timeout(message: "unknown", error: timeoutError, timeoutMillis: 2000)) // Try continue send events but fail. Should not track | id = 8 try interactor.trackRegisterEventsFailure(error: .badRequest(message: "bad request")) storedEvents = try dao.getEvents() diff --git a/BucketeerTests/Mock/MockEvents.swift b/BucketeerTests/Mock/MockEvents.swift index d5faa6f8..cad6f66c 100644 --- a/BucketeerTests/Mock/MockEvents.swift +++ b/BucketeerTests/Mock/MockEvents.swift @@ -290,7 +290,7 @@ extension Event { timestamp: 1, event: .timeoutError(.init( apiId: .getEvaluations, - labels: ["tag": "featureTag1"] + labels: ["tag": "featureTag1", "timeout": "2.0"] )), type: .timeoutError, sourceId: .ios,