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

Messaging: Remove messaging.source.* attributes #100

Merged
merged 14 commits into from
Jun 29, 2023

Conversation

joaopgrassi
Copy link
Member

@joaopgrassi joaopgrassi commented Jun 9, 2023

Port from open-telemetry/opentelemetry-specification#3450
CC @lmolkova

Fixes open-telemetry/opentelemetry-specification#3196

Changes

  • Removes messaging.source namespace. Destination and source namespaces are defined in ECS and unrelated to messaging. Removed to allow attribute reuse across signals (e.g. logs), to decrease confusion with ECS source, and resolve some usability issues with source/destination
  • Not removing messaging.destination because it's widely used in messaging
  • Introduces destination_original: in some cases when a message is routed, consumer also knows the original destination and the destination it received a message from. With messaging.source removed, we need another namespace for the original destination.

@joaopgrassi joaopgrassi requested review from lmolkova and pyohannes June 9, 2023 10:32
@joaopgrassi joaopgrassi requested review from a team June 9, 2023 10:32
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!

@trask
Copy link
Member

trask commented Jun 13, 2023

are there other options that might make sense besides .destination_original?

I'm wondering from the perspective of reserving _original for when an attribute was normalized, e.g. #17

@pyohannes
Copy link
Contributor

are there other options that might make sense besides .destination_original?

I'm wondering from the perspective of reserving _original for when an attribute was normalized, e.g. #17

What about destination.original? It would reduce messaging top-level namespaces, however, it feels a bit weird to have the original destination (formerly known as "source") as a sub-namespace of the actual destination.

@Oberon00
Copy link
Member

Oberon00 commented Jun 14, 2023

Are we even sure there are really practical cases where the original destination can be set? If not, or if we think it's not that important, I would suggest to remove the original-attributes from this PR (so that it actually does just what the title says) and create a follow-up issue to do it on-demand when we have a concrete use case.

To clarify: This is just a suggestion since it seems we are running into a bikeshedding hurdle with these attributes. Otherwise I have nothing against them.

@lmolkova
Copy link
Contributor

lmolkova commented Jun 14, 2023

are there other options that might make sense besides .destination_original?
I'm wondering from the perspective of reserving _original for when an attribute was normalized, e.g. #17

What about destination.original? It would reduce messaging top-level namespaces, however, it feels a bit weird to have the original destination (formerly known as "source") as a sub-namespace of the actual destination.

we have other attributes in the destination_original namespace which are the same as ones in destination, but it's a great question if we need them all.

Using . instead of '_' might be misleading - Prometheus replaces all dots with underscores and it might be a source of confusion. So I suggest using a different name for the original one.

@dpauls could you suggest a name based on the existing terminology, which is not "original"?

@dpauls
Copy link

dpauls commented Jun 15, 2023

The sort of use case where I think the message's original destination (published destination, producer destination?) is useful is when the broker maps a message's destination onto a consumer destination.

From an client application's point of view, this is easily illustrated with JMS:

Note in particular the documentation for Message.getDestination() says specifically:

When a message is received, its JMSDestination value must be equivalent to the value assigned when it was sent.

This is specifically highlighting that it may not match the MessageConsumer's destination.

JMS Providers where it would be common for these destinations to differ would be providers like Solace PubSub+, IBM MQ, Tibco EMS and AMQP. In the case of JMS over AMQP, they've documented the details of how to carry the JMSDestination in the message's x-opt-jms-dest message annotation. They state that if it isn't present (and the to field of properties is also not present) only then does a message's JMSDestination revert to match the Destination used to create the MessageConsumer.

Rabbit MQ also has this same concept where a message is sent to an (exchange, routing key), which directs the message to one or more queues for consumption. When a RabbitMQ Consumer receives a message in handleDelivery, it receives an Envelope, from which the message's exchange and routing key can be obtained.

I'll also point out that the originally published destination often contains particularly useful information about the message itself. In a loosely coupled Pub/Sub messaging system, the destination the consumer is created with would often be something rather static, perhaps the name of an application using the queue. It's the destinations the messages were sent to that really tell you about the message itself.

Given that standards such as AMQP and JMS have separated these two concepts, and I've listed several messaging systems that really do this, I think it would be beneficial to include this information in the conventions.

I liked the original terminology of "source" and "destination" the best. If this does not align with ECS, and such alignment is important for this group, then destination_original is "good enough" from my point of view. Other possibilities I can think of are hardly much of an improvement (destination_publish[ed], destination_sent). I'd be open to any other naming suggestions but the worst of all worlds is each messaging system that has the concept uses a different name for the same concept.

