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

chore(deps): pin minor API version #2531

Merged
merged 13 commits into from
Oct 27, 2021

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Oct 12, 2021

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

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #2531 (83f9950) into main (5de52ee) will decrease coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 83f9950 differs from pull request most recent head e4d3b0e. Consider uploading reports for the commit e4d3b0e to get more accurate results

@@            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              
Impacted Files Coverage Δ
...s/opentelemetry-core/src/platform/node/sdk-info.ts 0.00% <0.00%> (-100.00%) ⬇️
...opentelemetry-core/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
...emetry-core/src/platform/node/RandomIdGenerator.ts
...pentelemetry-core/src/platform/node/environment.ts
...ntelemetry-core/src/platform/node/hex-to-base64.ts
...opentelemetry-core/src/platform/node/timer-util.ts

@Flarna Flarna changed the title chore(deps): pin mainor API version chore(deps): pin minor API version Oct 12, 2021
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 Flarna force-pushed the avoid-minor-api-updates branch from cbb18c9 to f3e8df3 Compare October 12, 2021 05:55
@rauno56
Copy link
Member

rauno56 commented Oct 12, 2021

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.

@Flarna
Copy link
Member Author

Flarna commented Oct 12, 2021

o 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 could be the one or the other package here where pinning would be not needed.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Flarna
Copy link
Member Author

Flarna commented Oct 12, 2021

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.

@obecny
Copy link
Member

obecny commented Oct 12, 2021

How should an old SDK support new APIs not know at SDK implementation time? This sounds like magic to me.

Where did I write that old SDK should support new API??
I wrote that the old SDK should still be compatible with api. This should be true if the major version is the same. Having said that it means whatever change we do in api 1.x it has to remain compatible.
If we introduce a new api function this new function should be imho optional so that newer api can be used with older sdk as long as the major version matches.

@Flarna
Copy link
Member Author

Flarna commented Oct 12, 2021

Where did I write that old SDK should support new API??

Sorry, missunderstood your message.

If we introduce a new api function this new function should be imho optional so that newer api can be used with older sdk as long as the major version matches.

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.
Or do you mean there should be a noop implementation in API which is potentially overriden by SDK. So from API point of view it's always present.

@obecny
Copy link
Member

obecny commented Oct 12, 2021

Or do you mean there should be a noop implementation in API which is potentially overriden by SDK. So from API point of view it's always present.

Optional / Noop

Let say we release a new api with 2 new methods attach and detach - exactly the case we will have shortly -> open-telemetry/opentelemetry-js-api#123

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 context.attach it will not break. Maybe just warn that attach is not implemented in current version of context manager that is being used and that's it. Probably something to be discussed - the sooner the better.

@Flarna
Copy link
Member Author

Flarna commented Oct 12, 2021

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 context.attach()) is not implemented but an instrumentation relies on it.

Anyhow lets wait what others prefer regarding this.

@obecny
Copy link
Member

obecny commented Oct 12, 2021

Personally I would prefer to get a consistent end2end setup where the complete chain just works instead of "random" gaps at some APIs.

Imagine a simple situation. Company X created a compatible SDK with api ver. 1.0.x. They declared usage of api as ^1.x.x to make sure all they clients will get all updates etc. patches fixes etc. Now we released ver. 1.1.x and we have just broke their SDK. So this is not even about our SDK where we have control and we can release it fix it etc. or limit which version of api it should use (using tilde). But any1 that depends on our api should not afraid of upgrading to latest compatible major version when implementing his own SDK. I agree that I would like not to have gaps but I think this is inevitable once we declared api to be stable and a long term support / backward compatibility.

@Flarna
Copy link
Member Author

Flarna commented Oct 13, 2021

The scenario above is exactly what I would not like to get.
In my opinion it's a bug in the SDK to pretend compatibility to an API which is actually whether implemented nor tested.

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

@tigrannajaryan
Copy link
Member

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

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.

@tigrannajaryan
Copy link
Member

I filed an issue to clarify this open-telemetry/opentelemetry-specification#2018

@rauno56
Copy link
Member

rauno56 commented Oct 18, 2021

I'll bring the discussion in the thread in spec here because it's specific to JS I believe.

@obecny wrote: and after this change (why all this concern has been raised) -> #2531 you might endup having 2 plugins that will depend on different api because one will be 1.0.x and other 1.1.x and because they will have ~ they will accept only pinned minor version

