Skip to content

Commit

Permalink
Update grpctrace instrumentation span names
Browse files Browse the repository at this point in the history 
Span names MUST not contain the leading slash (`/`) that the grpc
package prepends to all `FullMethod` values. This replaces the
`serviceFromFullMethod` function with a parsing function. This parsing
function returns an span name adhering to the OpenTelemetry semantic
conventions as well as formatted span attributes.

Additionally, the service name needs to include the package if one
exists. This updates that attribute accordingly.

Once #900 is merged the method attributes can be added by uncommenting.

Resolves #916
  • Loading branch information
@MrAlias
MrAlias committed Jul 8, 2020
1 parent c506e99 commit bdc35ff
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 31 deletions.
49 changes: 34 additions & 15 deletions instrumentation/grpctrace/interceptor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,14 @@ func UnaryClientInterceptor(tracer trace.Tracer) grpc.UnaryClientInterceptor {
requestMetadata, _ := metadata.FromOutgoingContext(ctx)
metadataCopy := requestMetadata.Copy()

name, attr := parseFullMethod(method)
var span trace.Span
ctx, span = tracer.Start(
ctx, method,
ctx,
name,
trace.WithSpanKind(trace.SpanKindClient),
trace.WithAttributes(peerInfoFromTarget(cc.Target())...),
trace.WithAttributes(standard.RPCServiceKey.String(serviceFromFullMethod(method))),
trace.WithAttributes(attr...),
)
defer span.End()

Expand Down Expand Up @@ -259,12 +261,14 @@ func StreamClientInterceptor(tracer trace.Tracer) grpc.StreamClientInterceptor {
requestMetadata, _ := metadata.FromOutgoingContext(ctx)
metadataCopy := requestMetadata.Copy()

name, attr := parseFullMethod(method)
var span trace.Span
ctx, span = tracer.Start(
ctx, method,
ctx,
name,
trace.WithSpanKind(trace.SpanKindClient),
trace.WithAttributes(peerInfoFromTarget(cc.Target())...),
trace.WithAttributes(standard.RPCServiceKey.String(serviceFromFullMethod(method))),
trace.WithAttributes(attr...),
)

Inject(ctx, &metadataCopy)
Expand Down Expand Up @@ -313,12 +317,13 @@ func UnaryServerInterceptor(tracer trace.Tracer) grpc.UnaryServerInterceptor {
MultiKV: entries,
}))

name, attr := parseFullMethod(info.FullMethod)
ctx, span := tracer.Start(
trace.ContextWithRemoteSpanContext(ctx, spanCtx),
info.FullMethod,
name,
trace.WithSpanKind(trace.SpanKindServer),
trace.WithAttributes(peerInfoFromContext(ctx)...),
trace.WithAttributes(standard.RPCServiceKey.String(serviceFromFullMethod(info.FullMethod))),
trace.WithAttributes(attr...),
)
defer span.End()

Expand Down Expand Up @@ -403,12 +408,13 @@ func StreamServerInterceptor(tracer trace.Tracer) grpc.StreamServerInterceptor {
MultiKV: entries,
}))

name, attr := parseFullMethod(info.FullMethod)
ctx, span := tracer.Start(
trace.ContextWithRemoteSpanContext(ctx, spanCtx),
info.FullMethod,
name,
trace.WithSpanKind(trace.SpanKindServer),
trace.WithAttributes(peerInfoFromContext(ctx)...),
trace.WithAttributes(standard.RPCServiceKey.String(serviceFromFullMethod(info.FullMethod))),
trace.WithAttributes(attr...),
)
defer span.End()

