From 9a9fe20ad16d672dfe514db6dd45668840227e90 Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Tue, 2 Jun 2026 19:05:48 +0200 Subject: [PATCH 1/3] feat: include gRPC status details in the error log The unary interceptor logged failed calls with only the raw error string, dropping the structured google.rpc.Status details (BadRequest, ErrorInfo, etc.) that explain the failure. Add a details field that renders every status detail compactly (type + one-line value) via a small statusDetails helper covered by unit tests. Ref: roadrunner-server/roadrunner#1897 --- server.go | 28 +++++++++++++++++++++++++- server_test.go | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 server_test.go diff --git a/server.go b/server.go index 44719ac..b0ad096 100644 --- a/server.go +++ b/server.go @@ -19,6 +19,8 @@ import ( "google.golang.org/grpc/credentials" "google.golang.org/grpc/keepalive" "google.golang.org/grpc/status" + "google.golang.org/protobuf/encoding/prototext" + "google.golang.org/protobuf/proto" ) func (p *Plugin) createGRPCserver(interceptors map[string]api.Interceptor) (*grpc.Server, error) { @@ -109,7 +111,11 @@ func (p *Plugin) interceptor(ctx context.Context, req any, info *grpc.UnaryServe }() if err != nil { - p.log.Error("method call was finished with error", "error", err, "method", info.FullMethod, "start", start, "elapsed", time.Since(start).Milliseconds()) + args := []any{"error", err, "method", info.FullMethod, "start", start, "elapsed", time.Since(start).Milliseconds()} + if details := statusDetails(s); len(details) > 0 { + args = append(args, "details", details) + } + p.log.Error("method call was finished with error", args...) return nil, err } @@ -118,6 +124,26 @@ func (p *Plugin) interceptor(ctx context.Context, req any, info *grpc.UnaryServe return resp, nil } +// statusDetails renders the google.rpc.Status details attached to s as compact, +// one-line strings for logging. It returns nil when there are no details. +func statusDetails(s *status.Status) []string { + details := s.Details() + if len(details) == 0 { + return nil + } + + out := make([]string, 0, len(details)) + for _, d := range details { + if m, ok := d.(proto.Message); ok { + out = append(out, fmt.Sprintf("%s: %s", m.ProtoReflect().Descriptor().FullName(), prototext.MarshalOptions{}.Format(m))) + continue + } + out = append(out, fmt.Sprintf("%v", d)) + } + + return out +} + func (p *Plugin) serverOptions() ([]grpc.ServerOption, error) { const op = errors.Op("grpc_plugin_server_options") diff --git a/server_test.go b/server_test.go new file mode 100644 index 0000000..94103a5 --- /dev/null +++ b/server_test.go @@ -0,0 +1,53 @@ +package grpc + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/genproto/googleapis/rpc/errdetails" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func TestStatusDetails_NoDetails_Nil(t *testing.T) { + s := status.New(codes.InvalidArgument, "bad arg") + assert.Nil(t, statusDetails(s)) +} + +func TestStatusDetails_NonStatusError_Nil(t *testing.T) { + s, ok := status.FromError(errors.New("boom")) + require.False(t, ok) + assert.Nil(t, statusDetails(s), "a non-status error carries no details") +} + +func TestStatusDetails_SingleDetail(t *testing.T) { + s, err := status.New(codes.InvalidArgument, "bad arg").WithDetails(&errdetails.BadRequest{ + FieldViolations: []*errdetails.BadRequest_FieldViolation{ + {Field: "email", Description: "is required"}, + }, + }) + require.NoError(t, err) + + got := statusDetails(s) + require.Len(t, got, 1) + assert.Contains(t, got[0], "google.rpc.BadRequest", "should name the detail type") + assert.Contains(t, got[0], "email", "should include the field value") + assert.Contains(t, got[0], "is required") +} + +func TestStatusDetails_MultipleDetails(t *testing.T) { + s, err := status.New(codes.FailedPrecondition, "nope").WithDetails( + &errdetails.BadRequest{FieldViolations: []*errdetails.BadRequest_FieldViolation{{Field: "x", Description: "y"}}}, + &errdetails.ErrorInfo{Reason: "QUOTA", Domain: "example.com"}, + ) + require.NoError(t, err) + + got := statusDetails(s) + require.Len(t, got, 2) + joined := got[0] + " | " + got[1] + assert.Contains(t, joined, "google.rpc.BadRequest") + assert.Contains(t, joined, "google.rpc.ErrorInfo") + assert.Contains(t, joined, "QUOTA") +} From 517fb11884b65575e2079d92b520279671ff6007 Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Tue, 2 Jun 2026 21:17:55 +0200 Subject: [PATCH 2/3] perf: restrict logged gRPC status details to google.rpc.* types A handler can attach an arbitrarily large custom detail via Any; serializing it into the error log on every failed call is a needless cost. Log only the well-known google.rpc.* error details (BadRequest, ErrorInfo, ...), which carry the useful diagnostics, and skip anything else. Addresses CodeRabbit review on roadrunner-server/grpc#148. Ref: roadrunner-server/roadrunner#1897 --- server.go | 21 ++++++++++++++++----- server_test.go | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/server.go b/server.go index b0ad096..1c890b6 100644 --- a/server.go +++ b/server.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path" + "strings" "time" "github.com/roadrunner-server/errors" @@ -124,8 +125,9 @@ func (p *Plugin) interceptor(ctx context.Context, req any, info *grpc.UnaryServe return resp, nil } -// statusDetails renders the google.rpc.Status details attached to s as compact, -// one-line strings for logging. It returns nil when there are no details. +// statusDetails renders the well-known google.rpc.* error details attached to s as +// compact, one-line strings for logging, skipping any other (and potentially large) +// handler-attached detail. It returns nil when there are no such details. func statusDetails(s *status.Status) []string { details := s.Details() if len(details) == 0 { @@ -134,11 +136,20 @@ func statusDetails(s *status.Status) []string { out := make([]string, 0, len(details)) for _, d := range details { - if m, ok := d.(proto.Message); ok { - out = append(out, fmt.Sprintf("%s: %s", m.ProtoReflect().Descriptor().FullName(), prototext.MarshalOptions{}.Format(m))) + m, ok := d.(proto.Message) + if !ok { continue } - out = append(out, fmt.Sprintf("%v", d)) + + // Only the standard google.rpc.* error details (BadRequest, ErrorInfo, ...) + // are logged; arbitrary payloads a handler may attach are skipped so a failing + // call can't dump a large custom message into the logs. + name := m.ProtoReflect().Descriptor().FullName() + if !strings.HasPrefix(string(name), "google.rpc.") { + continue + } + + out = append(out, fmt.Sprintf("%s: %s", name, prototext.MarshalOptions{}.Format(m))) } return out diff --git a/server_test.go b/server_test.go index 94103a5..4088d15 100644 --- a/server_test.go +++ b/server_test.go @@ -9,6 +9,7 @@ import ( "google.golang.org/genproto/googleapis/rpc/errdetails" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/wrapperspb" ) func TestStatusDetails_NoDetails_Nil(t *testing.T) { @@ -51,3 +52,24 @@ func TestStatusDetails_MultipleDetails(t *testing.T) { assert.Contains(t, joined, "google.rpc.ErrorInfo") assert.Contains(t, joined, "QUOTA") } + +func TestStatusDetails_SkipsNonGoogleRpc(t *testing.T) { + // Handlers can attach arbitrary (and potentially large) details; only the + // standard google.rpc.* error details should be logged. + s, err := status.New(codes.Internal, "boom").WithDetails(&wrapperspb.StringValue{Value: "arbitrary payload"}) + require.NoError(t, err) + assert.Empty(t, statusDetails(s), "non-google.rpc details must be skipped") +} + +func TestStatusDetails_MixedKeepsOnlyGoogleRpc(t *testing.T) { + s, err := status.New(codes.InvalidArgument, "bad").WithDetails( + &wrapperspb.StringValue{Value: "arbitrary payload"}, + &errdetails.BadRequest{FieldViolations: []*errdetails.BadRequest_FieldViolation{{Field: "email", Description: "is required"}}}, + ) + require.NoError(t, err) + + got := statusDetails(s) + require.Len(t, got, 1, "only the google.rpc detail should be kept") + assert.Contains(t, got[0], "google.rpc.BadRequest") + assert.NotContains(t, got[0], "StringValue") +} From 0dcacf8a63311618815b3a5e6a0c5f015295119f Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Tue, 2 Jun 2026 21:28:23 +0200 Subject: [PATCH 3/3] refactor: select google.rpc.* error details with a type switch Replace the FullName-prefix check with a type switch over the errdetails types, so skipped (potentially large) details aren't touched by reflection at all. Ref: roadrunner-server/roadrunner#1897 --- server.go | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/server.go b/server.go index 1c890b6..8098602 100644 --- a/server.go +++ b/server.go @@ -7,7 +7,6 @@ import ( "fmt" "os" "path" - "strings" "time" "github.com/roadrunner-server/errors" @@ -15,6 +14,7 @@ import ( "github.com/roadrunner-server/grpc/v6/parser" "github.com/roadrunner-server/grpc/v6/proxy" "go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc" + "google.golang.org/genproto/googleapis/rpc/errdetails" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials" @@ -136,20 +136,23 @@ func statusDetails(s *status.Status) []string { out := make([]string, 0, len(details)) for _, d := range details { - m, ok := d.(proto.Message) - if !ok { - continue + // Only the standard google.rpc.* error details are logged; arbitrary payloads + // a handler may attach are skipped (without touching them), so a failing call + // can't dump a large custom message into the logs. + switch d.(type) { + case *errdetails.BadRequest, + *errdetails.ErrorInfo, + *errdetails.DebugInfo, + *errdetails.Help, + *errdetails.LocalizedMessage, + *errdetails.PreconditionFailure, + *errdetails.QuotaFailure, + *errdetails.RequestInfo, + *errdetails.ResourceInfo, + *errdetails.RetryInfo: + m := d.(proto.Message) + out = append(out, fmt.Sprintf("%s: %s", m.ProtoReflect().Descriptor().FullName(), prototext.MarshalOptions{}.Format(m))) } - - // Only the standard google.rpc.* error details (BadRequest, ErrorInfo, ...) - // are logged; arbitrary payloads a handler may attach are skipped so a failing - // call can't dump a large custom message into the logs. - name := m.ProtoReflect().Descriptor().FullName() - if !strings.HasPrefix(string(name), "google.rpc.") { - continue - } - - out = append(out, fmt.Sprintf("%s: %s", name, prototext.MarshalOptions{}.Format(m))) } return out