diff --git a/CHANGELOG.md b/CHANGELOG.md index 41f06d013be..221de9c5a17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed - Use `context.Background()` as default context instead of nil in `go.opentelemetry.io/contrib/bridges/otellogr`. (#6527) +- Don't start spans that never end for filtered out gRPC stats handler in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc`. (#6695) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go index c01cb897cd3..216127d6fb8 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go @@ -59,20 +59,26 @@ func (h *serverHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) cont name, attrs := internal.ParseFullMethod(info.FullMethodName) attrs = append(attrs, RPCSystemGRPC) - ctx, _ = h.tracer.Start( - trace.ContextWithRemoteSpanContext(ctx, trace.SpanContextFromContext(ctx)), - name, - trace.WithSpanKind(trace.SpanKindServer), - trace.WithAttributes(append(attrs, h.config.SpanAttributes...)...), - ) + + record := true + if h.config.Filter != nil { + record = h.config.Filter(info) + } + + if record { + ctx, _ = h.tracer.Start( + trace.ContextWithRemoteSpanContext(ctx, trace.SpanContextFromContext(ctx)), + name, + trace.WithSpanKind(trace.SpanKindServer), + trace.WithAttributes(append(attrs, h.config.SpanAttributes...)...), + ) + } gctx := gRPCContext{ metricAttrs: append(attrs, h.config.MetricAttributes...), - record: true, - } - if h.config.Filter != nil { - gctx.record = h.config.Filter(info) + record: record, } + return context.WithValue(ctx, gRPCContextKey{}, &gctx) } @@ -99,19 +105,24 @@ func NewClientHandler(opts ...Option) stats.Handler { func (h *clientHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context { name, attrs := internal.ParseFullMethod(info.FullMethodName) attrs = append(attrs, RPCSystemGRPC) - ctx, _ = h.tracer.Start( - ctx, - name, - trace.WithSpanKind(trace.SpanKindClient), - trace.WithAttributes(append(attrs, h.config.SpanAttributes...)...), - ) + + record := true + if h.config.Filter != nil { + record = h.config.Filter(info) + } + + if record { + ctx, _ = h.tracer.Start( + ctx, + name, + trace.WithSpanKind(trace.SpanKindClient), + trace.WithAttributes(append(attrs, h.config.SpanAttributes...)...), + ) + } gctx := gRPCContext{ metricAttrs: append(attrs, h.config.MetricAttributes...), - record: true, - } - if h.config.Filter != nil { - gctx.record = h.config.Filter(info) + record: record, } return inject(context.WithValue(ctx, gRPCContextKey{}, &gctx), h.config.Propagators) diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_stats_handler_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_stats_handler_test.go index 884c60c1c21..e434b43998d 100644 --- a/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_stats_handler_test.go +++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/grpc_stats_handler_test.go @@ -17,6 +17,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/codes" testpb "google.golang.org/grpc/interop/grpc_testing" + "google.golang.org/grpc/stats" "google.golang.org/grpc/status" "go.opentelemetry.io/otel/attribute" @@ -27,6 +28,7 @@ import ( "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" semconv "go.opentelemetry.io/otel/semconv/v1.17.0" + oteltrace "go.opentelemetry.io/otel/trace" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc/filters" @@ -1536,3 +1538,111 @@ func TestStatsHandlerConcurrentSafeContextCancellation(t *testing.T) { wg.Wait() } } + +func TestServerHandlerTagRPC(t *testing.T) { + tests := []struct { + name string + server stats.Handler + ctx context.Context + info *stats.RPCTagInfo + exp bool + }{ + { + name: "start a span without filters", + server: otelgrpc.NewServerHandler(otelgrpc.WithTracerProvider(trace.NewTracerProvider())), + ctx: context.Background(), + info: &stats.RPCTagInfo{ + FullMethodName: "/grpc.health.v1.Health/Check", + }, + exp: true, + }, + { + name: "don't start a span with filter and match", + server: otelgrpc.NewServerHandler(otelgrpc.WithTracerProvider(trace.NewTracerProvider()), otelgrpc.WithFilter(func(ri *stats.RPCTagInfo) bool { + return ri.FullMethodName != "/grpc.health.v1.Health/Check" + })), + ctx: context.Background(), + info: &stats.RPCTagInfo{ + FullMethodName: "/grpc.health.v1.Health/Check", + }, + exp: false, + }, + { + name: "start a span with filter and no match", + server: otelgrpc.NewServerHandler(otelgrpc.WithTracerProvider(trace.NewTracerProvider()), otelgrpc.WithFilter(func(ri *stats.RPCTagInfo) bool { + return ri.FullMethodName != "/grpc.health.v1.Health/Check" + })), + ctx: context.Background(), + info: &stats.RPCTagInfo{ + FullMethodName: "/app.v1.Service/Get", + }, + exp: true, + }, + } + + for _, tt := range tests { + t.Run(t.Name(), func(t *testing.T) { + ctx := tt.server.TagRPC(tt.ctx, tt.info) + + got := oteltrace.SpanFromContext(ctx).IsRecording() + + if tt.exp != got { + t.Errorf("expected %t, got %t", tt.exp, got) + } + }) + } +} + +func TestClientHandlerTagRPC(t *testing.T) { + tests := []struct { + name string + client stats.Handler + ctx context.Context + info *stats.RPCTagInfo + exp bool + }{ + { + name: "start a span without filters", + client: otelgrpc.NewClientHandler(otelgrpc.WithTracerProvider(trace.NewTracerProvider())), + ctx: context.Background(), + info: &stats.RPCTagInfo{ + FullMethodName: "/grpc.health.v1.Health/Check", + }, + exp: true, + }, + { + name: "don't start a span with filter and match", + client: otelgrpc.NewClientHandler(otelgrpc.WithTracerProvider(trace.NewTracerProvider()), otelgrpc.WithFilter(func(ri *stats.RPCTagInfo) bool { + return ri.FullMethodName != "/grpc.health.v1.Health/Check" + })), + ctx: context.Background(), + info: &stats.RPCTagInfo{ + FullMethodName: "/grpc.health.v1.Health/Check", + }, + exp: false, + }, + { + name: "start a span with filter and no match", + client: otelgrpc.NewClientHandler(otelgrpc.WithTracerProvider(trace.NewTracerProvider()), otelgrpc.WithFilter(func(ri *stats.RPCTagInfo) bool { + return ri.FullMethodName != "/grpc.health.v1.Health/Check" + })), + ctx: context.Background(), + info: &stats.RPCTagInfo{ + FullMethodName: "/app.v1.Service/Get", + }, + exp: true, + }, + } + + for _, tt := range tests { + t.Run(t.Name(), func(t *testing.T) { + ctx := tt.client.TagRPC(tt.ctx, tt.info) + + got := oteltrace.SpanFromContext(ctx).IsRecording() + + if tt.exp != got { + t.Errorf("expected %t, got %t", tt.exp, got) + } + }) + } +}