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

Versioning question: WithTLSConfig method removed between versions 1.11.1 and 1.11.2 #3548

Open
ajhcpt opened this issue Dec 19, 2022 · 11 comments
Labels
bug Something isn't working

Comments

@ajhcpt
Copy link

ajhcpt commented Dec 19, 2022

Description

A public method https://github.com/open-telemetry/opentelemetry-go/blob/v1.11.1/exporters/otlp/internal/envconfig/envconfig.go#L94 was removed between 1.11.1 and 1.11.2. Due to updates to our project's dependencies the result was the following indirect requires:

go.opentelemetry.io/otel v1.11.2 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.11.1 // indirect

This results in the build breaking since otlptrace 1.11.1 uses envconfig.WithTLSConfig from otel which is no longer there in 1.11.2.

Is the general intention that minor/patch versions should be backwards compatible and this instance was an error, or should we ensure that we keep all the versions of the opentel dependencies in a project on the same minor/patch versions?

@ajhcpt ajhcpt added the bug Something isn't working label Dec 19, 2022
@MrAlias
Copy link
Contributor

MrAlias commented Dec 19, 2022

Your link is to a type in an internal package. We follow the Go pattern here regarding internal packages, they are not included in our stability guarantees and are intended to only be used internal to the project.

Both exported options to configure TLS still remain available and backwards compatible12. I would recommend you use those as they are covered by our stability guarantees and we ensure their use remains backwards compatible across minor and patch versions.

Footnotes

  1. https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp#WithTLSClientConfig

  2. https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc#WithTLSCredentials

@ericklaus-wf
Copy link

ericklaus-wf commented Jan 4, 2023

I believe this was still a breaking change--updating to a new version of otel is going to break some of your users. @ajhcpt mentioned this problem, but it might deserve a more thorough walkthrough. Your users don't have to use any of the internal methods (indeed, they can't) to have their builds broken by this change.

Consider the following steps in some external module.

  1. Require go.opentelemetry.io/otel@v1.11.1 and go.opentelemetry.io/otel/exporters/otlp/otlpmetric@v0.32.1. The module builds.
  2. Update to latest otel go get go.opentelemetry.io/otel@latest. otlpmetric depends on otel@v1.11.1. The version checker finds that otel@v1.11.2 satisfies that requirement and does not complain.
  3. The module no longer builds. otlpmetric@v0.32.1 expects envconfig.WithTLS to exist but now it does not.

Do you intend to require your users to update every go.opentelemetry.io module every time they update any of the modules so they can avoid breaking changes?

EDIT: Thinking more about this, if deleting exported methods from internal packages used by other modules is a practice you want to continue, perhaps you could retract versions in the other affected modules. I'm not sure when your users would be alerted immediately, but they'd be more likely to find out about the problematic versions.

@austince
Copy link

@MrAlias, would you reconsider this one? Current breaking in exactly the way described above.

@process0
Copy link

Breaking for me

@pellared
Copy link
Member

pellared commented Feb 24, 2023

Unfortunately (for the users), OTel Go shares internal packages between modules which causes problems like described.

Because of this, currently, OTel Go packages are compatible only between the versions that were released from the same commit. E.g. users should only use v1.13.0 and v0.36.0 and not any other OTel Go (non-contrib) package version - if one uses a different version then something may break because of internal dependencies.

We could address the problem e.g. by using "code generation" to "copy/paste" the common code that is shared between modules.

However, it is not something that we will address in timely manner. But maybe I am wrong and someone from this thread has time to contribute and create PR. 😉 For now I do not think we can do anything more than describing this inconvenience in VERSIONING.md

@MadVikingGod
Copy link
Contributor

MadVikingGod commented Mar 2, 2023

Yes, I think the underlying problem is that we have dependencies on the internal package of different modules.
For example go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc depends on go.opentelemetry.io/otel/exporters/otlp/internal/retry, but this is a different module. This makes the internal API a semi-public API, one that no user can consume but will break compatibility between different versions of otlptracegrpc.

To that effort, I created a script to show where we have created these dependencies.
https://gist.github.com/MadVikingGod/65903c26582c991bd2e2704b0c2bf450

@ericklaus-wf
Copy link

Until OTEL has the generators described in #3548 (comment) (or a different approach), what do you think about a policy of releasing a major version whenever there is a breaking-to-other-OTEL-modules change in an internal directory?

I've been wracking my brain on this for a couple of weeks, and I think a major-on-internal-breaks is the only policy that keeps your integrators from random breaks, even though it is some pain for both OTEL and integrators.

(As a "bonus", the policy may discourage those internally breaking changes due to the pain of a major release and the many import changes that come along with it.)

@pellared
Copy link
Member

pellared commented Mar 2, 2023

what do you think about a policy of releasing a major version

I do not think it is worth going that path. It would be a big burden for most of the users that do not have this problem as they will need to update import path (they would need to add the major version suffix).

Also I do not believe we did this intentionally and we currently do not detect internal breaking changes...

Until OTEL has the generators described in #3548 (comment)

Assigning to myself. I will start working on it in near future.

@pellared pellared self-assigned this Mar 2, 2023
@ericklaus-wf
Copy link

Awesome. Thanks!

@pellared
Copy link
Member

pellared commented Mar 2, 2023

I think this issue can be split into at least 2 issues:

  1. Remove the dependencies on the internal package of different modules (probably via code generation and documentation)
  2. Automate detection of dependencies on the internal package of different modules (to avoid such problems in future)

In the beginning, I will try to make a PoC for the first one.

@pellared
Copy link
Member

pellared commented Mar 8, 2023

I created a "parent" issue #3846 that could address this bug for future releases.

@pellared pellared removed their assignment Mar 8, 2023
gfanton added a commit to gfanton/weshnet that referenced this issue Mar 20, 2023
remove `buf` in tools that create an incompatibility with open telemetry
in kubo (yes it's really anoying)
see: open-telemetry/opentelemetry-go#3548

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
gfanton added a commit to gfanton/weshnet that referenced this issue Mar 20, 2023
remove `buf` in tools that create an incompatibility with open telemetry
in kubo (yes it's really anoying)
see: open-telemetry/opentelemetry-go#3548

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
gfanton added a commit to gfanton/weshnet that referenced this issue Mar 21, 2023
remove `buf` in tools that create an incompatibility with open telemetry
in kubo (yes it's really anoying)
see: open-telemetry/opentelemetry-go#3548

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants