-
Notifications
You must be signed in to change notification settings - Fork 589
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
OpenTelemetry: Update messaging.operation span attribute to latest OTel Semantic Conventions #1715
Comments
@lukebakken sounds like a reasonable last minute 7.0 change? 😢 |
@iinuwa you are welcome to submit a PR that would rename the attribute. Whether it can ship in 7.0.0, I don't know. It sounds like the right moment to do it, since this feature is very new (in general and new in 7.x). |
That would be actually quite useful if supported built-in as some generic .net metrics. |
It's surprising that In any case, renaming a metric for 7.0 sounds acceptable. Introduction of an another metric will have to go into 7.1, you are welcome to submit PRs. |
Thanks Michael.
Yeah, I'm kind of confused too. I think
Added #1716 for changing the attribute name to |
I am pinging these people because they all reviewed #1528 - |
This is exactly the point. We want systems to have operation names that use their own terminology, but we also need unified type that's common across different systems. The latter one is useful for common dashboards, alerts and helps to move between messaging systems. There could also be multiple different operation names with the same type. E.g. Azure ServiceBus has I hear you both that it's confusing. Let me know if you have any thoughts on how to make it less confusing :) PS: will take a look at #1716 |
I'll take a look this morning, thanks! |
@lmolkova this is probably not the right place to discuss this but As for
In any case, our team understands that it's not a decision our team will probably influence a great deal, so we will follow the current naming scheme even if it leaves a bit to be desired. |
Thank for the feedback! You can actually influence our design quite a lot - this is a perfect time for it (we plan to stabilize sometime soon and after that breaking changes will have very high bar).
The When tracing backends visualize stuff, they need to know some things about these special operations to visualize them nicely. But also, if I use e.g. Azure ServiceBus, having a common name When it comes to queue/consumer deletion - these operations seems to be less of an interest when it comes to visualizations/common dashboards at least at the moment.
Do you feel that the we picked this name because it aligns well with similar notions we have for other technologies (e.g. we have
That's where things get tricky with operation types. They need to apply to most of the messaging systems. E.g. ack/nack/checkpoint/commit/completion/abandonment/dead lettering are all forms of settlement. For this one we want to unify since it's such an important part (you want to know how message processing has ended). As I mentioned above, In any case, happy to hear your thoughts |
@lmolkova for RabbitMQ specifically, high topology churn (due to questionable client library usage or design choices in applications) has been a serious enough headache over more than a decade. So declaration and deletion events are an important aspect to monitor, not just message publishing or consumption rates. These days RabbitMQ has three related metrics specifically so that operators can spot topology churning applications. Perhaps OpenTelemetry for messaging should provide a couple of well-known operation types. |
Only because to me an operation does not have a name, it has a type (or method/frame, in messaging protocol nerd speak).
The docs do not state that strongly enough.
I understand that and I've tried to not use terms that would be too specific to RabbitMQ or its original protocol. "settlement" from AMQP 1.0 covers a lot of ground but must be clarified, hence the The current naming, with the above explanation, should be enough for us to continue. But at the very least the purpose of I'm willing to provide a few examples for the first four in case you accept external contributions :) |
So my current understand is the following:
This client can work with this. Thank you for the clarification @lmolkova. |
Thanks a lot for the details @michaelklishin! I'll go through the points you raised and convert them into work items in semantic conventions later today (or if you want to do it - go ahead)
we do accept contributions and really appreciate them at https://github.com/open-telemetry/semantic-conventions - lmk if you need any help with it. |
@lmolkova I can only use examples from the five protocols supported by RabbitMQ, including AMQP 0-9-1 which one OTel "committee big shot" at would vehemently object to. If you think that's still a good idea, should I submit a PR with updates of this file specifically? https://github.com/open-telemetry/semantic-conventions/blob/14c1d79ade64a5fddccf337fc2b00e750e110c59/model/messaging/common.yaml#L18 |
Thanks for the discussion @michaelklishin and @lmolkova! I have some separate considerations Release considerationsNow that I'm looking at this a bit more, I wonder if we should just leave this as is for 7.0. I don't want to delay the release of this client, since the async and performance changes are much more important, and @lukebakken has already indicated that adding new attributes should be pushed to 7.1. I'm thinking we should defer these operation name and attribute name and value changes to 7.1 and make sure to coordinate operation names with other major libraries; at least the ones that the RabbitMQ team officially supports. A caveat: changing the operation and attribute names are technically breaking changes, since it will mess with people's dashboards. But the RabbitMQ and Messaging Semantic Conventions are marked as experimental, so perhaps it would be OK to break this in a minor RabbitMQ client version. (Maybe it'd be good to denote that in the release notes that OpenTelemetry schema in the RabbitMQ .NET client is also experimental and subject to change.) Naming thingsA quick sample of operation names for a few clients:
All of these use the old My recommendations for the names would be to use the AMQP operation names, stripping
(Alternatively, we can replace periods |
@iinuwa we will accept the changes you have submitted, they are small and bring us up-to-date with the current state of OpenTelemetry naming conventions. All other changes will have to wait until 7.1. |
@michaelklishin, OK. I'll split off the other work into a separate PR then. Thanks! |
@iinuwa by the way, the idea of stripping |
Is your feature request related to a problem? Please describe.
The Activities in RabbitMQ client currently emit the
messaging.operation
attribute. However, the Semantic Conventions for messaging currently specify usemessaging.operation.type
for the values that are currently being set there, and to addmessaging.operation.name
to values likeack
,nack
, etc.Describe the solution you'd like
At minimum, changing the
messaging.operation
attribute tomessaging.operation.type
would be helpful to comply with the conventions.Adding
messaging.operation.name
would also be helpful for tracking the number of/searching foracks
vs.nacks
in the system. I'm not sure if there's a good place to add this in this library though; perhaps it would need to be done in higher level libraries.Additional context
References #1528.
The text was updated successfully, but these errors were encountered: