chore(deps): pin minor API version #2531
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2531 +/- ##
==========================================
- Coverage 93.07% 93.02% -0.05%
==========================================
Files 140 136 -4
Lines 5169 5135 -34
Branches 1101 1099 -2
==========================================
- Hits 4811 4777 -34
Misses 358 358
|
Flarna
changed the title
Use ~ instead ^ range in API dependency to avoid that a semver minor API change is allowed. Semver minor API changes usually need an update in SDK therefore automatically update to a new minor would break the API contract for the end user.
Flarna
force-pushed
the
avoid-minor-api-updates
branch
from
cbb18c9
to
f3e8df3
Compare
|
To make sure I understand the situation correctly: We only need to pin the API in packages that implement a part of it to avoid breakages in case of additive changes, because those additions might require the SDKs to implement more than they currently are. |
Yes I think so. |
There was a problem hiding this comment.
I don't think we should be allowed to do that. Any SDK version 1.x should be compatible with any 1.x api version so if we release for example api 1.5.x this api should still work fine with SDK 1.0.x. This is a major promise that our sdk and api will be compatible for some long period. Changing caret to tilde will prevent SDK from getting latest api 1.x automatically. Changing ^ to ~ will break this rule in my opinion.
So unless I'm completely wrong pls correct me.
I'm requesting changes until this is discussed and better understood.
@open-telemetry/technical-committee pls elaborate if such change is even accepted, thank you
|
How should an old SDK support new APIs not know at SDK implementation time? This sounds like magic to me. Changing from ^ to ~ just means that a new SDK needs to be released to get the new API. Not automatically but instead with a working implementation. |
Where did I write that old SDK should support new API?? |
Sorry, missunderstood your message.
If the API method is optional users need some other API (or other mechanism) to find out if it is available in their setup or not. |
Optional / Noop Let say we release a new api with 2 new methods Those 2 new methods will be available in api ver 1.1.x. These 2 new methods assumes that context manager will also have those 2 new methods. But if you use api ver 1.1.x with SDK ver 1.0.x it will fail which is wrong imho. We should in api call either check for existence of this method from SDK or figure out any other mechanism (proxy for example) that will cover the missing functionality with a noop implementations so this way when you call |
|
Using Noop/Proxy could work. Personally I would prefer to get a consistent end2end setup where the complete chain just works instead of "random" gaps at some APIs. Depending on the API it may be hard to understand why something is not working as expected just because a single API (like Anyhow lets wait what others prefer regarding this. |
Imagine a simple situation. Company X created a compatible SDK with api ver. 1.0.x. They declared usage of api as |
|
The scenario above is exactly what I would not like to get. The SDK (and instrumentations) should clearly state their compatibility in dependencies/peer dependencies to get users notified as early as possible in case of incompatibilities. If users wants/have to stick on an older SDK they should also stick on an older API and on older instrumentations. Instrumentations only use the API. So they can be more relaxed in their requirements as a new minor will just add an API they don't use. So maybe I should change at least some of the packages back to use ^. |
I am trying to find out what our versioning document says on the topic and I am failing. The versioning doc says that the API and SDK are not required to move version numbers in the lockstep, but it fails to describe what the compatibility requirements are (unless it is there and I just cannot find it). I believe we need to clarify this in the versioning doc and there are a couple different ways we can do this. I will file a spec issue to get it clarified. |
|
I filed an issue to clarify this open-telemetry/opentelemetry-specification#2018 |
|
I'll bring the discussion in the thread in spec here because it's specific to JS I believe.
Two different installed API packages should not matter in this case because the globals will be shared, aren't they? |
The globals are shared within a major version and a semver check is done. In general I think the target should be to have only a single API version installed by using ranges which match in a single version. |
|
Sorry accidentally closed. I agree with @Flarna. Most users don't enable the diag logger and the behavior could be extremely confusing. From an end-user perspective, the API will appear to be working until they use some plugin which happens to use attach and they drop context. This might only happen in rare situations in production and be very difficult to track down. Having the API/SDK versioning such that it displays an error at install time is much less confusing. |
|
SDK could be ~ but I actually think explict range like |
In fact we should have only the upper bound |
Riight, the peer deps. I admit, I was still working with the old mental model. I understand @obecny's, take on doing as much of the work for users as possible - after all, we'd still be returning NOOP implementation if the API versions don't match - we might as well return some of the working functionality.
We could raise a warning if NOOP implementation was given, but it's also more(compared to returning partial impl) clear that something's wrong if none of the spans show up rather than some of them going to.
As Flarna said, that was a typo and will be fixed.
If My conclusion
I agree that mixing noop functionality into the returned global is a confusing idea.
@dyladan's explanation here was on point. Specifying the lower bound as well will give us more flexibility:
Interesting sidenote: if we'd make API additions(only relevant for API users) a patch version bump(we'd only bump minor if implementing packages would be broken), specifying the ranges would become easier:
... to me, it makes a lot of sense in this case actually, but will be confusing for semver aficionados, or anyone unfamiliar with that rule. But then again - it's just one package! |
|
Updated to use an upper bound for modules implementing API and a loose range for API users. |
| @@ -53,10 +53,10 @@ | |||
| "@opentelemetry/sdk-trace-node": "1.0.0" | |||
| }, | |||
| "peerDependencies": { | |||
| "@opentelemetry/api": "^1.0.2" | |||
| "@opentelemetry/api": ">=1.0.0 <1.1.0" | |||
There was a problem hiding this comment.
Why the explicit rejection of v1.1.0 or higher? Are we aware of breaking changes that are planned for v1.1.0 that will break with these packages?
If we do this here, should we not be doing this for all packages?
We can always release a later patch that opens up the scope...
There was a problem hiding this comment.
The SDK implements the API. So a semver minor change in API would result in a missing implementation.
There is a longer discussion in this PR which resulted in using an upper bound for modules implementing the API.
There was a problem hiding this comment.
Flarna is correct here. Without bounding on the (unreleased) minor version, it might pull in a version of the API which implements methods it doesn't know about.
There was a problem hiding this comment.
Then we should we not be doing this for all packages? which is the reason for my comment
There was a problem hiding this comment.
I see no reason. e.g. the HTTP instrumentation just uses the API. A new minor just means there is a new API not used.
These instrumentations specifiy the minimum needed and NPM will select the highest possible version.
As a result these instrumentations doesn't need to be updated if a new minor API is released. Only SDK needs to be updated - as it requires changes anyway.
You can't do just the upper bound because |
|
@obecny @vmarchaud I know you've both approved but there's been quite some discussion since then. IMO this is ready to merge but I wanted to make sure that's OK with you quickly. |
Fine for me, otherwise I would revoke it |
|
@dyladan I followed the discussion and my approve still stand so LGTM :) |
Use
~instead^range in API dependency to avoid that a semver minor API change is allowed.Semver minor API changes usually need an update in SDK therefore automatically update to a new minor would break the API contract for the end user.
Refs: open-telemetry/opentelemetry-js-api#123
Refs: open-telemetry/opentelemetry-js-api#125