From da2e3f4548358875b2b88bcd9375b4acb16d2329 Mon Sep 17 00:00:00 2001 From: Mert Buran Date: Thu, 10 Sep 2020 18:32:00 +0200 Subject: [PATCH 1/2] RUMM-709 Upload delay increases in case of network error clientTokenError added: unlike clientError, it doesn't decrease interval --- Datadog/Datadog.xcodeproj/project.pbxproj | 8 +- Sources/Datadog/Core/PerformancePreset.swift | 12 +- .../Datadog/Core/Upload/DataUploadDelay.swift | 25 +- .../Core/Upload/DataUploadWorker.swift | 12 +- .../Datadog/Core/Upload/DataUploader.swift | 4 + Sources/Datadog/Core/Upload/HTTPClient.swift | 2 +- .../Datadog/Core/PerformancePresetTests.swift | 8 +- ...Tests.swift => DataUploadDelayTests.swift} | 28 ++- .../Core/Upload/DataUploadWorkerTests.swift | 228 ++++++++++++++---- .../Core/Upload/DataUploaderTests.swift | 13 + .../Datadog/Logging/LoggingFeatureTests.swift | 2 +- .../Datadog/Mocks/CoreMocks.swift | 8 +- .../Datadog/RUM/RUMFeatureTests.swift | 2 +- .../Datadog/Tracing/TracingFeatureTests.swift | 2 +- 14 files changed, 259 insertions(+), 95 deletions(-) rename Tests/DatadogTests/Datadog/Core/Upload/{LogsUploadDelayTests.swift => DataUploadDelayTests.swift} (62%) diff --git a/Datadog/Datadog.xcodeproj/project.pbxproj b/Datadog/Datadog.xcodeproj/project.pbxproj index 43ed1c8fd9..bc47b9be7c 100644 --- a/Datadog/Datadog.xcodeproj/project.pbxproj +++ b/Datadog/Datadog.xcodeproj/project.pbxproj @@ -64,7 +64,7 @@ 61133C5B2423990D00786299 /* DirectoryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61133C2D2423990D00786299 /* DirectoryTests.swift */; }; 61133C5C2423990D00786299 /* DataUploadWorkerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61133C2F2423990D00786299 /* DataUploadWorkerTests.swift */; }; 61133C5D2423990D00786299 /* DataUploadConditionsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61133C302423990D00786299 /* DataUploadConditionsTests.swift */; }; - 61133C5E2423990D00786299 /* LogsUploadDelayTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61133C312423990D00786299 /* LogsUploadDelayTests.swift */; }; + 61133C5E2423990D00786299 /* DataUploadDelayTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61133C312423990D00786299 /* DataUploadDelayTests.swift */; }; 61133C5F2423990D00786299 /* DataUploaderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61133C322423990D00786299 /* DataUploaderTests.swift */; }; 61133C602423990D00786299 /* HTTPHeadersTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61133C332423990D00786299 /* HTTPHeadersTests.swift */; }; 61133C612423990D00786299 /* HTTPClientTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 61133C342423990D00786299 /* HTTPClientTests.swift */; }; @@ -384,7 +384,7 @@ 61133C2D2423990D00786299 /* DirectoryTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DirectoryTests.swift; sourceTree = ""; }; 61133C2F2423990D00786299 /* DataUploadWorkerTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DataUploadWorkerTests.swift; sourceTree = ""; }; 61133C302423990D00786299 /* DataUploadConditionsTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DataUploadConditionsTests.swift; sourceTree = ""; }; - 61133C312423990D00786299 /* LogsUploadDelayTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = LogsUploadDelayTests.swift; sourceTree = ""; }; + 61133C312423990D00786299 /* DataUploadDelayTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DataUploadDelayTests.swift; sourceTree = ""; }; 61133C322423990D00786299 /* DataUploaderTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DataUploaderTests.swift; sourceTree = ""; }; 61133C332423990D00786299 /* HTTPHeadersTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = HTTPHeadersTests.swift; sourceTree = ""; }; 61133C342423990D00786299 /* HTTPClientTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = HTTPClientTests.swift; sourceTree = ""; }; @@ -993,7 +993,7 @@ children = ( 61133C2F2423990D00786299 /* DataUploadWorkerTests.swift */, 61133C302423990D00786299 /* DataUploadConditionsTests.swift */, - 61133C312423990D00786299 /* LogsUploadDelayTests.swift */, + 61133C312423990D00786299 /* DataUploadDelayTests.swift */, 61133C322423990D00786299 /* DataUploaderTests.swift */, 61133C332423990D00786299 /* HTTPHeadersTests.swift */, 61133C342423990D00786299 /* HTTPClientTests.swift */, @@ -2068,7 +2068,7 @@ 61BBD19724ED50040023E65F /* FeaturesConfigurationTests.swift in Sources */, 61133C612423990D00786299 /* HTTPClientTests.swift in Sources */, 61133C6A2423990D00786299 /* DatadogTests.swift in Sources */, - 61133C5E2423990D00786299 /* LogsUploadDelayTests.swift in Sources */, + 61133C5E2423990D00786299 /* DataUploadDelayTests.swift in Sources */, 61133C5C2423990D00786299 /* DataUploadWorkerTests.swift in Sources */, 61E909F624A32D1C005EA2DE /* GlobalTests.swift in Sources */, 9E58E8E324615EDA008E5063 /* JSONEncoderTests.swift in Sources */, diff --git a/Sources/Datadog/Core/PerformancePreset.swift b/Sources/Datadog/Core/PerformancePreset.swift index dd81dde717..d86ae9ed42 100644 --- a/Sources/Datadog/Core/PerformancePreset.swift +++ b/Sources/Datadog/Core/PerformancePreset.swift @@ -45,9 +45,9 @@ internal protocol UploadPerformancePreset { var minUploadDelay: TimeInterval { get } /// Maximum interval of data upload (in seconds). var maxUploadDelay: TimeInterval { get } - /// If upload succeeds, current interval is multiplied by this factor. - /// Should be less or equal `1.0`. - var uploadDelayDecreaseFactor: Double { get } + /// If upload succeeds or fails, current interval is changed by this rate. Should be less or equal `1.0`. + /// E.g: if rate is `0.1` then `delay` can be increased or decreased by `delay * 0.1`. + var uploadDelayChangeRate: Double { get } } internal struct PerformancePreset: Equatable, StoragePerformancePreset, UploadPerformancePreset { @@ -67,7 +67,7 @@ internal struct PerformancePreset: Equatable, StoragePerformancePreset, UploadPe let defaultUploadDelay: TimeInterval let minUploadDelay: TimeInterval let maxUploadDelay: TimeInterval - let uploadDelayDecreaseFactor: Double + let uploadDelayChangeRate: Double // MARK: - Predefined presets @@ -91,7 +91,7 @@ internal struct PerformancePreset: Equatable, StoragePerformancePreset, UploadPe defaultUploadDelay: 5, minUploadDelay: 1, maxUploadDelay: 20, - uploadDelayDecreaseFactor: 0.9 + uploadDelayChangeRate: 0.1 ) /// Performance preset optimized for instant data delivery. @@ -111,7 +111,7 @@ internal struct PerformancePreset: Equatable, StoragePerformancePreset, UploadPe defaultUploadDelay: 3, minUploadDelay: 1, maxUploadDelay: 5, - uploadDelayDecreaseFactor: 0.5 // reduce significantly for more uploads in short-lived app extensions + uploadDelayChangeRate: 0.5 // reduce significantly for more uploads in short-lived app extensions ) static func best(for bundleType: BundleType) -> PerformancePreset { diff --git a/Sources/Datadog/Core/Upload/DataUploadDelay.swift b/Sources/Datadog/Core/Upload/DataUploadDelay.swift index dfa2dd420a..613a78ca17 100644 --- a/Sources/Datadog/Core/Upload/DataUploadDelay.swift +++ b/Sources/Datadog/Core/Upload/DataUploadDelay.swift @@ -6,12 +6,18 @@ import Foundation +internal protocol Delay { + func nextUploadDelay() -> TimeInterval + mutating func decrease() + mutating func increase() +} + /// Mutable interval used for periodic data uploads. -internal struct DataUploadDelay { +internal struct DataUploadDelay: Delay { private let defaultDelay: TimeInterval private let minDelay: TimeInterval private let maxDelay: TimeInterval - private let decreaseFactor: Double + private let changeRate: Double private var delay: TimeInterval @@ -19,24 +25,19 @@ internal struct DataUploadDelay { self.defaultDelay = performance.defaultUploadDelay self.minDelay = performance.minUploadDelay self.maxDelay = performance.maxUploadDelay - self.decreaseFactor = performance.uploadDelayDecreaseFactor + self.changeRate = performance.uploadDelayChangeRate self.delay = performance.initialUploadDelay } - mutating func nextUploadDelay() -> TimeInterval { - defer { - if delay == maxDelay { - delay = defaultDelay - } - } + func nextUploadDelay() -> TimeInterval { return delay } mutating func decrease() { - delay = max(minDelay, delay * decreaseFactor) + delay = max(minDelay, delay * (1.0 - changeRate)) } - mutating func increaseOnce() { - delay = maxDelay + mutating func increase() { + delay = min(delay * (1.0 + changeRate), maxDelay) } } diff --git a/Sources/Datadog/Core/Upload/DataUploadWorker.swift b/Sources/Datadog/Core/Upload/DataUploadWorker.swift index 187b80059d..1aba7382a0 100644 --- a/Sources/Datadog/Core/Upload/DataUploadWorker.swift +++ b/Sources/Datadog/Core/Upload/DataUploadWorker.swift @@ -27,14 +27,14 @@ internal class DataUploadWorker: DataUploadWorkerType { private let featureName: String /// Delay used to schedule consecutive uploads. - private var delay: DataUploadDelay + private var delay: Delay init( queue: DispatchQueue, fileReader: FileReaderType, dataUploader: DataUploader, uploadConditions: DataUploadConditions, - delay: DataUploadDelay, + delay: Delay, featureName: String ) { self.queue = queue @@ -67,21 +67,23 @@ internal class DataUploadWorker: DataUploadWorkerType { if shouldBeAccepted { self.fileReader.markBatchAsRead(batch) + self.delay.decrease() + developerLogger?.info(" → (\(self.featureName)) accepted, won't be retransmitted: \(uploadStatus)") userLogger.debug(" → (\(self.featureName)) accepted, won't be retransmitted: \(uploadStatus)") } else { + self.delay.increase() + developerLogger?.info(" → (\(self.featureName)) not delivered, will be retransmitted: \(uploadStatus)") userLogger.debug(" → (\(self.featureName)) not delivered, will be retransmitted: \(uploadStatus)") } - - self.delay.decrease() } else { let batchLabel = nextBatch != nil ? "YES" : (isSystemReady ? "NO" : "NOT CHECKED") let systemLabel = isSystemReady ? "✅" : "❌" developerLogger?.info("💡 (\(self.featureName)) No upload. Batch to upload: \(batchLabel), System conditions: \(systemLabel)") userLogger.debug("💡 (\(self.featureName)) No upload. Batch to upload: \(batchLabel), System conditions: \(systemLabel)") - self.delay.increaseOnce() + self.delay.increase() } self.scheduleNextUpload(after: self.delay.nextUploadDelay()) diff --git a/Sources/Datadog/Core/Upload/DataUploader.swift b/Sources/Datadog/Core/Upload/DataUploader.swift index 4188b55936..68fd4a9c51 100644 --- a/Sources/Datadog/Core/Upload/DataUploader.swift +++ b/Sources/Datadog/Core/Upload/DataUploader.swift @@ -108,6 +108,9 @@ internal enum DataUploadStatus: Equatable, Hashable { case success /// Corresponds to HTTP 3xx response status codes. case redirection + /// Corresponds to HTTP 403 response status codes, + /// which means client token is invalid + case clientTokenError /// Corresponds to HTTP 4xx response status codes. case clientError /// Corresponds to HTTP 5xx response status codes. @@ -121,6 +124,7 @@ internal enum DataUploadStatus: Equatable, Hashable { switch httpResponse.statusCode { case 200...299: self = .success case 300...399: self = .redirection + case 403: self = .clientTokenError case 400...499: self = .clientError case 500...599: self = .serverError default: self = .unknown diff --git a/Sources/Datadog/Core/Upload/HTTPClient.swift b/Sources/Datadog/Core/Upload/HTTPClient.swift index 8855c2ce25..2e8fabfcda 100644 --- a/Sources/Datadog/Core/Upload/HTTPClient.swift +++ b/Sources/Datadog/Core/Upload/HTTPClient.swift @@ -37,7 +37,7 @@ internal final class HTTPClient { /// The code execution in `URLSessionTransport` should never reach its initialization. internal struct URLSessionTransportInconsistencyException: Error {} -/// As `URLSession` returns 3-values-touple for request execution, this function applies consistency constraints and turns +/// As `URLSession` returns 3-values-tuple for request execution, this function applies consistency constraints and turns /// it into only two possible states of `HTTPTransportResult`. private func httpClientResult(for urlSessionTaskCompletion: (Data?, URLResponse?, Error?)) -> Result { let (_, response, error) = urlSessionTaskCompletion diff --git a/Tests/DatadogTests/Datadog/Core/PerformancePresetTests.swift b/Tests/DatadogTests/Datadog/Core/PerformancePresetTests.swift index 1cca361102..40fbc26684 100644 --- a/Tests/DatadogTests/Datadog/Core/PerformancePresetTests.swift +++ b/Tests/DatadogTests/Datadog/Core/PerformancePresetTests.swift @@ -47,14 +47,14 @@ class PerformancePresetTests: XCTestCase { "Upload delay boundaries must be consistent." ) XCTAssertLessThanOrEqual( - preset.uploadDelayDecreaseFactor, + preset.uploadDelayChangeRate, 1, - "Upload delay should not be increased towards infinity." + "Upload delay should not change by more than %100 at once." ) XCTAssertGreaterThan( - preset.uploadDelayDecreaseFactor, + preset.uploadDelayChangeRate, 0, - "Upload delay must never result with 0." + "Upload delay must change at non-zero rate." ) } } diff --git a/Tests/DatadogTests/Datadog/Core/Upload/LogsUploadDelayTests.swift b/Tests/DatadogTests/Datadog/Core/Upload/DataUploadDelayTests.swift similarity index 62% rename from Tests/DatadogTests/Datadog/Core/Upload/LogsUploadDelayTests.swift rename to Tests/DatadogTests/Datadog/Core/Upload/DataUploadDelayTests.swift index f5a52a887f..c44b7a55b1 100644 --- a/Tests/DatadogTests/Datadog/Core/Upload/LogsUploadDelayTests.swift +++ b/Tests/DatadogTests/Datadog/Core/Upload/DataUploadDelayTests.swift @@ -13,11 +13,11 @@ class DataUploadDelayTests: XCTestCase { defaultUploadDelay: 5, minUploadDelay: 1, maxUploadDelay: 20, - uploadDelayDecreaseFactor: 0.9 + uploadDelayChangeRate: 0.1 ) func testWhenNotModified_itReturnsInitialDelay() { - var delay = DataUploadDelay(performance: mockPerformance) + let delay = DataUploadDelay(performance: mockPerformance) XCTAssertEqual(delay.nextUploadDelay(), mockPerformance.initialUploadDelay) XCTAssertEqual(delay.nextUploadDelay(), mockPerformance.initialUploadDelay) } @@ -26,13 +26,13 @@ class DataUploadDelayTests: XCTestCase { var delay = DataUploadDelay(performance: mockPerformance) var previousValue: TimeInterval = delay.nextUploadDelay() - while previousValue != mockPerformance.minUploadDelay { + while previousValue > mockPerformance.minUploadDelay { delay.decrease() let nextValue = delay.nextUploadDelay() XCTAssertEqual( nextValue / previousValue, - mockPerformance.uploadDelayDecreaseFactor, + 1.0 - mockPerformance.uploadDelayChangeRate, accuracy: 0.1 ) XCTAssertLessThanOrEqual(nextValue, max(previousValue, mockPerformance.minUploadDelay)) @@ -41,13 +41,21 @@ class DataUploadDelayTests: XCTestCase { } } - func testWhenIncreasedOnce_itReturnsMaximumDelayOnceThenGoesBackToDefaultDelay() { + func testWhenIncreasing_itClampsToMaximumDelay() { var delay = DataUploadDelay(performance: mockPerformance) - delay.decrease() - delay.increaseOnce() + var previousValue: TimeInterval = delay.nextUploadDelay() + + while previousValue < mockPerformance.maxUploadDelay { + delay.increase() - XCTAssertEqual(delay.nextUploadDelay(), mockPerformance.maxUploadDelay) - XCTAssertEqual(delay.nextUploadDelay(), mockPerformance.defaultUploadDelay) - XCTAssertEqual(delay.nextUploadDelay(), mockPerformance.defaultUploadDelay) + let nextValue = delay.nextUploadDelay() + XCTAssertEqual( + nextValue / previousValue, + 1.0 + mockPerformance.uploadDelayChangeRate, + accuracy: 0.1 + ) + XCTAssertGreaterThanOrEqual(nextValue, min(previousValue, mockPerformance.maxUploadDelay)) + previousValue = nextValue + } } } diff --git a/Tests/DatadogTests/Datadog/Core/Upload/DataUploadWorkerTests.swift b/Tests/DatadogTests/Datadog/Core/Upload/DataUploadWorkerTests.swift index a07872c118..f6c06b2a36 100644 --- a/Tests/DatadogTests/Datadog/Core/Upload/DataUploadWorkerTests.swift +++ b/Tests/DatadogTests/Datadog/Core/Upload/DataUploadWorkerTests.swift @@ -11,6 +11,23 @@ class DataUploadWorkerTests: XCTestCase { private let fileReadWriteQueue = DispatchQueue(label: "dd-tests-read-write", target: .global(qos: .utility)) private let uploaderQueue = DispatchQueue(label: "dd-tests-uploader", target: .global(qos: .utility)) + lazy var dateProvider = RelativeDateProvider(advancingBySeconds: 1) + lazy var orchestrator = FilesOrchestrator( + directory: temporaryDirectory, + performance: StoragePerformanceMock.writeEachObjectToNewFileAndReadAllFiles, + dateProvider: dateProvider + ) + lazy var writer = FileWriter( + dataFormat: .mockWith(prefix: "[", suffix: "]"), + orchestrator: orchestrator, + queue: fileReadWriteQueue + ) + lazy var reader = FileReader( + dataFormat: .mockWith(prefix: "[", suffix: "]"), + orchestrator: orchestrator, + queue: fileReadWriteQueue + ) + override func setUp() { super.setUp() temporaryDirectory.create() @@ -22,68 +39,187 @@ class DataUploadWorkerTests: XCTestCase { } func testItUploadsAllData() throws { - let dateProvider = RelativeDateProvider(advancingBySeconds: 1) - let orchestrator = FilesOrchestrator( - directory: temporaryDirectory, - performance: StoragePerformanceMock.writeEachObjectToNewFileAndReadAllFiles, - dateProvider: dateProvider - ) - let writer = FileWriter( - dataFormat: .mockWith(prefix: "[", suffix: "]"), - orchestrator: orchestrator, - queue: fileReadWriteQueue - ) - let reader = FileReader( - dataFormat: .mockWith(prefix: "[", suffix: "]"), - orchestrator: orchestrator, - queue: fileReadWriteQueue - ) let server = ServerMock(delivery: .success(response: .mockResponseWith(statusCode: 200))) let dataUploader = DataUploader( urlProvider: .mockAny(), httpClient: HTTPClient(session: .serverMockURLSession), httpHeaders: .mockAny() ) - // Write 3 files writer.write(value: ["k1": "v1"]) writer.write(value: ["k2": "v2"]) writer.write(value: ["k3": "v3"]) + // Start logs uploader + try withExtendedLifetime( + DataUploadWorker( + queue: uploaderQueue, + fileReader: reader, + dataUploader: dataUploader, + uploadConditions: DataUploadConditions.alwaysUpload(), + delay: DataUploadDelay(performance: UploadPerformanceMock.veryQuick), + featureName: .mockAny() + ) + ) { + let recordedRequests = server.waitAndReturnRequests(count: 3) + XCTAssertTrue(recordedRequests.contains { $0.httpBody == #"[{"k1":"v1"}]"#.utf8Data }) + XCTAssertTrue(recordedRequests.contains { $0.httpBody == #"[{"k2":"v2"}]"#.utf8Data }) + XCTAssertTrue(recordedRequests.contains { $0.httpBody == #"[{"k3":"v3"}]"#.utf8Data }) + uploaderQueue.sync {} // wait until last "process upload" operation completes (to make sure "delete file" was requested) + fileReadWriteQueue.sync {} // wait until last scheduled "delete file" operation completed + + XCTAssertEqual(try temporaryDirectory.files().count, 0) + } + } + + // swiftlint:disable multiline_arguments_brackets + func test_whenThereIsNoBatch_thenIntervalIncreases() throws { + let expectation = XCTestExpectation(description: "high expectation") + let mockDelay = MockDelay { command in + if case .increase = command { + expectation.fulfill() + } else { + XCTFail("Wrong command is sent!") + } + } + let dataUploader = DataUploader( + urlProvider: .mockAny(), + httpClient: HTTPClient(session: .serverMockURLSession), + httpHeaders: .mockAny() + ) // Start logs uploader - let uploadWorker = DataUploadWorker( - queue: uploaderQueue, - fileReader: reader, - dataUploader: dataUploader, - uploadConditions: DataUploadConditions( - batteryStatus: BatteryStatusProviderMock.mockWith( - status: BatteryStatus(state: .full, level: 100, isLowPowerModeEnabled: false) // always upload - ), - networkConnectionInfo: NetworkConnectionInfoProviderMock( - networkConnectionInfo: NetworkConnectionInfo( - reachability: .yes, // always upload - availableInterfaces: [.wifi], - supportsIPv4: true, - supportsIPv6: true, - isExpensive: false, - isConstrained: false - ) - ) - ), - delay: DataUploadDelay(performance: UploadPerformanceMock.veryQuick), - featureName: .mockAny() + withExtendedLifetime([ + ServerMock(delivery: .success(response: .mockResponseWith(statusCode: 200))), + DataUploadWorker( + queue: uploaderQueue, + fileReader: reader, + dataUploader: dataUploader, + uploadConditions: DataUploadConditions.neverUpload(), + delay: mockDelay, + featureName: .mockAny() + ) + ]) { + self.wait(for: [expectation], timeout: (mockDelay.nextUploadDelay() + 0.1) * 2.0) + } + } + + func test_whenBatchFails_thenIntervalIncreases() throws { + let expectation = XCTestExpectation(description: "high expectation") + let mockDelay = MockDelay { command in + if case .increase = command { + expectation.fulfill() + } else { + XCTFail("Wrong command is sent!") + } + } + let dataUploader = DataUploader( + urlProvider: .mockAny(), + httpClient: HTTPClient(session: .serverMockURLSession), + httpHeaders: .mockAny() ) + // Write some content + writer.write(value: ["k1": "v1"]) + // Start logs uploader + withExtendedLifetime([ + ServerMock(delivery: .success(response: .mockResponseWith(statusCode: 500))), + DataUploadWorker( + queue: uploaderQueue, + fileReader: reader, + dataUploader: dataUploader, + uploadConditions: DataUploadConditions.alwaysUpload(), + delay: mockDelay, + featureName: .mockAny() + ) + ]) { + self.wait(for: [expectation], timeout: mockDelay.nextUploadDelay() + 0.1) + } + } - let recordedRequests = server.waitAndReturnRequests(count: 3) - XCTAssertTrue(recordedRequests.contains { $0.httpBody == #"[{"k1":"v1"}]"#.utf8Data }) - XCTAssertTrue(recordedRequests.contains { $0.httpBody == #"[{"k2":"v2"}]"#.utf8Data }) - XCTAssertTrue(recordedRequests.contains { $0.httpBody == #"[{"k3":"v3"}]"#.utf8Data }) + func test_whenBatchSucceeds_thenIntervalDecreases() throws { + let expectation = XCTestExpectation(description: "low expectation") + let mockDelay = MockDelay { command in + if case .decrease = command { + expectation.fulfill() + } else { + XCTFail("Wrong command is sent!") + } + } + let dataUploader = DataUploader( + urlProvider: .mockAny(), + httpClient: HTTPClient(session: .serverMockURLSession), + httpHeaders: .mockAny() + ) + // Write some content + writer.write(value: ["k1": "v1"]) + // Start logs uploader + withExtendedLifetime([ + ServerMock(delivery: .success(response: .mockResponseWith(statusCode: 200))), + DataUploadWorker( + queue: uploaderQueue, + fileReader: reader, + dataUploader: dataUploader, + uploadConditions: DataUploadConditions.alwaysUpload(), + delay: mockDelay, + featureName: .mockAny() + ) + ]) { + self.wait(for: [expectation], timeout: mockDelay.nextUploadDelay() + 0.1) + } + } + // swiftlint:enable multiline_arguments_brackets +} - uploaderQueue.sync {} // wait until last "process upload" operation completes (to make sure "delete file" was requested) - fileReadWriteQueue.sync {} // wait until last scheduled "delete file" operation completed +struct MockDelay: Delay { + enum Command { + case increase, decrease + } + let callback: (Command) -> Void - XCTAssertEqual(try temporaryDirectory.files().count, 0) + func nextUploadDelay() -> TimeInterval { + return 0.0 + } + func decrease() { + callback(.decrease) + } + func increase() { + callback(.increase) + } +} - _ = uploadWorker // keep the strong reference +private extension DataUploadConditions { + static func alwaysUpload() -> DataUploadConditions { + return DataUploadConditions( + batteryStatus: BatteryStatusProviderMock.mockWith( + status: BatteryStatus(state: .full, level: 100, isLowPowerModeEnabled: false) // always upload + ), + networkConnectionInfo: NetworkConnectionInfoProviderMock( + networkConnectionInfo: NetworkConnectionInfo( + reachability: .yes, // always upload + availableInterfaces: [.wifi], + supportsIPv4: true, + supportsIPv6: true, + isExpensive: false, + isConstrained: false + ) + ) + ) + } + + static func neverUpload() -> DataUploadConditions { + return DataUploadConditions( + batteryStatus: BatteryStatusProviderMock.mockWith( + status: BatteryStatus(state: .unplugged, level: 0, isLowPowerModeEnabled: true) // never upload + ), + networkConnectionInfo: NetworkConnectionInfoProviderMock( + networkConnectionInfo: NetworkConnectionInfo( + reachability: .no, // never upload + availableInterfaces: [.cellular], + supportsIPv4: true, + supportsIPv6: false, + isExpensive: true, + isConstrained: true + ) + ) + ) } } diff --git a/Tests/DatadogTests/Datadog/Core/Upload/DataUploaderTests.swift b/Tests/DatadogTests/Datadog/Core/Upload/DataUploaderTests.swift index a36a69d1e8..e8726e80b6 100644 --- a/Tests/DatadogTests/Datadog/Core/Upload/DataUploaderTests.swift +++ b/Tests/DatadogTests/Datadog/Core/Upload/DataUploaderTests.swift @@ -90,6 +90,19 @@ class DataUploaderTests: XCTestCase { server.waitFor(requestsCompletion: 1) } + func testWhenDataIsSentWith403Code_itReturnsDataUploadStatus_clientTokenError() { + let server = ServerMock(delivery: .success(response: .mockResponseWith(statusCode: 403))) + let uploader = DataUploader( + urlProvider: .mockAny(), + httpClient: HTTPClient(session: .serverMockURLSession), + httpHeaders: .mockAny() + ) + let status = uploader.upload(data: .mockAny()) + + XCTAssertEqual(status, .clientTokenError) + server.waitFor(requestsCompletion: 1) + } + func testWhenDataIsSentWith500Code_itReturnsDataUploadStatus_serverError() { let server = ServerMock(delivery: .success(response: .mockResponseWith(statusCode: 500))) let uploader = DataUploader( diff --git a/Tests/DatadogTests/Datadog/Logging/LoggingFeatureTests.swift b/Tests/DatadogTests/Datadog/Logging/LoggingFeatureTests.swift index da2118dc43..234c24d7c0 100644 --- a/Tests/DatadogTests/Datadog/Logging/LoggingFeatureTests.swift +++ b/Tests/DatadogTests/Datadog/Logging/LoggingFeatureTests.swift @@ -73,7 +73,7 @@ class LoggingFeatureTests: XCTestCase { defaultUploadDelay: 1, minUploadDelay: 1, maxUploadDelay: 1, - uploadDelayDecreaseFactor: 1 + uploadDelayChangeRate: 0 ) ) ) diff --git a/Tests/DatadogTests/Datadog/Mocks/CoreMocks.swift b/Tests/DatadogTests/Datadog/Mocks/CoreMocks.swift index 2a81d1104a..745cc900e5 100644 --- a/Tests/DatadogTests/Datadog/Mocks/CoreMocks.swift +++ b/Tests/DatadogTests/Datadog/Mocks/CoreMocks.swift @@ -192,14 +192,14 @@ struct UploadPerformanceMock: UploadPerformancePreset { let defaultUploadDelay: TimeInterval let minUploadDelay: TimeInterval let maxUploadDelay: TimeInterval - let uploadDelayDecreaseFactor: Double + let uploadDelayChangeRate: Double static let noOp = UploadPerformanceMock( initialUploadDelay: .distantFuture, defaultUploadDelay: .distantFuture, minUploadDelay: .distantFuture, maxUploadDelay: .distantFuture, - uploadDelayDecreaseFactor: 1 + uploadDelayChangeRate: 0 ) static let veryQuick = UploadPerformanceMock( @@ -207,7 +207,7 @@ struct UploadPerformanceMock: UploadPerformancePreset { defaultUploadDelay: 0.05, minUploadDelay: 0.05, maxUploadDelay: 0.05, - uploadDelayDecreaseFactor: 1 + uploadDelayChangeRate: 0 ) } @@ -225,7 +225,7 @@ extension PerformancePreset { defaultUploadDelay: upload.defaultUploadDelay, minUploadDelay: upload.minUploadDelay, maxUploadDelay: upload.maxUploadDelay, - uploadDelayDecreaseFactor: upload.uploadDelayDecreaseFactor + uploadDelayChangeRate: upload.uploadDelayChangeRate ) } } diff --git a/Tests/DatadogTests/Datadog/RUM/RUMFeatureTests.swift b/Tests/DatadogTests/Datadog/RUM/RUMFeatureTests.swift index dcafa5165b..4b807be5c2 100644 --- a/Tests/DatadogTests/Datadog/RUM/RUMFeatureTests.swift +++ b/Tests/DatadogTests/Datadog/RUM/RUMFeatureTests.swift @@ -82,7 +82,7 @@ class RUMFeatureTests: XCTestCase { defaultUploadDelay: 1, minUploadDelay: 1, maxUploadDelay: 1, - uploadDelayDecreaseFactor: 1 + uploadDelayChangeRate: 0 ) ) ) diff --git a/Tests/DatadogTests/Datadog/Tracing/TracingFeatureTests.swift b/Tests/DatadogTests/Datadog/Tracing/TracingFeatureTests.swift index 7e1dfdb43d..de31aacaa6 100644 --- a/Tests/DatadogTests/Datadog/Tracing/TracingFeatureTests.swift +++ b/Tests/DatadogTests/Datadog/Tracing/TracingFeatureTests.swift @@ -75,7 +75,7 @@ class TracingFeatureTests: XCTestCase { defaultUploadDelay: 1, minUploadDelay: 1, maxUploadDelay: 1, - uploadDelayDecreaseFactor: 1 + uploadDelayChangeRate: 0 ) ) ) From b65dc0ec3d3ad0f5cb7c4a428b762b8c6f871835 Mon Sep 17 00:00:00 2001 From: Mert Buran Date: Fri, 11 Sep 2020 17:48:17 +0200 Subject: [PATCH 2/2] RUMM-709 PR comments addressed --- .../Datadog/Core/Upload/DataUploadDelay.swift | 6 ++---- .../Datadog/Core/Upload/DataUploadWorker.swift | 4 ++-- .../Core/Upload/DataUploadDelayTests.swift | 12 ++++++------ .../Core/Upload/DataUploadWorkerTests.swift | 16 +++++++--------- Tests/DatadogTests/Datadog/TracerTests.swift | 2 +- 5 files changed, 18 insertions(+), 22 deletions(-) diff --git a/Sources/Datadog/Core/Upload/DataUploadDelay.swift b/Sources/Datadog/Core/Upload/DataUploadDelay.swift index 613a78ca17..8797c08bf6 100644 --- a/Sources/Datadog/Core/Upload/DataUploadDelay.swift +++ b/Sources/Datadog/Core/Upload/DataUploadDelay.swift @@ -7,7 +7,7 @@ import Foundation internal protocol Delay { - func nextUploadDelay() -> TimeInterval + var current: TimeInterval { get } mutating func decrease() mutating func increase() } @@ -29,9 +29,7 @@ internal struct DataUploadDelay: Delay { self.delay = performance.initialUploadDelay } - func nextUploadDelay() -> TimeInterval { - return delay - } + var current: TimeInterval { delay } mutating func decrease() { delay = max(minDelay, delay * (1.0 - changeRate)) diff --git a/Sources/Datadog/Core/Upload/DataUploadWorker.swift b/Sources/Datadog/Core/Upload/DataUploadWorker.swift index 1aba7382a0..8e93a3c980 100644 --- a/Sources/Datadog/Core/Upload/DataUploadWorker.swift +++ b/Sources/Datadog/Core/Upload/DataUploadWorker.swift @@ -44,7 +44,7 @@ internal class DataUploadWorker: DataUploadWorkerType { self.delay = delay self.featureName = featureName - scheduleNextUpload(after: self.delay.nextUploadDelay()) + scheduleNextUpload(after: self.delay.current) } private func scheduleNextUpload(after delay: TimeInterval) { @@ -86,7 +86,7 @@ internal class DataUploadWorker: DataUploadWorkerType { self.delay.increase() } - self.scheduleNextUpload(after: self.delay.nextUploadDelay()) + self.scheduleNextUpload(after: self.delay.current) } } } diff --git a/Tests/DatadogTests/Datadog/Core/Upload/DataUploadDelayTests.swift b/Tests/DatadogTests/Datadog/Core/Upload/DataUploadDelayTests.swift index c44b7a55b1..2f853dfb0f 100644 --- a/Tests/DatadogTests/Datadog/Core/Upload/DataUploadDelayTests.swift +++ b/Tests/DatadogTests/Datadog/Core/Upload/DataUploadDelayTests.swift @@ -18,18 +18,18 @@ class DataUploadDelayTests: XCTestCase { func testWhenNotModified_itReturnsInitialDelay() { let delay = DataUploadDelay(performance: mockPerformance) - XCTAssertEqual(delay.nextUploadDelay(), mockPerformance.initialUploadDelay) - XCTAssertEqual(delay.nextUploadDelay(), mockPerformance.initialUploadDelay) + XCTAssertEqual(delay.current, mockPerformance.initialUploadDelay) + XCTAssertEqual(delay.current, mockPerformance.initialUploadDelay) } func testWhenDecreasing_itGoesDownToMinimumDelay() { var delay = DataUploadDelay(performance: mockPerformance) - var previousValue: TimeInterval = delay.nextUploadDelay() + var previousValue: TimeInterval = delay.current while previousValue > mockPerformance.minUploadDelay { delay.decrease() - let nextValue = delay.nextUploadDelay() + let nextValue = delay.current XCTAssertEqual( nextValue / previousValue, 1.0 - mockPerformance.uploadDelayChangeRate, @@ -43,12 +43,12 @@ class DataUploadDelayTests: XCTestCase { func testWhenIncreasing_itClampsToMaximumDelay() { var delay = DataUploadDelay(performance: mockPerformance) - var previousValue: TimeInterval = delay.nextUploadDelay() + var previousValue: TimeInterval = delay.current while previousValue < mockPerformance.maxUploadDelay { delay.increase() - let nextValue = delay.nextUploadDelay() + let nextValue = delay.current XCTAssertEqual( nextValue / previousValue, 1.0 + mockPerformance.uploadDelayChangeRate, diff --git a/Tests/DatadogTests/Datadog/Core/Upload/DataUploadWorkerTests.swift b/Tests/DatadogTests/Datadog/Core/Upload/DataUploadWorkerTests.swift index f6c06b2a36..0dc364132b 100644 --- a/Tests/DatadogTests/Datadog/Core/Upload/DataUploadWorkerTests.swift +++ b/Tests/DatadogTests/Datadog/Core/Upload/DataUploadWorkerTests.swift @@ -73,7 +73,7 @@ class DataUploadWorkerTests: XCTestCase { } // swiftlint:disable multiline_arguments_brackets - func test_whenThereIsNoBatch_thenIntervalIncreases() throws { + func testWhenThereIsNoBatch_thenIntervalIncreases() throws { let expectation = XCTestExpectation(description: "high expectation") let mockDelay = MockDelay { command in if case .increase = command { @@ -99,11 +99,11 @@ class DataUploadWorkerTests: XCTestCase { featureName: .mockAny() ) ]) { - self.wait(for: [expectation], timeout: (mockDelay.nextUploadDelay() + 0.1) * 2.0) + self.wait(for: [expectation], timeout: (mockDelay.current + 0.1) * 1.5) } } - func test_whenBatchFails_thenIntervalIncreases() throws { + func testWhenBatchFails_thenIntervalIncreases() throws { let expectation = XCTestExpectation(description: "high expectation") let mockDelay = MockDelay { command in if case .increase = command { @@ -131,11 +131,11 @@ class DataUploadWorkerTests: XCTestCase { featureName: .mockAny() ) ]) { - self.wait(for: [expectation], timeout: mockDelay.nextUploadDelay() + 0.1) + self.wait(for: [expectation], timeout: (mockDelay.current + 0.1) * 1.5) } } - func test_whenBatchSucceeds_thenIntervalDecreases() throws { + func testWhenBatchSucceeds_thenIntervalDecreases() throws { let expectation = XCTestExpectation(description: "low expectation") let mockDelay = MockDelay { command in if case .decrease = command { @@ -163,7 +163,7 @@ class DataUploadWorkerTests: XCTestCase { featureName: .mockAny() ) ]) { - self.wait(for: [expectation], timeout: mockDelay.nextUploadDelay() + 0.1) + self.wait(for: [expectation], timeout: (mockDelay.current + 0.1) * 1.5) } } // swiftlint:enable multiline_arguments_brackets @@ -175,9 +175,7 @@ struct MockDelay: Delay { } let callback: (Command) -> Void - func nextUploadDelay() -> TimeInterval { - return 0.0 - } + var current: TimeInterval { 0.0 } func decrease() { callback(.decrease) } diff --git a/Tests/DatadogTests/Datadog/TracerTests.swift b/Tests/DatadogTests/Datadog/TracerTests.swift index 883b3ff075..c54bb0ed33 100644 --- a/Tests/DatadogTests/Datadog/TracerTests.swift +++ b/Tests/DatadogTests/Datadog/TracerTests.swift @@ -807,7 +807,7 @@ class TracerTests: XCTestCase { ) } - func test_whenSpanStateChangesFromDifferentThreads_itChangesSpanState() { + func testWhenSpanStateChangesFromDifferentThreads_itChangesSpanState() { TracingFeature.instance = .mockNoOp() defer { TracingFeature.instance = nil } let tracer = Tracer.initialize(configuration: .init())