-
Notifications
You must be signed in to change notification settings - Fork 898
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
add IsRemote field to SpanContext, set by propagators #216
add IsRemote field to SpanContext, set by propagators #216
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
@@ -30,7 +30,7 @@ Returns the sampling Decision for a `Span` to be created. | |||
- `SpanContext` of a parent `Span`. Typically extracted from the wire. Can be | |||
`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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that boolean is contained in the SpanContext, I'd remove the extra argument altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Without a field to mark if a
SpanContext
came from a remote processSpan
creation and sampling can not know if the new span has a remote parent.This PR adds a
IsRemote
field toSpanContext
and defines that it must be set when deserializing a propagatedSpanContext
.This is additionally important for implementations that can not pass the parent
Span
as an argument tostartSpan
but only aSpanContext
. That is the case with the Erlang/Elixir implementations because the content of aSpan
is kept in a table (since it needs to be able to be mutated) while theSpanContext
is an immutable record.Closes #187