-
Notifications
You must be signed in to change notification settings - Fork 125
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
Incompatibility betweeen opentelemetry_oban and opentelemetry_bandit #428
Comments
We're seeing the same issue between
|
|
I’ve opened this PR to resolve this: #435 |
Hello @bryannaegele and other contributors! I hope you're doing well! I wanted to reach out regarding the workaround you suggested with We're in a bit of a bind at the moment, as we're relying on the updates to these dependencies. The PRs targeting the upgrade of cowboy and oban to semantic conventions 1.27 are still awaiting review, and this has been holding us back for a few weeks now: We understand everyone has a lot on their plate, especially around this time of year, and we truly appreciate all the efforts being made. If there's anything we can do to help expedite the process, please let us know. It would be a wonderful gift to wrap up this year! Happy holidays! 🎄 |
@davidjulien I'd like to get them merged but it requires a deep review of the spec to ensure the PR complies with the spec. I will drop a note to the PR authors to ask but these updates to 1.27 usually involve quite a bit more work than just bumping the dependency and calling it a day. We try to rely on the codeowners to review PRs to their libraries which in this case are @whatyouhide and @tomtaylor for Broadway and @indrekj for Oban. Tristan and I just do not have the capacity to review every PR in a timely manner. I'm in the same boat as you for oban so I will get to it at some point if it doesn't get done earlier. |
I've a lot of the 1.27 spec work for Oban done in #436 but there's more to it, and requires deeper review. I've asked some questions there and laid out all the changes. I'm not sure if it's necessary to hold up fixing dependency conflicts as it seems there's backwards compability in 1.27 for 0.2? You'll just get deprecated warnings. |
We made the decision that updating to 1.27 will require breaking changes and just publishing updated versions that spew warnings isn't sufficient so let's just get it done with. The suggestion is something we evaluated and decided against at the time both for the volume of warnings that would be emitted and users seeing 1.27 in the dependencies but nothing matches the spec. I understand it is frustrating. |
Ah ok. When I get a moment I'll go over #436 again to see what's missing. My goal was to only touch the attributes that were incorrect and fixing the span name to not make the PR too heavy. IIRC there are other new attributes that probably can be added, and we'll also need to get rid of the oban specific attributes, along with adding creation context. |
Thank you for your explanations! |
Describe the bug
I have this on my
mix.exs
Then when I run
mix deps.get
I'm getting (formatted for better reading):Expected behavior
I'd like to use both
bandit
andoban
at the same time.The text was updated successfully, but these errors were encountered: