Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Add SELF reference #212

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

felixbarny
Copy link
Contributor

@coveralls
Copy link

coveralls commented Nov 3, 2017

Coverage Status

Coverage increased (+0.7%) to 74.046% when pulling 6c1c423 on felixbarny:self-reference into 5884626 on opentracing:master.

@yurishkuro
Copy link
Member

I think this change revealed an issue with the SELF reference - one cannot create a proper parent reference unless the parent ID is propagated on the wire, which many systems do not do, and it's not considered as part of https://github.com/TraceContext/tracecontext-spec

@felixbarny
Copy link
Contributor Author

Could you explaint that a bit more? If the parent id is not propagated how would you then create a parent reference? How is this any different when the tracer impl creates the SpanContext?

For me, it sounds like TraceContext does include the parentId: https://github.com/TraceContext/tracecontext-spec/blob/master/trace_context/HTTP_HEADER_FORMAT.md#span-id

@yurishkuro
Copy link
Member

I can explain using Jaeger spans as example. When Jaeger span is created, we always record in it trace ID, (new) span ID, and the ID of the parent span. Let's give some values: traceID=15, spanID=5, parentID=1. When we serialize that span into an RPC request, we always include traceID and spanID (Jaeger also happens to include parentID, but that's legacy behavior we can ignore for this discussion). So the server that receives the request gets traceID=15, spanID=5. The server creates a new span as ChildOf, with a new random span ID (let's say 7):

  • context extracted from the wire: traceID=15, spanID=5
  • new span's context: traceID=15, spanID=7, parentID=5

Note the parentID=5 part, it establishes the link to the span from the caller service. However, if we serialize that context, we'd only get traceID=15, spanID=7, so if you try to use that as a SELF reference, you're missing the ID of the parent span (5), and the linkage is broken.

Again, in Jaeger today it so happens that SELF will work because we actually send parentID over the wire too, but not if we were to use the TraceContext spec, which only sends two IDs, traceID and spanID.

It also won't work with the MockTracer in your current PR - you added parentID to the context, but not to the serialization format, so if you marshal and unmarshal the context and create a SELF span, it will be missing the parent.

@felixbarny
Copy link
Contributor Author

so if you marshal and unmarshal the context and create a SELF span, it will be missing the parent

That's not the quite use case for the SELF reference but rather ETL. For example if you are given a JSON document of a span which is in a specific format and you want to create a Java representation from it. So serializing aka. Tracer#inject the span context does not matter for this use case, only deserializing aka. Tracer#extract.

But indeed, when using a TraceContext io.opentracing.propagation.Format, you can't construct the trace context even if you have the parentID as associated extractor does not read the parentID. But even when using TraceContext as the main propagation format you could still extract the SpanContext using the B3 format, for example. Another alternative is to implement a custom Format and extractor which is specific to the format you want to read and the tracer you want to support.

@yurishkuro
Copy link
Member

I am not sure why it didn't occur to me earlier, but I think there is a simple solution here. For tracers like MockTracer that do not store parent span id in the SpanContext (and by extension for any generic instrumentation using SELF reference), one just needs to construct another instance of SpanContext and pass it as CHILD_OF.

For example, assume we want to create SELF span with traceID=1, spanID=2, parentID=1 (using Python for shorter syntax):

tracer.start_span(
    "operation name",
    references=[
      opentracing.self(createVendorContext(traceID=1, spanID=2)),
      opentracing.child_of(createVendorContext(traceID=1, spanID=1)),
    ],
)

@felixbarny felixbarny changed the base branch from v0.31.0 to master June 1, 2018 14:10
@felixbarny
Copy link
Contributor Author

I have incorporated @yurishkuro's suggestions and rebased against master.

if (parent == null) {
// We're a root Span.
this.context = new MockContext(nextId(), nextId(), new HashMap<String, String>());
this.context = new MockContext(self != null ? self.traceId : nextId(), self != null ? self.spanId : nextId(),
new HashMap<String, String>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't baggage come from self as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

/**
* The self reference class can be used to construct a {@link Span} instance with a specific {@link SpanContext}.
*/
public static final String SELF = "self";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tedsuo should this go through Specification RFC first?

If we are making this a new required reference type, there needs to be more details about the expected tracer behavior and/or description of the use case. For example, what happens to startTime field in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just seeing this! Yes, @felixbarny an RFC would be appreciated, you can find the template here: https://github.com/opentracing/specification/blob/master/rfc_template.md

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants