opentelemetry-js
opentelemetry-js copied to clipboard
open-telemetry
feat(otlp-transformer): Do not limit `@opentelemetry/api` upper range peerDependency
Today, some packages define a peer dependency on @opentelemetry/api that explicitly excludes future versions in the 1.x range. IMHO this is not helpful because 1.x release have to be backwards compatible anyhow, but this can lead to version conflicts when using different packages. I believe that it should be safe enough to simply have ^1.3.0 as peer dependency?
Fixes https://github.com/open-telemetry/opentelemetry-js/issues/4815
@pichlermarc I am also following this thread.
I struggle to understand the reasoning for clamping the version range like this. Especially considering the spec you linked:
When new functionality is added to the OpenTelemetry API, a new minor version of the API is released. These API changes are always additive and backwards compatible from the perspective of existing Instrumentation packages which import and call prior versions. Instrumentation written against all prior minor versions of the API continues to work, and may be composed together into the same application without creating a dependency conflict.
To me, this would very much indicate that it is absolutely safe to allow for a specific version and above within a major (^). I might totally be missing something.
Do you happen to have an example for how this is necessary?
I understand that extending an interface will be fine for consumers, but break for implementors, however, that seems to be forbidden by the spec and instead replacement interfaces should be provided:
Instead of breaking a Plugin Interface, a new interface is created and the existing interface is marked as deprecated. Plugins which target the deprecated interface continue to work, and the SDK provides default implementations of any missing functionality. After one year, the deprecated Plugin Interface may be removed in a major version release of the SDK.
Do you happen to have an example for how this is necessary?
Example: Adding a new instrument to the Meter interface in the API.
I understand that extending an interface will be fine for consumers, but break for implementors, however, that seems to be forbidden by the spec and instead replacement interfaces should be provided:
Instead of breaking a Plugin Interface, a new interface is created and the existing interface is marked as deprecated. Plugins which target the deprecated interface continue to work, and the SDK provides default implementations of any missing functionality. After one year, the deprecated Plugin Interface may be removed in a major version release of the SDK.
There's a difference between the API and the SDK.
This Section relates to the SDK (and Plugin interfaces - like Instrumentation, SpanExporter, SpanProcessor, etc.). Compared to the API the SDK may actually receive major version bumps where these deprecated types can be removed. The API spec is much more restrictive on that.
I updated this to only unclamp this for otlp-transformer! @pichlermarc thanks for the explanations!
Me and @mydea thought a lot about this offline and I settled my mental model on the fact that SemVer is sometimes actually really hard to properly get right when you have implementors. It becomes a trade-off between wanting to be super correct, and being user-friendly but risking breakage. In theory, even simple stuff that you may think will never break anybody, like for example turning the return type of a simple exported function from void to string may break somebody's implementation.
In our packages in the Sentry JS SDKs we probably don't adhere to SemVer in the strictest of ways (still we never had anybody complain). The only API where we are careful is our Client (which is the only realistic API surface that people actually may implement themselves): https://github.com/getsentry/sentry-javascript/blob/5eafa401c076796fba7663dad9ca254ab6f1972f/packages/types/src/client.ts#L30
Here, whenever we want to add surface, we first add it as optional field, and then in a major we may make it non-optional. This comes with a bit of internal implementation complexity but it is probably worth it. I am not familiar enough with the OTEL implementations and whether OTEL already has this pattern but I still wanted to bring it up for posterity.