RUM-2355 fix: Network Instrumentation #1588

maxep · 2023-12-13T14:19:15Z

Problem Statement

Task Interception

After first configuration, our new Network Instrumentation logic introduced in #1394 is intercepting all URLSessionTask regardless of their delegate.

Starting an interception can result in Start Resource in RUM. When the custom delegate is not in play, the interception will never finish and so the RUM Resource will never stop. This means that the resource event won't be transmitted, but it will keep the Resource scopes active and therefore the view scopes open.

Task Completion

Tasks created with completion handler on pre-iOS 15 are never completed: We apply #available(iOS 15, tvOS 15, *) condition for completing the interception when receiving metrics, but we have no condition for pre-iOS 15.

Swizzling Unbinding

Swizzlings on URLSession* are not removed when deallocating the NetworkInstrumentationFeature, this is preventing from re-enabling the instrumentation on a new core instance. It is blocking the 'stop core instance' feature on iOS.

Legacy `DatadogURLSessionDelegate`

Changes in the legacy delegate are preventing from initialising multiple instances with different first-party hosts: The DatadogURLSessionDelegate.init is doing a static URLSessionInstrumentation.enable which can be done only once for a given delegate type.

Solutions

Instance-based Swizzler

In #1564, we have introduced reliable unswizzling logic which allows to create multiple instances of swizzler for the same method. I'm leveraging this to associate swizzlers to an instance of NetworkInstrumentationFeature, we can then unswizzle safely at deallocation.

The instance-based swizzling also allow to fix the legacy DatadogURLSessionDelegate by keeping a swizzler per delegate instance.

Check for Task's delegate type

We now check for the delegate type before starting an interception,either when using the new URLSessionInstrumentation or the legacy DatadogURLSessionDelegate.

Task Completion

The URLSessionSwizzler now allow to swizzle the URLSession.dataTask(with:completionHandler:) methods on both URL and URLRequest and swizzling is applied for all OS versions.

Review

I suggest to review the following files first:

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference
Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

Run unit tests for Core, RUM, Trace, Logs, CR and WVT
Run unit tests for Session Replay
Run integration tests
Run smoke tests
Run tests for tools/

datadog-datadog-prod-us1 · 2023-12-13T14:59:48Z

Datadog Report

Branch report: maxep/RUM-2355/fix-network-instrumentation
Commit report: 8174c17

❄️ dd-sdk-ios: 0 Failed, 1 New Flaky, 2744 Passed, 0 Skipped, 31m 37.13s Wall Time

New Flaky Tests (1)

testGivenURLSessionWithCustomDelegate_whenUsingAsyncData_itPassesAllValuesToTheInterceptor - NetworkInstrumentationFeatureTests - Last Failure

Expand for error

 
 Assertion Failure: Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "Notify intercepion did complete".
 Assertion Failure at NetworkInstrumentationFeatureTests.swift:358: XCTAssertEqual failed: ("nil") is not equal to ("Optional("some error")")

maxep · 2023-12-14T12:04:34Z

DatadogInternal/Sources/Concurrency/ReadWriteLock.swift

-        set {
-            pthread_rwlock_wrlock(&rwlock)
-            value = newValue
-            pthread_rwlock_unlock(&rwlock)
-        }
+        set { mutate { $0 = newValue } }
    }

    /// Provides a non-escaping closure for mutation.
    /// The lock will be acquired once for writing before invoking the closure.
    ///
    /// - Parameter closure: The closure with the mutable value.
