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

apmotel: embed new opentelemetry interfaces to span/trace/traceprovider (fixes #1542) #1544

Merged
merged 1 commit into from Nov 22, 2023
Merged

apmotel: embed new opentelemetry interfaces to span/trace/traceprovider (fixes #1542) #1544

merged 1 commit into from Nov 22, 2023

Conversation

jacksehr
Copy link
Contributor

From #1542:

There was a breaking change in 1.20.0 which affected custom trace API implementations. As a result, the apmotel and similar modules using otel are currently broken and stuck with older versions.
This prevents projects depending on apmotel from bumping to newer otel versions.

Details: https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.20.0

This release brings a breaking change for custom trace API implementations. Some interfaces (TracerProvider, Tracer, Span) now embed the go.opentelemetry.io/otel/trace/embedded types. Implementors need to update their implementations based on what they want the default behavior to be.

As in the description of the new embedded package:

Implementers of the OpenTelemetry trace API can embed the relevant type from this package into their implementation directly. Doing so will result in a compilation error for users when the OpenTelemetry trace API is extended (which is something that can happen without a major version bump of the API package).

Embedding these interfaces is the simplest way to eliminate all compilation errors. However, it also means we will introduce compilation errors to users of apmotel if/when OpenTelemetry extends any of the embedded interfaces.

There are other options discussed by them here, but essentially, the alternatives are to panic, or noop, when the interfaces are changed. I'm open to suggestions about why either might be preferred, but have initially just gone with their recommendation.

@jacksehr jacksehr requested a review from a team as a code owner November 22, 2023 06:45
Copy link
Member

@kruskall kruskall left a comment

Choose a reason for hiding this comment

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

Thanks!

@kruskall kruskall enabled auto-merge (squash) November 22, 2023 09:20
@kruskall
Copy link
Member

@elasticmachine run elasticsearch-ci/docs

@v1v
Copy link
Member

v1v commented Nov 22, 2023

run elasticsearch-ci/docs

@kruskall
Copy link
Member

@elasticmachine run elasticsearch-ci/docs

@kruskall kruskall merged commit 8cd11bd into elastic:main Nov 22, 2023
13 checks passed
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.

None yet

3 participants