I don't see much value in removing it. It is optional to begin with and would only be included for messages where it differs from the consumer's destination, so there's no implementation burden for messaging systems where it wouldn't apply.

@nicholasdgoodman
Copy link

Hello, not to :neckbeard: and continue to bikeshed the conversation, and not fully aware of how much is planning on being re-worked in the ECS alignment effort, but I did want to jump in and second the notion that we could use a different terminology other than _original that is more consistent with the existing OTel semantic conventions.

Currently there exist well-defined messaging operations: publish, receive, and process, and the namespace in question is the clearly the "destination of the publish operation", which as we have pointed out is often different in routed messaging systems than the destination from which the message is received.

Since we already use the terminology publish (vs send or otherwise), perhaps we could name this attribute in some self-consistent way, e.g. publish_destination, destination_publish, or some other mashup of "publish" and "destination"?

@joaopgrassi
Copy link
Member Author

joaopgrassi commented Jun 16, 2023

Thanks @dpauls for the research that was great!. I read most of things and I think it's irrefutable that we have use cases for it, so I change my opinion to just remove it and agree with @dpauls we should have it. It's optional anyway.

About naming: I don't know if it's just me but the "destination" always throws me off when we talk about the "original place the message came from" (aka producer). It takes some minutes for my brain to catch up that we are talking about the producer.

Given this, I was then thinking if the suggestions from @nicholasdgoodman @dpauls to include "publisher/producer" on it wouldn't make it clearer we are talking about the producer while also staying away from conflicting with ECS/Other OTel semconv. We could name the namespace then as destination_publish.

I also think we need to adapt this sentence, because I think it's very confusing:

Note: Because messages could be routed by brokers, the destination a message is consumed from does not always match the destination it was published to. If information about the original destination is available on the consumer side, consumer instrumentations SHOULD populate the attributes about the original destination under the messaging.destination_original namespace.

To

Note: Because messages could be routed by brokers, the destination a message is consumed from does not always match the destination it was published from. If information about the publish destination is available on the consumer side, consumer instrumentations SHOULD populate the attributes about the publishdestination under the messaging.destination_publish namespace.

@Oberon00
Copy link
Member

so I change my opinion to just remove it and agree with @dpauls we should have it.

I think nobody was arguing against having it, at least I was just suggesting to split it to a new PR, since the main change of this PR is something different.

@joaopgrassi
Copy link
Member Author

@Oberon00 ah sorry, that wasn't clear. We discussed in our meeting yesterday and we couldn't come up with use cases so we decided to leave them out until a clear one appeared. But that now changed after @dpauls research.

@pyohannes
Copy link
Contributor

Thanks @dpauls for the research that was great! I read most of things and I think it's irrefutable that we have use cases for it, so I change my opinion to just remove it and agree with @dpauls we should have it. It's optional anyway.

I recommend to proceed like this:

  1. Adapt this PR to remove messaging.source attributes and references to the "source" term in the messaging semantic conventions.
  2. Submit an issue for coming up with an alternative name. The issue should summarize the discussion in this PR and should be a place where further discussion shappen.
  3. Submit a new PR based on the discussion results in the issue.

There's agreement that messaging.source as a name should be removed, as "source" is a very generic term not widely used in messaging contexts, furthermore it conflicts with ECS. I think it's worth the extra step to remove those attributes from the messaging conventions, as to my knowledge they aren't used at all, and we are very confident that we don't want to use the "source" terminology.

Naming discussions can take quite some time, and I'd like to avoid having "zombie" terminology lingering around while those discussions are going on. Also, we gain some flexibility in triaging the issue for coming up with an alternative name if we remove references to "source" from the conventions beforehand. Removing the "source" terminology must happen before stabilizing messaging conventions, however, introducing an alternative term can well happen after stabilization.

@joaopgrassi joaopgrassi changed the title Messaging: remove source to better align with ECS Messaging: Remove messaging.source.* attributes Jun 19, 2023
@joaopgrassi
Copy link
Member Author

I modified the PR to remove source.* entirely from the YAML definitions and from the markdown file. I also removed the Consumer Attributes section since there's no attributes there anymore. We can re-add them once we agree on a name for the "destination original".

As requested, I opened #123 to discuss the new name for the namespace.

@joaopgrassi joaopgrassi requested a review from lmolkova June 19, 2023 15:19
@joaopgrassi joaopgrassi requested a review from Oberon00 June 19, 2023 15:19
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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

Successfully merging this pull request may close these issues.

Consider impact of ECS attribute alignment on the messaging semantic convention stability effort
9 participants