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.

Sign up for GitHub

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

RUM-2355 fix: Network Instrumentation #1588

Merged
merged 17 commits into from
Jan 8, 2024
Merged

RUM-2355 fix: Network Instrumentation #1588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3fd3c25
RUM-1040 Unbind network instrumentation
maxep Nov 15, 2023
1143e1c
Fix deprecated DatadogURLSessionDelegate
maxep Nov 17, 2023
6f90704
Delete MetaTypeExtensions
maxep Nov 17, 2023
7168a9c
Fix network instrumentation tests
maxep Nov 21, 2023
7948805
Fix integration tests
maxep Nov 22, 2023
7854ae2
Add warning on multiple delegate configuration
maxep Nov 23, 2023
1af3f9a
Make URLSessionSwizzler thread-safe
maxep Nov 23, 2023
ec07865
Replace swizzling for completion by kvo
maxep Dec 13, 2023
00bf6cc
Fix rebase conflicts
maxep Dec 13, 2023
9c6ab65
Inject trace header to instrumented tasks
maxep Dec 14, 2023
519646f
restore swizzling
maxep Dec 15, 2023
ef7a846
Add support for dataTask with URL and completion handler
maxep Dec 18, 2023
b8a1eb5
Split swizzlers
maxep Dec 20, 2023
5b43ff1
apply cr suggestions
maxep Dec 21, 2023
738d599
Rename instrumentation methods in integration tests
maxep Jan 4, 2024
9e46d77
Split URLSessionTaskDelegateSwizzler and URLSessionDataDelegateSwizzler
maxep Jan 4, 2024
a45c90e
Improve unswizzle integrity check
maxep Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 55 additions & 67 deletions Datadog/Datadog.xcodeproj/project.pbxproj
View file
Open in desktop

Large diffs are not rendered by default.

30 changes: 1 addition & 29 deletions DatadogCore/Tests/TestsObserver/DatadogTestsObserver.swift
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

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()

Copy link
Member Author

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added this to MethodSwizzlerTests 👍

Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ internal class DatadogTestsObserver: NSObject, XCTestObservation {
Make sure all applied swizzling are reset by the end of test with `unswizzle()`.

`DatadogTestsObserver` found \(Swizzling.methods.count) leaked swizzlings:
\(Swizzling.methods)
\(Swizzling.description)
"""
),
.init(
Expand Down Expand Up @@ -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.
"""
),
Copy link
Contributor

Choose a reason for hiding this comment

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

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
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

.init(
assert: { URLSessionTaskDelegateSwizzler.isBinded == false },
problem: "No URLSessionTaskDelegate swizzling must be applied.",
solution: """
Make sure all the binded delegates are unbinded by the end of test with `URLSessionTaskDelegateSwizzler.unbind(delegate:)`.
"""
),
.init(
assert: { URLSessionDataDelegateSwizzler.isBinded == false },
problem: "No URLSessionDataDelegate swizzling must be applied.",
solution: """
Make sure all the binded delegates are unbinded by the end of test with `URLSessionDataDelegateSwizzler.unbind(delegate:)`.
"""
),
.init(
assert: { URLSessionTaskSwizzler.isBinded == false },
problem: "No URLSessionTask swizzling must be applied.",
solution: """
Make sure all the binded delegates are unbinded by the end of test with `URLSessionTaskSwizzler.unbind()`.
"""
),
.init(
assert: { URLSessionSwizzler.isBinded == false },
problem: "No URLSession swizzling must be applied.",
solution: """
Make sure all the binded delegates are unbinded by the end of test with `URLSessionSwizzler.unbind()`.
"""
)
]

Expand Down
12 changes: 4 additions & 8 deletions DatadogInternal/Sources/Concurrency/ReadWriteLock.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,16 @@ public final class ReadWriteLock<Value> {
defer { pthread_rwlock_unlock(&rwlock) }
return value
}
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)
Comment on lines -42 to +52
Copy link
Member Author

Choose a reason for hiding this comment

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

Not related, but better :)

}
}
221 changes: 95 additions & 126 deletions DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ internal final class NetworkInstrumentationFeature: DatadogFeature {
@ReadWriteLock
internal var handlers: [DatadogURLSessionHandler] = []

@ReadWriteLock
private var swizzlers: [ObjectIdentifier: NetworkInstrumentationSwizzler] = [:]

/// Maps `URLSessionTask` to its `TaskInterception` object.
///
/// The interceptions **must** be accessed using the `queue`.
Expand All @@ -49,80 +52,76 @@ internal final class NetworkInstrumentationFeature: DatadogFeature {
/// - Parameter configuration: The configuration to use for swizzling.
/// Note: We are only concerned with type of the delegate here but to provide compile time safety, we
/// use the instance of the delegate to get the type.
internal func bindIfNeeded(configuration: URLSessionInstrumentation.Configuration) throws {
internal func bind(configuration: URLSessionInstrumentation.Configuration) throws {
let configuredFirstPartyHosts = FirstPartyHosts(firstPartyHosts: configuration.firstPartyHostsTracing) ?? .init()

try URLSessionTaskDelegateSwizzler.bindIfNeeded(
let identifier = ObjectIdentifier(configuration.delegateClass)

if let swizzler = swizzlers[identifier] {
DD.logger.warn(
"""
The delegate class \(configuration.delegateClass) is already instrumented.
The previous instrumentation will be disabled in favor of the new one.
"""
)

swizzler.unswizzle()
}

let swizzler = NetworkInstrumentationSwizzler()
swizzlers[identifier] = swizzler

try swizzler.swizzle(
interceptResume: { [weak self] task in
// intercept task if delegate match
guard let self = self, task.dd.delegate?.isKind(of: configuration.delegateClass) == true else {
return
}

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

self.intercept(task: task, additionalFirstPartyHosts: configuredFirstPartyHosts)
}
)

try swizzler.swizzle(
delegateClass: configuration.delegateClass,
interceptDidFinishCollecting: { [weak self] session, task, metrics in
self?.queue.async { [weak self, weak session] in
self?._task(task, didFinishCollecting: metrics)
session?.delegate?.interceptor?.task(task, didFinishCollecting: metrics)
self?.task(task, didFinishCollecting: metrics)

// iOS 16 and above, didCompleteWithError is not called hence we use task state to detect task completion
if #available(iOS 15, tvOS 15, *) {
// 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
if #available(iOS 15, tvOS 15, *) {
self?._task(task, didCompleteWithError: task.error)
session?.delegate?.interceptor?.task(task, didCompleteWithError: task.error)
}
}
}, interceptDidCompleteWithError: { [weak self] session, task, error in
self?.queue.async { [weak self, weak session] in
// 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)
session?.delegate?.interceptor?.task(task, didCompleteWithError: task.error)
self?.task(task, didCompleteWithError: task.error)
}
},
interceptDidCompleteWithError: { [weak self] session, task, error in
self?.task(task, didCompleteWithError: error)
}
)

try URLSessionDataDelegateSwizzler.bindIfNeeded(delegateClass: configuration.delegateClass, interceptDidReceive: { [weak self] session, task, data in
// sync update to task prevents a race condition where the currentRequest could already be sent to the transport
self?.queue.sync { [weak self, weak session] in
self?._task(task, didReceive: data)
session?.delegate?.interceptor?.task(task, didReceive: data)
try swizzler.swizzle(
delegateClass: configuration.delegateClass,
interceptDidReceive: { [weak self] session, task, data in
self?.task(task, didReceive: data)
}
})

if #available(iOS 13, tvOS 13, *) {
try URLSessionTaskSwizzler.bindIfNeeded(interceptResume: { [weak self] task in
self?.queue.sync { [weak self] in
let additionalFirstPartyHosts = configuredFirstPartyHosts + task.firstPartyHosts
self?._intercept(task: task, additionalFirstPartyHosts: additionalFirstPartyHosts)
}
})
} else {
try URLSessionSwizzler.bindIfNeeded(interceptURLRequest: { request in
return self.intercept(request: request, additionalFirstPartyHosts: configuredFirstPartyHosts)
}, interceptTask: { [weak self] task in
self?.queue.async { [weak self] in
let additionalFirstPartyHosts = configuredFirstPartyHosts + task.firstPartyHosts
self?._intercept(task: task, additionalFirstPartyHosts: additionalFirstPartyHosts)
}
})
}
}
)

internal func unbindAll() {
URLSessionTaskDelegateSwizzler.unbindAll()
URLSessionDataDelegateSwizzler.unbindAll()
URLSessionTaskSwizzler.unbind()
URLSessionSwizzler.unbind()
try swizzler.swizzle(
interceptCompletionHandler: { [weak self] task, _, error in
self?.task(task, didCompleteWithError: error)
}
)
}

/// Unswizzles `URLSessionTaskDelegate`, `URLSessionDataDelegate`, `URLSessionTask` and `URLSession` methods
/// - Parameter delegateClass: The delegate class to unswizzle.
internal func unbind(delegateClass: URLSessionDataDelegate.Type) {
URLSessionTaskDelegateSwizzler.unbind(delegateClass: delegateClass)
URLSessionDataDelegateSwizzler.unbind(delegateClass: delegateClass)

guard URLSessionTaskDelegateSwizzler.didFinishCollectingMap.isEmpty,
URLSessionDataDelegateSwizzler.didReceiveMap.isEmpty else {
return
}

URLSessionTaskSwizzler.unbind()
URLSessionSwizzler.unbind()
let identifier = ObjectIdentifier(delegateClass)
swizzlers.removeValue(forKey: identifier)
}
}

Expand Down Expand Up @@ -152,47 +151,32 @@ extension NetworkInstrumentationFeature {
/// - task: The created task.
/// - additionalFirstPartyHosts: Extra hosts to consider in the interception.
func intercept(task: URLSessionTask, additionalFirstPartyHosts: FirstPartyHosts?) {
// sync update to task prevents a race condition where the currentRequest could already be sent to the transport
queue.sync { [weak self] in
self?._intercept(task: task, additionalFirstPartyHosts: additionalFirstPartyHosts)
}
}

private func _intercept(task: URLSessionTask, additionalFirstPartyHosts: FirstPartyHosts?) {
guard let request = task.currentRequest ?? task.originalRequest else {
return
}
queue.async { [weak self] in
guard let self = self, let request = task.currentRequest else {
return
}

var interceptedRequest: URLRequest
/// task.setValue is not available on iOS 12, hence for iOS 12 we modify the request by swizzling URLSession methods
if #available(iOS 13, tvOS 13, *) {
let request = self.intercept(request: request, additionalFirstPartyHosts: additionalFirstPartyHosts)
interceptedRequest = request
task.setValue(interceptedRequest, forKey: "currentRequest")
} else {
interceptedRequest = request
}
let firstPartyHosts = self.firstPartyHosts(with: additionalFirstPartyHosts)

let firstPartyHosts = self.firstPartyHosts(with: additionalFirstPartyHosts)
let interception = self.interceptions[task] ??
URLSessionTaskInterception(
request: request,
isFirstParty: firstPartyHosts.isFirstParty(url: request.url)
)

let interception = self.interceptions[task] ??
URLSessionTaskInterception(
request: interceptedRequest,
isFirstParty: firstPartyHosts.isFirstParty(url: interceptedRequest.url)
)
interception.register(request: request)

interception.register(request: interceptedRequest)
if let trace = self.extractTrace(firstPartyHosts: firstPartyHosts, request: request) {
interception.register(traceID: trace.traceID, spanID: trace.spanID, parentSpanID: trace.parentSpanID)
}

if let trace = self.extractTrace(firstPartyHosts: firstPartyHosts, request: interceptedRequest) {
interception.register(traceID: trace.traceID, spanID: trace.spanID, parentSpanID: trace.parentSpanID)
}
if let origin = request.value(forHTTPHeaderField: TracingHTTPHeaders.originField) {
interception.register(origin: origin)
}

if let origin = interceptedRequest.value(forHTTPHeaderField: TracingHTTPHeaders.originField) {
interception.register(origin: origin)
self.interceptions[task] = interception
self.handlers.forEach { $0.interceptionDidStart(interception: interception) }
}

self.interceptions[task] = interception
self.handlers.forEach { $0.interceptionDidStart(interception: interception) }
}

/// Tells the interceptors that metrics were collected for the given task.
Expand All @@ -202,21 +186,17 @@ extension NetworkInstrumentationFeature {
/// - metrics: The collected metrics.
func task(_ task: URLSessionTask, didFinishCollecting metrics: URLSessionTaskMetrics) {
queue.async { [weak self] in
self?._task(task, didFinishCollecting: metrics)
}
}

private func _task(_ task: URLSessionTask, didFinishCollecting metrics: URLSessionTaskMetrics) {
guard let interception = self.interceptions[task] else {
return
}
guard let self = self, let interception = self.interceptions[task] else {
return
}

interception.register(
metrics: ResourceMetrics(taskMetrics: metrics)
)
interception.register(
metrics: ResourceMetrics(taskMetrics: metrics)
)

if interception.isDone {
self.finish(task: task, interception: interception)
if interception.isDone {
self.finish(task: task, interception: interception)
}
}
}

Expand All @@ -227,40 +207,29 @@ extension NetworkInstrumentationFeature {
/// - data: A data object containing the transferred data.
func task(_ task: URLSessionTask, didReceive data: Data) {
queue.async { [weak self] in
self?._task(task, didReceive: data)
self?.interceptions[task]?.register(nextData: data)
}
}

private func _task(_ task: URLSessionTask, didReceive data: Data) {
guard let interception = self.interceptions[task] else {
return
}
interception.register(nextData: data)
}

/// Tells the interceptors that the task did complete.
///
/// - Parameters:
/// - task: The task that has finished transferring data.
/// - error: If an error occurred, an error object indicating how the transfer failed, otherwise NULL.
func task(_ task: URLSessionTask, didCompleteWithError error: Error?) {
queue.async { [weak self] in
self?._task(task, didCompleteWithError: error)
}
}

private func _task(_ task: URLSessionTask, didCompleteWithError error: Error?) {
guard let interception = self.interceptions[task] else {
return
}
guard let self = self, let interception = self.interceptions[task] else {
return
}

interception.register(
response: task.response,
error: error
)
interception.register(
response: task.response,
error: error
)

if interception.isDone {
self.finish(task: task, interception: interception)
if interception.isDone {
self.finish(task: task, interception: interception)
}
}
}

Expand Down
Loading