Span structure for messaging scenarios #220
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pyohannes
force-pushed
the
span-structure-messaging
branch
from
aca9f0e
to
e191e53
Compare
brunobat
reviewed
Oct 4, 2022
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
ppatierno
reviewed
Oct 4, 2022
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
trask
pushed a commit
to open-telemetry/opentelemetry-java-instrumentation
that referenced
this pull request
Oct 5, 2022
Resolves #6779 In JMS you can have either the consumer receive span or the consumer process span (unlike Kafka, where the process span is always there and the receive span is just an addition) - in scenarios where polling (receive) is used, I think it makes sense to add links to the producer span to preserve the producer-consumer connection. Current messaging semantic conventions don't really describe a situation like this one, but the open-telemetry/oteps#220 OTEP mentions that links might be used in a scenario like this one - which makes me think that adding links here might be a not that bad idea.
ppatierno
reviewed
Oct 6, 2022
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
LironKS
pushed a commit
to helios/opentelemetry-java-instrumentation
that referenced
this pull request
Oct 23, 2022
Resolves open-telemetry#6779 In JMS you can have either the consumer receive span or the consumer process span (unlike Kafka, where the process span is always there and the receive span is just an addition) - in scenarios where polling (receive) is used, I think it makes sense to add links to the producer span to preserve the producer-consumer connection. Current messaging semantic conventions don't really describe a situation like this one, but the open-telemetry/oteps#220 OTEP mentions that links might be used in a scenario like this one - which makes me think that adding links here might be a not that bad idea.
LironKS
pushed a commit
to helios/opentelemetry-java-instrumentation
that referenced
this pull request
Oct 31, 2022
Resolves open-telemetry#6779 In JMS you can have either the consumer receive span or the consumer process span (unlike Kafka, where the process span is always there and the receive span is just an addition) - in scenarios where polling (receive) is used, I think it makes sense to add links to the producer span to preserve the producer-consumer connection. Current messaging semantic conventions don't really describe a situation like this one, but the open-telemetry/oteps#220 OTEP mentions that links might be used in a scenario like this one - which makes me think that adding links here might be a not that bad idea.
lmolkova
reviewed
Nov 22, 2022
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
LironKS
pushed a commit
to helios/opentelemetry-java-instrumentation
that referenced
this pull request
Dec 4, 2022
Resolves open-telemetry#6779 In JMS you can have either the consumer receive span or the consumer process span (unlike Kafka, where the process span is always there and the receive span is just an addition) - in scenarios where polling (receive) is used, I think it makes sense to add links to the producer span to preserve the producer-consumer connection. Current messaging semantic conventions don't really describe a situation like this one, but the open-telemetry/oteps#220 OTEP mentions that links might be used in a scenario like this one - which makes me think that adding links here might be a not that bad idea.
pyohannes
force-pushed
the
span-structure-messaging
branch
from
dfc70e6
to
1d27d3c
Compare
lmolkova
reviewed
Mar 15, 2023
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
joaopgrassi
reviewed
Mar 17, 2023
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
lmolkova
reviewed
Mar 21, 2023
Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
Oberon00
reviewed
Jun 19, 2023
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
Oberon00
reviewed
Jun 19, 2023
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
Oberon00
reviewed
Jun 19, 2023
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
Oberon00
reviewed
Jun 19, 2023
There was a problem hiding this comment.
Summary of this & all my previous reviews today: Mostly just copy-editing and a few very minor points. I did not review the examples. Overall mostly LGTM.
There is just one point that kept me from clicking "Approve", though it is also not a hard blocker: The "Trace structure" and "Span relationships" sections in combination might easily lead to redundancy or even contradictions in the current state. See #220 (comment)
The only other points that I'd like to point out specifically, but only because I made too many totally trivial nitpicks and these are a bit more interesting:
- Span kind for "settle" could IMHO just be stated to be client by default: Span structure for messaging scenarios #220 (comment)
- Some confusion in wording re. process and consumer and producer tracer correlation: Span structure for messaging scenarios #220 (comment) and Span structure for messaging scenarios #220 (comment)
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Oberon00
reviewed
Jun 20, 2023
text/trace/0220-messaging-semantic-conventions-span-structure.md
Outdated
Show resolved
Hide resolved
Oberon00
approved these changes
Jun 22, 2023
There was a problem hiding this comment.
Approved, but for clarity, I suggest applying #220 (comment) and #220 (comment) or similar clarifications. ✅
no-reply
pushed a commit
to no-reply/opentelemetry-ruby-contrib
that referenced
this pull request
Jul 11, 2023
…emetry#526) open-telemetry/oteps#220 recommends: > For each message it accounts for, the "Deliver" or "Receive" span SHOULD link to the message's creation context. In addition, if it is possible the creation context MAY be set as a parent of the "Deliver" or "Receive" span. But also acknolwedges: > open-telemetry/opentelemetry-specification#454 To instrument pull-based "Receive" operations as described in this document, it is necessary to add links to spans after those spans were created. The reason for this is, that not all messages are present at the start of a "Receive" operations, so links to related contexts cannot be added at the start of the span. Our "Receive" spans do not link to the message's creation context, and indeed it doesn't seem possible to do so. Likewise for setting the parent. This tries to do it anyway, and results in: - "Receive" span links remain unset; - "Receive" span is a root span for a new trace; - "Process" span trace_id is correctly set to the "Send" context; - "Process" span's parent is incorrectly set to "Send" instead of "Receive".
no-reply
pushed a commit
to no-reply/opentelemetry-ruby-contrib
that referenced
this pull request
Jul 11, 2023
…emetry#526) open-telemetry/oteps#220 recommends: > For each message it accounts for, the "Deliver" or "Receive" span SHOULD link to the message's creation context. In addition, if it is possible the creation context MAY be set as a parent of the "Deliver" or "Receive" span. But also acknolwedges: > open-telemetry/opentelemetry-specification#454 To instrument pull-based "Receive" operations as described in this document, it is necessary to add links to spans after those spans were created. The reason for this is, that not all messages are present at the start of a "Receive" operations, so links to related contexts cannot be added at the start of the span. Our "Receive" spans do not link to the message's creation context, and indeed it doesn't seem possible to do so. Likewise for setting the parent. In lieu of a specification layer solution, this proposes adding a configuration flag so that users can opt-in to setting the "Send" context as parent for the "Process" span. This comes at the cost of a continuous trace including the "Send"-"Receive"-"Process" spans, with "Receive" orphaned from both "Send" and "Process". This doesn't attempt to reproduce the "none" `propagation_style` included in `sidekiq` instrumentation and other similar gems. If this is an interesting direction, we could consider implementing it.
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This OTEP aims at defining consistent conventions about what spans to create for messaging scenarios, and at defining how those spans relate to each other. Instrumentors should be able to rely on a consistent set of conventions, as opposed to deducing conventions from a set of examples.
This was split from OTEP open-telemetry/opentelemetry-specification#192, which became too comprehensive.
Fixes open-telemetry/opentelemetry-specification#1085.