From 1d3290da84503fb5db489381087dabd08b8a5141 Mon Sep 17 00:00:00 2001 From: Matej Gera <38492574+matej-g@users.noreply.github.com> Date: Thu, 19 Nov 2020 21:03:59 +0100 Subject: [PATCH] otelgrpc: Set attribute with gRPC status code (#453) * Add gRPC status code attribute * Adjust interceptor and gRPC tests * Update CHANGELOG Co-authored-by: Tyler Yahn --- CHANGELOG.md | 1 + .../grpc/otelgrpc/grpc_test.go | 71 ++++++++++-------- .../grpc/otelgrpc/grpctrace.go | 8 +- .../grpc/otelgrpc/interceptor.go | 17 +++++ .../grpc/otelgrpc/interceptor_test.go | 74 +++++++++++++++---- 5 files changed, 125 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44a3a939aa3..64549f1045d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add semantic version to `Tracer` / `Meter` created by instrumentation packages `otelsaram`, `otelrestful`, `otelmongo`, `otelhttp` and `otelhttptrace`. (#412) - Update instrumentation guidelines about tracer / meter semantic version. (#412) +- gRPC instrumentation sets span attribute `rpc.grpc.status_code`. (#453) ## Fixed diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/grpc_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/grpc_test.go index 08c5472f64f..57a583f9d31 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/grpc_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/grpc_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/interop" pb "google.golang.org/grpc/interop/grpc_testing" "google.golang.org/grpc/test/bufconn" @@ -136,9 +137,10 @@ func checkUnaryClientSpans(t *testing.T, spans []*tracetest.Span) { }, }, noTimestamp(emptySpan.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("EmptyCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("EmptyCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, emptySpan.Attributes()) largeSpan := spans[1] @@ -165,9 +167,10 @@ func checkUnaryClientSpans(t *testing.T, spans []*tracetest.Span) { }, }, noTimestamp(largeSpan.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("UnaryCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("UnaryCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, largeSpan.Attributes()) } @@ -214,9 +217,10 @@ func checkStreamClientSpans(t *testing.T, spans []*tracetest.Span) { // client does not record an event for the server response. }, noTimestamp(streamInput.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("StreamingInputCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("StreamingInputCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, streamInput.Attributes()) streamOutput := spans[1] @@ -266,9 +270,10 @@ func checkStreamClientSpans(t *testing.T, spans []*tracetest.Span) { }, }, noTimestamp(streamOutput.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("StreamingOutputCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("StreamingOutputCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, streamOutput.Attributes()) pingPong := spans[2] @@ -341,9 +346,10 @@ func checkStreamClientSpans(t *testing.T, spans []*tracetest.Span) { }, }, noTimestamp(pingPong.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("FullDuplexCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("FullDuplexCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, pingPong.Attributes()) } @@ -397,9 +403,10 @@ func checkStreamServerSpans(t *testing.T, spans []*tracetest.Span) { }, }, noTimestamp(streamInput.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("StreamingInputCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("StreamingInputCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, streamInput.Attributes()) streamOutput := spans[1] @@ -449,9 +456,10 @@ func checkStreamServerSpans(t *testing.T, spans []*tracetest.Span) { }, }, noTimestamp(streamOutput.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("StreamingOutputCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("StreamingOutputCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, streamOutput.Attributes()) pingPong := spans[2] @@ -524,9 +532,10 @@ func checkStreamServerSpans(t *testing.T, spans []*tracetest.Span) { }, }, noTimestamp(pingPong.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("FullDuplexCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("FullDuplexCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, pingPong.Attributes()) } @@ -555,9 +564,10 @@ func checkUnaryServerSpans(t *testing.T, spans []*tracetest.Span) { }, }, noTimestamp(emptySpan.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("EmptyCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("EmptyCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, emptySpan.Attributes()) largeSpan := spans[1] @@ -584,9 +594,10 @@ func checkUnaryServerSpans(t *testing.T, spans []*tracetest.Span) { }, }, noTimestamp(largeSpan.Events())) assert.Equal(t, map[label.Key]label.Value{ - semconv.RPCMethodKey: label.StringValue("UnaryCall"), - semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), - semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + semconv.RPCMethodKey: label.StringValue("UnaryCall"), + semconv.RPCServiceKey: label.StringValue("grpc.testing.TestService"), + semconv.RPCSystemGRPC.Key: semconv.RPCSystemGRPC.Value, + otelgrpc.GRPCStatusCodeKey: label.Uint32Value(uint32(codes.OK)), }, largeSpan.Attributes()) } diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/grpctrace.go b/instrumentation/google.golang.org/grpc/otelgrpc/grpctrace.go index f8c1956528c..69ef242502c 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/grpctrace.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/grpctrace.go @@ -25,8 +25,12 @@ import ( "go.opentelemetry.io/otel/label" ) -// instrumentationName is the name of this instrumentation package. -const instrumentationName = "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" +const ( + // instrumentationName is the name of this instrumentation package. + instrumentationName = "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" + // GRPCStatusCodeKey is convention for numeric status code of a gRPC request. + GRPCStatusCodeKey = label.Key("rpc.grpc.status_code") +) // config is a group of options for this instrumentation. type config struct { diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go index 0a661a0bf09..41d67373f60 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go @@ -25,6 +25,7 @@ import ( "github.com/golang/protobuf/proto" //nolint:staticcheck "google.golang.org/grpc" + grpc_codes "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" "google.golang.org/grpc/peer" "google.golang.org/grpc/status" @@ -104,6 +105,9 @@ func UnaryClientInterceptor(opts ...Option) grpc.UnaryClientInterceptor { if err != nil { s, _ := status.FromError(err) span.SetStatus(codes.Error, s.Message()) + span.SetAttributes(statusCodeAttr(s.Code())) + } else { + span.SetAttributes(statusCodeAttr(grpc_codes.OK)) } return err @@ -283,6 +287,9 @@ func StreamClientInterceptor(opts ...Option) grpc.StreamClientInterceptor { if err != nil { s, _ := status.FromError(err) span.SetStatus(codes.Error, s.Message()) + span.SetAttributes(statusCodeAttr(s.Code())) + } else { + span.SetAttributes(statusCodeAttr(grpc_codes.OK)) } span.End() @@ -327,8 +334,10 @@ func UnaryServerInterceptor(opts ...Option) grpc.UnaryServerInterceptor { if err != nil { s, _ := status.FromError(err) span.SetStatus(codes.Error, s.Message()) + span.SetAttributes(statusCodeAttr(s.Code())) messageSent.Event(ctx, 1, s.Proto()) } else { + span.SetAttributes(statusCodeAttr(grpc_codes.OK)) messageSent.Event(ctx, 1, resp) } @@ -413,6 +422,9 @@ func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor { if err != nil { s, _ := status.FromError(err) span.SetStatus(codes.Error, s.Message()) + span.SetAttributes(statusCodeAttr(s.Code())) + } else { + span.SetAttributes(statusCodeAttr(grpc_codes.OK)) } return err @@ -475,3 +487,8 @@ func parseFullMethod(fullMethod string) (string, []label.KeyValue) { } return name, attrs } + +// statusCodeAttr returns status code attribute based on given gRPC code +func statusCodeAttr(c grpc_codes.Code) label.KeyValue { + return GRPCStatusCodeKey.Uint32(uint32(c)) +} diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor_test.go index 9e822f04faf..9246b5c398e 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/interceptor_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/interceptor_test.go @@ -15,25 +15,24 @@ package otelgrpc import ( "context" + "strings" "sync" "testing" "time" "go.opentelemetry.io/otel/api/trace/tracetest" + "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/label" "go.opentelemetry.io/otel/semconv" + "github.com/golang/protobuf/proto" //nolint:staticcheck "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/golang/protobuf/proto" //nolint:staticcheck - "google.golang.org/grpc" - "google.golang.org/grpc/codes" + grpc_codes "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" - - otelcodes "go.opentelemetry.io/otel/codes" - "go.opentelemetry.io/otel/label" ) type SpanRecorder struct { @@ -66,6 +65,12 @@ type mockUICInvoker struct { func (mcuici *mockUICInvoker) invoker(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, opts ...grpc.CallOption) error { mcuici.ctx = ctx + + // if method contains error name, mock error return + if strings.Contains(method, "error") { + return status.Error(grpc_codes.Internal, "internal error") + } + return nil } @@ -96,10 +101,12 @@ func TestUnaryClientInterceptor(t *testing.T) { uniInterceptorInvoker := &mockUICInvoker{} checks := []struct { - method string - name string - expectedAttr map[label.Key]label.Value - eventsAttr []map[label.Key]label.Value + method string + name string + expectedSpanCode codes.Code + expectedAttr map[label.Key]label.Value + eventsAttr []map[label.Key]label.Value + expectErr bool }{ { method: "/github.com.serviceName/bar", @@ -108,6 +115,7 @@ func TestUnaryClientInterceptor(t *testing.T) { semconv.RPCSystemKey: label.StringValue("grpc"), semconv.RPCServiceKey: label.StringValue("github.com.serviceName"), semconv.RPCMethodKey: label.StringValue("bar"), + GRPCStatusCodeKey: label.Uint32Value(0), semconv.NetPeerIPKey: label.StringValue("fake"), semconv.NetPeerPortKey: label.StringValue("connection"), }, @@ -131,6 +139,7 @@ func TestUnaryClientInterceptor(t *testing.T) { semconv.RPCSystemKey: label.StringValue("grpc"), semconv.RPCServiceKey: label.StringValue("serviceName"), semconv.RPCMethodKey: label.StringValue("bar"), + GRPCStatusCodeKey: label.Uint32Value(0), semconv.NetPeerIPKey: label.StringValue("fake"), semconv.NetPeerPortKey: label.StringValue("connection"), }, @@ -154,6 +163,32 @@ func TestUnaryClientInterceptor(t *testing.T) { semconv.RPCSystemKey: label.StringValue("grpc"), semconv.RPCServiceKey: label.StringValue("serviceName"), semconv.RPCMethodKey: label.StringValue("bar"), + GRPCStatusCodeKey: label.Uint32Value(uint32(grpc_codes.OK)), + semconv.NetPeerIPKey: label.StringValue("fake"), + semconv.NetPeerPortKey: label.StringValue("connection"), + }, + eventsAttr: []map[label.Key]label.Value{ + { + semconv.RPCMessageTypeKey: label.StringValue("SENT"), + semconv.RPCMessageIDKey: label.IntValue(1), + semconv.RPCMessageUncompressedSizeKey: label.IntValue(proto.Size(proto.Message(req))), + }, + { + semconv.RPCMessageTypeKey: label.StringValue("RECEIVED"), + semconv.RPCMessageIDKey: label.IntValue(1), + semconv.RPCMessageUncompressedSizeKey: label.IntValue(proto.Size(proto.Message(reply))), + }, + }, + }, + { + method: "serviceName/bar_error", + name: "serviceName/bar_error", + expectedSpanCode: codes.Error, + expectedAttr: map[label.Key]label.Value{ + semconv.RPCSystemKey: label.StringValue("grpc"), + semconv.RPCServiceKey: label.StringValue("serviceName"), + semconv.RPCMethodKey: label.StringValue("bar_error"), + GRPCStatusCodeKey: label.Uint32Value(uint32(grpc_codes.Internal)), semconv.NetPeerIPKey: label.StringValue("fake"), semconv.NetPeerPortKey: label.StringValue("connection"), }, @@ -169,12 +204,14 @@ func TestUnaryClientInterceptor(t *testing.T) { semconv.RPCMessageUncompressedSizeKey: label.IntValue(proto.Size(proto.Message(reply))), }, }, + expectErr: true, }, { method: "invalidName", name: "invalidName", expectedAttr: map[label.Key]label.Value{ semconv.RPCSystemKey: label.StringValue("grpc"), + GRPCStatusCodeKey: label.Uint32Value(0), semconv.NetPeerIPKey: label.StringValue("fake"), semconv.NetPeerPortKey: label.StringValue("connection"), }, @@ -196,6 +233,7 @@ func TestUnaryClientInterceptor(t *testing.T) { name: "github.com.foo.serviceName_123/method", expectedAttr: map[label.Key]label.Value{ semconv.RPCSystemKey: label.StringValue("grpc"), + GRPCStatusCodeKey: label.Uint32Value(0), semconv.RPCServiceKey: label.StringValue("github.com.foo.serviceName_123"), semconv.RPCMethodKey: label.StringValue("method"), semconv.NetPeerIPKey: label.StringValue("fake"), @@ -217,13 +255,17 @@ func TestUnaryClientInterceptor(t *testing.T) { } for _, check := range checks { - if !assert.NoError(t, unaryInterceptor(context.Background(), check.method, req, reply, clientConn, uniInterceptorInvoker.invoker)) { - continue + err := unaryInterceptor(context.Background(), check.method, req, reply, clientConn, uniInterceptorInvoker.invoker) + if check.expectErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) } span, ok := sr.Get(check.name) if !assert.True(t, ok, "missing span %q", check.name) { continue } + assert.Equal(t, check.expectedSpanCode, span.StatusCode()) assert.Equal(t, check.expectedAttr, span.Attributes()) assert.Equal(t, check.eventsAttr, eventAttrMap(span.Events())) } @@ -309,6 +351,7 @@ func TestStreamClientInterceptor(t *testing.T) { expectedAttr := map[label.Key]label.Value{ semconv.RPCSystemKey: label.StringValue("grpc"), + GRPCStatusCodeKey: label.Uint32Value(uint32(grpc_codes.OK)), semconv.RPCServiceKey: label.StringValue("github.com.serviceName"), semconv.RPCMethodKey: label.StringValue("bar"), semconv.NetPeerIPKey: label.StringValue("fake"), @@ -342,7 +385,7 @@ func TestServerInterceptorError(t *testing.T) { sr := NewSpanRecorder() tp := tracetest.NewTracerProvider(tracetest.WithSpanRecorder(sr)) usi := UnaryServerInterceptor(WithTracerProvider(tp)) - deniedErr := status.Error(codes.PermissionDenied, "PERMISSION_DENIED_TEXT") + deniedErr := status.Error(grpc_codes.PermissionDenied, "PERMISSION_DENIED_TEXT") handler := func(_ context.Context, _ interface{}) (interface{}, error) { return nil, deniedErr } @@ -354,8 +397,11 @@ func TestServerInterceptorError(t *testing.T) { if !ok { t.Fatalf("failed to export error span") } - assert.Equal(t, span.StatusCode(), otelcodes.Error) + assert.Equal(t, codes.Error, span.StatusCode()) assert.Contains(t, deniedErr.Error(), span.StatusMessage()) + codeAttr, ok := span.Attributes()[GRPCStatusCodeKey] + assert.True(t, ok, "attributes contain gRPC status code") + assert.Equal(t, label.Uint32Value(uint32(grpc_codes.PermissionDenied)), codeAttr) assert.Len(t, span.Events(), 2) assert.Equal(t, map[label.Key]label.Value{ label.Key("message.type"): label.StringValue("SENT"),