Skip to content
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

RUMM-731 Refactor URLSession instrumentation to support RUM and Tracing #262

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Oct 2, 2020

What and why?

⚙️ This PR applies refactoring to the URLSession instrumentation code. Given the requirements of the RUM product, previous implementation didn't scale well to support the URLSessionDelegate, which is necessary to retrieve network task metrics.

With this refactoring:

  • we instrument all 4 URLSession.dataTask(with:) creation methods (vs only 2 before);
  • request timing information is read directly from URLSessionTaskMetrics object (vs measured in swizzled code);
  • the integration with Alamofire will now be possible (our delegate can be used in Alamofire.EventMonitor).

The impact of this refactoring was mitigated by adding integration tests last sprint, in #253.

How?

The URLSessionTask task is now observed by two objects:

  • URLSessionSwizzler - to notify task creation and (in some cases) completion,
  • DDURLSessionDelegate - to retrieve task metrics and (in other cases) notify the completion.

Both talk to the URLSessionInterceptor, where interception happens and tracing Span is created (through URLSessionTracingHandler).

Screenshot 2020-10-02 at 13 05 59

Testing strategy is simple and it follows component relationships:

  • URLSessionSwizzlerTests ensure that methods on URLSessionInterceptor are called properly,
  • DDURLSessionDelegateTests ensure that methods on URLSessionInterceptor are called properly,
  • URLSessionInterceptorTests ensure that the Span is send to the backend on given circumstances,
  • URLSessionTracingHandlerTests assert the values of produced Span.

Although this PR seems big, only its first commit relates to the actual implementation. The rest is duplication of test cases in different scenarios (with 2 new instrumented URLSession methods, we have way more cases than before).

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

@ncreated ncreated added this to the rum milestone Oct 2, 2020
@ncreated ncreated requested a review from a team as a code owner October 2, 2020 11:26
@ncreated ncreated self-assigned this Oct 2, 2020
Copy link
Contributor

@buranmert buranmert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i haven't looked at tests yet but i guess we can discuss those non-major issues first

@ncreated ncreated force-pushed the ncreated/RUMM-731-refactor-URLSession-instrumentation-to-handle-both-RUM-and-Tracing branch from 03bedfb to 68706d3 Compare October 6, 2020 06:19
@ncreated ncreated merged commit 8afa79f into master Oct 7, 2020
@ncreated ncreated deleted the ncreated/RUMM-731-refactor-URLSession-instrumentation-to-handle-both-RUM-and-Tracing branch October 7, 2020 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants