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

Add rationale and description of the release and versioning strategy. #2212

Closed

Conversation

jkwatson
Copy link
Contributor

@jkwatson jkwatson commented Dec 7, 2020

@codecov
Copy link

codecov bot commented Dec 7, 2020

Codecov Report

Merging #2212 (4ab81fe) into master (b478493) will increase coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2212      +/-   ##
============================================
+ Coverage     85.51%   85.72%   +0.21%     
  Complexity     1831     1831              
============================================
  Files           210      210              
  Lines          7207     7189      -18     
  Branches        812      811       -1     
============================================
  Hits           6163     6163              
+ Misses          730      712      -18     
  Partials        314      314              
Impacted Files Coverage Δ Complexity Δ
.../main/java/io/opentelemetry/api/OpenTelemetry.java 100.00% <0.00%> (ø) 15.00% <0.00%> (ø%)
...emetry/context/propagation/ContextPropagators.java 100.00% <0.00%> (ø) 2.00% <0.00%> (ø%)
...ava/io/opentelemetry/api/DefaultOpenTelemetry.java 98.30% <0.00%> (+6.24%) 17.00% <0.00%> (ø%)
...in/java/io/opentelemetry/sdk/OpenTelemetrySdk.java 100.00% <0.00%> (+7.22%) 10.00% <0.00%> (ø%)
...context/propagation/DefaultContextPropagators.java 100.00% <0.00%> (+53.33%) 4.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b478493...4ab81fe. Read the comment docs.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Seems ok to me, if not overspecified. 😁 . It could be nice to have an assumption about everything else being intended to improve compatibility and foster collaboration and explicitly not to stifle/slow development.

No other real feedback/concerns.

- Once the API for a given signal (spans, logs, metrics, baggage) has been officially released, that API module will
function, *with no recompilation required*, with any SDK that has the same major version, and equal or greater minor or patch version.
- For example, libraries that are instrumented with `opentelemetry-api-trace:1.0.1` will function with
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through https://github.com/open-telemetry/oteps/pull/143/files I couldn't find a point on this so I guess it's just #2203 :)

Given using a bom is easy and we can strongly encourage it, I don't know if we need to require this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are you saying we don't need to require? Shouldn't an 1.x API work with any 1.y SDK as long as y >= x?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's my suggestion - it's not unreasonable to require x and y to match up, it's one of the reasons to publish BOMs. Brave, Armeria, gRPC are all projects I know that expect using the BOM to prevent dependency conflicts (just had to work through an issue yesterday because of not using gRPC bom :) ), I don't think we have a reason we need to be stricter here either. It would allow sharing internal APIs between the two, and have less worry of e.g., adding methods to interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, if there is a library that is instrumented with API v. 1.0.0, that would mean you can't use SDK v. 1.1.0 with it? Doesn't that defeat the entire purpose of ABI compatibility, let alone API compatibility? This seems to directly contradict what you're saying about SDK ABI requirements!

Or else, we're talking at cross purposes about something!

Copy link
Contributor

Choose a reason for hiding this comment

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

@anuraaga this statement, as written here, MUST be true. If my app has a dependency on a library, which is instrumented (meaning was compiled against) API v1.0.1, then it MUST work if offered SDK v1.11.33 during runtime.

I think what you want to fix in writing is "should we allow having different versions of API and SDK during runtime". E.g. is it OK for application developer to specify in their build.gradle SDK v1.11.33 and API v1.0.1? I agree with Anuraag here that we should discourage that.

But! What if an application developer has SDK v1.11.33 and some library depends on API v1.20. What will SDK do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What will it do will obviously depend on the nature of the changes. If they happen to be backward compatible, then things should work. And, if they aren't, then probably all hell will break loose and NoClassDefFoundErrors will abound at runtime, unfortunately.

Can we maybe detect versions at startup and fail fast if there are likely to be compatibility issues?