Expand All @@ -427,7 +433,7 @@ func peerInfoFromTarget(target string) []kv.KeyValue {
host, port, err := net.SplitHostPort(target)

if err != nil {
return []kv.KeyValue{}
return []kv.KeyValue(nil)
}

if host == "" {
Expand All @@ -444,19 +450,32 @@ func peerInfoFromContext(ctx context.Context) []kv.KeyValue {
p, ok := peer.FromContext(ctx)

if !ok {
return []kv.KeyValue{}
return []kv.KeyValue(nil)
}

return peerInfoFromTarget(p.Addr.String())
}

var fullMethodRegexp = regexp.MustCompile(`^\/?(?:\S+\.)?(\S+)\/\S+$`)
var fullMethodRegexp = regexp.MustCompile(`^\/?(((?:\S+\.)?\S+)\/(\S+))$`)

func serviceFromFullMethod(method string) string {
match := fullMethodRegexp.FindStringSubmatch(method)
// parseFullMethod returns an span name following the OpenTelemetry semantic
// conventions as well as all applicable span kv.KeyValue attributes based
// on a gRPC's FullMethod. If no name is parsable, full is returned with an
// empty attribute slice.
func parseFullMethod(full string) (string, []kv.KeyValue) {
match := fullMethodRegexp.FindStringSubmatch(full)
if len(match) == 0 {
return ""
// Worse than incorrectly named spans is empty named spans.
return full, nil
}

return match[1]
var attrs []kv.KeyValue
if match[2] != "" {
attrs = append(attrs, standard.RPCServiceKey.String(match[2]))
}
// TODO [MrAlias]: uncomment when #900 merges
//if match[3] != "" {
// attrs = append(attrs, standard.RPCMethodKey.String(match[3]))
//}
return match[1], attrs
}
93 changes: 77 additions & 16 deletions instrumentation/grpctrace/interceptor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,16 @@ func TestUnaryClientInterceptor(t *testing.T) {
uniInterceptorInvoker := &mockUICInvoker{}

checks := []struct {
method string
name string
expectedAttr map[kv.Key]value.Value
eventsAttr []map[kv.Key]value.Value
}{
{
name: "/github.com.serviceName/bar",
method: "/github.com.serviceName/bar",
name: "github.com.serviceName/bar",
expectedAttr: map[kv.Key]value.Value{
standard.RPCServiceKey: value.String("serviceName"),
standard.RPCServiceKey: value.String("github.com.serviceName"),
standard.NetPeerIPKey: value.String("fake"),
standard.NetPeerPortKey: value.String("connection"),
},
Expand All @@ -115,7 +117,8 @@ func TestUnaryClientInterceptor(t *testing.T) {
},
},
{
name: "/serviceName/bar",
method: "/serviceName/bar",
name: "serviceName/bar",
expectedAttr: map[kv.Key]value.Value{
standard.RPCServiceKey: value.String("serviceName"),
standard.NetPeerIPKey: value.String("fake"),
Expand All @@ -135,7 +138,8 @@ func TestUnaryClientInterceptor(t *testing.T) {
},
},
{
name: "serviceName/bar",
method: "serviceName/bar",
name: "serviceName/bar",
expectedAttr: map[kv.Key]value.Value{
standard.RPCServiceKey: value.String("serviceName"),
standard.NetPeerIPKey: value.String("fake"),
Expand All @@ -155,9 +159,9 @@ func TestUnaryClientInterceptor(t *testing.T) {
},
},
{
name: "invalidName",
method: "invalidName",
name: "invalidName",
expectedAttr: map[kv.Key]value.Value{
standard.RPCServiceKey: value.String(""),
standard.NetPeerIPKey: value.String("fake"),
standard.NetPeerPortKey: value.String("connection"),
},
Expand All @@ -175,9 +179,10 @@ func TestUnaryClientInterceptor(t *testing.T) {
},
},
{
name: "/github.com.foo.serviceName_123/method",
method: "/github.com.foo.serviceName_123/method",
name: "github.com.foo.serviceName_123/method",
expectedAttr: map[kv.Key]value.Value{
standard.RPCServiceKey: value.String("serviceName_123"),
standard.RPCServiceKey: value.String("github.com.foo.serviceName_123"),
standard.NetPeerIPKey: value.String("fake"),
standard.NetPeerPortKey: value.String("connection"),
},
Expand All @@ -197,7 +202,7 @@ func TestUnaryClientInterceptor(t *testing.T) {
}

for _, check := range checks {
err = unaryInterceptor(context.Background(), check.name, req, reply, clientConn, uniInterceptorInvoker.invoker)
err = unaryInterceptor(context.Background(), check.method, req, reply, clientConn, uniInterceptorInvoker.invoker)
if err != nil {
t.Errorf("failed to run unary interceptor: %v", err)
continue
Expand Down Expand Up @@ -290,12 +295,13 @@ func TestStreamClientInterceptor(t *testing.T) {
streamCI := StreamClientInterceptor(tracer)

var mockClStr mockClientStream
methodName := "/github.com.serviceName/bar"
method := "/github.com.serviceName/bar"
name := "github.com.serviceName/bar"

streamClient, err := streamCI(context.Background(),
&grpc.StreamDesc{ServerStreams: true},
clientConn,
methodName,
method,
func(ctx context.Context,
desc *grpc.StreamDesc,
cc *grpc.ClientConn,
Expand All @@ -310,7 +316,7 @@ func TestStreamClientInterceptor(t *testing.T) {
}

// no span exported while stream is open
if _, ok := exp.spanMap[methodName]; ok {
if _, ok := exp.spanMap[method]; ok {
t.Fatalf("span shouldn't end while stream is open")
}

Expand All @@ -333,20 +339,20 @@ func TestStreamClientInterceptor(t *testing.T) {
for retry := 0; retry < 5; retry++ {
ok := false
exp.mu.Lock()
spanData, ok = exp.spanMap[methodName]
spanData, ok = exp.spanMap[name]
exp.mu.Unlock()
if ok {
break
}
time.Sleep(time.Second * 1)
}
if spanData == nil {
t.Fatalf("no span data found for name < %s >", methodName)
t.Fatalf("no span data found for name < %s >", name)
}

attrs := spanData.Attributes
expectedAttr := map[kv.Key]string{
standard.RPCServiceKey: "serviceName",
standard.RPCServiceKey: "github.com.serviceName",
standard.NetPeerIPKey: "fake",
standard.NetPeerPortKey: "connection",
}
Expand All @@ -355,7 +361,7 @@ func TestStreamClientInterceptor(t *testing.T) {
expected, ok := expectedAttr[attr.Key]
if ok {
if expected != attr.Value.AsString() {
t.Errorf("name: %s invalid %s found. expected %s, actual %s", methodName, string(attr.Key),
t.Errorf("name: %s invalid %s found. expected %s, actual %s", name, string(attr.Key),
expected, attr.Value.AsString())
}
}
Expand Down Expand Up @@ -418,3 +424,58 @@ func TestServerInterceptorError(t *testing.T) {
kv.Int("message.uncompressed_size", 26),
}, span.MessageEvents[1].Attributes)
}

func TestParseFullMethod(t *testing.T) {
tests := []struct {
fullMethod string
name string
attr []kv.KeyValue
}{
{
fullMethod: "/grpc.test.EchoService/Echo",
name: "grpc.test.EchoService/Echo",
attr: []kv.KeyValue{
standard.RPCServiceKey.String("grpc.test.EchoService"),
//standard.RPCMethodKey.String("Echo"),
},
}, {
fullMethod: "/com.example.ExampleRmiService/exampleMethod",
name: "com.example.ExampleRmiService/exampleMethod",
attr: []kv.KeyValue{
standard.RPCServiceKey.String("com.example.ExampleRmiService"),
//standard.RPCMethodKey.String("exampleMethod"),
},
}, {
fullMethod: "/MyCalcService.Calculator/Add",
name: "MyCalcService.Calculator/Add",
attr: []kv.KeyValue{
standard.RPCServiceKey.String("MyCalcService.Calculator"),
//standard.RPCMethodKey.String("Add"),
},
}, {
fullMethod: "/MyServiceReference.ICalculator/Add",
name: "MyServiceReference.ICalculator/Add",
attr: []kv.KeyValue{
standard.RPCServiceKey.String("MyServiceReference.ICalculator"),
//standard.RPCMethodKey.String("Add"),
},
}, {
fullMethod: "/MyServiceWithNoPackage/theMethod",
name: "MyServiceWithNoPackage/theMethod",
attr: []kv.KeyValue{
standard.RPCServiceKey.String("MyServiceWithNoPackage"),
//standard.RPCMethodKey.String("theMethod"),
},
}, {
fullMethod: "/pkg.srv/",
name: "/pkg.srv/",
attr: []kv.KeyValue(nil),
},
}

for _, test := range tests {
n, a := parseFullMethod(test.fullMethod)
assert.Equal(t, test.name, n)
assert.Equal(t, test.attr, a)
}
}

0 comments on commit bdc35ff

Please sign in to comment.