-
Notifications
You must be signed in to change notification settings - Fork 154
Conform attributes to v1.27 semantics for OpentelemetryOban #436
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
base: main
Are you sure you want to change the base?
Conform attributes to v1.27 semantics for OpentelemetryOban #436
Conversation
- Use the worker name as the description and transaction name instead of "{worker} process"
- Use "queue.process" as the op - this is according to the docs https://develop.sentry.dev/sdk/telemetry/traces/modules/queues/
Maybe this could be improved in the opentelemetry_oban package?
81ac160 to
191b5e9
Compare
191b5e9 to
8d6a9ad
Compare
|
Any updates on this one? |
|
@danschultzer Hello! need any helps with this? how do we this move forward? |
|
I don’t think there’s anything you can do @epinault. I’ve been waiting since December for my PRs to be reviewed, but this repo is in low maintenance mode with only one of my PR’s reviewed and merged so far. I doubt this will be reviewed anytime soon. I understand that these PR’s may require considerable time by @bryannaegele and @tsloughter to be addressed, but I’m at a point where I’ll have to fork and release alternative hex package to unblock development at work. I can ping here once rubber meet the road, probably a week or two out. |
|
Hey, folks. Just want to clear up that Tristan and I are not the codeowners of all of these libraries. We do not have the time or expertise to do everything. Please feel free to ping the codeowners for respective libraries for assistance. If you want to become an approver, see the contributing guidelines to familiarize yourself with the process. If the codeowner is unresponsive and you want to take ownership, we can discuss that, but let's start with them. |
|
Hi @indrekj could you take a look at this PR? |
indrekj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey. Sorry, I'm not really active contributing to opentelemetry any more. I probably should remove myself from the maintainers.
I did go over the PR and it seems to make sense to me. But again, I haven't worked on it quite a while.
|
@indrekj thanks for going over the PR anyway. That'd be great if we could keep the maintainers list up to date with those actively working on the applications. |
|
@danschultzer can this be marked ready for review? |
|
Wow, lots of open Oban PRs. We need someone to really maintain the Oban instrumentation library. I am not familiar with either Oban or the messaging semantic conventions so would prefer not to. I don't know if @bryannaegele is even familiar with Oban but seemed to be taking up the work anyway. It'd be great to find someone familiar with Oban who would want to become familiar with the semconv and take on getting this PRs either feedback, merged or closed. |
|
FWIW: Not an expert on the otel semantic conventions, but as a user of Oban and OpenTelemetry instrumentation, this looks good to me! |
|
Hey folks, we recently added support for OpenTelemetry to the Sentry Elixir SDK and now we'd love to get proper Oban support too. Is there anything we can do to help get this merged and released? 🙂 |
|
@solnic really we need someone to step up as a codeowner to feel confident in merging that they reviewed it fully (not just the code but that it matches the semconv too) since they'll be responsible for reviewing/releasing fixes :). It is also a draft and has conflicts. So whoever picks it up needs to open a new PR. I may do the full semconv compare myself since it is getting so many requests for merger but then I can't fix the conflicts and open the PR as I wouldn't be able to +1 it for merger. |
grzuy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danschultzer thanks for working on this!
Trying to provide my 2cents to help with some oban progress.
| [ | ||
| event( | ||
| name: "exception", | ||
| name: :exception, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already fixed separately in #558
| [ | ||
| event( | ||
| name: "exception", | ||
| name: :exception, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already fixed separately in #558
| [ | ||
| event( | ||
| name: "exception", | ||
| name: :exception, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already fixed separately in #558.
| [ | ||
| event( | ||
| name: "exception", | ||
| name: :exception, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already fixed separately in #558.
| [ | ||
| {:oban, "~> 2.0"}, | ||
| {:opentelemetry_api, "~> 1.2"}, | ||
| {:opentelemetry_api, "~> 1.4"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need to further restrict the opentelemetry_api version requirement as part of this?
| {:opentelemetry_semantic_conventions, "~> 0.2"}, | ||
| {:opentelemetry, "~> 1.0", only: [:test]}, | ||
| {:opentelemetry_semantic_conventions, "~> 1.27"}, | ||
| {:opentelemetry, "~> 1.4", only: [:test]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need to further restrict the opentelemetry version requirement as part of this?
| "messaging.destination.name": "TestJob", | ||
| "messaging.consumer.group.name": "events", | ||
| "messaging.operation.name": :send, | ||
| "messaging.operation.type": :publish, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "messaging.system": :oban, | ||
| "messaging.destination.name": "TestJob", | ||
| "messaging.consumer.group.name": "events", | ||
| "messaging.operation.name": :send, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "messaging.destination_kind": :queue, | ||
| "messaging.system": :oban, | ||
| "messaging.destination.name": "TestJob", | ||
| "messaging.consumer.group.name": "events", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means we're making the interpretation that for the latest "Semantic conventions for messaging spans"
Consumer group => Oban queue
Destination => Oban Worker
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what is worth, this other pull request (https://github.com/open-telemetry/opentelemetry-erlang-contrib/pull/528/files#r2500483804) seems to be taking a different approach and keeping the interpretation of "Destination" as the oban queue, like it is right now.
And setting the Oban Worker module atom as the messaging.client.id.


Stacked on #435 which needs to get in first. Check the latest commit for the changes here.
Follows the guideslines set out in https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/messaging/messaging-spans.md#destinations
This does introduce a bunch of breaking changes so let me know if backwards compability is required. I can keep the old attributes, but not sure if we should just go with major release instead since there's a lot of things that are different now? Same question as in #430
Changes
messaging.destination->messaging.consumer.group.namemessaging.destination.kindis removed{destination} {operation}changed to{operation} {destination}{plugin} processchanged tooban.plugin {plugin}Oban bulk insertchanged to{operation}or{operation} {destination}depending whether all are the same job typeAdded
messaging.destination.name(the worker module)messaging.operation.namemessaging.operation.typemessaging.message.idThere's also more to this to get it to be compliant with 1.27 conventions. A larger one is that we probably need to deal with creation context? https://github.com/open-telemetry/semantic-conventions/blob/v1.27.0/docs/messaging/messaging-spans.md#context-propagation
Related #367