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.

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

fix: pass through data when network request completes #1696

Merged
merged 3 commits into from
Mar 5, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion Datadog/Example/ExampleAppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,11 @@ class ExampleAppDelegate: UIResponder, UIApplicationDelegate {
RUM.enable(
with: RUM.Configuration(
applicationID: Environment.readRUMApplicationID(),
urlSessionTracking: .init(firstPartyHostsTracing: .trace(hosts: [], sampleRate: 100)),
urlSessionTracking: .init(
resourceAttributesProvider: { req, resp, data, err in
print("⭐️ [Attributes Provider] data: \(String(describing: data))")
return [:]
}),
trackBackgroundEvents: true,
customEndpoint: Environment.readCustomRUMURL(),
telemetrySampleRate: 100
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import XCTest
import TestUtilities
import DatadogInternal

@testable import DatadogRUM
@testable import DatadogTrace
@testable import DatadogCore

Expand Down Expand Up @@ -50,7 +51,7 @@ class NetworkInstrumentationIntegrationTests: XCTestCase {
core.flushAndTearDown()
core = nil
}

func testParentSpanPropagation() throws {
let expectation = expectation(description: "request completes")
// Given
Expand Down Expand Up @@ -88,4 +89,117 @@ class NetworkInstrumentationIntegrationTests: XCTestCase {

class MockDelegate: NSObject, URLSessionDataDelegate {
}

func testResourceAttributesProvider_givenURLSessionDataTaskRequest() {
core = DatadogCoreProxy(
context: .mockWith(
env: "test",
version: "1.1.1",
serverTimeOffset: 123
)
)

let providerExpectation = expectation(description: "provider called")
var providerDataCount = 0
RUM.enable(
with: .init(
applicationID: .mockAny(),
urlSessionTracking: .init(
resourceAttributesProvider: { req, resp, data, err in
XCTAssertNotNil(data)
XCTAssertTrue(data!.count > 0)
providerDataCount = data!.count
providerExpectation.fulfill()
return [:]
})
),
in: core
)

URLSessionInstrumentation.enable(
with: .init(
delegateClass: InstrumentedSessionDelegate.self
),
in: core
)

let session = URLSession(
configuration: .ephemeral,
delegate: InstrumentedSessionDelegate(),
delegateQueue: nil
)
var request = URLRequest(url: URL(string: "https://www.datadoghq.com/")!)
request.httpMethod = "GET"

let task = session.dataTask(with: request)
task.resume()

wait(for: [providerExpectation], timeout: 10)
XCTAssertTrue(providerDataCount > 0)
}

func testResourceAttributesProvider_givenURLSessionDataTaskRequestWithCompletionHandler() {
core = DatadogCoreProxy(
context: .mockWith(
env: "test",
version: "1.1.1",
serverTimeOffset: 123
)
)

let providerExpectation = expectation(description: "provider called")
var providerDataCount = 0
var providerData: Data?
RUM.enable(
with: .init(
applicationID: .mockAny(),
urlSessionTracking: .init(
resourceAttributesProvider: { req, resp, data, err in
XCTAssertNotNil(data)
XCTAssertTrue(data!.count > 0)
providerDataCount = data!.count
data.map { providerData = $0 }
providerExpectation.fulfill()
return [:]
})
),
in: core
)

URLSessionInstrumentation.enable(
with: .init(
delegateClass: InstrumentedSessionDelegate.self
),
in: core
)

let session = URLSession(
configuration: .ephemeral,
delegate: InstrumentedSessionDelegate(),
delegateQueue: nil
)
let request = URLRequest(url: URL(string: "https://www.datadoghq.com/")!)

let taskExpectation = self.expectation(description: "task completed")
var taskDataCount = 0
var taskData: Data?
let task = session.dataTask(with: request) { data, _, _ in
XCTAssertNotNil(data)
XCTAssertTrue(data!.count > 0)
taskDataCount = data!.count
data.map { taskData = $0 }
taskExpectation.fulfill()
}
task.resume()

wait(for: [providerExpectation, taskExpectation], timeout: 10)
XCTAssertEqual(providerDataCount, taskDataCount)
XCTAssertEqual(providerData, taskData)
}

class InstrumentedSessionDelegate: NSObject, URLSessionDataDelegate {
func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive data: Data) {
print(data)
ganeshnj marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -143,22 +143,22 @@ class StartingRUMSessionTests: XCTestCase {
let rumTime = DateProviderMock()
rumConfig.dateProvider = rumTime
rumConfig.trackBackgroundEvents = .mockRandom() // no matter BET state

// When
rumTime.now = sdkInitTime
RUM.enable(with: rumConfig, in: core)

rumTime.now = firstRUMTime
RUMMonitor.shared(in: core).startView(key: "key", name: "FirstView")

// Then
let session = try RUMSessionMatcher
.groupMatchersBySessions(try core.waitAndReturnRUMEventMatchers())
.takeSingle()

XCTAssertEqual(session.views.count, 1)
XCTAssertTrue(try session.has(sessionPrecondition: .backgroundLaunch), "Session must be marked as 'background launch'")

let firstView = try XCTUnwrap(session.views.first)
XCTAssertFalse(firstView.isApplicationLaunchView(), "Session should not begin with 'app launch' view")
XCTAssertEqual(firstView.name, "FirstView")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ internal final class NetworkInstrumentationFeature: DatadogFeature {
interceptDidFinishCollecting: { [weak self] session, task, metrics in
self?.task(task, didFinishCollecting: metrics)

if #available(iOS 15, tvOS 15, *) {
if #available(iOS 15, tvOS 15, *), !task.dd.hasCompletion {
// iOS 15 and above, didCompleteWithError is not called hence we use task state to detect task completion
// while prior to iOS 15, task state doesn't change to completed hence we use didCompleteWithError to detect task completion
self?.task(task, didCompleteWithError: task.error)
ganeshnj marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -113,6 +113,8 @@ internal final class NetworkInstrumentationFeature: DatadogFeature {
try swizzler.swizzle(
interceptCompletionHandler: { [weak self] task, _, error in
self?.task(task, didCompleteWithError: error)
}, didReceive: { [weak self] task, data in
self?.task(task, didReceive: data)
}
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ open class DatadogURLSessionDelegate: NSObject, URLSessionDataDelegate {
try swizzler.swizzle(
interceptCompletionHandler: { [weak self] task, _, error in
self?.interceptor?.task(task, didCompleteWithError: error)
}, didReceive: { _, _ in
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: didReceive param could be optional with default nil instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah but not worth the refactor.

}
)
} catch {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,13 @@ internal final class NetworkInstrumentationSwizzler {

/// Swizzles `URLSession.dataTask(with:completionHandler:)` methods (with `URL` and `URLRequest`).
func swizzle(
interceptCompletionHandler: @escaping (URLSessionTask, Data?, Error?) -> Void
interceptCompletionHandler: @escaping (URLSessionTask, Data?, Error?) -> Void,
didReceive: @escaping (URLSessionTask, Data) -> Void
) throws {
try urlSessionSwizzler.swizzle(interceptCompletionHandler: interceptCompletionHandler)
try urlSessionSwizzler.swizzle(
interceptCompletionHandler: interceptCompletionHandler,
didReceive: didReceive
)
}

/// Swizzles `URLSessionTask.resume()` method.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,26 @@ internal final class URLSessionSwizzler {

/// Swizzles `URLSession.dataTask(with:completionHandler:)` methods (with `URL` and `URLRequest`).
func swizzle(
interceptCompletionHandler: @escaping (URLSessionTask, Data?, Error?) -> Void
interceptCompletionHandler: @escaping (URLSessionTask, Data?, Error?) -> Void,
didReceive: @escaping (URLSessionTask, Data) -> Void
) throws {
lock.lock()
defer { lock.unlock() }
dataTaskURLRequestCompletionHandler = try DataTaskURLRequestCompletionHandler.build()
dataTaskURLRequestCompletionHandler?.swizzle(interceptCompletion: interceptCompletionHandler)
dataTaskURLRequestCompletionHandler?.swizzle(
interceptCompletion: interceptCompletionHandler,
didReceive: didReceive
)

if #available(iOS 13.0, *) {
// Prior to iOS 13.0 the `URLSession.dataTask(with:url, completionHandler:handler)` makes an internal
// call to `URLSession.dataTask(with:request, completionHandler:handler)`. To avoid duplicated call
// to the callback, we don't apply below swizzling prior to iOS 13.
dataTaskURLCompletionHandler = try DataTaskURLCompletionHandler.build()
dataTaskURLCompletionHandler?.swizzle(interceptCompletion: interceptCompletionHandler)
dataTaskURLCompletionHandler?.swizzle(
interceptCompletion: interceptCompletionHandler,
didReceive: didReceive
)
}
}

Expand Down Expand Up @@ -71,7 +78,8 @@ internal final class URLSessionSwizzler {
}

func swizzle(
interceptCompletion: @escaping (URLSessionTask, Data?, Error?) -> Void
interceptCompletion: @escaping (URLSessionTask, Data?, Error?) -> Void,
didReceive: @escaping (URLSessionTask, Data) -> Void
) {
typealias Signature = @convention(block) (URLSession, URLRequest, CompletionHandler?) -> URLSessionDataTask
swizzle(method) { previousImplementation -> Signature in
Expand All @@ -87,13 +95,18 @@ internal final class URLSessionSwizzler {

var _task: URLSessionDataTask?
let task = previousImplementation(session, Self.selector, request) { data, response, error in
completionHandler(data, response, error)
if let task = _task, let data = data {
didReceive(task, data)
}

if let task = _task { // sanity check, should always succeed
interceptCompletion(task, data, error)
}

completionHandler(data, response, error)
}
_task = task
_task?.dd.hasCompletion = true
return task
}
}
Expand Down Expand Up @@ -121,7 +134,8 @@ internal final class URLSessionSwizzler {
}

func swizzle(
interceptCompletion: @escaping (URLSessionTask, Data?, Error?) -> Void
interceptCompletion: @escaping (URLSessionTask, Data?, Error?) -> Void,
didReceive: @escaping (URLSessionTask, Data) -> Void
) {
typealias Signature = @convention(block) (URLSession, URL, CompletionHandler?) -> URLSessionDataTask
swizzle(method) { previousImplementation -> Signature in
Expand All @@ -137,13 +151,18 @@ internal final class URLSessionSwizzler {

var _task: URLSessionDataTask?
let task = previousImplementation(session, Self.selector, url) { data, response, error in
if let task = _task, let data = data {
didReceive(task, data)
}

completionHandler(data, response, error)

if let task = _task { // sanity check, should always succeed
interceptCompletion(task, data, error)
}
}
_task = task
_task?.dd.hasCompletion = true
return task
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,20 @@ extension DatadogExtension where ExtendedType: URLSessionTask {

return session.delegate
}

var hasCompletion: Bool {
get {
let value = objc_getAssociatedObject(type, &hasCompletionKey) as? Bool
return value == true
}
set {
if newValue {
objc_setAssociatedObject(type, &hasCompletionKey, true, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
ganeshnj marked this conversation as resolved.
Show resolved Hide resolved
} else {
objc_setAssociatedObject(type, &hasCompletionKey, nil, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
}
Comment on lines +43 to +47
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any benefit to clear it on false? Maybe that is enough:

Suggested change
if newValue {
objc_setAssociatedObject(type, &hasCompletionKey, true, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
} else {
objc_setAssociatedObject(type, &hasCompletionKey, nil, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
}
objc_setAssociatedObject(type, &hasCompletionKey, newValue, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)

not a big deal - just less code to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

more of personal preference, I don't think we would gain or lose either way.

}
}
}

private var hasCompletionKey: Void?
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import XCTest

class URLSessionSwizzlerTests: XCTestCase {
func testSwizzling_dataTaskWithCompletion() throws {
let didReceive = expectation(description: "didReceive")
didReceive.expectedFulfillmentCount = 2

let didInterceptCompletion = expectation(description: "interceptCompletion")
didInterceptCompletion.expectedFulfillmentCount = 2

Expand All @@ -18,6 +21,8 @@ class URLSessionSwizzlerTests: XCTestCase {
try swizzler.swizzle(
interceptCompletionHandler: { _, _, _ in
didInterceptCompletion.fulfill()
}, didReceive: { _, _ in
didReceive.fulfill()
}
)

Expand All @@ -30,6 +35,6 @@ class URLSessionSwizzlerTests: XCTestCase {
session.dataTask(with: url) { _, _, _ in }.resume() // not intercepted
session.dataTask(with: URLRequest(url: url)) { _, _, _ in }.resume() // not intercepted

wait(for: [didInterceptCompletion], timeout: 5)
wait(for: [didReceive, didInterceptCompletion], timeout: 5)
}
}
2 changes: 2 additions & 0 deletions DatadogRUM/Sources/RUMConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,8 @@ extension RUM {
/// attributes for RUM resource based on the provided request, response, data, and error.
/// Keep the implementation fast and do not make any assumptions on the thread used to run it.
///
/// Note: This is not supported for async-await APIs.
///
Comment on lines +224 to +225
Copy link
Collaborator

Choose a reason for hiding this comment

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

question/ Meaning that we don't fully solve #1680 as it was originally reported for async/await APIs. We may need to update the PR description or add more details to the issue, otherwise it will be closed automatically with the merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure, this is regression fix. There is no path forward I could see which can solve this issue for async-await APIs like I mentioned in the description of the PR.

Yes, we are not going to close the issue but update it with what we did.

/// Default: `nil`.
public var resourceAttributesProvider: RUM.ResourceAttributesProvider?

Expand Down