add IsRemote field to SpanContext, set by propagators#216
add IsRemote field to SpanContext, set by propagators#216SergeyKanzhelev merged 5 commits intoopen-telemetry:masterfrom
Conversation
|
@tsloughter let's wait couple of days until we put the RFC for sampling, the hasRemoteParent may be removed, otherwise I really like this proposal if the remote parent stays. |
|
@bogdandrutu ok, sounds good. Curious to see the sampling RFC. |
|
Please see the RFC proposal open-telemetry/oteps#24 |
|
Now that the sampling rfc has been merged is this ready? |
songy23
left a comment
There was a problem hiding this comment.
How about HasRemoteParent? IsRemote sounds like describing the property of a span context itself, rather than its parent.
|
it is indeed the property of the context itself. E.g. in this OpenTracing example: the |
| `null`. | ||
| - Boolean that indicates that `SpanContext` was extracted from the wire, i.e. | ||
| parent `Span` is from the different process. | ||
| parent `Span` is from the different process, meaning `IsRemote` is set to true. |
There was a problem hiding this comment.
If that boolean is contained in the SpanContext, I'd remove the extra argument altogether.
There was a problem hiding this comment.
Good point, probably can remove the boolean argument.
|
What about, instead of a boolean, the SpanContext having an optional reference to the in-process Span, |
|
@Oberon00 that is an interesting idea since in the Erlang impl I currently do similar. The term stored when a span is created is |
|
@Oberon00 Having a reference to the "Span" causes a circular dependencies because Span also has a reference to the SpanContext. I think we should not do that. |
Not necessarily. It could just store the required info directly (or in some SpanContextData object that is equivalent to SpanContext minus back-reference) and create a SpanContext on demand. Also, I don't think a circular dependency is really a problem here, Span and SpanContext are closely related anyway. |
|
@tsloughter do you mind elaborating why you need isRemote? What would startSpan do differently? |
|
@yurishkuro it is used in the probability sampler https://github.com/open-telemetry/oteps/blob/eeab6a2d1342abb58ad5e7c0aadaaa4b1ab32310/text/0006-sampling.md#default-samplers |
|
I would suggest putting the circular reference discussion aside, as it is not related to this issue, only as an implementation detail. If we want to support configurable upsamping scenarios at "local root" spans, and expose it via sampling API (not just implementation), then we need isRemote in the API. |
…en-telemetry#216) Co-authored-by: Johannes Tax <johannes@johannes.tax> Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Without a field to mark if a
SpanContextcame from a remote processSpancreation and sampling can not know if the new span has a remote parent.This PR adds a
IsRemotefield toSpanContextand defines that it must be set when deserializing a propagatedSpanContext.This is additionally important for implementations that can not pass the parent
Spanas an argument tostartSpanbut only aSpanContext. That is the case with the Erlang/Elixir implementations because the content of aSpanis kept in a table (since it needs to be able to be mutated) while theSpanContextis an immutable record.Closes #187