- We call this requirement the "ABI" compatibility requirement for "Application Binary Interface" compatibility.
- SDK Stability:
- Public portions of the SDK (constructors, configuration, und-user interfaces) must remain backwards compatible.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these first two points aren't unique to SDK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking this is a lesser requirement than the API requirements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't quite follow - how are they lesser? They read the same as API requirements to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably need to update this to be more clear. I think rather than "public", we need to provide a way to specify what pieces of the SDK are under this backwards-compatibility requirement and what isn't. Unless we put the whole SDK into one package, I think it will be hard to make sure that all public classes/interfaces are fully supported.

- Public portions of the SDK (constructors, configuration, und-user interfaces) must remain backwards compatible.
- Internal interfaces are allowed to break.
- Strict ABI compatibility is not required.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't be allowed right? I think many SDK classes are still too important (OpenTelemetrySdk.Builder for example, span processors, sampler) to risk dependency hell by breaking ABI. Imagine a custom sampler someone published, we can't break it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is worth having a discussion about. I'm pretty hesitant to say we're going to maintain ABI compatibility across the entire swath of the SDK at this point, given how youthful it is. A custom sampler should be something that it is easy for an SDK user to update, correct? It's not going to be backed into a library or instrumentation, is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It requires the sampler to also update to the newer SDK first, something we shouldn't force post-1.0 for SDK either. For whatever reason, it might be slow to update, or may be abandoned but still useful - the moment we create a situation where a user can't upgrade OTel because they have a dependency linked to the SDK, which IMO is quite valid), then we lose most of the value of semver in the first place. If we didn't, I'd say we'd need to list even trace as sdk-trace-experimental, we need the experimental flag to reflect whether we are going to introduce dependency hell on users or not.

Another example is Spring, which will be using the SDK classes. If our SDK is 1.0, we should be in a state where we don't cause version conflicts between Spring and OTel (Spring 4.2.3 requires OTel SDK 1.3.4, yikes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So... what SDK APIs are going to be locked in stone forever? How do we distinguish one public SDK API from another? I feel like if we're going to require strict ABI compatibility with some subset of the SDK, we need a way to publish that module separately, as something like opentelemetry-sdk-api or something. Any ideas on how to achieve this goal?

Copy link
Contributor

Choose a reason for hiding this comment

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

All of it I think - the reason we have any public methods at all is because we expect code outside our control to use them. As soon as that happens, we need to commit to those public methods, or we'll cause conflicts - any library that causes conflicts becomes unpopular fast. I don't see anything special about the SDK vs the API in this regard.

We should be as diligent as we can by 1.0 to get things into a good place - we probably won't get everything and we'll have methods deprecated until the next major, etc. It is what it is, but it's safer than introducing ABI breaks that could cause dependency locks for users.

Copy link
Contributor

Choose a reason for hiding this comment

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

First, I agree with Anuraag, that SDK should have stable public interface. With the same API/ABI compatibility guarantees as API.

Second, as I read it, OTEP 143 allows as to stabilise SDK separate from API. Thus we don't have to claim SDK stability yet. We can just release stable API and continue working on SDK.

Third, if we are going to provide stability guarantees for SDK, we have to distinguish public and internal parts of it. And guarantee only public parts. How to achieve that: separate packages, separate artifacts, JPMS, OSGI - is a separate question :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, after consideration and discussion, I also agree with this. I don't know how we're going to achieve it, but I also think it's important.

Ideas on how to effectively version the various parts of the project independently are very welcome!

Copy link
Member

Choose a reason for hiding this comment

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

Ya, being able to release breaking SDK 2.0 without having to bump API to 2.0 is nice

I think we have similar issue in instrumentation repo, e.g. being able to release grpc library instrumentation 2.0 without having to bump opentelemetry-instrumentation-api.

This will require a bunch more release infra to manage versioning/releasing modules independently (and using prefixed tags, e.g. api-1.1, sdk-2.0).

@jkwatson
Copy link
Contributor Author

closing in favor of #2250

@jkwatson jkwatson closed this Dec 22, 2020
@jkwatson jkwatson deleted the document_release_versioning branch June 1, 2021 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants