RUM-2355 fix: Network Instrumentation #1588
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
| """ | ||
|
There was a problem hiding this comment. 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
}There was a problem hiding this comment. 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. |
||
| ) | ||
| ] | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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`. | ||
|
|
@@ -49,80 +52,72 @@ 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 bind(configuration: URLSessionInstrumentation.Configuration) throws { | ||
| let configuredFirstPartyHosts = FirstPartyHosts(firstPartyHosts: configuration.firstPartyHostsTracing) ?? .init() | ||
|
|
||
| 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 | ||
| } | ||
|
There was a problem hiding this comment. How is the dd.delegate lifecycle is managed? There was a problem hiding this comment. It's a computed |
||
|
|
||
| if let currentRequest = task.currentRequest { | ||
| let request = self.intercept(request: currentRequest, additionalFirstPartyHosts: configuredFirstPartyHosts) | ||
| task.dd.override(currentRequest: request) | ||
| } | ||
|
There was a problem hiding this comment. question/ This condition is quite puzzling. In what circumstances we expect There was a problem hiding this comment. That's a good question, I don't know what could nillify the |
||
|
|
||
| self.intercept(task: task, additionalFirstPartyHosts: configuredFirstPartyHosts) | ||
| } | ||
| ) | ||
|
|
||
| try swizzler.swizzle( | ||
| delegateClass: configuration.delegateClass, | ||
| interceptDidReceive: { [weak self] session, task, data in | ||
| self?.task(task, didReceive: data) | ||
| }, | ||
| interceptDidFinishCollecting: { [weak self] session, task, metrics in | ||
| self?.task(task, didFinishCollecting: metrics) | ||
|
|
||
| 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 | ||
| self?.task(task, didCompleteWithError: task.error) | ||
| } | ||
| }, | ||
| interceptDidCompleteWithError: { [weak self] session, task, error in | ||
| self?.task(task, didCompleteWithError: error) | ||
| } | ||
| ) | ||
|
|
||
| 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) { | ||
| let identifier = ObjectIdentifier(delegateClass) | ||
| swizzlers.removeValue(forKey: identifier) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -152,47 +147,32 @@ extension NetworkInstrumentationFeature { | |
| /// - task: The created task. | ||
| /// - additionalFirstPartyHosts: Extra hosts to consider in the interception. | ||
| func intercept(task: URLSessionTask, additionalFirstPartyHosts: FirstPartyHosts?) { | ||
| queue.async { [weak self] in | ||
| guard let self = self, let request = task.currentRequest else { | ||
| return | ||
| } | ||
|
|
||
| let firstPartyHosts = self.firstPartyHosts(with: additionalFirstPartyHosts) | ||
|
|
||
| let interception = self.interceptions[task] ?? | ||
| URLSessionTaskInterception( | ||
| request: request, | ||
| isFirstParty: firstPartyHosts.isFirstParty(url: request.url) | ||
| ) | ||
|
|
||
| interception.register(request: request) | ||
|
|
||
| if let trace = self.extractTrace(firstPartyHosts: firstPartyHosts, request: request) { | ||
| interception.register(traceID: trace.traceID, spanID: trace.spanID, parentSpanID: trace.parentSpanID) | ||
| } | ||
|
|
||
| if let origin = request.value(forHTTPHeaderField: TracingHTTPHeaders.originField) { | ||
| interception.register(origin: origin) | ||
| } | ||
|
|
||
| self.interceptions[task] = interception | ||
| self.handlers.forEach { $0.interceptionDidStart(interception: interception) } | ||
| } | ||
| } | ||
|
|
||
| /// Tells the interceptors that metrics were collected for the given task. | ||
|
|
@@ -202,21 +182,17 @@ extension NetworkInstrumentationFeature { | |
| /// - metrics: The collected metrics. | ||
| func task(_ task: URLSessionTask, didFinishCollecting metrics: URLSessionTaskMetrics) { | ||
| queue.async { [weak self] in | ||
| guard let self = self, let interception = self.interceptions[task] else { | ||
| return | ||
| } | ||
|
|
||
| interception.register( | ||
| metrics: ResourceMetrics(taskMetrics: metrics) | ||
| ) | ||
|
|
||
| if interception.isDone { | ||
| self.finish(task: task, interception: interception) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -227,40 +203,29 @@ extension NetworkInstrumentationFeature { | |
| /// - data: A data object containing the transferred data. | ||
| func task(_ task: URLSessionTask, didReceive data: Data) { | ||
| queue.async { [weak self] in | ||
| self?.interceptions[task]?.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 | ||
| guard let self = self, let interception = self.interceptions[task] else { | ||
| return | ||
| } | ||
|
|
||
| interception.register( | ||
| response: task.response, | ||
| error: error | ||
| ) | ||
|
|
||
| if interception.isDone { | ||
| self.finish(task: task, interception: interception) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 ❄️:There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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👍