Two different installed API packages should not matter in this case because the globals will be shared, aren't they?

@Flarna
Copy link
Member Author

Flarna commented Oct 18, 2021

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.
For example if a global TracerProvider was installed via API 1.0.0 and a user requests a Tracer via API 1.1.0 it will return a NoopTracer.
If a global TracerProvider is installed via API 1.1.0 and a user requests a Tracer via API 1.0.0 it will get a full Tracer from this TracerProvider.

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.

@dyladan dyladan closed this Oct 19, 2021
@dyladan dyladan reopened this Oct 19, 2021
@dyladan
Copy link
Member

dyladan commented Oct 19, 2021

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.

@dyladan
Copy link
Member

dyladan commented Oct 21, 2021

SDK could be ~ but I actually think explict range like >=1.0.0 <1.2.0 would be best that way we can make sure you get patch updates for all supported minor versions instead of just the latest one and SDK can support old APIs if needed.

@obecny
Copy link
Member

obecny commented Oct 21, 2021

SDK could be ~ but I actually think explict range like >=1.0.0 <1.2.0 would be best that way we can make sure you get patch updates for all supported minor versions instead of just the latest one and SDK can support old APIs if needed.

In fact we should have only the upper bound
<1.2.0 only, by default the SDK 1.99 should still work fine with api 1.0.0 right, that is the promise we do ?
the lower bound should be always 1.0.0

@rauno56
Copy link
Member

rauno56 commented Oct 22, 2021

peer dependency will not install the api, you in yr my package@1 have to decide which api you want to install so you can still install api ver 1.1.

Riight, the peer deps. I admit, I was still working with the old mental model.
However, it doesn't change my point much - with the current solution, context manager(SDK) could still be made to look like it supports 1.3.0.

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.
However, I think there's a chance for a lot of confusion and debugging issues if we return partially implemented APIs.

How do you want to know that ? Will you raise some warning or ?

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.

In this PR the pinned version is done also for instrumentation, for me it is a main blocker

As Flarna said, that was a typo and will be fixed.

Imagine a simple thing you just want to release a new version you do npm i and you can't restart because it gave you an error. So what option you have return noop and warn ?.

If npm i fails the app will restart fine because node_modules were left unchanged.
If the situation you have in mind is when the end users semver restrictions are loose enough so that one of the instrumentation's peer dep got bumped without making any changes to package.json then yes, it might be confusing for people who do not ship their lock file.


My conclusion

noop object or noop for only missing functionality

I agree that mixing noop functionality into the returned global is a confusing idea.

versioning...

@dyladan's explanation here was on point. Specifying the lower bound as well will give us more flexibility:

  • ~ is not accurate because in some cases SDK can be compatible with older versions as well(if newer versions didn't add any implementable API).
  • range >= 1 < 1.2 will give more flexibility for the instrumentation libs to be used alongside a the SDK. If we just use ~ unneeded conflicts are more frequent.

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:

  • users would use ^.
  • SDKs would use ~.

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

@Flarna Flarna marked this pull request as ready for review October 23, 2021 20:34
@Flarna
Copy link
Member Author

Flarna commented Oct 23, 2021

Updated to use an upper bound for modules implementing API and a loose range for API users.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

package.json Show resolved Hide resolved
@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should we not be doing this for all packages? which is the reason for my comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dyladan
Copy link
Member

dyladan commented Oct 25, 2021

SDK could be ~ but I actually think explict range like >=1.0.0 <1.2.0 would be best that way we can make sure you get patch updates for all supported minor versions instead of just the latest one and SDK can support old APIs if needed.

In fact we should have only the upper bound <1.2.0 only, by default the SDK 1.99 should still work fine with api 1.0.0 right, that is the promise we do ? the lower bound should be always 1.0.0

You can't do just the upper bound because <1.2.0 is still satisfied by 0.16.0 and <2.1.0 in the future would still be satisfied by 1.5.0

@vmarchaud vmarchaud requested a review from MSNev October 26, 2021 19:20
@dyladan
Copy link
Member

dyladan commented Oct 26, 2021

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

@obecny
Copy link
Member

obecny commented Oct 27, 2021

@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

@vmarchaud
Copy link
Member

@dyladan I followed the discussion and my approve still stand so LGTM :)

@dyladan dyladan merged commit fa0cb72 into open-telemetry:main Oct 27, 2021
@Flarna Flarna deleted the avoid-minor-api-updates branch October 27, 2021 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants