feat(otlp-transformer): Do not limit @opentelemetry/api upper range peerDependency

[mydea]

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 #4815

[pichlermarc]

There's a spec reason for this: https://github.com/open-telemetry/opentelemetry-specification/blob/a03382ada8afa9415266a84dafac0510ec8c160f/specification/upgrading.md?plain=1#L97-L122

[pichlermarc]

This here is fine to unpin as it does not implement the API. 👍

[mydea]

OK, I think I kind of get it now. Took me some time and iterations to really wrap my head around this. I think what confused me additionally is that this package here had this (which, as you stated, does not need it), which is also how we stumbled over this problem in the first place. So I'll just fix this one place instead! 👍

[lforst]

@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.

[pichlermarc]

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.

[mydea]

I updated this to only unclamp this for otlp-transformer! @pichlermarc thanks for the explanations!

[pichlermarc]

Thanks 👍
Please also add a changelog entry, then we can merge this. 🙂

A side note about this situation: I agree that ideally we'd have an API setup that allows for just depending on the caret range, no matter if you're implementor or consumer. We may be able to do such a thing but we'd need to do a lot of prototyping before we can consider changing the API in such a way. There are a lot of edge-cases that come with providing a "partial" no-op (which would be the natural result of that) that can cause issues.

The spec kind of intends it to be this way and I can see the original reasoning behind that choice. However, combined with some TypeScript/NPM limitations it ends up being quite clunky and not very user-friendly.

I think we should have at least documentation about API limitations and best-practices (it was also hard to wrap my head around the topic when I joined the SIG).

[lforst]

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.

[mydea]

I added a changelog entry, I figure this is an enhancement 🤔

[trentm]

Leaving to Marc to merge. He's the "otlp-transformer" and "exporter"-king.