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

Do not depend on internal packages of different modules #3846

Closed
pellared opened this issue Mar 8, 2023 · 9 comments
Closed

Do not depend on internal packages of different modules #3846

pellared opened this issue Mar 8, 2023 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@pellared
Copy link
Member

pellared commented Mar 8, 2023

Why

Depending on the internal packages of different modules causes being dependent module's internal API.
Therefore, a breaking change of such internal API may result in build failure for the consumer.

An example of such behavior was reported here: #3548

Below is a list of internal packages that are used across different modules:

  1. go.opentelemetry.io/otel/exporters/otlp/internal/envconfig
  2. go.opentelemetry.io/otel/exporters/otlp/internal/retry
  3. go.opentelemetry.io/otel/exporters/otlp/otlpmetric/internal/oconf
  4. go.opentelemetry.io/otel/exporters/otlp/otlpmetric/internal/otest
  5. go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal/otlpconfig
  6. go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal/otlptracetest
  7. go.opentelemetry.io/otel/internal/baggage
  8. go.opentelemetry.io/otel/internal/global
  9. go.opentelemetry.io/otel/internal/internaltest

The list was created thanks to @MadVikingGod work: https://gist.github.com/MadVikingGod/65903c26582c991bd2e2704b0c2bf450

What

To solve the issue the following should be done:

  1. Donate gocpy to opentelemetry-go-build-tools
  2. Remove the dependencies on the internal package of different modules. I suggest having a separate issue for each internal package that is reused in a different package.
  3. Document in CONTRIBUTING.md what are the possibilities to share code across modules.
  4. Automate detection of dependencies on the internal package of different modules (to avoid such problems in future)

How

For most of the packages we could copy the shared source code across modules via go:generete as proposed in #3841.

Some packages (like go.opentelemetry.io/otel/internal/global) contain a shared global state. Therefore, the approach described above will not work. These packages would require special treatment on a per use-case basis.

These packages may require exposing some API publicly or publishing an internal Go module that would have to be always backward compatible (if we are not confident about extending the public API). We could also consider exposing a minimal API (required to access the shared state) and use gocpy to generate not-exported functions. For instance, we could export otel.Logger() and generate logError, logInfo, logDebug functions.

Take notice that the gocpy approach will not fix the issue for older modules as they will still depend on the internal API.
We can keep the existing internal modules to make lives easier for the users (and mark all internal API as deprecated),
I am unsure if the maintenance cost of keeping the deprecated internal packages is worth the value (possible inconvenience that we will introduce). EDIT: @MadVikingGod suggests to both leave the current APIs where they are and depreciate them when the copied versions exist.

What is the priority of this issue? Do the maintainers see to need to have the issue solved? While I can work on addressing it, I assume it will still consume a lot of effort to review it.

@MrAlias
Copy link
Contributor

MrAlias commented Mar 9, 2023

From the SIG meeting: sounds like we would like to move forward with implementing this.

@MadVikingGod
Copy link
Contributor

Some of my thoughts on this:

  1. We should both leave the current APIs where they are and depreciate them when the copied versions exist. This prevents us from further breaking any existing installs.
  2. Not everything NEEDS to be copied, but it should be the default. That means we have to find and document some reason for not copying. This should also have a documentation notice for future developers. This solves our global use case.
  3. I think the priority of this doesn't need to be very high. It will be nice to have a real blank space for speculative work, but we are just constrained on API-level changes within our codebase until this is complete. That being said, I would start by getting gocp working with one package to keep the PR small enough.
  4. Could we call it gocp instead of gocpy?

@pellared
Copy link
Member Author

pellared commented Mar 13, 2023

@MadVikingGod I agree with all that you have written.

I think that I will start with go.opentelemetry.io/otel/internal/internaltest and first create a PR where it is used only in one package. However, before open-telemetry/opentelemetry-go-build-tools#275 is merged (and released) you can also take a look at #3841

chrisdoherty4 added a commit to tinkerbell/tink that referenced this issue May 9, 2023
The otelgrpc package depedends on the core OpenTelemetry module. The
OpenTelemetry module has a peculiar construction where independent
modules reference internal types creating a lockstep upgrade that
overflows to consuming projects.

See open-telemetry/opentelemetry-go#3846 for
tracking.
chrisdoherty4 added a commit to tinkerbell/tink that referenced this issue May 9, 2023
The otelgrpc package depedends on the core OpenTelemetry module. The
OpenTelemetry module has a peculiar construction where independent
modules reference internal types creating a lockstep upgrade that
overflows to consuming projects.

See open-telemetry/opentelemetry-go#3846 for
tracking.

Signed-off-by: Chris Doherty <chris.doherty4@gmail.com>
@MrAlias MrAlias self-assigned this Jul 31, 2023
@MrAlias
Copy link
Contributor

MrAlias commented Jul 31, 2023

I have a working branch for a partial fix of otlpmetric stuff using gotmpl. I'm going to pair it down to just adding the gotmpl tooling add split the retry package. This seems like the smallest entry point into trying to resolve this.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 1, 2023

I have a working branch for a partial fix of otlpmetric stuff using gotmpl. I'm going to pair it down to just adding the gotmpl tooling add split the retry package. This seems like the smallest entry point into trying to resolve this.

Looks like just doing the retry package was too small of an approach. I have a PR staged to decouple all of otlptrace/internal from otlp/internal. It should be a good starting point.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 1, 2023

I have a working branch for a partial fix of otlpmetric stuff using gotmpl. I'm going to pair it down to just adding the gotmpl tooling add split the retry package. This seems like the smallest entry point into trying to resolve this.

Looks like just doing the retry package was too small of an approach. I have a PR staged to decouple all of otlptrace/internal from otlp/internal. It should be a good starting point.

#4397

@MrAlias
Copy link
Contributor

MrAlias commented Aug 8, 2023

Both go.opentelemetry.io/otel/internal/baggage and go.opentelemetry.io/otel/internal/global are going to be out of scope for this issue. Both provide a unique and global value or type. That uniqueness is critical for certain features:

  • go.opentelemetry.io/otel/internal/baggage: A common baggage type needs to be able to be able to be stored and retrieved from a context.Context by both go.opentelemetry.io/otel/baggage and go.opentelemetry.io/otel/bridge/opentracing. This functionality cannot be accomplished by using different packages (the types and context keys would be different).
  • go.opentelemetry.io/otel/internal/global: This package store global unique values for loggers, error handlers, tracer providers, and meter providers. This functionality inherently cannot be accomplished by using different packages (they would not be global values then).

@MrAlias
Copy link
Contributor

MrAlias commented Aug 8, 2023

Remaining work:

@MrAlias
Copy link
Contributor

MrAlias commented Aug 11, 2023

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants