Skip to content

Commit

Permalink
Revert "core: Update gRPC to use span kind and raw full method. (#5328)"
Browse files Browse the repository at this point in the history
This reverts commit d473799.

This caused test failures internally, where gRPC failed with
"IllegalArgumentException: Invalid trace name". Not only was this
failure unexpected, it was also weird that it failed with an
IllegalArgumentException instead of the normal StatusRuntimeException.
  • Loading branch information
ejona86 committed Feb 13, 2019
1 parent bb39413 commit 9a38dea
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 22 deletions.
26 changes: 21 additions & 5 deletions core/src/main/java/io/grpc/internal/CensusTracingModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import io.opencensus.trace.MessageEvent;
import io.opencensus.trace.MessageEvent.Type;
import io.opencensus.trace.Span;
import io.opencensus.trace.Span.Kind;
import io.opencensus.trace.SpanContext;
import io.opencensus.trace.Status;
import io.opencensus.trace.Tracer;
Expand Down Expand Up @@ -235,9 +234,10 @@ final class ClientCallTracer extends ClientStreamTracer.Factory {
this.isSampledToLocalTracing = method.isSampledToLocalTracing();
this.span =
censusTracer
.spanBuilderWithExplicitParent(method.getFullMethodName(), parentSpan)
.spanBuilderWithExplicitParent(
generateTraceSpanName(false, method.getFullMethodName()),
parentSpan)
.setRecordEvents(true)
.setSpanKind(Kind.CLIENT)
.startSpan();
}

Expand Down Expand Up @@ -303,9 +303,10 @@ private final class ServerTracer extends ServerStreamTracer {
checkNotNull(fullMethodName, "fullMethodName");
this.span =
censusTracer
.spanBuilderWithRemoteParent(fullMethodName, remoteSpan)
.spanBuilderWithRemoteParent(
generateTraceSpanName(true, fullMethodName),
remoteSpan)
.setRecordEvents(true)
.setSpanKind(Kind.SERVER)
.startSpan();
}

Expand Down Expand Up @@ -401,4 +402,19 @@ public void onClose(io.grpc.Status status, Metadata trailers) {
};
}
}

/**
* Convert a full method name to a tracing span name.
*
* @param isServer {@code false} if the span is on the client-side, {@code true} if on the
* server-side
* @param fullMethodName the method name as returned by
* {@link MethodDescriptor#getFullMethodName}.
*/
@VisibleForTesting
static String generateTraceSpanName(boolean isServer, String fullMethodName) {
String prefix = isServer ? "Recv" : "Sent";
return prefix + "." + fullMethodName.replace('/', '.');
}

}
34 changes: 17 additions & 17 deletions core/src/test/java/io/grpc/internal/CensusModulesTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@
import io.opencensus.trace.MessageEvent;
import io.opencensus.trace.MessageEvent.Type;
import io.opencensus.trace.Span;
import io.opencensus.trace.Span.Kind;
import io.opencensus.trace.SpanBuilder;
import io.opencensus.trace.SpanContext;
import io.opencensus.trace.Tracer;
Expand Down Expand Up @@ -295,14 +294,12 @@ public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(

if (nonDefaultContext) {
verify(tracer).spanBuilderWithExplicitParent(
eq("package1.service2/method3"), same(fakeClientParentSpan));
eq("Sent.package1.service2.method3"), same(fakeClientParentSpan));
verify(spyClientSpanBuilder).setRecordEvents(eq(true));
verify(spyClientSpanBuilder).setSpanKind(eq(Kind.CLIENT));
} else {
verify(tracer).spanBuilderWithExplicitParent(
eq("package1.service2/method3"), isNull(Span.class));
eq("Sent.package1.service2.method3"), isNull(Span.class));
verify(spyClientSpanBuilder).setRecordEvents(eq(true));
verify(spyClientSpanBuilder).setSpanKind(eq(Kind.CLIENT));
}
verify(spyClientSpan, never()).end(any(EndSpanOptions.class));

Expand Down Expand Up @@ -500,7 +497,7 @@ public void clientBasicTracingDefaultSpan() {
ClientStreamTracer clientStreamTracer =
callTracer.newClientStreamTracer(CallOptions.DEFAULT, headers);
verify(tracer).spanBuilderWithExplicitParent(
eq("package1.service2/method3"), isNull(Span.class));
eq("Sent.package1.service2.method3"), isNull(Span.class));
verify(spyClientSpan, never()).end(any(EndSpanOptions.class));

