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.

Sign up for GitHub

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

Jump to bottom

Span structure for messaging scenarios #220

Merged

carlosalberto merged 65 commits into open-telemetry:main from pyohannes:span-structure-messaging

Jun 29, 2023

Contributor

pyohannes commented Oct 4, 2022 •

edited

Loading

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.

pyohannes force-pushed the span-structure-messaging branch from aca9f0e to e191e53 Compare

October 4, 2022 00:09

brunobat reviewed

View reviewed changes

text/trace/0220-messaging-semantic-conventions-span-structure.md Show resolved Hide resolved

text/trace/0220-messaging-semantic-conventions-span-structure.md 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 Show resolved Hide resolved

ppatierno reviewed

View reviewed changes

text/trace/0220-messaging-semantic-conventions-span-structure.md Outdated Show resolved Hide resolved

text/trace/0220-messaging-semantic-conventions-span-structure.md 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

mateuszrzeszutek mentioned this pull request

Link JMS receive span with the producer span open-telemetry/opentelemetry-java-instrumentation#6804

Merged

trask pushed a commit to open-telemetry/opentelemetry-java-instrumentation that referenced this pull request


          Link JMS receive span with the producer span (#6804)

f195ec0

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

View reviewed changes

text/trace/0220-messaging-semantic-conventions-span-structure.md 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 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 Show resolved Hide resolved

pyohannes mentioned this pull request

Stabilize messaging semantic conventions for tracing #192

Closed

LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request


          Link JMS receive span with the producer span (open-telemetry#6804)

bad7e33

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


          Link JMS receive span with the producer span (open-telemetry#6804)

ec68009

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 mentioned this pull request

Refactor messaging attributes and specify per-message attributes open-telemetry/opentelemetry-specification#2957

Merged

lmolkova reviewed

View reviewed changes

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

text/trace/0220-messaging-semantic-conventions-span-structure.md Show resolved Hide resolved

LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request


          Link JMS receive span with the producer span (open-telemetry#6804)

3a22e67

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.

tedsuo added area:messaging priority:p0 labels

tedsuo added the triaged label

pyohannes force-pushed the span-structure-messaging branch from dfc70e6 to 1d27d3c Compare

February 16, 2023 00:23

pyohannes marked this pull request as ready for review

March 10, 2023 23:11

pyohannes requested review from a team

March 10, 2023 23:11

lmolkova reviewed

View reviewed changes

Contributor

lmolkova left a comment

left a few (mostly) cosmetic suggestions. The only major question is how we go ahead with producer/consumer controversy in the spec.

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 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 Show resolved Hide resolved

text/trace/0220-messaging-semantic-conventions-span-structure.md Show resolved Hide resolved

joaopgrassi reviewed

View reviewed changes

Member

joaopgrassi left a comment

Finally found some time to review this. I think we are getting very close!

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 Show resolved Hide resolved

text/trace/0220-messaging-semantic-conventions-span-structure.md 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 Show resolved Hide resolved

Oberon00 mentioned this pull request

Separate semantic conventions from the specification. #227

Merged

lmolkova reviewed

View reviewed changes

text/trace/0220-messaging-semantic-conventions-span-structure.md Show resolved Hide resolved

pyohannes added 7 commits

March 29, 2023 14:03


          Initial version

17af4be


          PR comments

fc8251e


          PR comments

b72800b


          PR comments

69e6b33


          Add requirement for out-of-the-box messaging SDK instrumentation

b279f2a


          Add possibility for addition parent/child relationships

61051e5


          Make optional spans opaque

9fe3af6

Oberon00 reviewed

View reviewed changes

Member

Oberon00 left a comment •

edited

Loading

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

pyohannes and others added 7 commits

June 20, 2023 07:01


          Update text/trace/0220-messaging-semantic-conventions-span-structure.md

e712f45

Co-authored-by: Christian Neumüller <[email protected]>


          Update text/trace/0220-messaging-semantic-conventions-span-structure.md

b0117f2

Co-authored-by: Christian Neumüller <[email protected]>


          Update text/trace/0220-messaging-semantic-conventions-span-structure.md

fe2be92

Co-authored-by: Christian Neumüller <[email protected]>


          Update text/trace/0220-messaging-semantic-conventions-span-structure.md

d9120df

Co-authored-by: Christian Neumüller <[email protected]>


          Update text/trace/0220-messaging-semantic-conventions-span-structure.md

Co-authored-by: Christian Neumüller <[email protected]>


          PR comments

a9222f9


          PR comments

c562c6a

Oberon00 reviewed

View reviewed changes

text/trace/0220-messaging-semantic-conventions-span-structure.md Show resolved Hide resolved

text/trace/0220-messaging-semantic-conventions-span-structure.md 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 Show resolved Hide resolved

pyohannes added 2 commits

June 20, 2023 18:38


          Fix links

bf8072f


          PR comments

bfddf15

Oberon00 approved these changes

View reviewed changes

Member

Oberon00 left a comment •

edited

Loading

Approved, ~~but for clarity, I suggest applying #220 (comment) and #220 (comment) or similar clarifications.~~ ✅


          Adding an open issue regarding parent/child relationship

a47b077

robbkidd mentioned this pull request

bunny instrumentation is not propagating contexts open-telemetry/opentelemetry-ruby-contrib#526

Open

carlosalberto merged commit bfddd25 into open-telemetry:main

bolandoboy mentioned this pull request

Sidekiq integration can't be combined with other integrations open-telemetry/opentelemetry-ruby-contrib#535

Closed

joaopgrassi mentioned this pull request

Clarify usage of Distributed Tracing Extension by OpenTelemetry cloudevents/spec#912

Closed

no-reply pushed a commit to no-reply/opentelemetry-ruby-contrib that referenced this pull request


          feat: Add Bunny configuration for context propagation_style (open-tel…

…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


          feat: Add Bunny configuration for context propagation_style (open-tel…

ae0bbe0

…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.

carlosalberto mentioned this pull request

Do not add AddLink(), use option to AddEvent() instead open-telemetry/opentelemetry-specification#3337

Closed

pyohannes mentioned this pull request

Update messaging "Receive", "Deliver", and "Create" operations according to OTEP 220 open-telemetry/semantic-conventions#284

Merged

3 tasks

pyohannes mentioned this pull request

Add conventions for settlement spans for messaging open-telemetry/semantic-conventions#621

Closed

carlosalberto mentioned this pull request

Clarify relationship between messaging, faas, and RPC open-telemetry/semantic-conventions#652

Closed

carlosalberto pushed a commit to carlosalberto/oteps that referenced this pull request


          Span structure for messaging scenarios (open-telemetry#220)

ac53d1f

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#192, which became too comprehensive.

carlosalberto pushed a commit to carlosalberto/oteps that referenced this pull request


          Span structure for messaging scenarios (open-telemetry#220)

66ecd12

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#192, which became too comprehensive.

carlosalberto pushed a commit to carlosalberto/oteps that referenced this pull request


          Span structure for messaging scenarios (open-telemetry#220)

e379cba

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#192, which became too comprehensive.

carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request


          Span structure for messaging scenarios (open-telemetry/oteps#220)

40c530a

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#192, which became too comprehensive.

carlosalberto pushed a commit to carlosalberto/oteps that referenced this pull request


          Span structure for messaging scenarios (open-telemetry#220)

5f05a7c

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#192, which became too comprehensive.

carlosalberto pushed a commit to carlosalberto/oteps that referenced this pull request


          Span structure for messaging scenarios (open-telemetry#220)

4c75550

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#192, which became too comprehensive.

carlosalberto pushed a commit to open-telemetry/opentelemetry-specification that referenced this pull request


          Span structure for messaging scenarios (open-telemetry/oteps#220)

bb1a76a

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 #192, which became too comprehensive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joaopgrassi joaopgrassi approved these changes

ppatierno ppatierno left review comments

yurishkuro yurishkuro left review comments

lmolkova lmolkova approved these changes

brunobat brunobat left review comments

tylerbenson tylerbenson left review comments

dpauls dpauls approved these changes

Oberon00 Oberon00 approved these changes

carlosalberto carlosalberto approved these changes

Labels

area:messaging priority:p0 triaged