Skip to content

Clarify some details about span kind and the meanings of the values.#1738

Merged
carlosalberto merged 3 commits intoopen-telemetry:mainfrom
jkwatson:clarify_span_kind
Jun 8, 2021
Merged

Clarify some details about span kind and the meanings of the values.#1738
carlosalberto merged 3 commits intoopen-telemetry:mainfrom
jkwatson:clarify_span_kind

Conversation

@jkwatson
Copy link
Copy Markdown
Contributor

@jkwatson jkwatson commented Jun 1, 2021

Fixes #1360

Changes

Provides some additional clarity around Span.Kind and the meanings of the various values.

@jkwatson jkwatson requested review from a team June 1, 2021 20:00
iNikem
iNikem previously requested changes Jun 2, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@iNikem iNikem left a comment

Choose a reason for hiding this comment

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

I am not sure that these changes resolved #1360 completely.

Also, if the goal of these changes is to allow nested CLIENT spans, then I am against it :) For a very specific reason: if we have several CLIENT spans during one remote call, which of those spans should be propagated?

Comment thread specification/trace/api.md
The first property described by `SpanKind` reflects whether the Span
is a remote child or parent. Spans with a remote parent are
is a "logical" remote child or parent. By "logical", we mean that
the span is logically a remote child or parent, from the point of view
Copy link
Copy Markdown
Contributor

@iNikem iNikem Jun 2, 2021

Choose a reason for hiding this comment

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

In this section you say both "remote child" and "span with a remote parent". I am afraid "remote child" will be somewhat confusing term. Or at least we have to explicitly define what is "remote child".

@jkwatson
Copy link
Copy Markdown
Contributor Author

jkwatson commented Jun 2, 2021

I am not sure that these changes resolved #1360 completely.

Also, if the goal of these changes is to allow nested CLIENT spans, then I am against it :) For a very specific reason: if we have several CLIENT spans during one remote call, which of those spans should be propagated?

I think it's very simple: the last one to touch the headers is the one that is propagated. Is there more complication that I'm not considering?

@iNikem iNikem self-requested a review June 2, 2021 15:44
@iNikem
Copy link
Copy Markdown
Contributor

iNikem commented Jun 2, 2021

I removed my objection after offline discussion with @jkwatson

@iNikem iNikem dismissed their stale review June 2, 2021 15:45

I removed my objection after offline discussion with @jkwatson

@trask
Copy link
Copy Markdown
Member

trask commented Jun 3, 2021

I agree it's needed to relax this constraint on nesting. I'm wondering if there's anything we can do to make it easier on backends that use CLIENT/SERVER and PRODUCER/CONSUMER spans to draw service maps, e.g. we had this discussion in Java a while ago before we started suppressing nested CLIENT spans: open-telemetry/opentelemetry-java-instrumentation#440

@carlosalberto carlosalberto added area:api Cross language API specification issue spec:trace Related to the specification/trace directory labels Jun 4, 2021
@carlosalberto
Copy link
Copy Markdown
Contributor

carlosalberto commented Jun 5, 2021 •
edited
Loading

Let's merge this right after the 1.4 release as we have enough approvals.

@carlosalberto carlosalberto merged commit 21632a6 into open-telemetry:main Jun 8, 2021
@iNikem iNikem mentioned this pull request Jul 5, 2021
Closed
@arminru arminru mentioned this pull request Jul 7, 2021
Merged
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@jkwatson
Clarify some details about span kind. (open-telemetry#1738)
ef503c9
schmikei pushed a commit to schmikei/opentelemetry-specification that referenced this pull request Apr 17, 2025
@trask
Add profiling approvers to CODEOWNERS (open-telemetry#1738)
cb3a6db 
Co-authored-by: Liudmila Molkova <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carlosalberto carlosalberto carlosalberto approved these changes

@reyang reyang reyang approved these changes

@iNikem iNikem Awaiting requested review from iNikem

+1 more reviewer

@yurishkuro yurishkuro yurishkuro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tigrannajaryan tigrannajaryan

Labels

area:api Cross language API specification issue spec:trace Related to the specification/trace directory

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The need for more granularity/clarity in CLIENT span conventions

7 participants

@jkwatson @iNikem @trask @carlosalberto @yurishkuro @reyang @tigrannajaryan