-    public func mutate(_ closure: (inout Value) -> Void) {
+    public func mutate(_ closure: (inout Value) throws -> Void) rethrows {
        pthread_rwlock_wrlock(&rwlock)
-        closure(&value)
-        pthread_rwlock_unlock(&rwlock)
+        defer { pthread_rwlock_unlock(&rwlock) }
+        try closure(&value)


Not related, but better :)

maxep · 2023-12-14T12:11:43Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/DatadogURLSessionDelegate.swift

+    /// Datadog delegate object.
+    /// 
+    /// The class implementing `DDURLSessionDelegateProviding` must ensure that following method calls are forwarded to `ddURLSessionDelegate`:
+    /// - `func urlSession(_:task:didFinishCollecting:)`
+    /// - `func urlSession(_:task:didCompleteWithError:)`
+    /// - `func urlSession(_:dataTask:didReceive:)`
+    var ddURLSessionDelegate: DatadogURLSessionDelegate { get }


We have to re-introduce this constrain to restore legacy behaviour.

which particular behavior?

This one:

Legacy DatadogURLSessionDelegate

Changes in the legacy delegate are preventing from initialising multiple instances with different first-party hosts:
The DatadogURLSessionDelegate.init is doing a static URLSessionInstrumentation.enable which can be done only once for a given delegate type.

The DatadogURLSessionDelegate class cannot be instrumented like a custom delegate type since it defines first-party hosts per instance. It will now define it's own NetworkInstrumentationSwizzler instance to intercept task events, therefor the deprecated __URLSessionDelegateProviding still needs to forward data to the ddURLSessionDelegate.

maxep · 2023-12-14T12:17:25Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/DatadogURLSessionDelegate.swift

@@ -28,7 +35,7 @@ open class DatadogURLSessionDelegate: NSObject, URLSessionDataDelegate {
        return URLSessionInterceptor.shared(in: core)
    }

-    /* private */ public let firstPartyHosts: FirstPartyHosts
+    let swizzler = URLSessionSwizzler()


Instances of DatadogURLSessionDelegate hold their own swizzling instance to instrument the task on resume.

ganeshnj · 2023-12-20T08:49:54Z

DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift

@@ -38,6 +38,9 @@ internal final class NetworkInstrumentationFeature: DatadogFeature {
    @ReadWriteLock
    internal var handlers: [DatadogURLSessionHandler] = []

+    @ReadWriteLock
+    internal var swizzlers: [ObjectIdentifier: URLSessionSwizzler] = [:]


Neat, never knew such type existed.

ganeshnj · 2023-12-20T08:55:55Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionSwizzler.swift

-        }
-    }
+/// Swizzles `URLSession*` methods.
+internal final class URLSessionSwizzler {


/Blocking

Combining all the swizzling code inside one type was never the intent but I understand the reasoning behind. However, this can be done with the existing approach by making the each swizzler type as intance type.

This makes the testing each swizzling class easy, better code navigation. You can compose those types in a NetworkInstrumentationSwizzler type.

something like

class NetworkInstrumentationSwizzler { let urlSessionSwizzler: URLSessionSwizzler let urlSessionTaskDelegateSwizzler: URLSessionTaskDelegateSwizzler .... }

As suggested, I have split the swizzler class, see the last commit. But TBH, I don't see the benefit 🤔 It just adds another layer and doesn't help much with code navigation.

ncreated

Massive efforts 👌 - I noticed only one blocking change, other look ok. It is really hard to see it well through (the domain is complex, code looks fine), hence I count on the quality of tests. Glad we add more integration scenarios.

Last question, about:

Task Interception
After first configuration, our new Network Instrumentation logic introduced in #1394 is intercepting all URLSessionTask regardless of their delegate.

Is there a test we add to verify that not instrumented requests are ignored?

ncreated · 2023-12-21T09:43:27Z

DatadogCore/Tests/TestsObserver/DatadogTestsObserver.swift

blocking/ The new logic introduces new global swizzlings that need to be covered in integrity checks. Here we remove old checks instead of updating them ⛔. Balancing following calls must be enforced by DatadogTestsObserver, otherwise we will run into flakiness ❄️:

urlSessionSwizzler.swizzle() urlSessionTaskSwizzler.swizzle() urlSessionTaskDelegateSwizzler.swizzle() urlSessionSwizzler.unswizzle() urlSessionTaskSwizzler.unswizzle() urlSessionTaskDelegateSwizzler.unswizzle()

Previously, it was actually covered twice. Swizzling integrity is already checked by this which will fail if any swizzling is left, printing the method signatures 👍

Nice, can you add some simple test case?

swizzling once, Swizzling.methods.count == 1

same method again, Swizzling.methods.count == 2

another method, Swizzling.methods.count == 3

I have added this to MethodSwizzlerTests 👍

ncreated · 2023-12-21T09:54:58Z

DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift

@@ -38,6 +38,9 @@ internal final class NetworkInstrumentationFeature: DatadogFeature {
    @ReadWriteLock
    internal var handlers: [DatadogURLSessionHandler] = []

+    @ReadWriteLock
+    internal var swizzlers: [ObjectIdentifier: NetworkInstrumentationSwizzler] = [:]


suggestion/ We should either mark it private or make useful in troubleshooting. The problem with its usability is that ObjectIdentifier completely erases the identity of underlying (identified) meta type. We can preserve it with properly abstracting the ObjectIdentifier:

internal struct SwizzledDelegateClass: Hashable, Equatable { internal let delegateClass: URLSessionDataDelegate.Type private let identifier: ObjectIdentifier init(delegateClass: URLSessionDataDelegate.Type) { self.delegateClass = delegateClass self.identifier = ObjectIdentifier(delegateClass) } func hash(into hasher: inout Hasher) { hasher.combine(identifier) } static func == (lhs: SwizzledDelegateClass, rhs: SwizzledDelegateClass) -> Bool { lhs.identifier == rhs.identifier } }

I'm making this private and adding debugDescription to MethodSwizzler that should gives us enough information to troubleshoot.

ncreated · 2023-12-21T10:05:23Z

DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift

+                if let currentRequest = task.currentRequest {
+                    let request = self.intercept(request: currentRequest, additionalFirstPartyHosts: configuredFirstPartyHosts)
+                    task.dd.override(currentRequest: request)
                }


question/ This condition is quite puzzling. In what circumstances we expect task to not have currentRequest? Let's explain it in the code comment if appropriate.

That's a good question, I don't know what could nillify the originalRequest or currentRequest 🤔 the doc doesn't say..

DatadogInternal/Sources/NetworkInstrumentation/URLSession/DatadogURLSessionDelegate.swift

IntegrationTests/Runner/Environment.swift

ncreated · 2023-12-21T10:31:16Z

IntegrationTests/Runner/Environment.swift

+        /// Use a custom delegate.
+        /// and define `firstPartyHosts` in feature configuration.
+        case customWithFeatureFirstPartyHosts
+        /// Use a custom delegate.
+        /// and define `firstPartyHosts` with delegate's configuration.
+        case customWithAdditionalFirstyPartyHosts


wip

datadog-datadog-prod-us1 · 2024-01-04T09:28:49Z

Datadog Report

Branch report: maxep/RUM-2355/fix-network-instrumentation
Commit report: 9e46d77

✅ dd-sdk-ios: 0 Failed, 0 New Flaky, 2774 Passed, 0 Skipped, 28m 17.47s Wall Time

maxep · 2024-01-04T09:49:09Z

It is ready for another round of review :)

@ncreated, to your question:

Last question, about:

Task Interception
After first configuration, our new Network Instrumentation logic introduced in #1394 is intercepting all URLSessionTask regardless of their delegate.

Is there a test we add to verify that not instrumented requests are ignored?

Yes, the NetworkInstrumentationFeatureTests now tests not instrumented requests: here, here, and here. It covers async/await, session delegate, and task delegate.

ganeshnj · 2024-01-04T10:43:57Z

DatadogCore/Tests/TestsObserver/DatadogTestsObserver.swift

@@ -132,34 +132,6 @@ internal class DatadogTestsObserver: NSObject, XCTestObservation {

            If all above conditions are met, this failure might indicate a memory leak in the implementation.
            """
-        ),


Removing these checks seems like a back step - in case of a memory leak we will not able to catch any lingering swizzlings.

A check something like if there is an instance of swizzler in memory or not will be helpful.

something like

func checkInstancesOfType(_ type: AnyClass) -> Bool { let typeString = String(describing: type) for bundle in Bundle.allBundles { if let className = bundle.classNamed(typeString) { // Check if an instance of the class exists if class_getInstanceSize(className) > 0 { return true } } } return false }

It is already there, see my comment. These checks were redundant, we are already asserting that no swizzling is left here. Failure will print the method signature, so we will know which sizzling is at fault.

ganeshnj · 2024-01-04T10:45:18Z

DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift

+                guard let self = self, task.dd.delegate?.isKind(of: configuration.delegateClass) == true else {
+                    return
                }


How is the dd.delegate lifecycle is managed?

It's a computed var and not retained. see its implementation here

ganeshnj · 2024-01-04T10:46:35Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/DatadogURLSessionDelegate.swift

+    /// Datadog delegate object.
+    /// 
+    /// The class implementing `DDURLSessionDelegateProviding` must ensure that following method calls are forwarded to `ddURLSessionDelegate`:
+    /// - `func urlSession(_:task:didFinishCollecting:)`
+    /// - `func urlSession(_:task:didCompleteWithError:)`
+    /// - `func urlSession(_:dataTask:didReceive:)`
+    var ddURLSessionDelegate: DatadogURLSessionDelegate { get }


which particular behavior?

DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionDataDelegateSwizzler.swift

DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTaskDelegateSwizzler.swift

ncreated

Looks good 👍. Seeing a lot of changes that involve OS-version-specific swizzlings, can we verify if unit tests pass all major iOS versions from 12 to 16 🙏? Currently we can't rely on nightly tests for this.

maxep · 2024-01-05T17:38:11Z

I have ran the test-suites on the following versions:

OS Version	Unit Tests	Integration Tests
iOS 12.4	✅	🚫	Unable to test due to SPM failure
iOS 13.7	❌	🚫	units fails on `master` as well
iOS 14.4	✅	✅
iOS 15.5	✅	✅
iOS 16.0	✅	✅

It fails on iOS 13, from what I see it's due to the URLSessionTask.resume which doesnt seem to trigger the swizzled method. It fails the same on master so it is not due to the change in this PR nor the change in swizzling.
As this additional issue is not introduce by changes in this PR, we can tackle this seperatly.

maxep self-assigned this Dec 13, 2023

maxep mentioned this pull request Dec 13, 2023

RUM-1040 Fix URLSessionTask interception #1566

Closed

8 tasks

maxep force-pushed the maxep/RUM-2355/fix-network-instrumentation branch from 8d1c890 to 894bf48 Compare December 14, 2023 09:31

maxep changed the title ~~RUM-2355 Fix Network Instrumentation~~ RUM-2355 fix: Network Instrumentation Dec 14, 2023

maxep force-pushed the maxep/RUM-2355/fix-network-instrumentation branch from f99a874 to 86001c6 Compare December 14, 2023 11:08

maxep marked this pull request as ready for review December 14, 2023 11:10

maxep requested review from a team as code owners December 14, 2023 11:10

maxep commented Dec 14, 2023

View reviewed changes

maxep marked this pull request as draft December 14, 2023 17:04

maxep force-pushed the maxep/RUM-2355/fix-network-instrumentation branch from e9228cc to b2bd78a Compare December 18, 2023 09:48

maxep marked this pull request as ready for review December 18, 2023 09:51

ganeshnj requested changes Dec 20, 2023

View reviewed changes

maxep requested a review from ganeshnj December 20, 2023 18:25

ncreated requested changes Dec 21, 2023

View reviewed changes

maxep force-pushed the maxep/RUM-2355/fix-network-instrumentation branch from d276853 to 3e92e18 Compare December 22, 2023 11:49

maxep added 13 commits January 4, 2024 09:56

RUM-1040 Unbind network instrumentation

3fd3c25

wip

Fix deprecated DatadogURLSessionDelegate

1143e1c

Delete MetaTypeExtensions

6f90704

Fix network instrumentation tests

7168a9c

Fix integration tests

7948805

Add warning on multiple delegate configuration

7854ae2

Make URLSessionSwizzler thread-safe

1af3f9a

Replace swizzling for completion by kvo

ec07865

Fix rebase conflicts

00bf6cc

Inject trace header to instrumented tasks

9c6ab65

restore swizzling

519646f

Add support for dataTask with URL and completion handler

ef7a846

Split swizzlers

b8a1eb5

maxep force-pushed the maxep/RUM-2355/fix-network-instrumentation branch from 3e92e18 to 4bfa7c4 Compare January 4, 2024 09:21

maxep requested a review from ncreated January 4, 2024 09:22

maxep added 2 commits January 4, 2024 10:35

apply cr suggestions

5b43ff1

Rename instrumentation methods in integration tests

738d599

maxep force-pushed the maxep/RUM-2355/fix-network-instrumentation branch from 4bfa7c4 to 738d599 Compare January 4, 2024 09:35

ganeshnj requested changes Jan 4, 2024

View reviewed changes

Split URLSessionTaskDelegateSwizzler and URLSessionDataDelegateSwizzler

9e46d77

maxep requested a review from ganeshnj January 4, 2024 13:30

Improve unswizzle integrity check

a45c90e

ncreated approved these changes Jan 5, 2024

View reviewed changes

ganeshnj approved these changes Jan 5, 2024

View reviewed changes

maxep merged commit d5d9967 into develop Jan 8, 2024
11 checks passed

maxep deleted the maxep/RUM-2355/fix-network-instrumentation branch January 8, 2024 09:29

saad352 mentioned this pull request Sep 12, 2024

application_start event is not sent for Datadog sdk version >= 2.6.0 #2041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUM-2355 fix: Network Instrumentation #1588

RUM-2355 fix: Network Instrumentation #1588

maxep commented Dec 13, 2023 •

edited

Loading

datadog-datadog-prod-us1 bot commented Dec 13, 2023 •

edited

Loading

maxep Dec 14, 2023

maxep Dec 14, 2023

ganeshnj Jan 4, 2024

maxep Jan 4, 2024

maxep Dec 14, 2023

ganeshnj Dec 20, 2023

ganeshnj Dec 20, 2023

maxep Dec 20, 2023

ncreated left a comment

ncreated Dec 21, 2023

maxep Dec 21, 2023

ganeshnj Jan 4, 2024

maxep Jan 4, 2024

ncreated Dec 21, 2023

maxep Dec 21, 2023

ncreated Dec 21, 2023

maxep Dec 21, 2023

ncreated Dec 21, 2023

datadog-datadog-prod-us1 bot commented Jan 4, 2024 •

edited

Loading

maxep commented Jan 4, 2024

ganeshnj Jan 4, 2024

maxep Jan 4, 2024

ganeshnj Jan 4, 2024

maxep Jan 4, 2024

ganeshnj Jan 4, 2024

ncreated left a comment

maxep commented Jan 5, 2024

RUM-2355 fix: Network Instrumentation #1588

RUM-2355 fix: Network Instrumentation #1588

Conversation

maxep commented Dec 13, 2023 • edited Loading

Problem Statement

Task Interception

Task Completion

Swizzling Unbinding

Legacy DatadogURLSessionDelegate

Solutions

Instance-based Swizzler

Check for Task's delegate type

Task Completion

Review

Review checklist

Custom CI job configuration (optional)

datadog-datadog-prod-us1 bot commented Dec 13, 2023 • edited Loading

Datadog Report

New Flaky Tests (1)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Legacy DatadogURLSessionDelegate

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-datadog-prod-us1 bot commented Jan 4, 2024 • edited Loading

Datadog Report

maxep commented Jan 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

maxep commented Jan 5, 2024

maxep commented Dec 13, 2023 •

edited

Loading

Legacy `DatadogURLSessionDelegate`

datadog-datadog-prod-us1 bot commented Dec 13, 2023 •

edited

Loading

Legacy `DatadogURLSessionDelegate`

datadog-datadog-prod-us1 bot commented Jan 4, 2024 •

edited

Loading