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

Prepare release, include fixes with abi breaking changes #1983

Closed
marcalff opened this issue Feb 13, 2023 · 9 comments · Fixed by #2222
Closed

Prepare release, include fixes with abi breaking changes #1983

marcalff opened this issue Feb 13, 2023 · 9 comments · Fixed by #2222
Assignees
Labels
breaking change API or ABI breaking change bug Something isn't working do-not-stale

Comments

@marcalff
Copy link
Member

marcalff commented Feb 13, 2023

Prepare future releases with breaking changes.

Due to expected changes in the spec, there will be breaking changes in the API/ABI.

Due to changes in the implementation, there are also known changes in the SDK interface, which can break applications when using shared libraries.

It is desirable to group many fixes that causes breaking changes at once, so listing here all the known issues that impact API/ABI.

It is also desirable to define the overall strategy to introduce breaking changes.

API/ABI:

SDK:

@marcalff marcalff added bug Something isn't working breaking change API or ABI breaking change labels Feb 13, 2023
@lalitb
Copy link
Member

lalitb commented Feb 13, 2023

Adding few more issues in the list -

@marcalff
Copy link
Member Author

marcalff commented Feb 15, 2023

On hold until the spec change is settled:

open-telemetry/opentelemetry-specification#3186

If there is an ABI change, we may need to expose both old and new ABI at the same time, by using something similar to

#define OPENTELEMETRY_BEGIN_OLD_NAMESPACE(VER) \
  namespace opentelemetry { namespace VER {

#define OPENTELEMETRY_END_OLD_NAMESPACE \
  }}

#define OPENTELEMETRY_OLD_NAMESPACE(VER) opentelemetry :: VER

and then expose old abi (for binary compatibility) and new abi, as follows:

OPENTELEMETRY_BEGIN_OLD_NAMESPACE(v1)
... declaration for class Span in v1 goes here, code unchanged ...
OPENTELEMETRY_END_OLD_NAMESPACE

#define OPENTELEMETRY_ABI_VERSION_NO 2

OPENTELEMETRY_BEGIN_NAMESPACE
... declaration for class Span in v2 goes here, with added methods ...
OPENTELEMETRY_END_NAMESPACE

@marcalff marcalff changed the title Prepare next release, include fixes with breaking changes Prepare next release, include fixes with abi breaking changes Feb 15, 2023
@owent
Copy link
Member

owent commented Feb 16, 2023

Adding logs API changes:

@lalitb
Copy link
Member

lalitb commented Feb 22, 2023

#1884

the Logs API and SDK are still not stable, so we don't need to increment ABI version specifically for them.

@lalitb
Copy link
Member

lalitb commented Feb 22, 2023

The original intent of having OPENTELEMETRY_ABI_VERSION_NO was to increment this version whenever there is ABI breaking change, in-order to catch any ABI breaking issues as unresolved symbols during link time, and thus avoid any weird memory corruption issues at runtime. It was thought not to maintain older API versions, probably to keep the API simple and easy to maintain.

As per the otel-cpp Versioning.md document, created in response to Versioning and stability guidelines defined by Specs which was also reviewed at that time by specs contributor -

In case of ABI breaking changes, a new inline namespace version will be introduced, and the existing linked applications can continue using the older version unless they relink with newer version.

But this is good to continue discussing maintaining older versions, and there is no reason we can't change our existing guidelines for betterment.

@lalitb lalitb changed the title Prepare next release, include fixes with abi breaking changes Prepare release, include fixes with abi breaking changes (tentative: 27th March 2023) Feb 27, 2023
@lalitb lalitb changed the title Prepare release, include fixes with abi breaking changes (tentative: 27th March 2023) Prepare release, include fixes with abi breaking changes Feb 27, 2023
@owent
Copy link
Member

owent commented Mar 6, 2023

@github-actions
Copy link

github-actions bot commented May 8, 2023

This issue was marked as stale due to lack of activity.

@lalitb
Copy link
Member

lalitb commented May 25, 2023

Add #2155 to the list.

@lalitb lalitb mentioned this issue Jun 30, 2023
3 tasks
@marcalff marcalff self-assigned this Jul 5, 2023
@marcalff
Copy link
Member Author

marcalff commented Jul 7, 2023

Please comment on the linked PR #2222 for the technical discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change API or ABI breaking change bug Something isn't working do-not-stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants