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

Update OpentelemetryOban to use semantic conventions 1.27 #435

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danschultzer
Copy link

@danschultzer danschultzer commented Dec 15, 2024

We have to override the :semantic_conversions dep because oban requires ~> 0.2 but most otel libraries here uses 1.27.

Resolves #428

@bryannaegele
Copy link
Collaborator

@danschultzer can you please confirm you have thoroughly reviewed the Messaging Specification for any new or changed attributes that are applicable as well as any other sections? As far as I can tell this was not performed since nothing has changed in the setup for opting into experimental attirbutes which is essentially the entire messaging specification. Please refer to bandit, phoenix, cowboy, etc to familiarize yourself with the expected API.

It is not sufficient to just bump the dependency.

Thanks

@danschultzer
Copy link
Author

Yes, it's all in the followup PR #436, which adheres as much as possible to 1.27 specs for messaging.

But it'll introduce many breaking change.

This PR only focuses on updating the dependency, but all the attributes are backwards compatible so I don't think it's necessary to force the changes here?

FWIW the dependency was not locked to 0.2, but allowed the range >= 0.2 and < 1.0, and most of the current libraries allows >= 1.27 and < 2.0.

@danschultzer
Copy link
Author

Also one thing on all my PRs, I can update the changelog if it's preferred to do in the PR's. I'm not sure how the releases are managed.

@bryannaegele
Copy link
Collaborator

Right, 1.27 does introduce a lot of breaking changes which is why we want it done properly and not just publishing a version with an updated dep with no actual updates. By going to 1.27 we are saying it is 100% 1.27 compliant to our knowledge. That's why this is a difficult update and folks have to exercise some patience. It's the most difficult update we'll have to make and hopefully the last for a long time.

@danschultzer
Copy link
Author

danschultzer commented Dec 28, 2024

I see, but then do we need to fixate the semantic conventions version to 1.27? Because it's at 1.29 now with more breaking changes, so whenever the semconv library gets updated we end up in the same place with drift if we continue using the ~> version requirement.

I was thinking of using this PR in a minor release to bump the semconv version (we could make the version here be ~> 0.2 or ~> 1.27 to really make it backwards compatible), and have a major release where all the breaking changes are included to conform it to 1.27 guidelines.

It gets the current dependency conflict out of the way without also breaking everything for the end-user (but they will get deprecation warnings which I think is ok).

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

Successfully merging this pull request may close these issues.

Incompatibility betweeen opentelemetry_oban and opentelemetry_bandit
2 participants