fix: pass through data when network request completes #1696

ganeshnj · 2024-02-22T10:56:00Z

What and why?

How?

This PR mimics the old behavior of the instrumentation.

When completion handler is provided in URLSession APIs, urlSession(_:dataTask:didReceive:) is not called, which means we can't buffer the data and in the called the Resource Attribute Provider.

This is not only true for the completion handler based APIs but also true for async-await APIs.

Meanwhile we use urlSession(_:task:didFinishCollecting:) to mark the completion of the network operation which is the only reliable API at this moment. We use this to mark completion for async-await APIs.

How did the regression happen?

For some APIs, both urlSession(_:task:didFinishCollecting:) and urlSession(_:task:didCompleteWithError:) are called. But there is not specific order guaranteed which one comes first, ie the one called first, marks the end of the network operation. In the breaking change, urlSession(_:task:didFinishCollecting:) and given urlSession(_:dataTask:didReceive:) was not called either, Resource Attribute Provider executes with nil data.

How does this PR fix it?

With this PR we make sure, if the API has a completion handler given, we are not going to use urlSession(_:task:didFinishCollecting:) as end of operation because we are sure urlSession(_:task:didCompleteWithError:) will be called.

Along side, when the operation finishes, we also call handler for registering the data when the operation completes. The APIs guarantees from behavior that either urlSession(_:dataTask:didReceive:) is called or the compeltion handler, hence we don't register data two times.

Do we support async-await?

No, because they don't have completion handler, so experience remain broken with them.

Tested locally on

iOS 12.4
iOS 13.0
iOS 14.5
iOS 15.5

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 · 2024-02-22T11:03:02Z

Datadog Report

Branch report: ganeshnj/fix/resource-attributes-provider
Commit report: adcc669
Test service: dd-sdk-ios

❌ 3 Failed (0 Known Flaky), 1202 Passed, 0 Skipped, 1m 37.56s Wall Time
🔻 Test Sessions change in coverage: 2 decreased, 3 increased, 1 no change

❌ Failed Tests (3)

testGivenURLSessionWithCustomDelegate_whenUsingAsyncDataForURLRequest_itNotifiesInterceptor - NetworkInstrumentationFeatureTests - Details
Expand for error
```
 Assertion Failure: Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "Notify intercepion did complete".
```

testGivenURLSessionWithCustomDelegate_whenUsingAsyncDataFromURL_itNotifiesInterceptor - NetworkInstrumentationFeatureTests - Details

Expand for error

 Assertion Failure: Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "Notify intercepion did complete".

testGivenURLSessionWithCustomDelegate_whenUsingAsyncData_itPassesAllValuesToTheInterceptor - NetworkInstrumentationFeatureTests - Details

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")")
 Assertion Failure at NetworkInstrumentationFeatureTests.swift:358: XCTAssertEqual failed: ("nil") is not equal to ("Optional("some error")")

🔻 Code Coverage Decreases vs Default Branch (2)

test DatadogRUMTests iOS 79.30% (-0.24%) - Details
test DatadogInternalTests iOS 81.21% (-0.05%) - Details

ncreated

blocking/ On top of in-line comments, I think we're missing the fact that tasks are pooled in URLSession and may be reused for succeeding operations. That means the task state set with task.dd.hasCompletion must be reset before putting the task back to pool.

Datadog/IntegrationUnitTests/Public/NetworkInstrumentationIntegrationTests.swift

DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift

DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTask+Tracking.swift

ncreated · 2024-03-04T09:22:12Z

DatadogRUM/Sources/RUMConfiguration.swift

+            /// Note: This is not supported for async-await APIs.
+            ///


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.

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.

ganeshnj · 2024-03-04T09:55:24Z

blocking/ On top of in-line comments, I think we're missing the fact that tasks are pooled in URLSession and may be reused for succeeding operations. That means the task state set with task.dd.hasCompletion must be reset before putting the task back to pool.

What do you mean here? I don't expect same instance of URLSessionTask being used again. Rather I expect the connection to be used.

…grationTests.swift Co-authored-by: Maciek Grzybowski <[email protected]>

