-
Notifications
You must be signed in to change notification settings - Fork 232
Upgrade to OpenTracing java 0.30.0 with in-process propagation support #188
Conversation
Codecov Report
@@ Coverage Diff @@
## master #188 +/- ##
===========================================
+ Coverage 80.98% 81.48% +0.5%
- Complexity 462 463 +1
===========================================
Files 77 77
Lines 1793 1804 +11
Branches 210 211 +1
===========================================
+ Hits 1452 1470 +18
+ Misses 251 244 -7
Partials 90 90
Continue to review full report at Codecov.
|
@@ -188,7 +208,7 @@ public void close() { | |||
} | |||
|
|||
@Override | |||
public io.opentracing.Tracer.SpanBuilder asChildOf(io.opentracing.Span parent) { | |||
public io.opentracing.Tracer.SpanBuilder asChildOf(BaseSpan<?> parent) { |
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.
not io.opentracing.BaseSpan
?
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.
It is io.opentracing.BaseSpan
.
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.
given my comments about the build, it seems we're missing unit tests for various scenarios involving the active span
|
||
// Check if active span should be established as CHILD_OF relationship | ||
if (!ignoreActiveSpan && null != activeSpanSource.activeSpan()) { | ||
asChildOf(activeSpanSource.activeSpan()); |
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.
this seems too late, since the new context was already created above.
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.
maybe move it into createNewContext(), to cover the case below
// Check if active span should be established as CHILD_OF relationship | ||
if (!ignoreActiveSpan && null != activeSpanSource.activeSpan()) { | ||
asChildOf(activeSpanSource.activeSpan()); | ||
} | ||
} else if (debugId != null) { | ||
context = createNewContext(debugId); |
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.
technically this also qualifies for active-spanness
Ok thanks @yurishkuro I will sort it out when back next week. |
We should start deprecating instrumentations and context propagation. It can be done in a separate PR. |
@yurishkuro Moved the code into the |
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.
Sorry for slow response, this week has been very busy.
@@ -169,7 +169,6 @@ private void finishWithDuration(long durationMicros) { | |||
} | |||
} | |||
|
|||
@Override | |||
public void close() { |
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.
should we even have this method? IIRC the OT Span is no longer closable
if (!ignoreActiveSpan && null != activeSpanSource.activeSpan()) { | ||
asChildOf(activeSpanSource.activeSpan()); | ||
} | ||
|
||
return new SpanContext(id, id, 0, flags); |
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.
parentID == 0 and traceID == random()
are incorrect if we have a parent span
tracer.buildSpan("child").startActive().deactivate(); | ||
} | ||
assertEquals(2, reporter.getSpans().size()); | ||
assertEquals("child", reporter.getSpans().get(0).getOperationName()); |
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.
nit: extract to vars to make the code easier to read
Span childSpan = reporter.getSpans().get(0);
Span parentSpan = reporter.getSpans().get(1);
assertTrue(reporter.getSpans().get(1).getReferences().isEmpty()); | ||
assertEquals(References.CHILD_OF, reporter.getSpans().get(0).getReferences().get(0).getType()); | ||
assertEquals(reporter.getSpans().get(1).context(), | ||
reporter.getSpans().get(0).getReferences().get(0).getSpanContext()); |
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.
missing checks:
parentSpan.traceID == childSpan.traceID
parentSpan.spanID == childSpan.parentID
} | ||
assertEquals(2, reporter.getSpans().size()); | ||
assertTrue(reporter.getSpans().get(0).getReferences().isEmpty()); | ||
assertTrue(reporter.getSpans().get(1).getReferences().isEmpty()); |
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.
missing:
outerSpan.traceID != inner.traceID
innerSpan.parentID == 0
… and add further checks in tests
parentId = parentContext.getSpanId(); | ||
} | ||
|
||
return new SpanContext(traceId, id, parentId, flags); |
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.
I think there's another issue - span ID can be unique or inherited from parent depending on zipkinSharedRpcSpan
flag. Looking at this with a fresh eye, perhaps this child logic does not belong to createNewContext
. Instead, of we move it to the beginning of startManual
would the rest just work automatically?
public io.opentracing.Span startManual() {
// move here
if (!ignoreActiveSpan && null != activeSpanSource.activeSpan()) {
asChildOf((SpanContext) activeSpanSource.activeSpan().context());
}
SpanContext context;
String debugId = debugId();
if (references.isEmpty()) {
context = createNewContext(null);
} else if (debugId != null) {
context = createNewContext(debugId);
} else {
context = createChildContext();
}
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.
@yurishkuro Update applied - although not sure it is right, as the conditions for adding the childOf for the active span is that no other explicit references have been added? Possibly it just needs an extract condition - e.g. references.isEmpty() && !ignoreActiveSpan && ...
?
// Check if active span should be established as CHILD_OF relationship | ||
if (!ignoreActiveSpan && null != activeSpanSource.activeSpan()) { | ||
asChildOf(activeSpanSource.activeSpan()); | ||
} |
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.
lgtm. Just one more thing - reading the API docs, the active span should only be used as a parent if there are no other references added explicitly. So need to add && references. isEmpty
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.
And probably a respective unit test is missing for this scenario.
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.
Agree
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.
Will add test soon - have to sign off for a while.
Thanks @objectiser ! |
Resolves #179
Changed all calls to
SpanBuilder.start()
method tostartManual()
to avoid deprecation warnings.