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 Obans semantic conventions #400

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

joshk
Copy link

@joshk joshk commented Oct 25, 2024

This also required the removal of messaging_destination_kind() as I couldn't find an equivalent in opentelemetry_semantic_conventions ~> 1.27

Copy link

linux-foundation-easycla bot commented Oct 25, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@joshk
Copy link
Author

joshk commented Oct 28, 2024

@tsloughter, sorry to ask. Would it be possible to get a review on this PR?

@joshk
Copy link
Author

joshk commented Nov 2, 2024

Hi @bryannaegele , sorry to ask, would it be possible to get approval for the CI to run for this PR?

@bryannaegele
Copy link
Collaborator

Sure. Approving to run. Just took a brief glance (not a review) and there look to be a number of issues still to address with the spec. Please take a pass through the spec to cover your bases.

https://opentelemetry.io/docs/specs/semconv/messaging/messaging-spans/

@joshk
Copy link
Author

joshk commented Nov 6, 2024

Hi @bryannaegele , I've updated the code based on the spec, and adjusted the test suite to match.

Could you please approve the CI run once more?

@joshk joshk force-pushed the update-obans-semantic-conventions branch from 02bff19 to 2ada0a1 Compare November 7, 2024 04:03
@joshk
Copy link
Author

joshk commented Nov 11, 2024

@bryannaegele sorry, is there anything i can do to help this move forward?

@bryannaegele
Copy link
Collaborator

Hey @joshk, it can take a couple of weeks for me to have time to review a PR. From another quick glance, it looks like there's still a fair amount that doesn't match the spec. A simple one is the span names. There are also updates since the original implementation around batching, so create and process may be more appropriate for operations.

If you can read through the spec top to bottom and go through the requirements one at a time, that can help move things forward.

@joshk
Copy link
Author

joshk commented Nov 11, 2024

Hey @bryannaegele , I'm sorry to ping you, and I appreciate the guidance.

Hmmm, I went through the spec and thought I had everything up to date. I'll go through everything again. Could you point me to an example of something I haven't implemented correctly? (It would help me see where I'm not reading the spec, if that makes sense)

I'm very happy to contribute further, once I'm getting this right :)

@bryannaegele
Copy link
Collaborator

I pointed out span names as one example. My recommendation is open the spec page and view it as a check list going from top to bottom, applying anything that's relevant to how oban works

@aloukissas
Copy link

How can I help get this through?

@danschultzer
Copy link

I missed this PR, but I have been updating Oban to 1.27 conventions in #436. There's more to it as well, e.g. creation context. Also I want to track everything from Oban with #437 because otherwise you need custom sampler due to all the stray Ecto traces.

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.

4 participants