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

Trace payload/message size in semantic convention is inconsistent #1053

Closed
alexvanboxel opened this issue Oct 5, 2020 · 7 comments · Fixed by open-telemetry/semantic-conventions#229
Assignees
Labels
area:semantic-conventions Related to semantic conventions priority:p3 Lowest priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory

Comments

@alexvanboxel
Copy link

alexvanboxel commented Oct 5, 2020

What are you trying to achieve?

Consistency should exist between the naming of payload/message size attribute names. Currently, we have a mix of how the name is constructed over 3 different protocols. HTTP has measure-name(length)(compressed) convention, gRPC has message.un/compressedmeasure_name(size) and message systems have message.measure-name(size)_unit(bytes). Here you have the complete list:

HTTP (attribute on a span):

  • http.request_content_length
  • http.request_content_length_uncompressed
  • http.response_content_length
  • http.response_content_length_uncompressed

gRPC (attribute on link in a stream)

  • message.compressed_size
  • message.uncompressed_size

Messaging system (attribute on a span):

  • messaging.message_payload_size_bytes
  • messaging.message_payload_compressed_size_bytes

What did you expect to see?

Although this doesn't strictly violates the spec (although message in gRPC is borderline), we should seek consistency in the naming of measures. I would have expected something more like this:

HTTP (keeping this as this is the most established)

  • http.request_content_length
  • http.request_content_length_uncompressed
  • http.response_content_length
  • http.response_content_length_uncompressed

gRPC and messaging:

  • messaging.payload_size
  • messaging.payload_size_uncompressed

changes for message: joined RPC and messaging and:

  • Removed unit (bytes)
  • Changed compressed to uncompressed to align with HTTP
  • Removed message from message_payload_size

I think it would be wise too add a note in the attributes naming convention some guidelines for messages, like:

  • not adding the unit (bytes, mm, ...) in the attribute name
@alexvanboxel alexvanboxel added the spec:trace Related to the specification/trace directory label Oct 5, 2020
@alexvanboxel alexvanboxel changed the title Trace payload/message size in semantic convention is inconsistent spec Trace payload/message size in semantic convention is inconsistent Oct 5, 2020
@arminru arminru added the area:semantic-conventions Related to semantic conventions label Oct 5, 2020
@arminru
Copy link
Member

arminru commented Oct 5, 2020

Hi @alexvanboxel, thanks for pointing this out!

gRPC and messaging do not share the same namespace on purpose since the messaging semantic conventions are a new concept added in OpenTelemetry whereas the gRPC message.* event attributes were taken over from OpenCensus for compatibility reasons IIRC. @bogdandrutu please confirm.

The messaging attributes were added first and afterwards the HTTP ones were added with a different naming to keep aligned with established terminology from HTTP headers, thus the divergence here.
We have some instrumentations using both already, so I'm not sure if it makes sense to modify the conventions this late in the game just for the sake of consistent naming.

I understand that it is desired for attribute names to be aligned, however. With the YAML definitions we added in #571, we would expect most OTel implementations to either ship with constants for all semantic convention attribute keys or provide them in a separate package. This would allow for easy discoverability in IDEs for both developers writing instrumentations as well as consumers. Do you think this is sufficient or would you still expect an overhaul here?

@alexvanboxel
Copy link
Author

I understand that it is desired for attribute names to be aligned, however. With the YAML definitions we added in #571, we would expect most OTel implementations to either ship with constants for all semantic convention attribute keys or provide them in a separate package. This would allow for easy discoverability in IDEs for both developers writing instrumentations as well as consumers. Do you think this is sufficient or would you still expect an overhaul here?

I understand that compatibility issues will not fix the current attributes. But would you think that an addendum to the attribute guideline would be a good idea?

I came to the issue as I'm building our internal semantic conventions on-top of OTel. If yes, I can create another issue or something else? And if yes: I would keep this ticket open to add the addendum you just explained to the files as a note. WDYT?

@arminru
Copy link
Member

arminru commented Oct 6, 2020

Adding guidelines for the future would be a good idea, yes, and so could be an addendum explaining why the attributes you mentioned initially don't follow these. Would you like to create a PR for that?

@andrewhsu andrewhsu added release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs priority:p3 Lowest priority level labels Oct 6, 2020
@alexvanboxel
Copy link
Author

Adding guidelines for the future would be a good idea, yes, and so could be an addendum explaining why the attributes you mentioned initially don't follow these. Would you like to create a PR for that?

I will, I will create first a PR for the attributes and use the issue to reference the addendum.

alexvanboxel added a commit to alexvanboxel/opentelemetry-specification that referenced this issue Dec 28, 2020
…1053)

Add a paragraph in the attribute name guidelines that discourages the use
of unit names and prefixes.

Add note about historical naming violation in messaging.md, the attribute names
message_payload_size_bytes and message_payload_compressed_size_bytes about the
incorrect use of bytes in the name.
alexvanboxel added a commit to alexvanboxel/opentelemetry-specification that referenced this issue Dec 28, 2020
…1053)

Add a paragraph in the attribute name guidelines that discourage the use
of unit names and prefixes.

Add note about historical naming violation in messaging.md, the attribute names
message_payload_size_bytes and message_payload_compressed_size_bytes about the
incorrect use of bytes in the name.
alexvanboxel added a commit to alexvanboxel/opentelemetry-specification that referenced this issue Dec 28, 2020
…1053)

Add a paragraph in the attribute name guidelines that discourage the use
of unit names and prefixes.

Add note about historical naming violation in messaging.md, the attribute names
message_payload_size_bytes and message_payload_compressed_size_bytes about the
incorrect use of bytes in the name.
alexvanboxel added a commit to alexvanboxel/opentelemetry-specification that referenced this issue Dec 28, 2020
…1053)

Add a paragraph in the attribute name guidelines that discourage the use
of unit names and prefixes.

Add note about historical naming violation in messaging.md, the attribute names
message_payload_size_bytes and message_payload_compressed_size_bytes about the
incorrect use of bytes in the name.
@alexvanboxel
Copy link
Author

Adding guidelines for the future would be a good idea, yes, and so could be an addendum explaining why the attributes you mentioned initially don't follow these. Would you like to create a PR for that?

@arminru sorry it took so long, I've added an extra paragraph about units in names. Maybe after this is resolved this issue could be closed: #1307

alexvanboxel added a commit to alexvanboxel/opentelemetry-specification that referenced this issue Jan 12, 2021
…1053)

Add a paragraph in the attribute name guidelines that discourage the use
of unit names and prefixes.

Add note about historical naming violation in messaging.md, the attribute names
message_payload_size_bytes and message_payload_compressed_size_bytes about the
incorrect use of bytes in the name.
alexvanboxel added a commit to alexvanboxel/opentelemetry-specification that referenced this issue Jan 12, 2021
…1053)

Add a paragraph in the attribute name guidelines that discourage the use
of unit names and prefixes.

Add note about historical naming violation in messaging.md, the attribute names
message_payload_size_bytes and message_payload_compressed_size_bytes about the
incorrect use of bytes in the name.
@pyohannes
Copy link
Contributor

pyohannes commented Aug 3, 2023

messaging.message_payload_size_bytes
messaging.message_payload_compressed_size_bytes

Discussed in today's messaging workgroup:

  • Clarify that the term "payload" refers to what goes over the wire (can either be a single message or a batch, including metdata), both in the initial definitions and in the namespace description.
  • To be consistent with HTTP, skip the messaging.message_payload_compressed_size_bytes.
  • To be consistent with HTTP, rename messaging.message_payload_size_bytes to messaging.payload.size.

@pyohannes pyohannes self-assigned this Aug 3, 2023
@Oberon00
Copy link
Member

Oberon00 commented Aug 3, 2023

"payload" refers to what goes over the wire (can either be a single message or a batch, including metdata)

That's the opposite of what the meaning of the word "payload" usually is https://en.wikipedia.org/wiki/Payload_(computing). Payload is usually used to distinguish metadata from the actual payload, so in the messaging context I would expect payload to be just the body. Maybe you should avoid the term "payload" entirely and use "body" and "complete_message" for more clarity.

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 priority:p3 Lowest priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory
Projects
Status: V1 - Stable Semantics
5 participants