From 54fffd64673def0e84eb3014ab0889d8f31a74db Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 9 Jul 2020 13:12:00 -0700 Subject: [PATCH] Update grpctrace instrumentation span names (#922) * Update grpctrace instrumentation span names 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 * Update Changelog * Update comment to plural * Switch from regexp to string parsing * Consolidate attributes before creating span * Update Changelog with addition of rpc.method in grpctrace * Fix test spanMap lookup key * Update instrumentation/grpctrace/interceptor.go Co-authored-by: ET * Unify on explicit typed return value * Fix copy paste error Co-authored-by: ET --- CHANGELOG.md | 4 + instrumentation/grpctrace/interceptor.go | 78 ++++++++------ instrumentation/grpctrace/interceptor_test.go | 100 +++++++++++++++--- 3 files changed, 135 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0df0168b922..eebdb179478 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add test for api.standard `HTTPClientAttributesFromHTTPRequest`. (#905) - Bump github.com/golangci/golangci-lint from 1.27.0 to 1.28.1 in /tools. (#901, #913) - Update otel-colector example to use the v0.5.0 collector. (#915) +- The `grpctrace` instrumentation uses a span name conforming to the OpenTelemetry semantic conventions (does not contain a leading slash (`/`)). (#922) +- The `grpctrace` instrumentation includes an `rpc.method` attribute now set to the gRPC method name. (#900, #922) +- The `grpctrace` instrumentation `rpc.service` attribute now contains the package name if one exists. + This is in accordance with OpenTelemetry semantic conventions. (#922) - Correlation Context extractor will no longer insert an empty map into the returned context when no valid values are extracted. (#923) ## [0.7.0] - 2020-06-26 diff --git a/instrumentation/grpctrace/interceptor.go b/instrumentation/grpctrace/interceptor.go index d6564825c43..3b4f0886dea 100644 --- a/instrumentation/grpctrace/interceptor.go +++ b/instrumentation/grpctrace/interceptor.go @@ -81,13 +81,13 @@ func UnaryClientInterceptor(tracer trace.Tracer) grpc.UnaryClientInterceptor { requestMetadata, _ := metadata.FromOutgoingContext(ctx) metadataCopy := requestMetadata.Copy() + name, attr := spanInfo(method, cc.Target()) var span trace.Span ctx, span = tracer.Start( - ctx, method, + ctx, + name, trace.WithSpanKind(trace.SpanKindClient), - trace.WithAttributes(peerInfoFromTarget(cc.Target())...), - trace.WithAttributes(standard.RPCSystemGRPC), - trace.WithAttributes(serviceAndMethodFromFullName(method)...), + trace.WithAttributes(attr...), ) defer span.End() @@ -260,13 +260,13 @@ func StreamClientInterceptor(tracer trace.Tracer) grpc.StreamClientInterceptor { requestMetadata, _ := metadata.FromOutgoingContext(ctx) metadataCopy := requestMetadata.Copy() + name, attr := spanInfo(method, cc.Target()) var span trace.Span ctx, span = tracer.Start( - ctx, method, + ctx, + name, trace.WithSpanKind(trace.SpanKindClient), - trace.WithAttributes(peerInfoFromTarget(cc.Target())...), - trace.WithAttributes(standard.RPCSystemGRPC), - trace.WithAttributes(serviceAndMethodFromFullName(method)...), + trace.WithAttributes(attr...), ) Inject(ctx, &metadataCopy) @@ -315,13 +315,12 @@ func UnaryServerInterceptor(tracer trace.Tracer) grpc.UnaryServerInterceptor { MultiKV: entries, })) + name, attr := spanInfo(info.FullMethod, peerFromCtx(ctx)) ctx, span := tracer.Start( trace.ContextWithRemoteSpanContext(ctx, spanCtx), - info.FullMethod, + name, trace.WithSpanKind(trace.SpanKindServer), - trace.WithAttributes(peerInfoFromContext(ctx)...), - trace.WithAttributes(standard.RPCSystemGRPC), - trace.WithAttributes(serviceAndMethodFromFullName(info.FullMethod)...), + trace.WithAttributes(attr...), ) defer span.End() @@ -406,13 +405,12 @@ func StreamServerInterceptor(tracer trace.Tracer) grpc.StreamServerInterceptor { MultiKV: entries, })) + name, attr := spanInfo(info.FullMethod, peerFromCtx(ctx)) ctx, span := tracer.Start( trace.ContextWithRemoteSpanContext(ctx, spanCtx), - info.FullMethod, + name, trace.WithSpanKind(trace.SpanKindServer), - trace.WithAttributes(peerInfoFromContext(ctx)...), - trace.WithAttributes(standard.RPCSystemGRPC), - trace.WithAttributes(serviceAndMethodFromFullName(info.FullMethod)...), + trace.WithAttributes(attr...), ) defer span.End() @@ -427,11 +425,21 @@ func StreamServerInterceptor(tracer trace.Tracer) grpc.StreamServerInterceptor { } } -func peerInfoFromTarget(target string) []kv.KeyValue { - host, port, err := net.SplitHostPort(target) +// spanInfo returns a span name and all appropriate attributes from the gRPC +// method and peer address. +func spanInfo(fullMethod, peerAddress string) (string, []kv.KeyValue) { + attrs := []kv.KeyValue{standard.RPCSystemGRPC} + name, mAttrs := parseFullMethod(fullMethod) + attrs = append(attrs, mAttrs...) + attrs = append(attrs, peerAttr(peerAddress)...) + return name, attrs +} +// peerAttr returns attributes about the peer address. +func peerAttr(addr string) []kv.KeyValue { + host, port, err := net.SplitHostPort(addr) if err != nil { - return []kv.KeyValue{} + return []kv.KeyValue(nil) } if host == "" { @@ -444,24 +452,32 @@ func peerInfoFromTarget(target string) []kv.KeyValue { } } -func peerInfoFromContext(ctx context.Context) []kv.KeyValue { +// peerFromCtx returns a peer address from a context, if one exists. +func peerFromCtx(ctx context.Context) string { p, ok := peer.FromContext(ctx) - if !ok { - return []kv.KeyValue{} + return "" } - - return peerInfoFromTarget(p.Addr.String()) + return p.Addr.String() } -func serviceAndMethodFromFullName(method string) []kv.KeyValue { - l := strings.LastIndexByte(method, '/') - if l == -1 { - return []kv.KeyValue{} +// parseFullMethod returns a span name following the OpenTelemetry semantic +// conventions as well as all applicable span kv.KeyValue attributes based +// on a gRPC's FullMethod. +func parseFullMethod(fullMethod string) (string, []kv.KeyValue) { + name := strings.TrimLeft(fullMethod, "/") + parts := strings.SplitN(name, "/", 2) + if len(parts) != 2 { + // Invalid format, does not follow `/package.service/method`. + return name, []kv.KeyValue(nil) } - return []kv.KeyValue{ - standard.RPCServiceKey.String(method[:l]), - standard.RPCMethodKey.String(method[l+1:]), + var attrs []kv.KeyValue + if service := parts[0]; service != "" { + attrs = append(attrs, standard.RPCServiceKey.String(service)) + } + if method := parts[1]; method != "" { + attrs = append(attrs, standard.RPCMethodKey.String(method)) } + return name, attrs } diff --git a/instrumentation/grpctrace/interceptor_test.go b/instrumentation/grpctrace/interceptor_test.go index 252f81061bb..fce3be59058 100644 --- a/instrumentation/grpctrace/interceptor_test.go +++ b/instrumentation/grpctrace/interceptor_test.go @@ -90,15 +90,17 @@ 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.RPCSystemKey: value.String("grpc"), - standard.RPCServiceKey: value.String("/github.com.serviceName"), + standard.RPCServiceKey: value.String("github.com.serviceName"), standard.RPCMethodKey: value.String("bar"), standard.NetPeerIPKey: value.String("fake"), standard.NetPeerPortKey: value.String("connection"), @@ -117,10 +119,11 @@ func TestUnaryClientInterceptor(t *testing.T) { }, }, { - name: "/serviceName/bar", + method: "/serviceName/bar", + name: "serviceName/bar", expectedAttr: map[kv.Key]value.Value{ standard.RPCSystemKey: value.String("grpc"), - standard.RPCServiceKey: value.String("/serviceName"), + standard.RPCServiceKey: value.String("serviceName"), standard.RPCMethodKey: value.String("bar"), standard.NetPeerIPKey: value.String("fake"), standard.NetPeerPortKey: value.String("connection"), @@ -139,7 +142,8 @@ func TestUnaryClientInterceptor(t *testing.T) { }, }, { - name: "serviceName/bar", + method: "serviceName/bar", + name: "serviceName/bar", expectedAttr: map[kv.Key]value.Value{ standard.RPCSystemKey: value.String("grpc"), standard.RPCServiceKey: value.String("serviceName"), @@ -161,7 +165,8 @@ func TestUnaryClientInterceptor(t *testing.T) { }, }, { - name: "invalidName", + method: "invalidName", + name: "invalidName", expectedAttr: map[kv.Key]value.Value{ standard.RPCSystemKey: value.String("grpc"), standard.NetPeerIPKey: value.String("fake"), @@ -181,10 +186,11 @@ 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.RPCSystemKey: value.String("grpc"), - standard.RPCServiceKey: value.String("/github.com.foo.serviceName_123"), + standard.RPCServiceKey: value.String("github.com.foo.serviceName_123"), standard.RPCMethodKey: value.String("method"), standard.NetPeerIPKey: value.String("fake"), standard.NetPeerPortKey: value.String("connection"), @@ -205,7 +211,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 @@ -298,12 +304,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, @@ -318,7 +325,7 @@ func TestStreamClientInterceptor(t *testing.T) { } // no span exported while stream is open - if _, ok := exp.spanMap[methodName]; ok { + if _, ok := exp.spanMap[name]; ok { t.Fatalf("span shouldn't end while stream is open") } @@ -341,7 +348,7 @@ 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 @@ -349,13 +356,13 @@ func TestStreamClientInterceptor(t *testing.T) { 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.RPCSystemKey: "grpc", - standard.RPCServiceKey: "/github.com.serviceName", + standard.RPCServiceKey: "github.com.serviceName", standard.RPCMethodKey: "bar", standard.NetPeerIPKey: "fake", standard.NetPeerPortKey: "connection", @@ -365,7 +372,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()) } } @@ -428,3 +435,64 @@ 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), + }, { + fullMethod: "/pkg.srv/", + name: "pkg.srv/", + attr: []kv.KeyValue{ + standard.RPCServiceKey.String("pkg.srv"), + }, + }, + } + + for _, test := range tests { + n, a := parseFullMethod(test.fullMethod) + assert.Equal(t, test.name, n) + assert.Equal(t, test.attr, a) + } +}