RUM-2690 fix: URLSession* swizzling issues on iOS 13 and 12
#1637
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 2790 Passed, 0 Skipped, 29m 29.64s Wall Time 🔻 Code Coverage Decreases vs Default Branch (13)
|
| guard let klass = NSClassFromString(className) else { | ||
| throw InternalError(description: "Failed to swizzle `URLSessionTask`: `\(className)` class not found.") | ||
| } |
There was a problem hiding this comment.
Ideally, we should dump the runtime description of URLSessionTask in this telemetry, so we can see the list of subclasses to what is broken in assumed swizzling, like:
var stack = ""
dump(task, to: &stack)
print(stack)
// ```
// - LocalDataTask <...>.<1>
// - super: __NSCFLocalSessionTask
// - super: NSURLSessionTask
// - super: NSObject
// ```However, this can only be done at runtime as we need a task object. I was unable to get this information from any intercepted task (e.g. in session.dataTask(...) swizzling) as these interceptions are extensively called multiple times if more than one session is instrumented. Ultimately it was leading to many false-positives.
There was a problem hiding this comment.
Indeed, we should start investigating as soon as we get this telemetry error. It will most likely be a change on a new OS version so we should be able to dump the object while debugging.
Maybe we should start having monitors of specific error!
| 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) | ||
| } |
There was a problem hiding this comment.
This change is porting the configuration from previous version of URLSession instrumentation, this:
This is required to make unit tests pass on iOS 12.4. Without this change, the expectation in this test was fulfilled 4 times instead of 2.
Rationale: The promise of URLSessionSwizzler is to intercept completion handler by abstracting runtime details (swizzling). In no circumstances this block should be called more than once. If iOS 12.4 details is different, the difference should be erased in this abstraction, delivering the right promise to higher level.
There was a problem hiding this comment.
Added this little selector to RUM debugging flow, so we can send instrumented request. Required for debugging in iOS 12.4 where the other flow like this is not available (as it is using SwiftUI).
ncreated
force-pushed
the
ncreated/RUM-2690/quick-fix-iOS-13-and-12-fail-to-intercept-URLSessionTask-resume
branch
from
658db76
to
ad26b44
Compare
ncreated
force-pushed
the
ncreated/RUM-2690/quick-fix-iOS-13-and-12-fail-to-intercept-URLSessionTask-resume
branch
from
ad26b44
to
786d506
Compare
ncreated
deleted the
ncreated/RUM-2690/quick-fix-iOS-13-and-12-fail-to-intercept-URLSessionTask-resume
branch
What and why?
🧰 Fixing broken network instrumentation on iOS 13.x and 12.x by remedying the
URLSessionTask.resume()swizzling and making sureDatadogInternalTestspass on these OSes.How?
When swizzling
URLSessionTask.resume()selector, the interception callback was not called on iOS 13 and 12. Being inspired with these hints, this PR changes the swizzled class to private__NSCFLocalSessionTask.Observation:
__NSCFLocalSessionTaskseems to be uniformly used across existing major versions of the OS, making it a good candidate to swizzle. We can't anticipate if this will continue to work in future versions of the OS. I did several attempts on adding observability to this assumption (including this), unfortunately failed due to lack of consistency in some of our swizzlings. This is something I want to follow up on in short-term.Here,
taskdumps from specific versions:Review checklist
Custom CI job configuration (optional)
tools/