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

Change convention for encoding version into package name #932

Open
trask opened this issue Aug 9, 2020 · 7 comments
Open

Change convention for encoding version into package name #932

trask opened this issue Aug 9, 2020 · 7 comments

Comments

@trask
Copy link
Member

trask commented Aug 9, 2020

Currently we only encode the version into the package name when there is more than one version, e.g.

  • io.opentelemetry.auto.instrumentation.apachehttpclient.v2_0
  • io.opentelemetry.auto.instrumentation.apachehttpclient.v4_0

but we don't encode the version if there is only one version, e.g.

  • io.opentelemetry.auto.instrumentation.apachehttpasyncclient

The idea was that the package names weren't public, so we could change them later if/when we needed a second version.

With the introduction of library instrumentation now, these package names are public, so i think makes sense to change our convention and always encode the version into the package name (for both library and auto instrumentation package names, to keep them similar).

EDIT: this way, if we need to support a new version later, we don't have to add the version to the old package name at that time.

@trask
Copy link
Member Author

trask commented Aug 15, 2020

@anuraaga @iNikem let me know if you have thoughts about this. Trying to get the various package renaming out of the way.

@anuraaga
Copy link
Contributor

I don't have a strong preference for either proactively adding the version or keep it the first version of an instrumentation unversioned and adding the suffix for newer ones (without changing the old one) when it's necessary. I guess it's trade-off of consistency in our codebase vs somewhat ugly import statements for users - I guess the latter could be considered better UX at the cost of some cognitive load on the maintainers.

@iNikem
Copy link
Contributor

iNikem commented Aug 16, 2020

I don't like it but it seems to be the necessary trade-off. If encode versions into package name, then we could as well do it from the beginning.

@trask
Copy link
Member Author

trask commented Aug 5, 2022

I think it's worth re-confirming this decision for library instrumentation in light of our more recent decision to only support a single min version for library instrumentations, and to bump that min version as needed.

Encoding the package name for library instrumentation means that we can break the API as needed when updating to a newer library version, which could be useful if the underlying instrumented library has breaking changes that affect us.

But it also means that we probably will break with core repo policy of continuing to publish stable artifacts until the next major version, e.g. if we bumped from xyzlib-2.0 to xyzlib-3.0, we would not continue to publish xyzlib-2.0, but this would require not encoding the min version in the library instrumentation artifact name either.

@mateuszrzeszutek
Copy link
Member

One more reason to encode the supported library version in the package name would be the Java Module System -- as in, it would break if 2 different jars (spring-webmvc-3.1 and spring-webmvc-5.3) containing the same classes in the same package name were to be present on the classpath at the same time. I don't think that we can exclude that scenario; some libs may include the instrumentation libraries as transitive dep, for example.

But it also means that we probably will break with core repo policy of continuing to publish stable artifacts until the next major version, e.g. if we bumped from xyzlib-2.0 to xyzlib-3.0, we would not continue to publish xyzlib-2.0, but this would require not encoding the min version in the library instrumentation artifact name either.

On the other hand, we're not really breaking anything for xyzlib-2.0 users -- their code, using the older version of the instrumentation lib, should still work properly. If we were to publish just xyzlib (and keep the "version in package name" convention) we'd probably have to include several XyzlibTelemetry classes in the library (one "real", the rest deprecated); possibly totally incompatible with each other (if the library introduced breaking API changes)

mateuszrzeszutek pushed a commit that referenced this issue Jan 19, 2023
Part of #932

I wanted to get this done before starting the spring boot starter v3
work.
trask added a commit that referenced this issue Feb 14, 2023
Another part of #932

In this PR I changed all the library instrumentation packages -- these
are breaking changes, so I figured the earlier we do this the less
painful it'll be to the users. I know that at least some of them are
actively used, so we'll need to spell this out in the release notes.

---------

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@trask trask removed the priority:p3 label Aug 23, 2023
@trask
Copy link
Member Author

trask commented Dec 22, 2024

Closing, I think this is done.

@trask trask closed this as completed Dec 22, 2024
@trask
Copy link
Member Author

trask commented Dec 23, 2024

reopening to fix remaining occurrences and to add a CI check

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

No branches or pull requests

4 participants