otelgrpc: gctx nil check before make metricAttrs

[zchee]

If wrapped otelgrpc.StatsHander, sometimes occur nil panic.

[dashpole]

Can you add a test?

[dashpole]

Can you add a comment here? In what cases are we skipping the recording?

[zchee]

Actually, I was trying to wrap otelgrpc.StatsHandler with otelgrpc/filters support in the company's internal code.

---

What comment is correct?

[dashpole]

I'm not sure... Why are you skipping these lines when the context is nil? Any why skip rpcRequestsPerRPC and pcResponsesPerRPC, but not rpcDuration?

[zchee]

We want to just use filters.HealthCheck, like #4575.

[zchee]

Well, there are already checks gctx nil check on

opentelemetry-go-contrib/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go

Line 153 in d50aa19

if gctx != nil {

, This change will be more safer (without wraps otelgrpc.StatsHandler, etc)

[zchee]

@dashpole Added testcase.
But there are some duplicated codes. I considered to below convert to a table-driven test but more complexity I think.

PTAL.

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 f8dd8871..14696a10 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
@@ -26,6 +26,7 @@ import (
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/interop"
+	"google.golang.org/grpc/stats"
 	"google.golang.org/grpc/status"
 
 	"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"
@@ -42,33 +43,84 @@ import (
 	semconv "go.opentelemetry.io/otel/semconv/v1.17.0"
 )
 
+type statsFn func(opts ...otelgrpc.Option) stats.Handler
+
+type noopHandler struct {
+	handler stats.Handler
+}
+
+func newNoopClientHandler(opts ...otelgrpc.Option) stats.Handler {
+	return &noopHandler{
+		handler: otelgrpc.NewClientHandler(opts...),
+	}
+}
+
+func newNoopServerHandler(opts ...otelgrpc.Option) stats.Handler {
+	return &noopHandler{
+		handler: otelgrpc.NewServerHandler(opts...),
+	}
+}
+
+func (h *noopHandler) TagRPC(ctx context.Context, _ *stats.RPCTagInfo) context.Context {
+	return ctx
+}
+func (h *noopHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
+	h.handler.HandleRPC(ctx, rs)
+}
+func (h *noopHandler) TagConn(ctx context.Context, ci *stats.ConnTagInfo) context.Context {
+	return ctx
+}
+func (h *noopHandler) HandleConn(context.Context, stats.ConnStats) {}
+
 func TestStatsHandler(t *testing.T) {
+	tests := map[string]struct {
+		clientStatsFn statsFn
+		serverStatsFn statsFn
+	}{
+		"Nornal": {
+			clientStatsFn: otelgrpc.NewClientHandler,
+			serverStatsFn: otelgrpc.NewServerHandler,
+		},
+		"Noop": {
+			clientStatsFn: newNoopClientHandler,
+			serverStatsFn: newNoopServerHandler,
+		},
+	}
+	for _, tt := range tests {
+		testStatsHandler(t, tt.clientStatsFn, tt.serverStatsFn)
+	}
+}
+
+func testStatsHandler(t *testing.T, clientFn, serverFn statsFn) {
 	clientSR := tracetest.NewSpanRecorder()
 	clientTP := trace.NewTracerProvider(trace.WithSpanProcessor(clientSR))
 	clientMetricReader := metric.NewManualReader()
 	clientMP := metric.NewMeterProvider(metric.WithReader(clientMetricReader))
 
 	serverSR := tracetest.NewSpanRecorder()
 	serverTP := trace.NewTracerProvider(trace.WithSpanProcessor(serverSR))
 	serverMetricReader := metric.NewManualReader()
 	serverMP := metric.NewMeterProvider(metric.WithReader(serverMetricReader))
 
 	listener, err := net.Listen("tcp", "127.0.0.1:0")
 	require.NoError(t, err, "failed to open port")
+
+	copts := []otelgrpc.Option{
+		otelgrpc.WithTracerProvider(clientTP),
+		otelgrpc.WithMeterProvider(clientMP),
+		otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents),
+	}
+	sopts := []otelgrpc.Option{
+		otelgrpc.WithTracerProvider(serverTP),
+		otelgrpc.WithMeterProvider(serverMP),
+		otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents),
+	}
 	client := newGrpcTest(t, listener,
 		[]grpc.DialOption{
-			grpc.WithStatsHandler(otelgrpc.NewClientHandler(
-				otelgrpc.WithTracerProvider(clientTP),
-				otelgrpc.WithMeterProvider(clientMP),
-				otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents)),
-			),
+			grpc.WithStatsHandler(clientFn(copts...)),
 		},
 		[]grpc.ServerOption{
-			grpc.StatsHandler(otelgrpc.NewServerHandler(
-				otelgrpc.WithTracerProvider(serverTP),
-				otelgrpc.WithMeterProvider(serverMP),
-				otelgrpc.WithMessageEvents(otelgrpc.ReceivedEvents, otelgrpc.SentEvents)),
-			),
+			grpc.StatsHandler(serverFn(sopts...)),
 		},
 	)
 	doCalls(client)

[codecov]

Codecov Report

Merging #4632 (4b2f3d0) into main (d50aa19) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #4632   +/-   ##
=====================================
  Coverage   80.8%   80.8%           
=====================================
  Files        150     150           
  Lines      10374   10379    +5     
=====================================
+ Hits        8388    8393    +5     
  Misses      1844    1844           
  Partials     142     142           

Files	Coverage Δ
...n/google.golang.org/grpc/otelgrpc/stats_handler.go	93.3% <100.0%> (+0.2%)	⬆️

[zchee]

@dashpole Alternatively, can I send a PR for filters support in StatsHandler?