Skip to content

Commit 308ddb8

Browse files
natemortnatemort
committed
Propagate Tracer when building TChannel
While workflows still capture the tracing information in the headers, we should pass the Tracer to TChannel so that we correctly trace all RPC requests sent to the Cadence server. Ensure that Tracer can't be set to null and correct the testing library version to avoid classpath conflicts.
1 parent 951d18d commit 308ddb8

File tree

5 files changed

+87
-37
lines changed

5 files changed

+87
-37
lines changed

‎build.gradle‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ dependencies {
100100
testCompile group: 'com.googlecode.junit-toolbox', name: 'junit-toolbox', version: '2.4'
101101
testCompile group: 'ch.qos.logback', name: 'logback-classic', version: '1.2.3'
102102
testCompile group: 'io.grpc', name: 'grpc-testing', version: '1.54.2'
103-
testImplementation 'io.opentracing:opentracing-mock:0.33.0'
103+
testImplementation 'io.opentracing:opentracing-mock:0.32.0'
104104
testImplementation group: 'org.mockito', name: 'mockito-core', version: '4.5.1'
105105
testImplementation "org.mockito:mockito-inline:4.5.1" // for mocking final classes
106106
}

‎src/main/java/com/uber/cadence/serviceclient/ClientOptions.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import io.grpc.ManagedChannel;
2727
import io.opentracing.Tracer;
2828
import io.opentracing.noop.NoopTracerFactory;
29+
import io.opentracing.util.GlobalTracer;
2930
import java.util.Map;
3031

3132
public class ClientOptions {
@@ -130,7 +131,13 @@ private ClientOptions(Builder builder) {
130131
}
131132
this.authProvider = builder.authProvider;
132133
this.isolationGroup = builder.isolationGroup;
133-
this.tracer = builder.tracer;
134+
if (builder.tracer != null) {
135+
this.tracer = builder.tracer;
136+
} else {
137+
// Default value is GlobalTracer. If the user overrides it with null, fall back to NoopTracer.
138+
// We need some tracer instance
139+
this.tracer = NoopTracerFactory.create();
140+
}
134141
}
135142

