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 source to better align with ECS #3450

Closed
wants to merge 3 commits into from

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Apr 27, 2023

Fixes #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.

@lmolkova lmolkova changed the title Messaging: remove source and other minor alignments with ECS Messaging: remove source to better align with ECS Apr 28, 2023
@lmolkova lmolkova marked this pull request as ready for review April 28, 2023 05:38
@lmolkova lmolkova requested review from a team April 28, 2023 05:38
brief: 'Semantic convention for attributes that describe messaging source on broker'
brief: >
Semantic convention for attributes that describe destination messages were originally published to.
These attributes should be used on the consumer side when original destination is available and different than destination message are consumed from.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could add some clarification the "original destination" refers to the destination the message was originally published to?

Comment on lines +15 to +18
messaging.source.name: messaging.destination.name
messaging.source.template: messaging.destination.template
messaging.source.temporary: messaging.destination.temporary
messaging.source.anonymous: messaging.destination.anonymous
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a nit, but I wonder what the desired outcome would be in case both messaging.source.* and messaging.destination.* attributes were specified on consumer side.

Copy link
Member

Choose a reason for hiding this comment

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

We will remove these from here right? From what I remember from our last SIG meeting.

Comment on lines +15 to +18
messaging.source.name: messaging.destination.name
messaging.source.template: messaging.destination.template
messaging.source.temporary: messaging.destination.temporary
messaging.source.anonymous: messaging.destination.anonymous
Copy link
Member

Choose a reason for hiding this comment

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

We will remove these from here right? From what I remember from our last SIG meeting.

@@ -31,6 +31,10 @@ groups:
prefix: messaging.destination
type: attribute_group
brief: 'Semantic convention for attributes that describe messaging destination on broker'
note: >
Destination attributes SHOULD be set on publish, receive, or other span describing messaging messaging operation
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Destination attributes SHOULD be set on publish, receive, or other span describing messaging messaging operation
Destination attributes SHOULD be set on publish, receive, or other spans describing messaging operations.

@@ -31,6 +31,10 @@ groups:
prefix: messaging.destination
type: attribute_group
brief: 'Semantic convention for attributes that describe messaging destination on broker'
note: >
Destination attributes SHOULD be set on publish, receive, or other span describing messaging messaging operation
if it operates with single message or if the attribute value applies to all messages in the batch.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if it operates with single message or if the attribute value applies to all messages in the batch.
Destination attributes should be set when the messaging operation handles single messages. When the operation handles a batch of messages, the destination attributes should only be applied when the attribute value applies to all messages in the batch.

I feel this sentence was "merged" together with the above, so it was a bit confusing. I tried separating then so they can be read completely separately.

I'm not sure about the should here, so I left it non-normative.


A destination is usually uniquely identified by name within the messaging system instance. Examples of a destination name would be a URL or a simple one-word identifier.
Sending messages to a destination is called "*publish*" in context of this specification.
A destination represents an entity within messaging system messages are published to and consumed from. A destination
Copy link
Member

Choose a reason for hiding this comment

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

I can't put my finger on it, but I find hard to understand the sentence. Here's some attempts I tried.. (not sure it's better)

Suggested change
A destination represents an entity within messaging system messages are published to and consumed from. A destination
A destination represents an entity within a messaging system. Messages are published to and consumed from a destination.

OR

Suggested change
A destination represents an entity within messaging system messages are published to and consumed from. A destination
A destination represents an entity within a messaging system. A destination can act as a producer or a consumer.


> Note: Consumer spans can have attributes describing source and destination. Since messages could be routed by brokers, source and destination don't always match. If original destination information is available on the consumer, consumer instrumentations SHOULD populate corresponding `messaging.destination` attributes.
> Note: Messages could be routed by brokers, destination a message is consumed from doesn't always match the destination it was published to. If original destination information is available on the consumer side, consumer instrumentations SHOULD populate information about original destination on `messaging.destination_original` attributes.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
> Note: Messages could be routed by brokers, destination a message is consumed from doesn't always match the destination it was published to. If original destination information is available on the consumer side, consumer instrumentations SHOULD populate information about original destination on `messaging.destination_original` attributes.
> 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.

| `messaging.message.payload_compressed_size_bytes` | int | The compressed size of the message payload in bytes. | `2048` | Recommended: [6] |
| `messaging.message.payload_size_bytes` | int | The (uncompressed) size of the message payload in bytes. Also use this attribute if it is unknown whether the compressed or uncompressed payload size is reported. | `2738` | Recommended: [7] |
| [`net.peer.name`](span-general.md) | string | Logical remote hostname, see note below. [8] | `example.com` | Conditionally Required: If available. |
| `messaging.destination.anonymous` | boolean | A boolean that is true if the message destination is anonymous (could be unnamed or have auto-generated name). | | Conditionally Required: [4] |
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could we add the destination.* attributes after we are done with all the attributes under message.? I find it a bit hard to read and follow them when they are mixed like that. Another plus would be the diff here doesn't change much :)

@@ -56,34 +60,40 @@ groups:
type: boolean
brief: 'A boolean that is true if the message destination is anonymous (could be unnamed or have auto-generated name).'

- id: messaging.source
prefix: messaging.source
- id: messaging.destination_original
Copy link
Member

Choose a reason for hiding this comment

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

Hum, for some reason I thought we would just get rid of the "source" concept entirely and not just rename it. Don't we need then to change the schema file to redirect to messaging.destination_original then?

@reyang reyang added the area:semantic-conventions Related to semantic conventions label May 9, 2023
@reyang
Copy link
Member

reyang commented May 9, 2023

@lmolkova heads up - most likely this PR will be closed, and we'll ask you to resubmit the PR in a new repo, please refer to #3474 (comment).

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

@lmolkova
Copy link
Contributor Author

closing and will reopen in https://github.com/open-telemetry/semantic-conventions repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions semconv:messaging
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
5 participants