Repository: open-telemetry/opentelemetry-go-contrib (go)

Issue:
otelgrpc: server stats handler sets all errors to error=true
### Description

Any server gRPC error codes are treated with `error=true`,  according to [gRPC status spec](https://opentelemetry.io/docs/specs/semconv/rpc/grpc/#grpc-status) only certain server gRPC status codes should set `error=true`.

In the interceptor we check the serverStatus with https://github.com/open-telemetry/opentelemetry-go-contrib/blob/ba19074a6785b2b65c86a52c0649cf040319fa96/instrumentation/google.golang.org/grpc/otelgrpc/interceptor.go#L328-L330 (correct behavior)

stats handler does not call `serverStatus` for server spans to check if error should be set to true: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/ba19074a6785b2b65c86a52c0649cf040319fa96/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go#L187-L189 (incorrect behavior)

(Need to check if it is server or client stats handler to determine if we want to call `serverStatus`)

### Environment

- OS: macOS
- Architecture: [e.g. x86, i386]
- Go Version: 1.21
- `otelgrpc` version: v0.46.1

### Steps To Reproduce

1. Write a grpc endpoint that returns `PERMISSION_DENIED`
2. Use `grpc.NewServer(grpc.StatsHandler(otelgrpc.NewServerHandler()))`
3. Call grpc endpoint
4. Trace has error set to true
resulting to this (from stdout)
```
"Status": {
	"Code": "Error",
	"Description": "PERMISSION_DENIED"
},
```

### Expected behavior

If I switch to using `grpc.NewServer(grpc.UnaryInterceptor(otelgrpc.UnaryServerInterceptor()))`
I get the expected behavior:
```
"Status": {
	"Code": "Unset",
	"Description": ""
},
```


Pull request:
otelgrpc: Fix NewServerHandler to correctly set span status
fix #4586

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 6d674d136..80b518ca4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108)
 
+### Fixed
+
+- Fix `NewServerHandler` in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc` to correctly set the span status depending on the gRPC status. (#4587)
+
 ## [1.21.1/0.46.1/0.15.1/0.1.1] - 2023-11-16
 
 ### Changed
diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
index e41e6df61..365b1cfea 100644
--- a/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
+++ b/instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go
@@ -85,7 +85,8 @@ func (h *serverHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) cont
 
 // HandleRPC processes the RPC stats.
 func (h *serverHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
-	h.handleRPC(ctx, rs)
+	isServer := true
+	h.handleRPC(ctx, rs, isServer)
 }
 
 type clientHandler struct {
@@ -121,7 +122,8 @@ func (h *clientHandler) TagRPC(ctx context.Context, info *stats.RPCTagInfo) cont
 
 // HandleRPC processes the RPC stats.
 func (h *clientHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
-	h.handleRPC(ctx, rs)
+	isServer := false
+	h.handleRPC(ctx, rs, isServer)
 }
 
 // TagConn can attach some information to the given context.
@@ -137,7 +139,7 @@ func (h *clientHandler) HandleConn(context.Context, stats.ConnStats) {
 	// no-op
 }
 
-func (c *config) handleRPC(ctx context.Context, rs stats.RPCStats) {
+func (c *config) handleRPC(ctx context.Context, rs stats.RPCStats, isServer bool) { // nolint: revive  // isServer is not a control flag.
 	span := trace.SpanFromContext(ctx)
 	gctx, _ := ctx.Value(gRPCContextKey{}).(*gRPCContext)
 	var messageId int64
@@ -185,7 +187,12 @@ func (c *config) handleRPC(ctx context.Context, rs stats.RPCStats) {
 
 		if rs.Error != nil {
 			s, _ := status.FromError(rs.Error)
-			span.SetStatus(codes.Error, s.Message())
+			if isServer {
+				statusCode, msg := serverStatus(s)
+				span.SetStatus(statusCode, msg)
+			} else {
+				span.SetStatus(codes.Error, s.Message())
+			}
 			rpcStatusAttr = semconv.RPCGRPCStatusCodeKey.Int(int(s.Code()))
 		} else {
 			rpcStatusAttr = semconv.RPCGRPCStatusCodeKey.Int(int(grpc_codes.OK))


Test patch:
diff --git a/instrumentation/google.golang.org/grpc/otelgrpc/test/stats_handler_test.go b/instrumentation/google.golang.org/grpc/otelgrpc/test/stats_handler_test.go
new file mode 100644
index 000000000..48a996d2e
--- /dev/null
+++ b/instrumentation/google.golang.org/grpc/otelgrpc/test/stats_handler_test.go
@@ -0,0 +1,141 @@
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	grpc_codes "google.golang.org/grpc/codes"
+	"google.golang.org/grpc/stats"
+	"google.golang.org/grpc/status"
+
+	"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"
+	"go.opentelemetry.io/otel/attribute"
+	"go.opentelemetry.io/otel/sdk/metric"
+	"go.opentelemetry.io/otel/sdk/metric/metricdata"
+	"go.opentelemetry.io/otel/sdk/metric/metricdata/metricdatatest"
+	"go.opentelemetry.io/otel/sdk/trace"
+	"go.opentelemetry.io/otel/sdk/trace/tracetest"
+	semconv "go.opentelemetry.io/otel/semconv/v1.17.0"
+)
+
+func TestStatsHandlerHandleRPCServerErrors(t *testing.T) {
+	for _, check := range serverChecks {
+		name := check.grpcCode.String()
+		t.Run(name, func(t *testing.T) {
+			sr := tracetest.NewSpanRecorder()
+			tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr))
+
+			mr := metric.NewManualReader()
+			mp := metric.NewMeterProvider(metric.WithReader(mr))
+
+			serverHandler := otelgrpc.NewServerHandler(
+				otelgrpc.WithTracerProvider(tp),
+				otelgrpc.WithMeterProvider(mp),
+			)
+
+			serviceName := "TestGrpcService"
+			methodName := serviceName + "/" + name
+			fullMethodName := "/" + methodName
+			// call the server handler
+			ctx := serverHandler.TagRPC(context.Background(), &stats.RPCTagInfo{
+				FullMethodName: fullMethodName,
+			})
+
+			grpcErr := status.Error(check.grpcCode, check.grpcCode.String())
+			serverHandler.HandleRPC(ctx, &stats.End{
+				Error: grpcErr,
+			})
+
+			// validate span
+			span, ok := getSpanFromRecorder(sr, methodName)
+			require.True(t, ok, "missing span %s", methodName)
+			assertServerSpan(t, check.wantSpanCode, check.wantSpanStatusDescription, check.grpcCode, span)
+
+			// validate metric
+			assertStatsHandlerServerMetrics(t, mr, serviceName, name, check.grpcCode)
+		})
+	}
+}
+
+func assertStatsHandlerServerMetrics(t *testing.T, reader metric.Reader, serviceName, name string, code grpc_codes.Code) {
+	want := metricdata.ScopeMetrics{
+		Scope: wantInstrumentationScope,
+		Metrics: []metricdata.Metrics{
+			{
+				Name:        "rpc.server.duration",
+				Description: "Measures the duration of inbound RPC.",
+				Unit:        "ms",
+				Data: metricdata.Histogram[float64]{
+					Temporality: metricdata.CumulativeTemporality,
+					DataPoints: []metricdata.HistogramDataPoint[float64]{
+						{
+							Attributes: attribute.NewSet(
+								semconv.RPCMethod(name),
+								semconv.RPCService(serviceName),
+								otelgrpc.RPCSystemGRPC,
+								otelgrpc.GRPCStatusCodeKey.Int64(int64(code)),
+							),
+						},
+					},
+				},
+			},
+			{
+				Name:        "rpc.server.requests_per_rpc",
+				Description: "Measures the number of messages received per RPC. Should be 1 for all non-streaming RPCs.",
+				Unit:        "{count}",
+				Data: metricdata.Histogram[int64]{
+					Temporality: metricdata.CumulativeTemporality,
+					DataPoints: []metricdata.HistogramDataPoint[int64]{
+						{
+							Attributes: attribute.NewSet(
+								semconv.RPCMethod(name),
+								semconv.RPCService(serviceName),
+								otelgrpc.RPCSystemGRPC,
+								otelgrpc.GRPCStatusCodeKey.Int64(int64(code)),
+							),
+						},
+					},
+				},
+			},
+			{
+				Name:        "rpc.server.responses_per_rpc",
+				Description: "Measures the number of messages received per RPC. Should be 1 for all non-streaming RPCs.",
+				Unit:        "{count}",
+				Data: metricdata.Histogram[int64]{
+					Temporality: metricdata.CumulativeTemporality,
+					DataPoints: []metricdata.HistogramDataPoint[int64]{
+						{
+							Attributes: attribute.NewSet(
+								semconv.RPCMethod(name),
+								semconv.RPCService(serviceName),
+								otelgrpc.RPCSystemGRPC,
+								otelgrpc.GRPCStatusCodeKey.Int64(int64(code)),
+							),
+						},
+					},
+				},
+			},
+		},
+	}
+	rm := metricdata.ResourceMetrics{}
+	err := reader.Collect(context.Background(), &rm)
+	assert.NoError(t, err)
+	require.Len(t, rm.ScopeMetrics, 1)
+	metricdatatest.AssertEqual(t, want, rm.ScopeMetrics[0], metricdatatest.IgnoreTimestamp(), metricdatatest.IgnoreValue())
+}