136143
public static ClientOptions defaultInstance() {
@@ -233,8 +240,8 @@ public static class Builder {
233240
private IAuthorizationProvider authProvider;
234241
private FeatureFlags featureFlags;
235242
private String isolationGroup;
236-
// by default NoopTracer
237-
private Tracer tracer = NoopTracerFactory.create();
243+
// by default GlobalTracer
244+
private Tracer tracer = GlobalTracer.get();
238245

239246
private Builder() {}
240247

‎src/main/java/com/uber/cadence/serviceclient/WorkflowServiceTChannel.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,8 @@ public class WorkflowServiceTChannel implements IWorkflowService {
8686
public WorkflowServiceTChannel(ClientOptions options) {
8787
this.options = options;
8888
this.thriftHeaders = getThriftHeaders(options);
89-
this.tChannel = new TChannel.Builder(options.getClientAppName()).build();
89+
this.tChannel =
90+
new TChannel.Builder(options.getClientAppName()).setTracer(options.getTracer()).build();
9091
this.tracingPropagator = new TracingPropagator(options.getTracer());
9192
this.tracer = options.getTracer();
9293

‎src/test/java/com/uber/cadence/internal/sync/WorkflowClientInternalTest.java‎

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,18 @@ public void testEnqueueStart_includesTracing() {
116116
stub.enqueueStart("input");
117117

118118
StartWorkflowExecutionAsyncRequest request = requestFuture.getNow(null).getStartRequest();
119-
assertEquals(1, fakeService.getTracer().finishedSpans().size());
120-
MockSpan mockSpan = fakeService.getTracer().finishedSpans().get(0);
119+
MockSpan mockSpan =
120+
fakeService
121+
.getTracer()
122+
.finishedSpans()
123+
.stream()
124+
.filter(span -> "cadence-StartWorkflowExecutionAsync".equals(span.operationName()))
125+
.findFirst()
126+
.orElseThrow(
127+
() ->
128+
new AssertionError(
129+
"No span found for StartWorkflowExecutionAsync:"
130+
+ fakeService.getTracer().finishedSpans()));
121131
assertEquals(
122132
mockSpan.context().toTraceId(),
123133
Charsets.UTF_8
@@ -269,8 +279,21 @@ public void testEnqueueSignalWithStart_includesTracing() {
269279

270280
SignalWithStartWorkflowExecutionRequest request =
271281
requestFuture.getNow(null).getSignalWithStartRequest().getRequest();
272-
assertEquals(1, fakeService.getTracer().finishedSpans().size());
273-
MockSpan mockSpan = fakeService.getTracer().finishedSpans().get(0);
282+
assertEquals(2, fakeService.getTracer().finishedSpans().size());
283+
MockSpan mockSpan =
284+
fakeService
285+
.getTracer()
286+
.finishedSpans()
287+
.stream()
288+
.filter(
289+
span ->
290+
"cadence-SignalWithStartWorkflowExecutionAsync".equals(span.operationName()))
291+
.findFirst()
292+
.orElseThrow(
293+
() ->
294+
new AssertionError(
295+
"No span found for SignalWithStartWorkflowExecutionAsync:"
296+
+ fakeService.getTracer().finishedSpans()));
274297
assertEquals(
275298
mockSpan.context().toTraceId(),
276299
Charsets.UTF_8.decode(request.getHeader().getFields().get("traceid")).toString());

‎src/test/java/com/uber/cadence/internal/tracing/StartWorkflowTest.java‎

Lines changed: 47 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,8 @@ public void testSignalWithStartWorkflowGRPC() {
267267
public void testStartWorkflowTchannelNoPropagation() {
268268
Assume.assumeTrue(useDockerService);
269269
MockTracer mockTracer = new MockTracer();
270-
IWorkflowService service = new WorkflowServiceTChannel(ClientOptions.newBuilder().build());
270+
IWorkflowService service =
271+
new WorkflowServiceTChannel(ClientOptions.newBuilder().setTracer(null).build());
271272
testStartWorkflowHelper(service, mockTracer, false);
272273
}
273274

@@ -277,15 +278,17 @@ public void testStartWorkflowGRPCNoPropagation() {
277278
MockTracer mockTracer = new MockTracer();
278279
IWorkflowService service =
279280
new Thrift2ProtoAdapter(
280-
IGrpcServiceStubs.newInstance(ClientOptions.newBuilder().setPort(7833).build()));
281+
IGrpcServiceStubs.newInstance(
282+
ClientOptions.newBuilder().setPort(7833).setTracer(null).build()));
281283
testStartWorkflowHelper(service, mockTracer, false);
282284
}
283285

284286
@Test
285287
public void testSignalStartWorkflowTchannelNoPropagation() {
286288
Assume.assumeTrue(useDockerService);
287289
MockTracer mockTracer = new MockTracer();
288-
IWorkflowService service = new WorkflowServiceTChannel(ClientOptions.newBuilder().build());
290+
IWorkflowService service =
291+
new WorkflowServiceTChannel(ClientOptions.newBuilder().setTracer(null).build());
289292
testSignalWithStartWorkflowHelper(service, mockTracer, false);
290293
}
291294

@@ -295,7 +298,8 @@ public void testSignalStartWorkflowGRPCNoPropagation() {
295298
MockTracer mockTracer = new MockTracer();
296299
IWorkflowService service =
297300
new Thrift2ProtoAdapter(
298-
IGrpcServiceStubs.newInstance(ClientOptions.newBuilder().setPort(7833).build()));
301+
IGrpcServiceStubs.newInstance(
302+
ClientOptions.newBuilder().setPort(7833).setTracer(null).build()));
299303
testSignalWithStartWorkflowHelper(service, mockTracer, false);
300304
}
301305

@@ -330,7 +334,7 @@ private void testStartWorkflowHelper(
330334
int res = wf.AddOneThenDouble(3);
331335
assertEquals(8, res);
332336
} catch (Exception e) {
333-
fail("workflow failure: " + e);
337+
throw new AssertionError("workflow failure", e);
334338
} finally {
335339
rootSpan.finish();
336340
List<MockSpan> spans = mockTracer.finishedSpans();
@@ -358,19 +362,11 @@ private void testStartWorkflowHelper(
358362
} else {
359363
// assert start workflow
360364
MockSpan spanStartWorkflow =
361-
spans
362-
.stream()
363-
.filter(span -> span.operationName().contains("StartWorkflow"))
364-
.findFirst()
365-
.orElse(null);
366-
if (spanStartWorkflow == null) {
367-
fail("StartWorkflow span not found");
368-
}
365+
getFirstSpanByOperationName(spans, "cadence-StartWorkflowExecution");
369366

370367
// assert workflow spans
371-
MockSpan spanExecuteWF = getLinkedSpans(spans, spanStartWorkflow.context()).get(0);
372-
assertEquals(spanExecuteWF.operationName(), "cadence-ExecuteWorkflow");
373-
assertSpanReferences(spanExecuteWF, "follows_from", spanStartWorkflow);
368+
MockSpan spanExecuteWF = getFirstSpanByOperationName(spans, "cadence-ExecuteWorkflow");
369+
assertChildOf(spans, spanExecuteWF, spanStartWorkflow);
374370

375371
MockSpan spanExecuteActivity = getLinkedSpans(spans, spanExecuteWF.context()).get(0);
376372
assertEquals(spanExecuteActivity.operationName(), "cadence-ExecuteActivity");
@@ -432,7 +428,7 @@ private void testSignalWithStartWorkflowHelper(
432428
int res = wf.getResult(Integer.class);
433429
assertEquals(8, res);
434430
} catch (Exception e) {
435-
fail("workflow failure: " + e);
431+
throw new AssertionError("Workflow failure", e);
436432
} finally {
437433
rootSpan.finish();
438434
List<MockSpan> spans = mockTracer.finishedSpans();
@@ -460,21 +456,14 @@ private void testSignalWithStartWorkflowHelper(
460456
} else {
461457
// assert start workflow
462458
MockSpan spanStartWorkflow =
463-
spans
464-
.stream()
465-
.filter(span -> span.operationName().contains("StartWorkflow"))
466-
.findFirst()
467-
.orElse(null);
468-
if (spanStartWorkflow == null) {
469-
fail("StartWorkflow span not found");
470-
}
459+
getFirstSpanByOperationName(spans, "cadence-SignalWithStartWorkflowExecution");
471460

472461
// assert workflow spans
473462
List<MockSpan> workflowSpans =
474463
getSpansByTraceID(spans, spanStartWorkflow.context().toTraceId());
475-
MockSpan spanExecuteWF = getLinkedSpans(spans, spanStartWorkflow.context()).get(0);
476-
assertEquals(spanExecuteWF.operationName(), "cadence-ExecuteWorkflow");
477-
assertSpanReferences(spanExecuteWF, "follows_from", spanStartWorkflow);
464+
MockSpan spanExecuteWF =
465+
getFirstSpanByOperationName(workflowSpans, "cadence-ExecuteWorkflow");
466+
assertChildOf(spans, spanExecuteWF, spanStartWorkflow);
478467

479468
MockSpan spanExecuteActivity = getLinkedSpans(spans, spanExecuteWF.context()).get(0);
480469
assertEquals(spanExecuteActivity.operationName(), "cadence-ExecuteActivity");
@@ -493,6 +482,15 @@ private void testSignalWithStartWorkflowHelper(
493482
}
494483
}
495484

485+
private MockSpan getFirstSpanByOperationName(List<MockSpan> spans, String operation) {
486+
return spans
487+
.stream()
488+
.filter(span -> span.operationName().equals(operation))
489+
.findFirst()
490+
.orElseThrow(
491+
() -> new IllegalStateException("Failed to find span with operation: " + operation));
492+
}
493+
496494
private List<MockSpan> getSpansByTraceID(List<MockSpan> spans, String traceID) {
497495
return spans
498496
.stream()
@@ -517,6 +515,27 @@ private List<MockSpan> getLinkedSpans(List<MockSpan> spans, SpanContext spanCont
517515
.collect(Collectors.toList());
518516
}
519517

518+
void assertChildOf(List<MockSpan> spans, MockSpan span, MockSpan parentSpan) {
519+
Map<String, MockSpan> byId = new HashMap<>();
520+
for (MockSpan s : spans) {
521+
byId.put(s.context().traceId() + "-" + s.context().toSpanId(), s);
522+
}
523+
Queue<MockSpan> toVisit = new LinkedList<>();
524+
toVisit.add(span);
525+
while (!toVisit.isEmpty()) {
526+
MockSpan current = toVisit.poll();
527+
MockSpan parent = byId.get(current.context().traceId() + "-" + current.parentId());
528+
if (parent != null) {
529+
if (parent.equals(parentSpan)) {
530+
return;
531+
} else {
532+
toVisit.add(parent);
533+
}
534+
}
535+
}
536+
fail(String.format("span %s is not a child of parent span %s", span, parentSpan));
537+
}
538+
520539
void assertSpanReferences(MockSpan span, String referenceType, MockSpan parentSpan) {
521540
assertTrue(
522541
String.format(

0 commit comments

Comments
 (0)