clientStreamTracer.outboundMessage(0);
Expand Down Expand Up @@ -607,9 +604,8 @@ public void clientStreamNeverCreatedStillRecordTracing() {
CensusTracingModule.ClientCallTracer callTracer =
censusTracing.newClientCallTracer(fakeClientParentSpan, method);
verify(tracer).spanBuilderWithExplicitParent(
eq("package1.service2/method3"), same(fakeClientParentSpan));
eq("Sent.package1.service2.method3"), same(fakeClientParentSpan));
verify(spyClientSpanBuilder).setRecordEvents(eq(true));
verify(spyClientSpanBuilder).setSpanKind(eq(Kind.CLIENT));

callTracer.callEnded(Status.DEADLINE_EXCEEDED.withDescription("3 seconds"));
verify(spyClientSpan).end(
Expand Down Expand Up @@ -784,9 +780,8 @@ public void traceHeadersPropagateSpanContext() throws Exception {
verify(mockTracingPropagationHandler).toByteArray(same(fakeClientSpanContext));
verifyNoMoreInteractions(mockTracingPropagationHandler);
verify(tracer).spanBuilderWithExplicitParent(
eq("package1.service2/method3"), same(fakeClientParentSpan));
eq("Sent.package1.service2.method3"), same(fakeClientParentSpan));
verify(spyClientSpanBuilder).setRecordEvents(eq(true));
verify(spyClientSpanBuilder).setSpanKind(eq(Kind.CLIENT));
verifyNoMoreInteractions(tracer);
assertTrue(headers.containsKey(censusTracing.tracingHeader));

Expand All @@ -795,9 +790,8 @@ public void traceHeadersPropagateSpanContext() throws Exception {
method.getFullMethodName(), headers);
verify(mockTracingPropagationHandler).fromByteArray(same(binarySpanContext));
verify(tracer).spanBuilderWithRemoteParent(
eq("package1.service2/method3"), same(spyClientSpan.getContext()));
eq("Recv.package1.service2.method3"), same(spyClientSpan.getContext()));
verify(spyServerSpanBuilder).setRecordEvents(eq(true));
verify(spyServerSpanBuilder).setSpanKind(eq(Kind.SERVER));

Context filteredContext = serverTracer.filterContext(Context.ROOT);
assertSame(spyServerSpan, ContextUtils.CONTEXT_SPAN_KEY.get(filteredContext));
Expand Down Expand Up @@ -867,10 +861,8 @@ public void traceHeaderMalformed() throws Exception {
censusTracing.getServerTracerFactory().newServerStreamTracer(
method.getFullMethodName(), headers);
verify(tracer).spanBuilderWithRemoteParent(
eq("package1.service2/method3"), isNull(SpanContext.class));
eq("Recv.package1.service2.method3"), isNull(SpanContext.class));
verify(spyServerSpanBuilder).setRecordEvents(eq(true));
verify(spyServerSpanBuilder).setSpanKind(eq(Kind.SERVER));

}

@Test
Expand Down Expand Up @@ -1021,9 +1013,8 @@ public void serverBasicTracingNoHeaders() {
tracerFactory.newServerStreamTracer(method.getFullMethodName(), new Metadata());
verifyZeroInteractions(mockTracingPropagationHandler);
verify(tracer).spanBuilderWithRemoteParent(
eq("package1.service2/method3"), isNull(SpanContext.class));
eq("Recv.package1.service2.method3"), isNull(SpanContext.class));
verify(spyServerSpanBuilder).setRecordEvents(eq(true));
verify(spyServerSpanBuilder).setSpanKind(eq(Kind.SERVER));

Context filteredContext = serverStreamTracer.filterContext(Context.ROOT);
assertSame(spyServerSpan, ContextUtils.CONTEXT_SPAN_KEY.get(filteredContext));
Expand Down Expand Up @@ -1119,6 +1110,15 @@ public void convertToTracingStatus() {
}
}


@Test
public void generateTraceSpanName() {
assertEquals(
"Sent.io.grpc.Foo", CensusTracingModule.generateTraceSpanName(false, "io.grpc/Foo"));
assertEquals(
"Recv.io.grpc.Bar", CensusTracingModule.generateTraceSpanName(true, "io.grpc/Bar"));
}

private static void assertNoServerContent(StatsTestUtils.MetricsRecord record) {
assertNull(record.getMetric(DeprecatedCensusConstants.RPC_SERVER_ERROR_COUNT));
assertNull(record.getMetric(DeprecatedCensusConstants.RPC_SERVER_REQUEST_COUNT));
Expand Down

0 comments on commit 9a38dea

Please sign in to comment.