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

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

Merged
merged 3 commits into from
Jun 8, 2021

Conversation

jkwatson
Copy link
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
Copy link
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?

@@ -697,7 +697,9 @@ and its children in a Trace. `SpanKind` describes two independent
properties that benefit tracing systems during analysis.

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
Contributor

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
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
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
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
Contributor

carlosalberto commented Jun 5, 2021

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
@arminru arminru mentioned this pull request Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The need for more granularity/clarity in CLIENT span conventions
7 participants