RUMM-1064 PR comments addressed

buranmert · buranmert · commit 3808aca664a5 · 2021-03-24T17:10:24.000+01:00
diff --git a/Sources/Datadog/Core/System/AppStateListener.swift b/Sources/Datadog/Core/System/AppStateListener.swift
@@ -10,18 +10,13 @@ import class UIKit.UIApplication
 /// A data structure to represent recorded app states in a given period of time
 internal struct AppStateHistory: Equatable {
     /// Snapshot of the app state at `date`
-    struct Snapshot: Equatable, Comparable {
+    struct Snapshot: Equatable {
         let isActive: Bool
         let date: Date
-
-        static func < (lhs: Snapshot, rhs: Snapshot) -> Bool {
-            return lhs.date < rhs.date
-        }
     }
 
     var initialState: Snapshot
-    private(set) var changes = [Snapshot]()
-    // NOTE: RUMM-1064 changes.last.date > finalDate case isn't handled as not realistic
+    var changes = [Snapshot]()
     var finalDate: Date
     var finalState: Snapshot {
         return Snapshot(
@@ -30,11 +25,6 @@ internal struct AppStateHistory: Equatable {
         )
     }
 
-    mutating func add(change: Snapshot) {
-        changes.append(change)
-        changes.sort()
-    }
-
     /// Limits or extrapolates app state history to the given range
     /// This is useful when you record between 0...3t but you are concerned of t...2t only
     /// - Parameter range: if outside of `initialState` and `finalState`, it extrapolates; otherwise it limits
@@ -82,14 +72,14 @@ internal struct AppStateHistory: Equatable {
             // and no change after final state
             return finalState.isActive
         }
-        var active = initialState.isActive
+        var active = initialState
         for change in changes {
             if date < change.date {
                 break
             }
-            active = change.isActive
+            active = change
         }
-        return active
+        return active.isActive
     }
 }
 
@@ -134,15 +124,15 @@ internal class AppStateListener: AppStateListening {
     @objc
     private func appWillResignActive() {
         let now = dateProvider.currentDate()
-        publisher.mutateAsync {
-            $0.add(change: Snapshot(isActive: false, date: now))
-        }
+        var value = publisher.currentValue
+        value.changes.append(Snapshot(isActive: false, date: now))
+        publisher.publishAsync(value)
     }
     @objc
     private func appDidBecomeActive() {
         let now = dateProvider.currentDate()
-        publisher.mutateAsync {
-            $0.add(change: Snapshot(isActive: true, date: now))
-        }
+        var value = publisher.currentValue
+        value.changes.append(Snapshot(isActive: true, date: now))
+        publisher.publishAsync(value)
     }
 }
diff --git a/Sources/Datadog/Core/Utils/ValuePublisher.swift b/Sources/Datadog/Core/Utils/ValuePublisher.swift
@@ -66,11 +66,6 @@ internal class ValuePublisher<Value> {
         concurrentQueue.async(flags: .barrier) { self.unsafeValue = newValue }
     }
 
-    /// Mutates the `currentValue` asynchronously, without blocking the caller thread.
-    func mutateAsync(_ block: @escaping (inout Value) -> Void) {
-        concurrentQueue.async(flags: .barrier) { block(&self.unsafeValue) }
-    }
-
     /// Registers an observer that will be notified on all value changes.
     /// All calls to the `observer` will be synchronised using internal concurrent queue.
     func subscribe<Observer: ValueObserver>(_ observer: Observer) where Observer.ObservedValue == Value {
diff --git a/Sources/Datadog/Tracing/AutoInstrumentation/URLSessionTracingHandler.swift b/Sources/Datadog/Tracing/AutoInstrumentation/URLSessionTracingHandler.swift
@@ -7,6 +7,13 @@
 import Foundation
 
 internal class URLSessionTracingHandler: URLSessionInterceptionHandler {
+    /// Listening to app state changes and use it to report `foreground_duration`
+    let appStateListener: AppStateListening
+
+    init(appStateListener: AppStateListening) {
+        self.appStateListener = appStateListener
+    }
+
     // MARK: - URLSessionInterceptionHandler
 
     func notify_taskInterceptionStarted(interception: TaskInterception) {
@@ -68,12 +75,12 @@ internal class URLSessionTracingHandler: URLSessionInterceptionHandler {
                 }
             }
         }
-        let appStateHistory = interception.appStateListener.history.take(
+        let appStateHistory = appStateListener.history.take(
             between: resourceMetrics.fetch.start...resourceMetrics.fetch.end
         )
         // TODO: RUMM-1064 tag doesn't seem like the best fit but can't find anything better
-        span.setTag(key: "foreground_duration", value: "\(appStateHistory.foregroundDuration)")
-        span.setTag(key: "is_background", value: "\(appStateHistory.didRunInBackground)")
+        span.setTag(key: "foreground_duration", value: appStateHistory.foregroundDuration.toNanoseconds)
+        span.setTag(key: "is_background", value: appStateHistory.didRunInBackground)
 
         span.finish(at: resourceMetrics.fetch.end)
     }
diff --git a/Sources/Datadog/URLSessionAutoInstrumentation/Interception/TaskInterception.swift b/Sources/Datadog/URLSessionAutoInstrumentation/Interception/TaskInterception.swift
@@ -21,18 +21,14 @@ internal class TaskInterception {
     /// Trace information propagated with the task. Not available when Tracing is disabled
     /// or when the task was created through `URLSession.dataTask(with:url)` on some iOS13+.
     private(set) var spanContext: DDSpanContext?
-    /// Listening to app state changes and use it to report `foreground_duration`
-    let appStateListener: AppStateListening
 
     init(
         request: URLRequest,
-        isFirstParty: Bool,
-        appStateListener: AppStateListening
+        isFirstParty: Bool
     ) {
         self.identifier = UUID()
         self.request = request
         self.isFirstPartyRequest = isFirstParty
-        self.appStateListener = appStateListener
     }
 
     func register(metrics: ResourceMetrics) {
diff --git a/Sources/Datadog/URLSessionAutoInstrumentation/Interception/URLSessionInterceptor.swift b/Sources/Datadog/URLSessionAutoInstrumentation/Interception/URLSessionInterceptor.swift
@@ -23,9 +23,6 @@ public class URLSessionInterceptor: URLSessionInterceptorType {
     private let defaultFirstPartyURLsFilter: FirstPartyURLsFilter
     /// Filters internal `URLs` used by the SDK.
     private let internalURLsFilter: InternalURLsFilter
-    /// Listening to app state changes and use it to report `foreground_duration`
-    internal let appStateListener: AppStateListening
-
     /// Handles resources interception.
     /// Depending on which instrumentation is enabled, this can be either RUM or Tracing handler sending respectively: RUM Resource or tracing Span.
     internal let handler: URLSessionInterceptionHandler
@@ -48,7 +45,7 @@ public class URLSessionInterceptor: URLSessionInterceptorType {
         if configuration.instrumentRUM {
             handler = URLSessionRUMResourcesHandler(dateProvider: dateProvider)
         } else {
-            handler = URLSessionTracingHandler()
+            handler = URLSessionTracingHandler(appStateListener: appStateListener)
         }
 
         self.init(configuration: configuration, handler: handler, appStateListener: appStateListener)
@@ -62,7 +59,6 @@ public class URLSessionInterceptor: URLSessionInterceptorType {
         self.defaultFirstPartyURLsFilter = FirstPartyURLsFilter(hosts: configuration.userDefinedFirstPartyHosts)
         self.internalURLsFilter = InternalURLsFilter(urls: configuration.sdkInternalURLs)
         self.handler = handler
-        self.appStateListener = appStateListener
 
         if configuration.instrumentTracing {
             self.injectTracingHeadersToFirstPartyRequests = true
@@ -118,8 +114,7 @@ public class URLSessionInterceptor: URLSessionInterceptorType {
             let isFirstPartyRequest = self.isFirstParty(request: request, for: session)
             let interception = TaskInterception(
                 request: request,
-                isFirstParty: isFirstPartyRequest,
-                appStateListener: self.appStateListener
+                isFirstParty: isFirstPartyRequest
             )
             self.interceptionByTask[task] = interception
 
diff --git a/Tests/DatadogTests/Datadog/Core/Utils/ValuePublisherTests.swift b/Tests/DatadogTests/Datadog/Core/Utils/ValuePublisherTests.swift
@@ -55,32 +55,6 @@ class ValuePublisherTests: XCTestCase {
         waitForExpectations(timeout: 1, handler: nil)
     }
 
-    func testWhenValueMutates_itNotifiesObservers() {
-        let numberOfObservers = 10
-        let initialValue: Int = .mockRandom()
-        let newValue: Int = .mockRandom()
-
-        let expectation = self.expectation(description: "All observers received new value")
-        expectation.expectedFulfillmentCount = numberOfObservers
-
-        let observers: [ValueObserverMock<Int>] = (0..<numberOfObservers).map { _ in
-            ValueObserverMock<Int> { old, new in
-                XCTAssertEqual(old, initialValue)
-                XCTAssertEqual(new, newValue)
-                expectation.fulfill()
-            }
-        }
-
-        let publisher = ValuePublisher<Int>(initialValue: initialValue)
-        observers.forEach { publisher.subscribe($0) }
-
-        // When
-        publisher.mutateAsync { $0 = newValue }
-
-        // Then
-        waitForExpectations(timeout: 1, handler: nil)
-    }
-
     func testWhenNonEquatableValueChanges_itNotifiesObserversOnAllChanges() {
         struct NonEquatableValue {
             let value: Int
diff --git a/Tests/DatadogTests/Datadog/RUM/AutoInstrumentation/Resources/URLSessionRUMResourcesHandlerTests.swift b/Tests/DatadogTests/Datadog/RUM/AutoInstrumentation/Resources/URLSessionRUMResourcesHandlerTests.swift
@@ -26,7 +26,7 @@ class URLSessionRUMResourcesHandlerTests: XCTestCase {
         // Given
         var request = URLRequest(url: .mockRandom())
         request.httpMethod = ["GET", "POST", "PUT", "DELETE"].randomElement()!
-        let taskInterception = TaskInterception(request: request, isFirstParty: .random(), appStateListener: AppStateListener.mockAny())
+        let taskInterception = TaskInterception(request: request, isFirstParty: .random())
         XCTAssertNil(taskInterception.spanContext)
 
         // When
@@ -49,7 +49,7 @@ class URLSessionRUMResourcesHandlerTests: XCTestCase {
         commandSubscriber.onCommandReceived = { _ in receiveCommand.fulfill() }
 
         // Given
-        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: true, appStateListener: AppStateListener.mockAny())
+        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: true)
 
         // When
         handler.notify_taskInterceptionStarted(interception: taskInterception)
@@ -66,7 +66,7 @@ class URLSessionRUMResourcesHandlerTests: XCTestCase {
         commandSubscriber.onCommandReceived = { _ in receiveCommand.fulfill() }
 
         // Given
-        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: false, appStateListener: AppStateListener.mockAny())
+        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: false)
 
         // When
         handler.notify_taskInterceptionStarted(interception: taskInterception)
@@ -83,7 +83,7 @@ class URLSessionRUMResourcesHandlerTests: XCTestCase {
         commandSubscriber.onCommandReceived = { _ in receiveCommand.fulfill() }
 
         // Given
-        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: .random(), appStateListener: AppStateListener.mockAny())
+        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: .random())
         taskInterception.register(spanContext: .mockWith(traceID: 1, spanID: 2))
         XCTAssertNotNil(taskInterception.spanContext)
 
@@ -108,7 +108,7 @@ class URLSessionRUMResourcesHandlerTests: XCTestCase {
         }
 
         // Given
-        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: .random(), appStateListener: AppStateListener.mockAny())
+        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: .random())
         let resourceMetrics: ResourceMetrics = .mockAny()
         let resourceCompletion: ResourceCompletion = .mockWith(response: .mockResponseWith(statusCode: 200), error: nil)
         taskInterception.register(metrics: resourceMetrics)
@@ -145,7 +145,7 @@ class URLSessionRUMResourcesHandlerTests: XCTestCase {
         }
 
         // Given
-        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: .random(), appStateListener: AppStateListener.mockAny())
+        let taskInterception = TaskInterception(request: .mockAny(), isFirstParty: .random())
         let taskError = NSError(domain: "domain", code: 123, userInfo: [NSLocalizedDescriptionKey: "network error"])
         let resourceMetrics: ResourceMetrics = .mockAny()
         let resourceCompletion: ResourceCompletion = .mockWith(response: nil, error: taskError)
diff --git a/Tests/DatadogTests/Datadog/RUMMonitorTests.swift b/Tests/DatadogTests/Datadog/RUMMonitorTests.swift
@@ -1136,7 +1136,7 @@ class RUMMonitorTests: XCTestCase {
         XCTAssertTrue(Global.rum is DDNoopRUMMonitor)
 
         // Then
-        resourcesHandler.notify_taskInterceptionCompleted(interception: TaskInterception(request: .mockAny(), isFirstParty: .mockAny(), appStateListener: AppStateListener.mockAny()))
+        resourcesHandler.notify_taskInterceptionCompleted(interception: TaskInterception(request: .mockAny(), isFirstParty: .mockAny()))
         XCTAssertEqual(output.recordedLog?.status, .warn)
         XCTAssertEqual(
             output.recordedLog?.message,
diff --git a/Tests/DatadogTests/Datadog/TracerTests.swift b/Tests/DatadogTests/Datadog/TracerTests.swift
@@ -1100,7 +1100,7 @@ class TracerTests: XCTestCase {
         XCTAssertTrue(Global.sharedTracer is DDNoopTracer)
 
         // Then
-        tracingHandler.notify_taskInterceptionCompleted(interception: TaskInterception(request: .mockAny(), isFirstParty: true, appStateListener: AppStateListener.mockAny()))
+        tracingHandler.notify_taskInterceptionCompleted(interception: TaskInterception(request: .mockAny(), isFirstParty: true))
         XCTAssertEqual(output.recordedLog?.status, .warn)
         XCTAssertEqual(
             output.recordedLog?.message,
diff --git a/Tests/DatadogTests/Datadog/Tracing/Autoinstrumentation/URLSessionTracingHandlerTests.swift b/Tests/DatadogTests/Datadog/Tracing/Autoinstrumentation/URLSessionTracingHandlerTests.swift
@@ -7,10 +7,17 @@
 import XCTest
 @testable import Datadog
 
+private class MockAppStateListener: AppStateListening {
+    let history = AppStateHistory(
+        initialState: .init(isActive: true, date: .mockDecember15th2019At10AMUTC()),
+        finalDate: .mockDecember15th2019At10AMUTC() + 10
+    )
+}
+
 class URLSessionTracingHandlerTests: XCTestCase {
     private let spanOutput = SpanOutputMock()
     private let logOutput = LogOutputMock()
-    private let handler = URLSessionTracingHandler()
+    private let handler = URLSessionTracingHandler(appStateListener: MockAppStateListener())
 
     override func setUp() {
         Global.sharedTracer = Tracer.mockWith(
@@ -33,7 +40,7 @@ class URLSessionTracingHandlerTests: XCTestCase {
         spanOutput.onSpanRecorded = { _ in spanSentExpectation.fulfill() }
 
         // Given
-        let interception = TaskInterception(request: .mockAny(), isFirstParty: true, appStateListener: AppStateListener.mockAny())
+        let interception = TaskInterception(request: .mockAny(), isFirstParty: true)
         interception.register(completion: .mockAny())
         interception.register(
             metrics: .mockWith(
@@ -70,7 +77,7 @@ class URLSessionTracingHandlerTests: XCTestCase {
 
         // Given
         let request: URLRequest = .mockWith(httpMethod: "POST")
-        let interception = TaskInterception(request: request, isFirstParty: true, appStateListener: AppStateListener.mockAny())
+        let interception = TaskInterception(request: request, isFirstParty: true)
         interception.register(completion: .mockWith(response: .mockResponseWith(statusCode: 200), error: nil))
         interception.register(
             metrics: .mockWith(
@@ -108,7 +115,7 @@ class URLSessionTracingHandlerTests: XCTestCase {
         // Given
         let request: URLRequest = .mockWith(httpMethod: "GET")
         let error = NSError(domain: "domain", code: 123, userInfo: [NSLocalizedDescriptionKey: "network error"])
-        let interception = TaskInterception(request: request, isFirstParty: true, appStateListener: AppStateListener.mockAny())
+        let interception = TaskInterception(request: request, isFirstParty: true)
         interception.register(completion: .mockWith(response: nil, error: error))
         interception.register(
             metrics: .mockWith(
@@ -170,7 +177,7 @@ class URLSessionTracingHandlerTests: XCTestCase {
 
         // Given
         let request: URLRequest = .mockWith(httpMethod: "GET")
-        let interception = TaskInterception(request: request, isFirstParty: true, appStateListener: AppStateListener.mockAny())
+        let interception = TaskInterception(request: request, isFirstParty: true)
         interception.register(completion: .mockWith(response: .mockResponseWith(statusCode: 404), error: nil))
         interception.register(
             metrics: .mockWith(
@@ -233,7 +240,7 @@ class URLSessionTracingHandlerTests: XCTestCase {
         spanOutput.onSpanRecorded = { _ in spanNotSentExpectation.fulfill() }
 
         // Given
-        let incompleteInterception = TaskInterception(request: .mockAny(), isFirstParty: true, appStateListener: AppStateListener.mockAny())
+        let incompleteInterception = TaskInterception(request: .mockAny(), isFirstParty: true)
         // `incompleteInterception` has no metrics and no completion
 
         // When
@@ -250,7 +257,7 @@ class URLSessionTracingHandlerTests: XCTestCase {
         spanOutput.onSpanRecorded = { _ in spanNotSentExpectation.fulfill() }
 
         // Given
-        let interception = TaskInterception(request: .mockAny(), isFirstParty: false, appStateListener: AppStateListener.mockAny())
+        let interception = TaskInterception(request: .mockAny(), isFirstParty: false)
         interception.register(completion: .mockAny())
         interception.register(
             metrics: .mockWith(
@@ -271,19 +278,8 @@ class URLSessionTracingHandlerTests: XCTestCase {
     }
 
     func testGivenAnyInterception_itAddsAppStateInformationToSpan() throws {
-        class MockAppStateListener: AppStateListening {
-            let history = AppStateHistory(
-                initialState: .init(isActive: true, date: .mockDecember15th2019At10AMUTC()),
-                finalDate: .mockDecember15th2019At10AMUTC() + 10
-            )
-        }
-
         // Given
-        let interception = TaskInterception(
-            request: .mockAny(),
-            isFirstParty: true,
-            appStateListener: MockAppStateListener()
-        )
+        let interception = TaskInterception(request: .mockAny(), isFirstParty: true)
         interception.register(completion: .mockAny())
         interception.register(
             metrics: .mockWith(
@@ -299,7 +295,7 @@ class URLSessionTracingHandlerTests: XCTestCase {
 
         // Then
         let recordedSpan = try XCTUnwrap(spanOutput.recordedSpan)
-        XCTAssertEqual(recordedSpan.tags["foreground_duration"]?.encodable.value as? String, "10.0")
-        XCTAssertEqual(recordedSpan.tags["is_background"]?.encodable.value as? String, "false")
+        XCTAssertEqual(recordedSpan.tags["foreground_duration"]?.encodable.value as? UInt64, 10_000_000_000)
+        XCTAssertEqual(recordedSpan.tags["is_background"]?.encodable.value as? Bool, false)
     }
 }
diff --git a/Tests/DatadogTests/Datadog/URLSessionAutoInstrumentation/Interception/AppStateListenerTests.swift b/Tests/DatadogTests/Datadog/URLSessionAutoInstrumentation/Interception/AppStateListenerTests.swift
diff --git a/Tests/DatadogTests/Datadog/URLSessionAutoInstrumentation/Interception/TaskInterceptionTests.swift b/Tests/DatadogTests/Datadog/URLSessionAutoInstrumentation/Interception/TaskInterceptionTests.swift
diff --git a/Tests/DatadogTests/Datadog/URLSessionAutoInstrumentation/Interception/URLSessionInterceptorTests.swift b/Tests/DatadogTests/Datadog/URLSessionAutoInstrumentation/Interception/URLSessionInterceptorTests.swift

Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,13 @@`
`7`	`7`	`import Foundation`
`8`	`8`
`9`	`9`	`internal class URLSessionTracingHandler: URLSessionInterceptionHandler {`
	`10`	+ /// Listening to app state changes and use it to report `foreground_duration`
	`11`	`+ let appStateListener: AppStateListening`
	`12`	`+`
	`13`	`+ init(appStateListener: AppStateListening) {`
	`14`	`+ self.appStateListener = appStateListener`
	`15`	`+ }`
	`16`	`+`
`10`	`17`	`// MARK: - URLSessionInterceptionHandler`
`11`	`18`
`12`	`19`	`func notify_taskInterceptionStarted(interception: TaskInterception) {`
`@@ -68,12 +75,12 @@ internal class URLSessionTracingHandler: URLSessionInterceptionHandler {`
`68`	`75`	`}`
`69`	`76`	`}`
`70`	`77`	`}`
`71`		`- let appStateHistory = interception.appStateListener.history.take(`
	`78`	`+ let appStateHistory = appStateListener.history.take(`
`72`	`79`	`between: resourceMetrics.fetch.start...resourceMetrics.fetch.end`
`73`	`80`	`)`
`74`	`81`	`// TODO: RUMM-1064 tag doesn't seem like the best fit but can't find anything better`
`75`		`- span.setTag(key: "foreground_duration", value: "\(appStateHistory.foregroundDuration)")`
`76`		`- span.setTag(key: "is_background", value: "\(appStateHistory.didRunInBackground)")`
	`82`	`+ span.setTag(key: "foreground_duration", value: appStateHistory.foregroundDuration.toNanoseconds)`
	`83`	`+ span.setTag(key: "is_background", value: appStateHistory.didRunInBackground)`
`77`	`84`
`78`	`85`	`span.finish(at: resourceMetrics.fetch.end)`
`79`	`86`	`}`