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

Request: Split out OpenTelemetry support into a separate Package target #1877

Closed
JaviSoto opened this issue Jun 3, 2024 · 15 comments
Closed

Comments

@JaviSoto
Copy link

JaviSoto commented Jun 3, 2024

Feature description

v2.12.0 introduced support for OpenTelemetry. As a result, DatadogTracer now depends on opentelemetry-swift. This has a ton of transitive dependencies. Since my understanding is that the use of OpenTelemetry is optional, this introduces a lot of extra binary size and complexity to projects that depend on DatadogTracer which aren't neccesary.
I'm particularly concerned about making our team's experience with Xcode and the buggy SPM support even worse by adding all these extra packages:

  • Opentracing
  • grpc-swift
  • Reachability.swift
  • swift-atomics
  • swift-http-types
  • swift-metrics
  • swift-nio
  • swift-nio-extras
  • swift-nio-http2
  • swift-nio-ssl
  • swift-nio-transport-services
  • swift-system
  • Thrift-Swift

Proposed solution

Hopefully creating some kind of DatadogTracerOpenTelemetry target can alleviate this. Only users who need this would depend on it, and those who don't can avoid all these transitive dependencies.

Other relevant information

No response

@JaviSoto JaviSoto changed the title Request: split out OpenTelemetry into a separate Package target Request: Split out OpenTelemetry support into a separate Package target Jun 3, 2024
@ganeshnj
Copy link
Contributor

ganeshnj commented Jun 4, 2024

Thanks for contacting @JaviSoto

The introduction of OpenTelemetry support takes a dependency on OpenTelemetryApi target only which has 0 dependencies. Hence, the application only includes the OpenTelemetryApi target and not the transitive dependencies.

In my testing with a sample application, the binary size increased from 33.6MB to 37.5MB when adding the OpenTelemetry support which we believe is acceptable. (Used https://github.com/DataDog/dd-sdk-ios/tree/develop/dependency-manager-tests/spm for testing)

However, I do agree with the developer experience of having to deal with extra packages in Xcode which can totally be avoided. We share the same concerns and opened an issue in the OpenTelemetry-Swift repository to address this. Feel free to +1 it.

Regarding separating the OpenTelemetry support into a separate target, we discussed this at length and decided against it for the following reasons:

  • Currently DatadogTrace implements OpenTracing standard which is deprecated by community and customers are requesting OpenTelemetry support.
  • Adding more packages creates migration pain when OpenTracing will be deprecated in the SDK.
  • We try to not add new packages and avoid wherever possible to keep a check on number of packages shipped with Datadog SDK.

@sebskuse
Copy link

sebskuse commented Jun 7, 2024

Hi,

Just found this thread after noticing that after updating to the latest version of the DataDog SDK we have sixteen new dependencies that have appeared.

In my testing with a sample application, the binary size increased from 33.6MB to 37.5MB when adding the OpenTelemetry support which we believe is acceptable. (Used https://github.com/DataDog/dd-sdk-ios/tree/develop/dependency-manager-tests/spm for testing)

Whilst I appreciate we're not compiling and shipping these extra libraries, there are other considerations than purely shipped binary size. We are still having to check out and cache those sixteen new repositories, which has significantly increased our build time locally and has doubled the size of our SPM cache (adding just under 1GB of checkout at the time of writing).

For us, the penalty of upgrading to 2.12.0 means that we will have to pin our version to 2.11.0.

@ganeshnj
Copy link
Contributor

ganeshnj commented Jun 7, 2024

Totally @sebskuse, feel free to +1 on the linked issue to split otel APIs.

However, I wouldn't recommend pinning the version and using the old version of the SDK for obvious reasons. We keep pushing fixes and improvements in the SDK and you would be missing out on those.

@sebskuse
Copy link

sebskuse commented Jun 7, 2024

Thanks @ganeshnj, I have +1'd it. My worry here is that that issue has been open since Nov 20, 2023 without much traction, so is this realistically going to get actioned?

I understand about pinning. My issue is the majority are being punished by DD supporting this pretty niche feature. If this is the route DD is going down, we may need to re-evaluate our usage of it. I'd rather not have to download and cache the whole of SwiftNIO every time any of our dependencies changes.

@Kaspik
Copy link

Kaspik commented Jun 11, 2024

However, I wouldn't recommend pinning the version and using the old version of the SDK for obvious reasons. We keep pushing fixes and improvements in the SDK and you would be missing out on those.

I have to say we are gonna do the same - pin to the 2.11.0 until this is resolved / reverted. We are not willing to checkout another dozen of SPMs for a feature we are not gonna use.

@clausjoergensen
Copy link

Same here. I'm not willing to take in dependencies on all of these Packages, of which a lot are missing the required PrivacyInfo.xcprivacy files, and/or are old and have questionable maintainable history.

@ganeshnj
Copy link
Contributor

I want to keep people updated here that we are working with otel folks to resolve this problem. We expected this friction but all this feedback is quite valuable for us in order to make a case to do the right thing. I really appreciate it.

@Kaspik
Copy link

Kaspik commented Jun 18, 2024

Appreciate the info here @ganeshnj , thanks!

Right now it's a weird spot for us - 2.11.0 doesn't compile on XCode 16 (it's fixed on master) and 2.12+ has OpenTelemetry dependencies. Will look forward for a solution here!

@ganeshnj
Copy link
Contributor

A quick update here, we have open-telemetry/opentelemetry-specification#4084 spec change request. I can't promise any timeline but we are working actively to make it happen.

@Kaspik would it be helpful if we release a patch 2.11.1 that has Xcode 16 fix to unblock you?

@Kaspik
Copy link

Kaspik commented Jun 26, 2024

Absolutely! 2.11.1 would be very helpful, thank you!

@ncreated
Copy link
Member

ncreated commented Jul 1, 2024

@Kaspik 2.11.1 is out. It includes the fix for Xcode 16 build.

@ganeshnj
Copy link
Contributor

ganeshnj commented Jul 3, 2024

As we are not sure about the timeline and uncertainty towards the change, we decided to fork the otel repo which only contains API only package. The develop branch is already updated to use the fork and will be released in upcoming version.

@maxep
Copy link
Member

maxep commented Jul 4, 2024

Release 2.14.0 now uses our otel fork which only contains APIs.

@GerardPaligot
Copy link

In one of my project, we are using directly OpenTelemetry Swift to instrument the iOS application. But because one of our dependency are using Datadog with an otel api fork, we got this kind of error now:

multiple targets named 'OpenTelemetryApi' in: 'opentelemetry-swift', 'opentelemetry-swift-packages'

It wasn't the case before the release 2.14.0.

@ganeshnj
Copy link
Contributor

ganeshnj commented Aug 5, 2024

Replied #1989

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

No branches or pull requests

8 participants