Propagate FollowsFrom span relationships in SDK
#190
Conversation
iredelmeier
requested review from
bogdandrutu,
jmacd,
pjanotti,
rghetia,
sjkaris and
tedsuo
as code owners
| opts.Reference.RelationshipType == apitrace.ChildOfRelationship { | ||
| parent = opts.Reference.SpanContext | ||
| remoteParent = true | ||
| if reference := opts.Reference; reference.SpanContext != core.EmptySpanContext() { |
There was a problem hiding this comment.
Reference is not defined in the specs. What does a follow from mean?
There was a problem hiding this comment.
Then we should remove FollowsFrom completely.
There was a problem hiding this comment.
Reference should be replaced simply with SpanContext in SpanOptions and add 'remoteParent bool' in SpanContext. Propagators should set it to true.
With remoteParent bool in SpanContext, there is no need for 'remote' parameters/fields in bunch of places.
There was a problem hiding this comment.
Why should remoteParent go in the span context? If you start a span with an explicit ChildOf or FollowsFrom, isn't that a sufficient indicator? I.e., this bit can be computed and stored in the SDK if needed.
There was a problem hiding this comment.
Folks, remember that we have a OpenTracing compatibility story. OpenTracing defined FollowsFrom.
There was a problem hiding this comment.
I agree with @jmacd on both counts, particularly about not adding remoteParent to SpanContext - that feels to me like it would introduce more room for confusion. If we want to make the "remote" vs "local" aspect more explicit, what about something like renaming the start options so that they clearly indicate they're meant for remote parents only (e.g., RemoteChildOf)?
I also feel like this conversation should be moved outside of this PR, which is meant to be about what seems like a relatively contained bug based on the existing code.
There was a problem hiding this comment.
I thought we were using the word "Relation", not "Reference", but that's not something for this PR. I see that StartSpanOptions has a Reference, but I think it would be clearer if we used Relation or Relationship.
There was a problem hiding this comment.
Why should
remoteParentgo in the span context? If you start a span with an explicitChildOforFollowsFrom, isn't that a sufficient indicator? I.e., this bit can be computed and stored in the SDK if needed.
Spec makes it seem like 'remoteParent' is part of SpanContext.
Anyway, as far as this PR goes the change looks fine.
iredelmeier
force-pushed
the
respect-follows-from
branch
from
1c96105 to
486bb9b
Compare
| opts.Reference.RelationshipType == apitrace.ChildOfRelationship { | ||
| parent = opts.Reference.SpanContext | ||
| remoteParent = true | ||
| if reference := opts.Reference; reference.SpanContext != core.EmptySpanContext() { |
There was a problem hiding this comment.
I thought we were using the word "Relation", not "Reference", but that's not something for this PR. I see that StartSpanOptions has a Reference, but I think it would be clearer if we used Relation or Relationship.
* Move othttp instrumentation to contrib repo * api/standard package has moved to semconv * Replace othttp references with otelhttp * Revert "api/standard package has moved to semconv" This reverts commit eceaa351e438ff986af904fbeb40a9ff0eb79322 as the change has not yet been published in a versioned module. Leaving the revert commit in history for ease of resurrection. * reference correct contrib module version * Add net/http/httptrace instrumentation * add httptrace module to dependabot config * fix precommit issues * Add net/http example * add httptrace example * Restore response writer wrapper from opentelemery-go#979 * Add CHANGELOG entry Co-authored-by: Tyler Yahn <[email protected]>
[Closes #189]