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

Upgrade to OpenTelemetry API 1.30.x #25

Closed
gaeljw opened this issue Oct 6, 2023 · 12 comments · Fixed by #26 or #30
Closed

Upgrade to OpenTelemetry API 1.30.x #25

gaeljw opened this issue Oct 6, 2023 · 12 comments · Fixed by #26 or #30

Comments

@gaeljw
Copy link
Contributor

gaeljw commented Oct 6, 2023

Hello folks,

I just switched from OTEL 1.29 to 1.30 (Java) and following the deprecation of the other semconv module I added this one in my build definition but it still relies on OTEL 1.29.

I'm pretty confident it works with OTEL 1.30 but I have a rule in my build definition to enforce consistency and make sure all OTEL dependencies target the same minor version of the API (1.30 right now) as we've had surprises in the past when mixing two different minor versions (incompatibilities).

Thus I'm wondering if there's any reason not to upgrade to OTEL 1.30 in this module?

I can open the PR if you want :)

@jkwatson
Copy link

jkwatson commented Oct 9, 2023

Yes, a PR would be great! Thanks for the heads-up!

@gaeljw
Copy link
Contributor Author

gaeljw commented Oct 9, 2023

Sure, here it is @jkwatson: #26.

May I suggest setting up a tool like RenovateBot or Dependabot to get automatic PR for such cases in the future?

@jkwatson
Copy link

@jack-berg @trask should we have dependabot/renovate keeping this repo up to date with the api versioning?

@trask
Copy link
Member

trask commented Oct 10, 2023

I think yes, so new releases from this repo will always have up-to-date dependencies.

I'm not sure if that will completely solve the issue though since we may not always release from this repo after an SDK release, and I wouldn't recommend holding back on upgrading the SDK just because a new version of the semconv artifact pointing at the latest SDK hasn't been released.

@gaeljw I'm not sure if we would do this, but if we made opentelemetry-api a compileOnly dependency of the semconv artifact, would that avoid triggering your build enforcement rule?

I have a rule in my build definition to enforce consistency and make sure all OTEL dependencies target the same minor version of the API

@gaeljw
Copy link
Contributor Author

gaeljw commented Oct 10, 2023

@trask Indeed, considering how fast OTEL is moving and if there's no plan to follow closely releases of opentelemetry-api with new releases of this project, then having opentelemetry-api as compileOnly would be better for me.

I don't know is this make sense from this project POV though.

I'd say it's okay as it makes no sense that someone would use this project without having opentelemetry-api pulled from another dependency (SDK or other).

@jack-berg
Copy link
Member

How does compileOnly vs implementation change the gradle / maven mechanics of version conflict resolution? Does choosing compileOnly somehow imply a lower priority than implementation such that gradle will choose the version from elsewhere?

@trask
Copy link
Member

trask commented Oct 11, 2023

with compileOnly I think the dependency won't even show up in the pom.xml published to maven central

@gaeljw
Copy link
Contributor Author

gaeljw commented Oct 11, 2023

with compileOnly I think the dependency won't even show up in the pom.xml published to maven central

This is what I'm thinking as well but I can double check on an actual case to confirm.

For this reason, an "old" opentelemetry-api pulled by this project would not even show in the dependency tree and thus it would not trigger my "consistency check".

@jack-berg
Copy link
Member

I see. I'm fine with that. Its a bit odd because the "API" of this artifact definitely includes references to classes / interfaces from opentelemetry-api, suggesting that we should have an gradle api level dependency on opentelemetry-api. However in practice, its not useful to use semconv without the API so I think its reasonable to have a policy of BYOA (bring your own API).

If we agree, let's document update to compileOnly and document the expectation that users provide their own dependency on opentelemetry-api in the README.

@gaeljw
Copy link
Contributor Author

gaeljw commented Oct 12, 2023

with compileOnly I think the dependency won't even show up in the pom.xml published to maven central

This is what I'm thinking as well but I can double check on an actual case to confirm.

The dependency still appears in the published pom but with scope provided. This makes it "invisible" when displaying the dependency tree and makes it pass my consistency check (tested on another case in local).

@trask
Copy link
Member

trask commented Oct 12, 2023

The dependency still appears in the published pom but with scope provided.

provided scope would make sense to me, but I'm not seeing the dependency when I change it to compileOnly and run ./gradle publishToMavenLocal, how did you test this? thx

@gaeljw
Copy link
Contributor Author

gaeljw commented Oct 13, 2023

how did you test this?

I've used sbt as build tool, not Gradle, to test this. I've set the dependency scope to Provided (should be = to Gradle compileOnly) and observed the POM contains the dependency. Same happens with Maven.

This might be different with Gradle though?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants