RUMM-1322 Allow initializing DDURLSessionDelegate before starting the SDK
#483
Conversation
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
…ion tests This will randomly initialize `URLSession` before or after `Datadog.initialize()` in all session-based integration tests.
ncreated
commented
May 5, 2021
| } | ||
|
|
||
| /// An object performing interception of requests sent with `URLSession`. | ||
| public class URLSessionInterceptor: URLSessionInterceptorType { | ||
| public static var shared: URLSessionInterceptor? { | ||
| URLSessionAutoInstrumentation.instance?.interceptor | ||
| URLSessionAutoInstrumentation.instance?.interceptor as? URLSessionInterceptor |
There was a problem hiding this comment.
💡 This casting is now required and is tested in existing URLSessionAutoInstrumentationTests tests.
ncreated
commented
May 5, 2021
| internal protocol URLSessionInterceptorType: class { | ||
| func modify(request: URLRequest, session: URLSession?) -> URLRequest | ||
| func taskCreated(task: URLSessionTask, session: URLSession?) | ||
| func taskMetricsCollected(task: URLSessionTask, metrics: URLSessionTaskMetrics) | ||
| func taskReceivedData(task: URLSessionTask, data: Data) | ||
| func taskCompleted(task: URLSessionTask, error: Error?) | ||
|
|
||
| var handler: URLSessionInterceptionHandler { get } |
There was a problem hiding this comment.
💡 This adds a bit noise to the URLSessionInterceptorType interface and could be additionally mitigated by decoupling the handler creation to upstream (to URLSessionAutoInstrumentation class). This however seems over engineering if done in the scope of this PR - we may want to refactor this on a stronger reason with more constraints for changing the abstraction.
buranmert
approved these changes
May 5, 2021
Sources/Datadog/URLSessionAutoInstrumentation/DDURLSessionDelegate.swift
Outdated
Show resolved
Hide resolved
ncreated
deleted the
ncreated/RUMM-1322-allow-DDURLSessionDelegate-init-before-SDK-init
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What and why?
🚚 This PR removes the constraint of initializing
DDURLSessionDelegateafterDatadog.initialize(). From now on, users can create our delegate freely, no matter ofDatadog.initialize(). The SDK will only track requests issued after starting the SDK.This is to provide more freedom for the user setup and cut off error-prone edge cases (like race conditions).
How?
I changed the
DDURLSessionDelegateto obtain the interceptor's reference through lazily computed variable, instead of injecting its value oninit(). The obsolete misconfiguration warnings were also removed.To test the new behaviour, I randomized the
URLSession-based integration tests setup to randomly create delegate before or after theDatadog.initialize(). This includes 4 integration tests usingURLSessionandNSURLSession: 2 for RUM Resources and 2 for Tracing auto instrumentation.Review checklist