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

should not allow Linking spans belonging to same trace #207

Closed
rghetia opened this issue Aug 7, 2019 · 8 comments · Fixed by #283
Closed

should not allow Linking spans belonging to same trace #207

rghetia opened this issue Aug 7, 2019 · 8 comments · Fixed by #283
Assignees
Labels
area:span-relationships Related to span relationships
Milestone

Comments

@rghetia
Copy link
Contributor

rghetia commented Aug 7, 2019

As per the current definition of Link

"
A Span may be linked to zero or more other Spans (defined by SpanContext) that are causally related. Links can point to SpanContexts inside a single Trace or across different Traces.
"

What is the use case of linking span inside single Trace?

There is no apparent benefit of linking span inside the same trace. The fact that two spans belongs to same trace itself means that they are linked together sharing same TraceId. Any APM displaying trace should display both spans anyway.

Allowing linking span within same Trace can be miss-used. For example a parent span is added as a parent and as a link.

The example usage of Link listed in the spec are valid usage but are for different traces.

Link usage on Untrusted Endpoints:

Here incoming request is not trusted, hence a new trace is started. However incoming trace is linked for a reference. Two traces are different and are linked together using the Link.

Link usage for Batch Processing:

Here number of requests are batched. Each requests belongs to a specific trace (same or different). The batch of request is processed in another process/thread/machine. A new trace is started for processing each batch. Again two traces are different.

Proposal

Restrict the Linking to span across different traces only. This could be enforced by Link and AddLink api.

@rghetia rghetia changed the title Should allow Linking spans belonging to same trace? should not allow Linking spans belonging to same trace Aug 7, 2019
@rghetia
Copy link
Contributor Author

rghetia commented Aug 7, 2019

/cc @tedsuo @bogdandrutu @iredelmeier

@axw
Copy link
Contributor

axw commented Aug 8, 2019

How would you represent a scatter/gather type of pattern? i.e. single root -> multiple downstreams -> aggregate results. The final step would have multiple parents, but they each have the same origin.

I do feel that Link is a little under specified. A few related issues I've come across recently:

  • As mentioned in the issue description, is it valid for the parent to be recorded in Links?
  • For operations that have multiple parents: which should be recorded in Parent, and which in Links?
  • "Link" is too vague to draw causality without also specifying the link type (parent/child, as in OpenCensus).

@rghetia
Copy link
Contributor Author

rghetia commented Aug 8, 2019

I agree on adding link type. How about following types (instead of Parent and RemoteParent)?

  • Predecessor - used when it belongs to same trace.
  • RemotePredecessor - used when it belongs to a different trace.

In case you have multiple Predecessors (scatter/gather use case) you could either choose not to set parent at all OR choose any predecessor as a parent.

@axw
Copy link
Contributor

axw commented Aug 12, 2019

@rghetia having a Predecessor link type would work for me. I don't personally have a need for a RemotePredecessor link type, assuming HasRemoteParent is preserved. Parent vs. Predecessor still feels a little bit arbitrary.

I assume that types were intentionally left out of the link definition, since they are in OpenCensus. It'd be good to see the rationale for that - couldn't see anything in the spec.

@rghetia
Copy link
Contributor Author

rghetia commented Aug 12, 2019

I assume that types were intentionally left out of the link definition, since they are in OpenCensus. It'd be good to see the rationale for that - couldn't see anything in the spec.

@bogdandrutu , @SergeyKanzhelev do you know why link type was removed?

@iredelmeier iredelmeier added the area:span-relationships Related to span relationships label Aug 13, 2019
@pcwiese
Copy link

pcwiese commented Aug 14, 2019

I completely agree that the Link specification needs to be fine-tuned a bit.

The Span specification says that there is a single optional parent. Given that, it seems like that parent should be not be recorded as a Link. However, the Link specification says nothing about what should happen if you create a link back to the current Span's parent.

The overview specification does say that a Trace is a directed acyclic graph so that implies that we cannot reference a parent as a Link. It seems to me that implementations for adding Links have to check for Span cycles.

Additionally, the description of a Link as it relates to batch processing says that a Span can have multiple parents where each parent represents an item in the batch which contradicts the specification for a single optional parent on a Span. A bit a of mess here.

@haf
Copy link

haf commented Aug 22, 2019

It seems to me that you could define it like this:

/// A Span may reference zero or more other Spans or Traces that are causally related.
/// 
/// > OpenTracing presently defines two types of references: ChildOf and FollowsFrom.
/// > Both reference types specifically model direct causal relationships between a child Span and a parent Span.
/// > In the future, OpenTracing may also support reference types for Spans with non-causal relationships
/// > (e.g., Spans that are batched together, Spans that are stuck in the same queue, etc).
///
/// https://opentracing.io/specification/#references-between-spans
/// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#add-Links
///
/// It would seem we're not really clear on what sort of links we actually want. Follow this:
/// https://github.com/open-telemetry/opentelemetry-specification/issues/207
///
/// Parent-Child relationships are tracked in SpanContext, not via links.
type SpanLink =
  /// Some parent Spans do not depend in any way on the result of their child Spans. In these cases, we say merely that the child Span FollowsFrom the parent Span in a causal sense. There are many distinct FollowsFrom reference sub-categories, and in future versions of OpenTracing they may be distinguished more formally.
  /// Here `TraceId` could be the equivalent of a CommandId as sent from an end-user client, intended to be
  /// atomically & consistently applied to the system.
  | FollowsFromTrace of predecessor: TraceId * attrs: (string * SpanAttrValue)[]
  /// Some Spans cause their own "follow up" work. You can link to those predecessors with `FollowsFromSpan`
  | FollowsFromSpan of predecessor: SpanContext * attrs: (string * SpanAttrValue)[]

@SergeyKanzhelev SergeyKanzhelev added this to the Alpha v0.2 milestone Sep 27, 2019
@SergeyKanzhelev SergeyKanzhelev self-assigned this Oct 3, 2019
@SergeyKanzhelev
Copy link
Member

Link types discussion is in this issue: #65

As for this issue - I understand the feedback now is to clarify Links vs Parent relationship. I will clarify this relationship in a PR for this milestone.

TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this issue Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:span-relationships Related to span relationships
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants