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-883 Add support for 3rd party networking libraries (including Alamofire) #340

Merged

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Dec 11, 2020

What and why?

📦 This PR makes it possible to add integrations with 3rd party networking libraries (including Alamofire).

How?

Our network interceptor is pretty straightforward and follows the interception model in Alamofire:

  • modify request;
  • process task creation;
  • process task metrics;
  • process task completion (with optional error).

By polishing its interface and making the URLSessionInterceptor public, now it is possible to use our SDK with Alamofire by implementing custom AF's EventMonitor (for task start, metrics and completion) and AF's RequestInterceptor (for request modification).

Implementations of DDEventMonitor and DDRequestInterceptor are provided in Sources/DatadogExtensions/Alamofire/.

Those implementations are delivered in DatadogSDKAlamofireExtension pod:

pod 'DatadogSDKAlamofireExtension'
import DatadogAlamofireExtension
import Alamofire

let alamofireSession = Session(
   interceptor: DDRequestInterceptor(),
   eventMonitors: [DDEventMonitor()]
)

DatadogAlamofireExtension for Carthage and SPM

It is not yet possible to distribute DatadogAlamofireExtension as SPM package, because dynamic frameworks are still broken in SPM. Doing so, raises the compiler warning of duplicated symbols for Datadog and Kronos (as those two are (in-)directly linked to the app by both Datadog and DatadogAlamofireExtension dependencies).

Distributing DatadogAlamofireExtension through Carthage is very inconvenient and may be error-prone for non-Alamofire users, as both DatadogAlamofireExtension.framework and Alamofire.framework will appear in Carthage/Build/iOS folder. This would require additional explanation in Carthage installation steps for the SDK.

Note on DatadogSDKAlamofireExtension.podspec

Instead of creating separate podspec, I've also considered also adding AlamofireExtension subspec to DatadogSDK.podspec or creating DatadogSDKExtensions/Alamofire spec. Neither of this seems valid, as it doesn't scale for other integrations and eventual SPM / Carthage support.

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 requested a review from a team as a code owner December 11, 2020 17:21
@ncreated ncreated self-assigned this Dec 11, 2020
@ncreated ncreated force-pushed the ncreated/RUMM-883-add-support-to-Alamofire-networking branch from 4e5071e to 45bbbb3 Compare December 15, 2020 08:00
@ncreated ncreated marked this pull request as draft December 17, 2020 08:29
@ncreated ncreated force-pushed the ncreated/RUMM-883-add-support-to-Alamofire-networking branch 4 times, most recently from f068fff to 5d4ec9b Compare December 17, 2020 11:39
@ncreated ncreated force-pushed the ncreated/RUMM-883-add-support-to-Alamofire-networking branch 4 times, most recently from 8c22098 to ee0a583 Compare December 17, 2020 12:33
@ncreated ncreated force-pushed the ncreated/RUMM-883-add-support-to-Alamofire-networking branch from ee0a583 to 1ef78e3 Compare December 17, 2020 14:01
@ncreated ncreated force-pushed the ncreated/RUMM-883-add-support-to-Alamofire-networking branch from 1ef78e3 to 4f5d4b0 Compare December 17, 2020 14:05
@ncreated
Copy link
Member Author

As discussed on daily, I created DatadogSDKAlamofireExtension pod which provides the helpers. The description of this PR was updated with more details.

@ncreated ncreated marked this pull request as ready for review December 17, 2020 14:49
@ncreated ncreated requested a review from a team as a code owner December 17, 2020 14:49
Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

Docs review

README.md Outdated Show resolved Hide resolved
Sources/DatadogExtensions/Alamofire/README.md Outdated Show resolved Hide resolved
Sources/DatadogExtensions/Alamofire/README.md Outdated Show resolved Hide resolved
Sources/DatadogExtensions/Alamofire/README.md Outdated Show resolved Hide resolved
Sources/DatadogExtensions/Alamofire/README.md Outdated Show resolved Hide resolved
Sources/DatadogExtensions/Alamofire/README.md Outdated Show resolved Hide resolved
Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com>
@ncreated ncreated requested a review from ruthnaebeck December 28, 2020 08:15
Copy link
Contributor

@ruthnaebeck ruthnaebeck left a comment

Choose a reason for hiding this comment

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

👍 for docs

@ncreated ncreated merged commit 0fe108a into master Jan 4, 2021
@ncreated ncreated deleted the ncreated/RUMM-883-add-support-to-Alamofire-networking branch January 4, 2021 19:08
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.

3 participants