ncreated

@ganeshnj I was wrong, sorry. I always thought that URLSessionTask is one of these few Apple objects that are reused through pool 🤔💭, like UITouch or table cells. Interesting 🙂. There is indeed no note on that in URLSession docs, and I don't observe this behavior in tests.

Looks good 👍. Please make sure it all works correct iOS 13-17, as we have no automated coverage at this moment.

maciejburda

Looks good. Some minor nits to consider :)

maciejburda · 2024-03-05T11:16:36Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/DatadogURLSessionDelegate.swift

@@ -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


nit: didReceive param could be optional with default nil instead

yeah but not worth the refactor.

maciejburda · 2024-03-05T11:19:25Z

DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTask+Tracking.swift

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


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

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

ganeshnj force-pushed the ganeshnj/fix/resource-attributes-provider branch from adcc669 to 7e761a8 Compare February 23, 2024 16:23

fix: pass through data when network request completes

50bf3f5

ganeshnj force-pushed the ganeshnj/fix/resource-attributes-provider branch from 7e761a8 to 50bf3f5 Compare February 23, 2024 16:49

fix warning

3daf624

ganeshnj marked this pull request as ready for review February 26, 2024 12:30

ganeshnj requested review from a team as code owners February 26, 2024 12:30

ncreated requested changes Mar 4, 2024

View reviewed changes

Update Datadog/IntegrationUnitTests/Public/NetworkInstrumentationInte…

5cf5f8c

…grationTests.swift Co-authored-by: Maciek Grzybowski <[email protected]>

ncreated approved these changes Mar 5, 2024

View reviewed changes

maciejburda approved these changes Mar 5, 2024

View reviewed changes

ganeshnj merged commit f1f3d40 into develop Mar 5, 2024
8 checks passed

ganeshnj deleted the ganeshnj/fix/resource-attributes-provider branch March 5, 2024 17:58

ganeshnj mentioned this pull request Mar 5, 2024

RUM.ResourceAttributesProvider not providing data information in URLSession tracking #1680

Open

ncreated mentioned this pull request Mar 12, 2024

Dogfood recent changes #1719

Merged

8 tasks

maciejburda mentioned this pull request Mar 19, 2024

Release 2.8.0 #1732

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pass through data when network request completes #1696

fix: pass through data when network request completes #1696

ganeshnj commented Feb 22, 2024 •

edited

Loading

datadog-datadog-prod-us1 bot commented Feb 22, 2024

ncreated left a comment

ncreated Mar 4, 2024

ganeshnj Mar 4, 2024

ganeshnj commented Mar 4, 2024

ncreated left a comment

maciejburda left a comment

maciejburda Mar 5, 2024 •

edited

Loading

ganeshnj Mar 5, 2024

maciejburda Mar 5, 2024

ganeshnj Mar 5, 2024

fix: pass through data when network request completes #1696

fix: pass through data when network request completes #1696

Conversation

ganeshnj commented Feb 22, 2024 • edited Loading

What and why?

How?

How did the regression happen?

How does this PR fix it?

Do we support async-await?

Tested locally on

Review checklist

Custom CI job configuration (optional)

datadog-datadog-prod-us1 bot commented Feb 22, 2024

Datadog Report

❌ Failed Tests (3)

🔻 Code Coverage Decreases vs Default Branch (2)

ncreated left a comment

Choose a reason for hiding this comment

ncreated Mar 4, 2024

Choose a reason for hiding this comment

ganeshnj Mar 4, 2024

Choose a reason for hiding this comment

ganeshnj commented Mar 4, 2024

ncreated left a comment

Choose a reason for hiding this comment

maciejburda left a comment

Choose a reason for hiding this comment

maciejburda Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

ganeshnj Mar 5, 2024

Choose a reason for hiding this comment

maciejburda Mar 5, 2024

Choose a reason for hiding this comment

ganeshnj Mar 5, 2024

Choose a reason for hiding this comment

ganeshnj commented Feb 22, 2024 •

edited

Loading

maciejburda Mar 5, 2024 •

edited

Loading