-
Notifications
You must be signed in to change notification settings - Fork 133
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.
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-717 feat!: Add network instrumentation for async/await URLSession APIs #1394
RUM-717 feat!: Add network instrumentation for async/await URLSession APIs #1394
Conversation
f5a06fe
to
e9edac7
Compare
abf8ee6
to
c875fc6
Compare
Datadog ReportBranch report: ✅ |
632156f
to
96d8edf
Compare
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.
Only preliminary feedbacks for now, I need more time 🙏.
DatadogInternal/Sources/NetworkInstrumentation/DatadogURLSessionHandler.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift
Outdated
Show resolved
Hide resolved
IntegrationTests/IntegrationScenarios/Scenarios/RUM/RUMResourcesScenarioTests.swift
Outdated
Show resolved
Hide resolved
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 work here!
In additions to @ncreated comments, I've left some suggestions. One important topic is delegate instance vs. type when enabling the instrumentation, I think we should be clear that we are instrumenting the class and not the instance.
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionInstrumentation.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionDataDelegateSwizzler.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionSwizzler.swift
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionDataDelegateSwizzler.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift
Outdated
Show resolved
Hide resolved
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.
Overall looks good! +1 to other's comments + a few minor ones from my side
DatadogInternal/Sources/NetworkInstrumentation/FirstPartyHosts.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTask+Tracking.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTaskInterception.swift
Show resolved
Hide resolved
Datadog ReportBranch report: ❌ ❌ Failed Tests (1)
|
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.
Looks neat 👌. I left few questions. On top of it:
- Shouldn't we mark
DDURLSessionDelegate
as deprecated? - What about exposing this new API in
Objective-C
?
DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/NetworkInstrumentationFeature.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionDataDelegateSwizzler.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTask+Tracking.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTaskDelegate+Tracking.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTaskDelegateSwizzler.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Tests/NetworkInstrumentation/URLSessionDataDelegateSwizzlerTests.swift
Outdated
Show resolved
Hide resolved
IntegrationTests/Runner/Scenarios/URLSession/URLSessionScenarios.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionInstrumentation.swift
Outdated
Show resolved
Hide resolved
Added objc interfaces - good call. Some tests are also updated along with this. Deprecation - I will handle in seperate PR as it is very specific and shouldn't be coupled. |
DatadogInternal/Sources/NetworkInstrumentation/URLSession/DatadogURLSessionDelegate.swift
Show resolved
Hide resolved
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.
It looks great!! It's a complex logic but you were able to keep it readable, well done 👍
I've one remaining concern in various swizzler where properties are exposed while a lock is required.
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTaskSwizzler.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionSwizzler.swift
Show resolved
Hide resolved
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.
Great effort!
One potential blocker on associated object implementation
Datadog/Datadog.xcodeproj/xcshareddata/xcschemes/DatadogCore iOS.xcscheme
Show resolved
Hide resolved
Datadog/Datadog.xcodeproj/xcshareddata/xcschemes/DatadogCore tvOS.xcscheme
Show resolved
Hide resolved
DatadogCore/Tests/DatadogObjc/DDNSURLSessionDelegateTests.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/DatadogURLSessionDelegate.swift
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTaskDelegate+Tracking.swift
Outdated
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTaskDelegate+Tracking.swift
Show resolved
Hide resolved
DatadogInternal/Sources/NetworkInstrumentation/URLSession/URLSessionTaskDelegateSwizzler.swift
Show resolved
Hide resolved
2267484
to
30b731f
Compare
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.
Looks good 👌. Solid work 👍. Have we ran unit tests with iOS 12 - 15 Simulators? (to make sure it works and we won't have nightly job problems after merge)
DatadogCore/Tests/DatadogObjc/ObjcAPITests/DDNSURLSessionDelegate+apiTests.m
Show resolved
Hide resolved
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.
Thank you @ganeshnj for addressing all our comments and questions, it looks great!
Datadog ReportBranch report: ✅ |
507fa89
to
e1de388
Compare
Datadog ReportBranch report: ✅ |
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.
🚀
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.
LGTM
39c138e
to
f1ef9db
Compare
Datadog ReportBranch report: ✅ |
f1ef9db
to
404021c
Compare
404021c
to
0b56eb5
Compare
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.
🚀
What and why?
Fixes #1288
How?
https://datadoghq.atlassian.net/wiki/spaces/RUMP/pages/3130426092
TLDR
Dogfood: https://github.com/DataDog/shopist-ios/pull/55
Customers no longer have to implement Datadog delegate to instrument rather they can simply use high level APIs.
or
Review checklist
Custom CI job configuration (optional)