open-telemetry / opentelemetry-swift

OpenTelemetry API for Swift
https://opentelemetry.io/docs/instrumentation/swift/
Apache License 2.0
214 stars 133 forks source link

Splitting APIs in their own repo to allow custom implementations #486

Open ganeshnj opened 10 months ago

ganeshnj commented 10 months ago

Motivation

Increasingly customers are asking for OpenTelemetry support in Datadog SDKs at the same time using other Datadog SDK features. With the current setup of the opentelemetry-swift SDK, it is impossible to take dependency on API only packages. This becomes a bigger problem for mobile developers (dd-sdk-ios users) where Xcode loads and indexes all the packages in the workspace and slows down the entire development experience.

Proposal

Split the APIs from the implementation in opentelemetry-swift. Ideally there must exist two swift package repositories:

  1. opentelemetry-swift-api (subject to change name)
  2. opentelemetry-swift

I experimented the approach at https://github.com/ganeshnj/opentelemetry-swift-sdk & https://github.com/ganeshnj/opentelemetry-swift-api and it seems to work fine. I was able to take dependency on opentelemetry-swift-api and use the API without any issues.

It does comes with some challenges such as versioning, release and breaking changes (will be clearly visible) but I think it is worth it. It also goes with OpenTelemetry's requirements.

The OpenTelemetry API must be well-defined and clearly decoupled from the implementation. This allows end users to consume API only without also consuming the implementation (see points 2 and 3 for why it is important).

The developers of the final application normally decide how to configure OpenTelemetry SDK and what extensions to use. They should be also free to choose to not use any OpenTelemetry implementation at all, even though the application and/or its libraries are already instrumented

I'm happy to discuss this in the next Swift otel sync.

bryce-b commented 10 months ago

We discussed this issue during the last Swift-SIG (the recording of which is available here) and I will summarize our discussion: We believe that this issue is a matter of opinion, and doesn't provide significant value for the trade-offs, so we will not be implementing this requested change. However, I will leave this ticket open in the case there are more interested parties that would like to see this change made. Nacho also mentioned potentially using git submodules to only import the open-telemetry-api portion of the project to avoid adding all the additional dependencies in the sdk.

ganeshnj commented 10 months ago

Thanks for quick summary @bryce-b

Using submodules looks counterintuitive from package management point of view (not the preferred choice here) but I can see a fork which only contains the APIs. We can eventually include support for other Swift/iOS package managers.

ancostas commented 9 months ago

@bryce-b I'll add my +1 to @ganeshnj's request. IMO if one wants for the OpenTelemetry API to be implemented by any OS developer (which I think is within the spirit of the project), then it needs to ship without a reference implementation in the same package. Same with the SDK interfaces.

jbluntz commented 6 months ago

Just came to register my support for an API-only OpenTelemetry package - our app is a hierarchy of modules and I would ideally want to have all our intermediate modules dependent on on the OTel API only, with the ability to independently plug in the DataDog (or another) implementation at the app layer. The goal would be to enforce loose coupling between the majority of the app and the actual Tracer implementation and reduce the risk of leaking implementation detail into our intermediate modules.

heckj commented 4 months ago

Wanted to add in my support for separating implementation from a core API interface. In my case I have a library I want to enable for tracing, but I don’t want to mandate the whole dependency sequence of this tracing implementation on the consumers of my library. Instead I want to allow it as an option, and have the developer provide a tracing impl external to my library.

So add a +1 to the queue of folks supporting an alternate decision. (full disclosure - i’ve started to opt into https://github.com/apple/swift-distributed-tracing and using the NIO based tracer for my library)

yurishkuro commented 3 months ago

The separation of API and SDK is required by the Specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/library-guidelines.md#requirements

The OpenTelemetry API must be well-defined and clearly decoupled from the implementation. This allows end users to consume API only without also consuming the implementation (see points 2 and 3 for why it is important).

Third party libraries and frameworks that add instrumentation to their code will have a dependency only on the API of OpenTelemetry client. The developers of third party libraries and frameworks do not care (and cannot know) what specific implementation of OpenTelemetry is used in the final application.

nachoBonafonte commented 3 months ago

The separation between API and SDK already exists, and doesn't force the user to depend on both. The problem here is that the way the SPM package works it downloads all the dependencies of all the libraries when including in a project, not that users are forced to link both.

ganeshnj commented 3 months ago

We all understand SPM can do better here but our priority should be here how can we make the developer experience better which is not just limited to final linking. Now customers have to deal with ~340MB of transitive dependencies and their caching.

We have few customer reports that they are not happy with such a degradation in experience and will not move to the next version of the SDK. Considering these reports, we should look into how we can make the experience better with current SPM limitations.

jpkrohling commented 3 months ago

The problem here is that the way the SPM package works it downloads all the dependencies of all the libraries when including in a project, not that users are forced to link both.

@nachoBonafonte, does it mean you'd be open to the change being proposed here?

nachoBonafonte commented 3 months ago

This topic was already discussed in the SIG meeting and as Bryce clearly explains at the beginning of the thread it was discarded. We can bring the topic again to the meeting but we are still very low on human resources and it brings quite a lot of maintenance burden.

ganeshnj commented 3 months ago

We should discuss the maintenance cost and impact it will bring (+/-). We are willing to help here.

GerardPaligot commented 2 months ago

I'd like to add a new argument in favor of this new repo: Because OpenTelemetryApi is packaged with other targets, it isn't possible to use telemetry SDKs that are using a fork of OpenTelemetryApi and OpenTelemetry-Swift in the same project (in my case, Datadog is used by a dependency I need to use and OpenTelemetry by my own project).

But if you split OpenTelemetryApi with the rest of the codebase, every telemetry SDK can use the API specification of OpenTelemetry without any incidence in the consumer side.

Please reconsider your original response. 🙏

The background to this problem can be found here: https://github.com/DataDog/dd-sdk-ios/issues/1989

CrownedPhoenix commented 2 weeks ago

+1 to splitting the API into a separate package/repo.

NeedleInAJayStack commented 2 weeks ago

@nachoBonafonte

we are still very low on human resources and it brings quite a lot of maintenance burden.

I'd also be interested in hearing about the maintenance burden it brings. I'd definitely be willing to contribute time to this effort 🙂