Fixes #566: set CurrentRootSpan whenever created span is RootSpan

[mayurkale22]

After consolidation of Span and RootSpan, we set the RootSpan to CurrentRootSpan only when parentSpanId is null.

Which is:

if (!this.parentSpanId) {
  this.tracer.setCurrentRootSpan(this);
}

This causes an issue when parentSpanId is propagating on wire. In that case, although we create a new RootSpan it does not get set to CurrentRootSpan, therefore we create a new root span (instead of child span) for subsequent (outgoing) request.

which is:

// Checks if this outgoing request is part of an operation by checking
// if there is a current root span, if so, we create a child span. In
// case there is no root span, this means that the outgoing request is
// the first operation, therefore we create a root span.

if (!plugin.tracer.currentRootSpan) {
  // outgoingRequest starting a root span
  return plugin.tracer.startRootSpan(traceOptions);
} else {
  // outgoingRequest starting a child span
  const span = plugin.tracer.startChildSpan(traceOptions);
  ....
}

Please use #566 (comment) example to reproduce the problem.

This PR will set CurrentRootSpan whenever created/started span is RootSpan.

[codecov-io]

Codecov Report

Merging #570 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #570      +/-   ##
==========================================
+ Coverage    95.3%   95.35%   +0.04%     
==========================================
  Files         148      148              
  Lines       10590    10570      -20     
  Branches      746      744       -2     
==========================================
- Hits        10093    10079      -14     
+ Misses        497      491       -6

Impacted Files	Coverage Δ
src/http-stats.ts	100% <0%> (ø)	⬆️
...c/zpages-frontend/controllers/tracez.controller.ts	100% <0%> (ø)	⬆️
...es-frontend/controllers/traceconfigz.controller.ts	100% <0%> (ø)	⬆️
test/test-span.ts	100% <0%> (ø)	⬆️
src/zpages-frontend/controllers/rpcz.controller.ts	100% <0%> (ø)	⬆️
src/zpages-frontend/routes.ts	100% <0%> (ø)	⬆️
...c/zpages-frontend/controllers/statsz.controller.ts	100% <0%> (ø)	⬆️
src/zpages-frontend/page-handlers/templates-dir.ts	100% <0%> (ø)	⬆️
src/trace/model/span.ts	97.93% <0%> (+1.35%)	⬆️
test/test-instana.ts	100% <0%> (+14.28%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 039c695...a52e57a. Read the comment docs.

[jdbennet2001]

The logic seems reasonable. I wonder if:

 if (this.isRootSpan()) this.tracer.setCurrentRootSpan(this);

should be:

 if (this.isRootSpan() && !this.currentRootSpan ){
   this.tracer.setCurrentRootSpan(this);
}

which would leave you less vulnerable to changes in the http code.

[draffensperger]

Why is !this.parentSpanId insufficient for determining that a span is a root span?

[jdbennet2001]

A span, with a parent, can be generated in one of two ways:

1. An outgoing call that's a child of an incoming request. (Handled correctly)
2. An incoming call that's generated with trace and span id headers. (Not handled correctly).

In case #2, the incoming span id is set to the parent id and outgoing calls are assigned a completely new trace id. It seems like a stop ship defect, tbh.

[draffensperger]

OK, I understand now, in case #2 the span is not really a "root" span from the overall trace's perspective (the true root of the trace lies in an upstream service), but rather is only a "root" from this particular Node.js process's perspective.

In my opinion at some time in the future, it might be nice to just consolidate the distinction between a "root span" and a regular span as in this case it's actually incorrect to call it a root since it's only a root relative to this process. But I can approve this to unblock the issue.

[mayurkale22]

In my opinion at some time in the future, it might be nice to just consolidate the distinction between a "root span" and a regular span as in this case it's actually incorrect to call it a root since it's only a root relative to this process. But I can approve this to unblock the issue.

I agree with you and I think we should consider this in OpenTelemetry project.

[jdbennet2001]

@draffensperger Any more thoughts on this? It's a blocking defect for us.

[mayurkale22]

 if (this.isRootSpan() && !this.currentRootSpan ){
       this.tracer.setCurrentRootSpan(this);
}

which would leave you less vulnerable to changes in the http code.

Thanks for the suggestion, but I am not sure about the extra check. AFAIK Root Span should always be set to context manager. For me, !this.currentRootSpan check looks like a superfluous. Let me know your thoughts, I am happy to follow your suggestion.

[jdbennet2001]

 if (this.isRootSpan() && !this.currentRootSpan ){
       this.tracer.setCurrentRootSpan(this);
}

which would leave you less vulnerable to changes in the http code.

Thanks for the suggestion, but I am not sure about the extra check. AFAIK Root Span should always be set to context manager. For me, !this.currentRootSpan check looks like a superfluous. Let me know your thoughts, I am happy to follow your suggestion.

Fair enough. Please merge away. I will use our systems to thoroughly test the fix and report whatever I find.