From 907d0bc40bd4a80934987587133278ad7a4e9dd7 Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Sun, 27 Jun 2021 20:19:14 -0700 Subject: [PATCH 01/11] initial cut at structure of configurable status handler architecture --- v3/integrations/nrgrpc/nrgrpc_client_test.go | 52 +++--- v3/integrations/nrgrpc/nrgrpc_server.go | 157 +++++++++++++++++-- v3/integrations/nrgrpc/nrgrpc_server_test.go | 28 ++-- 3 files changed, 186 insertions(+), 51 deletions(-) diff --git a/v3/integrations/nrgrpc/nrgrpc_client_test.go b/v3/integrations/nrgrpc/nrgrpc_client_test.go index 902e28a1a..f91d568d4 100644 --- a/v3/integrations/nrgrpc/nrgrpc_client_test.go +++ b/v3/integrations/nrgrpc/nrgrpc_client_test.go @@ -95,15 +95,15 @@ func TestUnaryClientInterceptor(t *testing.T) { client := testapp.NewTestApplicationClient(conn) resp, err := client.DoUnaryUnary(ctx, &testapp.Message{}) - if nil != err { + if err != nil { t.Fatal("client call to DoUnaryUnary failed", err) } var hdrs map[string][]string err = json.Unmarshal([]byte(resp.Text), &hdrs) - if nil != err { + if err != nil { t.Fatal("cannot unmarshall client response", err) } - if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || "" == hdr[0] { + if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || hdr[0] == "" { t.Error("distributed trace header not sent", hdrs) } txn.End() @@ -175,7 +175,7 @@ func TestUnaryStreamClientInterceptor(t *testing.T) { client := testapp.NewTestApplicationClient(conn) stream, err := client.DoUnaryStream(ctx, &testapp.Message{}) - if nil != err { + if err != nil { t.Fatal("client call to DoUnaryStream failed", err) } var recved int @@ -184,15 +184,15 @@ func TestUnaryStreamClientInterceptor(t *testing.T) { if err == io.EOF { break } - if nil != err { + if err != nil { t.Fatal("error receiving message", err) } var hdrs map[string][]string err = json.Unmarshal([]byte(msg.Text), &hdrs) - if nil != err { + if err != nil { t.Fatal("cannot unmarshall client response", err) } - if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || "" == hdr[0] { + if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || hdr[0] == "" { t.Error("distributed trace header not sent", hdrs) } recved++ @@ -269,11 +269,11 @@ func TestStreamUnaryClientInterceptor(t *testing.T) { client := testapp.NewTestApplicationClient(conn) stream, err := client.DoStreamUnary(ctx) - if nil != err { + if err != nil { t.Fatal("client call to DoStreamUnary failed", err) } for i := 0; i < 3; i++ { - if err := stream.Send(&testapp.Message{Text: "Hello DoStreamUnary"}); nil != err { + if err := stream.Send(&testapp.Message{Text: "Hello DoStreamUnary"}); err != nil { if err == io.EOF { break } @@ -281,15 +281,15 @@ func TestStreamUnaryClientInterceptor(t *testing.T) { } } msg, err := stream.CloseAndRecv() - if nil != err { + if err != nil { t.Fatal("failure to CloseAndRecv", err) } var hdrs map[string][]string err = json.Unmarshal([]byte(msg.Text), &hdrs) - if nil != err { + if err != nil { t.Fatal("cannot unmarshall client response", err) } - if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || "" == hdr[0] { + if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || hdr[0] == "" { t.Error("distributed trace header not sent", hdrs) } txn.End() @@ -361,7 +361,7 @@ func TestStreamStreamClientInterceptor(t *testing.T) { client := testapp.NewTestApplicationClient(conn) stream, err := client.DoStreamStream(ctx) - if nil != err { + if err != nil { t.Fatal("client call to DoStreamStream failed", err) } waitc := make(chan struct{}) @@ -378,10 +378,10 @@ func TestStreamStreamClientInterceptor(t *testing.T) { } var hdrs map[string][]string err = json.Unmarshal([]byte(msg.Text), &hdrs) - if nil != err { + if err != nil { t.Fatal("cannot unmarshall client response", err) } - if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || "" == hdr[0] { + if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || hdr[0] == "" { t.Error("distributed trace header not sent", hdrs) } recved++ @@ -473,18 +473,18 @@ func TestClientUnaryMetadata(t *testing.T) { client := testapp.NewTestApplicationClient(conn) resp, err := client.DoUnaryUnary(ctx, &testapp.Message{}) - if nil != err { + if err != nil { t.Fatal("client call to DoUnaryUnary failed", err) } var hdrs map[string][]string err = json.Unmarshal([]byte(resp.Text), &hdrs) - if nil != err { + if err != nil { t.Fatal("cannot unmarshall client response", err) } - if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || "" == hdr[0] || "payload" == hdr[0] { + if hdr, ok := hdrs["newrelic"]; !ok || len(hdr) != 1 || hdr[0] == "" || hdr[0] == "payload" { t.Error("distributed trace header not sent", hdrs) } - if hdr, ok := hdrs["testing"]; !ok || len(hdr) != 1 || "hello world" != hdr[0] { + if hdr, ok := hdrs["testing"]; !ok || len(hdr) != 1 || hdr[0] != "hello world" { t.Error("testing header not sent", hdrs) } } @@ -496,12 +496,12 @@ func TestNilTxnClientUnary(t *testing.T) { client := testapp.NewTestApplicationClient(conn) resp, err := client.DoUnaryUnary(context.Background(), &testapp.Message{}) - if nil != err { + if err != nil { t.Fatal("client call to DoUnaryUnary failed", err) } var hdrs map[string][]string err = json.Unmarshal([]byte(resp.Text), &hdrs) - if nil != err { + if err != nil { t.Fatal("cannot unmarshall client response", err) } if _, ok := hdrs["newrelic"]; ok { @@ -516,11 +516,11 @@ func TestNilTxnClientStreaming(t *testing.T) { client := testapp.NewTestApplicationClient(conn) stream, err := client.DoStreamUnary(context.Background()) - if nil != err { + if err != nil { t.Fatal("client call to DoStreamUnary failed", err) } for i := 0; i < 3; i++ { - if err := stream.Send(&testapp.Message{Text: "Hello DoStreamUnary"}); nil != err { + if err := stream.Send(&testapp.Message{Text: "Hello DoStreamUnary"}); err != nil { if err == io.EOF { break } @@ -528,12 +528,12 @@ func TestNilTxnClientStreaming(t *testing.T) { } } msg, err := stream.CloseAndRecv() - if nil != err { + if err != nil { t.Fatal("failure to CloseAndRecv", err) } var hdrs map[string][]string err = json.Unmarshal([]byte(msg.Text), &hdrs) - if nil != err { + if err != nil { t.Fatal("cannot unmarshall client response", err) } if _, ok := hdrs["newrelic"]; ok { @@ -557,7 +557,7 @@ func TestClientStreamingError(t *testing.T) { defer cancel() ctx = newrelic.NewContext(ctx, txn) _, err := client.DoUnaryStream(ctx, &testapp.Message{}) - if nil == err { + if err == nil { t.Fatal("client call to DoUnaryStream did not return error") } txn.End() diff --git a/v3/integrations/nrgrpc/nrgrpc_server.go b/v3/integrations/nrgrpc/nrgrpc_server.go index e8119c66e..ab96a0d5d 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server.go +++ b/v3/integrations/nrgrpc/nrgrpc_server.go @@ -10,6 +10,7 @@ import ( "github.com/newrelic/go-agent/v3/newrelic" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" ) @@ -45,7 +46,7 @@ func startTransaction(ctx context.Context, app *newrelic.Application, fullMethod // UnaryServerInterceptor instruments server unary RPCs. // // Use this function with grpc.UnaryInterceptor and a newrelic.Application to -// create a grpc.ServerOption to pass to grpc.NewServer. This interceptor +// sreate a grpc.ServerOption to pass to grpc.NewServer. This interceptor // records each unary call with a transaction. You must use both // UnaryServerInterceptor and StreamServerInterceptor to instrument unary and // streaming calls. @@ -62,29 +63,158 @@ func startTransaction(ctx context.Context, app *newrelic.Application, fullMethod // grpc.StreamInterceptor(nrgrpc.StreamServerInterceptor(app)), // ) // +// The nrgrpc integration has a built-in set of handlers for each gRPC status +// code encountered. Serious errors are reported as error traces ala the +// `newrelic.NoticeError()` function, while the others are reported but not +// counted as errors. +// +// If you wish to change this behavior, you may do so at a global level for +// all intercepted functions by calling the `Configure()` function, passing +// any number of `WithStatusHandler(code, handler)` functions as parameters. +// In each of these, `code` represents a gRPC code such as `codes.Unknown`, +// and `handler` is a function with the calling signature +// func myHandler(c context.Context, t *newrelic.Transaction, s *status.Status) +// If the given gRPC code is produced by the intercepted function, then `myHandler()` +// will be called to report that out in the current transaction, in whatever way +// is appropriate. To assist with this, `myHandler()` is provided with the corresponding +// context and transaction along with the actual gRPC `Status` value captured. +// +// We provide a set of standard handlers which should suffice in most cases to report +// non-error, info-level, warning-level, and error-level statuses: +// ErrorInterceptorStatusHandler // report as error +// IgnoreInterceptorStatusHandler // no report AT ALL +// InfoInterceptorStatusHandler // report as informational message +// OKInterceptorStatusHandler // report as successful +// WarningInterceptorStatusHandler // report as warning +// +// Thus, to specify that all codes with status `OutOfRange` should be logged as warnings +// and all `Unimplemented` ones should be informational, then make this call: +// Config( +// WithStatusHandler(codes.OutOfRange, WarningInterceptorStatusHandler), +// WithStatusHandler(codes.Unimplemented, InfoInterceptorStatusHandler)) +// +// This will affect the behavior of calls to `UnaryInterceptor()` and `StreamInterceptor()` +// which occur after this. You may call `Config()` again to change the handling of errors +// from that point forward (but note that once an interceptor is created, it will use whatever +// handlers were defined at that point, even if the intercepted service call happens later. +// +// You can also specify a custom set of handlers with each interceptor creation by adding +// `WithStatusHandler()` calls at the end of the `StreamInterceptor()` call's parameter list, +// like so: +// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app, +// nrgrpc.WithStatusHandler(codes.OutOfRange, nrgrpc.WarningInterceptorStatusHandler), +// nrgrpc.WithStatusHandler(codes.Unimplemented, nrgrpc.InfoInterceptorStatusHandler))) +// In this case, those two handlers are used (along with the current defaults for the other status +// codes) only for that interceptor. +// +// // These interceptors add the transaction to the call context so it may be // accessed in your method handlers using newrelic.FromContext. // // Full example: // https://github.com/newrelic/go-agent/blob/master/v3/integrations/nrgrpc/example/server/server.go // -func UnaryServerInterceptor(app *newrelic.Application) grpc.UnaryServerInterceptor { + +type ErrorHandler func(context.Context, *newrelic.Transaction, *status.Status) +type StatusHandlerMap map[codes.Code]ErrorHandler + +var interceptorStatusHandlerRegistry = StatusHandlerMap{ + codes.OK: OKInterceptorStatusHandler, + codes.Canceled: InfoInterceptorStatusHandler, + codes.Unknown: ErrorInterceptorStatusHandler, + codes.InvalidArgument: InfoInterceptorStatusHandler, + codes.DeadlineExceeded: WarningInterceptorStatusHandler, + codes.NotFound: InfoInterceptorStatusHandler, + codes.AlreadyExists: InfoInterceptorStatusHandler, + codes.PermissionDenied: WarningInterceptorStatusHandler, + codes.ResourceExhausted: WarningInterceptorStatusHandler, + codes.FailedPrecondition: WarningInterceptorStatusHandler, + codes.Aborted: WarningInterceptorStatusHandler, + codes.OutOfRange: WarningInterceptorStatusHandler, + codes.Unimplemented: ErrorInterceptorStatusHandler, + codes.Internal: ErrorInterceptorStatusHandler, + codes.Unavailable: WarningInterceptorStatusHandler, + codes.DataLoss: ErrorInterceptorStatusHandler, + codes.Unauthenticated: InfoInterceptorStatusHandler, +} + +type handlerOption func(StatusHandlerMap) + +func WithStatusHandler(c codes.Code, h ErrorHandler) handlerOption { + return func(m StatusHandlerMap) { + m[c] = h + } +} + +func Configure(options ...handlerOption) { + for _, option := range options { + option(interceptorStatusHandlerRegistry) + } +} + +func IgnoreInterceptorStatusHandler(_ context.Context, _ *newrelic.Transaction, _ *status.Status) {} + +func OKInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { + txn.SetWebResponse(nil).WriteHeader(int(codes.OK)) +} + +func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { + txn.SetWebResponse(nil).WriteHeader(int(s.Code())) + // + // TODO: Figure out specifically how we want to set up the custom attributes for this + // (it was txn.NoticeError(s.Err())) + txn.NoticeError(&newrelic.Error{ + Message: s.Err().Error(), + Class: "...", + Attributes: map[string]interface{}{ + "...": "...", + }, + }) +} + +func WarningInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { + txn.SetWebResponse(nil).WriteHeader(int(s.Code())) + // TODO: just add some attributes about this (treat as WARN, not NoticeError()) +} + +func InfoInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { + txn.SetWebResponse(nil).WriteHeader(int(s.Code())) + // TODO: just add some attributes about this (treat as INFO, not NoticeError()) +} + +var DefaultInterceptorStatusHandler = InfoInterceptorStatusHandler + +func reportInterceptorStatus(ctx context.Context, txn *newrelic.Transaction, handlers StatusHandlerMap, err error) { + grpcStatus := status.Convert(err) + handler, ok := handlers[grpcStatus.Code()] + if !ok { + handler = DefaultInterceptorStatusHandler + } + handler(ctx, txn, grpcStatus) +} + +func UnaryServerInterceptor(app *newrelic.Application, options ...handlerOption) grpc.UnaryServerInterceptor { if app == nil { return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { return handler(ctx, req) } } + localHandlerMap := make(StatusHandlerMap) + for code, handler := range interceptorStatusHandlerRegistry { + localHandlerMap[code] = handler + } + for _, option := range options { + option(localHandlerMap) + } + return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) { txn := startTransaction(ctx, app, info.FullMethod) defer txn.End() ctx = newrelic.NewContext(ctx, txn) resp, err = handler(ctx, req) - txn.SetWebResponse(nil).WriteHeader(int(status.Code(err))) - if err != nil { - txn.NoticeError(err) - } + reportInterceptorStatus(ctx, txn, localHandlerMap, err) return } } @@ -132,22 +262,27 @@ func newWrappedServerStream(stream grpc.ServerStream, txn *newrelic.Transaction) // Full example: // https://github.com/newrelic/go-agent/blob/master/v3/integrations/nrgrpc/example/server/server.go // -func StreamServerInterceptor(app *newrelic.Application) grpc.StreamServerInterceptor { +func StreamServerInterceptor(app *newrelic.Application, options ...handlerOption) grpc.StreamServerInterceptor { if app == nil { return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { return handler(srv, ss) } } + localHandlerMap := make(StatusHandlerMap) + for code, handler := range interceptorStatusHandlerRegistry { + localHandlerMap[code] = handler + } + for _, option := range options { + option(localHandlerMap) + } + return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { txn := startTransaction(ss.Context(), app, info.FullMethod) defer txn.End() err := handler(srv, newWrappedServerStream(ss, txn)) - txn.SetWebResponse(nil).WriteHeader(int(status.Code(err))) - if err != nil { - txn.NoticeError(err) - } + reportInterceptorStatus(ss.Context(), txn, localHandlerMap, err) return err } } diff --git a/v3/integrations/nrgrpc/nrgrpc_server_test.go b/v3/integrations/nrgrpc/nrgrpc_server_test.go index 4dce85bdf..d06e77b06 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server_test.go +++ b/v3/integrations/nrgrpc/nrgrpc_server_test.go @@ -62,7 +62,7 @@ func TestUnaryServerInterceptor(t *testing.T) { txn := app.StartTransaction("client") ctx := newrelic.NewContext(context.Background(), txn) _, err := client.DoUnaryUnary(ctx, &testapp.Message{}) - if nil != err { + if err != nil { t.Fatal("unable to call client DoUnaryUnary", err) } @@ -152,7 +152,7 @@ func TestUnaryServerInterceptorError(t *testing.T) { client := testapp.NewTestApplicationClient(conn) _, err := client.DoUnaryUnaryError(context.Background(), &testapp.Message{}) - if nil == err { + if err == nil { t.Fatal("DoUnaryUnaryError should have returned an error") } @@ -246,7 +246,7 @@ func TestUnaryStreamServerInterceptor(t *testing.T) { txn := app.StartTransaction("client") ctx := newrelic.NewContext(context.Background(), txn) stream, err := client.DoUnaryStream(ctx, &testapp.Message{}) - if nil != err { + if err != nil { t.Fatal("client call to DoUnaryStream failed", err) } var recved int @@ -255,7 +255,7 @@ func TestUnaryStreamServerInterceptor(t *testing.T) { if err == io.EOF { break } - if nil != err { + if err != nil { t.Fatal("error receiving message", err) } recved++ @@ -352,11 +352,11 @@ func TestStreamUnaryServerInterceptor(t *testing.T) { txn := app.StartTransaction("client") ctx := newrelic.NewContext(context.Background(), txn) stream, err := client.DoStreamUnary(ctx) - if nil != err { + if err != nil { t.Fatal("client call to DoStreamUnary failed", err) } for i := 0; i < 3; i++ { - if err := stream.Send(&testapp.Message{Text: "Hello DoStreamUnary"}); nil != err { + if err := stream.Send(&testapp.Message{Text: "Hello DoStreamUnary"}); err != nil { if err == io.EOF { break } @@ -364,7 +364,7 @@ func TestStreamUnaryServerInterceptor(t *testing.T) { } } _, err = stream.CloseAndRecv() - if nil != err { + if err != nil { t.Fatal("failure to CloseAndRecv", err) } @@ -456,7 +456,7 @@ func TestStreamStreamServerInterceptor(t *testing.T) { txn := app.StartTransaction("client") ctx := newrelic.NewContext(context.Background(), txn) stream, err := client.DoStreamStream(ctx) - if nil != err { + if err != nil { t.Fatal("client call to DoStreamStream failed", err) } waitc := make(chan struct{}) @@ -571,11 +571,11 @@ func TestStreamServerInterceptorError(t *testing.T) { client := testapp.NewTestApplicationClient(conn) stream, err := client.DoUnaryStreamError(context.Background(), &testapp.Message{}) - if nil != err { + if err != nil { t.Fatal("client call to DoUnaryStream failed", err) } _, err = stream.Recv() - if nil == err { + if err == nil { t.Fatal("DoUnaryStreamError should have returned an error") } @@ -665,7 +665,7 @@ func TestUnaryServerInterceptorNilApp(t *testing.T) { client := testapp.NewTestApplicationClient(conn) msg, err := client.DoUnaryUnary(context.Background(), &testapp.Message{}) - if nil != err { + if err != nil { t.Fatal("unable to call client DoUnaryUnary", err) } if !strings.Contains(msg.Text, "content-type") { @@ -680,11 +680,11 @@ func TestStreamServerInterceptorNilApp(t *testing.T) { client := testapp.NewTestApplicationClient(conn) stream, err := client.DoStreamUnary(context.Background()) - if nil != err { + if err != nil { t.Fatal("client call to DoStreamUnary failed", err) } for i := 0; i < 3; i++ { - if err := stream.Send(&testapp.Message{Text: "Hello DoStreamUnary"}); nil != err { + if err := stream.Send(&testapp.Message{Text: "Hello DoStreamUnary"}); err != nil { if err == io.EOF { break } @@ -692,7 +692,7 @@ func TestStreamServerInterceptorNilApp(t *testing.T) { } } msg, err := stream.CloseAndRecv() - if nil != err { + if err != nil { t.Fatal("failure to CloseAndRecv", err) } if !strings.Contains(msg.Text, "content-type") { From 26708ed0fffd3537b468d8d435527e9f314281c0 Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Mon, 28 Jun 2021 11:23:14 -0700 Subject: [PATCH 02/11] documentation/comment cleanup --- v3/integrations/nrgrpc/nrgrpc_server.go | 251 +++++++++++++++--------- 1 file changed, 159 insertions(+), 92 deletions(-) diff --git a/v3/integrations/nrgrpc/nrgrpc_server.go b/v3/integrations/nrgrpc/nrgrpc_server.go index ab96a0d5d..9e92103d0 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server.go +++ b/v3/integrations/nrgrpc/nrgrpc_server.go @@ -1,6 +1,35 @@ // Copyright 2020 New Relic Corporation. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +// +// This integration instruments grpc service calls via +// `UnaryServerInterceptor()` and `StreamServerInterceptor()` functions. +// +// The results of these calls are reported as errors or as informational +// messages (of levels OK, Info, Warning, or Error) based on the gRPC status +// code they return. +// +// In the simplest case, simply add interceptors as in the following example: +// +// app, _ := newrelic.NewApplication( +// newrelic.ConfigAppName("gRPC Server"), +// newrelic.ConfigLicense(os.Getenv("NEW_RELIC_LICENSE_KEY")), +// newrelic.ConfigDebugLogger(os.Stdout), +// ) +// server := grpc.NewServer( +// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app)), +// grpc.StreamInterceptor(nrgrpc.StreamServerInterceptor(app)), +// ) +// +// The disposition of each, in terms of how to report each of the various +// gRPC status codes, is determined by a built-in set of defaults. These +// may be overridden on a case-by-case basis using `WithStatusHandler()` +// options to each `UnaryServerInterceptor()` or `StreamServerInterceptor()` +// call, or globally via the `Configure()` function. +// +// Full example: +// https://github.com/newrelic/go-agent/blob/master/v3/integrations/nrgrpc/example/server/server.go +// package nrgrpc import ( @@ -43,82 +72,25 @@ func startTransaction(ctx context.Context, app *newrelic.Application, fullMethod return txn } -// UnaryServerInterceptor instruments server unary RPCs. -// -// Use this function with grpc.UnaryInterceptor and a newrelic.Application to -// sreate a grpc.ServerOption to pass to grpc.NewServer. This interceptor -// records each unary call with a transaction. You must use both -// UnaryServerInterceptor and StreamServerInterceptor to instrument unary and -// streaming calls. -// -// Example: -// -// app, _ := newrelic.NewApplication( -// newrelic.ConfigAppName("gRPC Server"), -// newrelic.ConfigLicense(os.Getenv("NEW_RELIC_LICENSE_KEY")), -// newrelic.ConfigDebugLogger(os.Stdout), -// ) -// server := grpc.NewServer( -// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app)), -// grpc.StreamInterceptor(nrgrpc.StreamServerInterceptor(app)), -// ) // -// The nrgrpc integration has a built-in set of handlers for each gRPC status -// code encountered. Serious errors are reported as error traces ala the -// `newrelic.NoticeError()` function, while the others are reported but not -// counted as errors. +// `ErrorHandler` is the type of a gRPC status handler function. +// Normally the supplied set of `ErrorHandler` functions will suffice, but +// a custom handler may be crafted by the user and installed as a handler +// if needed. // -// If you wish to change this behavior, you may do so at a global level for -// all intercepted functions by calling the `Configure()` function, passing -// any number of `WithStatusHandler(code, handler)` functions as parameters. -// In each of these, `code` represents a gRPC code such as `codes.Unknown`, -// and `handler` is a function with the calling signature -// func myHandler(c context.Context, t *newrelic.Transaction, s *status.Status) -// If the given gRPC code is produced by the intercepted function, then `myHandler()` -// will be called to report that out in the current transaction, in whatever way -// is appropriate. To assist with this, `myHandler()` is provided with the corresponding -// context and transaction along with the actual gRPC `Status` value captured. -// -// We provide a set of standard handlers which should suffice in most cases to report -// non-error, info-level, warning-level, and error-level statuses: -// ErrorInterceptorStatusHandler // report as error -// IgnoreInterceptorStatusHandler // no report AT ALL -// InfoInterceptorStatusHandler // report as informational message -// OKInterceptorStatusHandler // report as successful -// WarningInterceptorStatusHandler // report as warning -// -// Thus, to specify that all codes with status `OutOfRange` should be logged as warnings -// and all `Unimplemented` ones should be informational, then make this call: -// Config( -// WithStatusHandler(codes.OutOfRange, WarningInterceptorStatusHandler), -// WithStatusHandler(codes.Unimplemented, InfoInterceptorStatusHandler)) -// -// This will affect the behavior of calls to `UnaryInterceptor()` and `StreamInterceptor()` -// which occur after this. You may call `Config()` again to change the handling of errors -// from that point forward (but note that once an interceptor is created, it will use whatever -// handlers were defined at that point, even if the intercepted service call happens later. -// -// You can also specify a custom set of handlers with each interceptor creation by adding -// `WithStatusHandler()` calls at the end of the `StreamInterceptor()` call's parameter list, -// like so: -// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app, -// nrgrpc.WithStatusHandler(codes.OutOfRange, nrgrpc.WarningInterceptorStatusHandler), -// nrgrpc.WithStatusHandler(codes.Unimplemented, nrgrpc.InfoInterceptorStatusHandler))) -// In this case, those two handlers are used (along with the current defaults for the other status -// codes) only for that interceptor. +type ErrorHandler func(context.Context, *newrelic.Transaction, *status.Status) + // +// Internal registry of handlers associated with various +// status codes. // -// These interceptors add the transaction to the call context so it may be -// accessed in your method handlers using newrelic.FromContext. +type statusHandlerMap map[codes.Code]ErrorHandler + // -// Full example: -// https://github.com/newrelic/go-agent/blob/master/v3/integrations/nrgrpc/example/server/server.go +// interceptorStatusHandlerRegistry is the current default set of handlers +// used by each interceptor. // - -type ErrorHandler func(context.Context, *newrelic.Transaction, *status.Status) -type StatusHandlerMap map[codes.Code]ErrorHandler - -var interceptorStatusHandlerRegistry = StatusHandlerMap{ +var interceptorStatusHandlerRegistry = statusHandlerMap{ codes.OK: OKInterceptorStatusHandler, codes.Canceled: InfoInterceptorStatusHandler, codes.Unknown: ErrorInterceptorStatusHandler, @@ -138,26 +110,78 @@ var interceptorStatusHandlerRegistry = StatusHandlerMap{ codes.Unauthenticated: InfoInterceptorStatusHandler, } -type handlerOption func(StatusHandlerMap) +type handlerOption func(statusHandlerMap) +// +// `WithStatusHandler()` indicates a handler function to be used to +// report the indicated gRPC status. Zero or more of these may be +// given to the `Configure()`, `StreamServiceInterceptor()`, or +// `UnaryServiceInterceptor()` functions. +// +// The `ErrorHandler` parameter is generally one of the provided standard +// reporting functions: +// `OKInterceptorStatusHandler` // report the operation as successful +// `ErrorInterceptorStatusHandler` // report the operation as an error +// `WarningInterceptorStatusHandler` // report the operation as a warning +// `InfoInterceptorStatusHandler` // report the operation as an informational message +// +// The following reporting function should only be used if you know for sure +// you want this. It does not report the error in any way at all, but completely +// ignores it. +// `IgnoreInterceptorStatusHandler` // report the operation as successful +// +// Finally, if you have a custom reporting need that isn't covered by the standard +// handler functions, you can create your own handler function as +// func myHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { +// ... +// } +// Within the function, do whatever you need to do with the `txn` parameter to report the +// gRPC status passed as `s`. If needed, the context is also passed to your function. +// +// If you wish to use your custom handler for a code such as `codes.NotFound`, you would +// include the parameter +// WithStatusHandler(codes.NotFound, myHandler) +// to your `Configure()`, `StreamServiceInterceptor()`, or `UnaryServiceInterceptor()` function. +// func WithStatusHandler(c codes.Code, h ErrorHandler) handlerOption { - return func(m StatusHandlerMap) { + return func(m statusHandlerMap) { m[c] = h } } +// +// `Configure()` takes a list of `WithStatusHandler()` options and sets them +// as the new default handlers for the specified gRPC status codes, in the same +// way as if `WithStatusHandler()` were given to the `StreamServiceInterceptor()` +// or `UnaryServiceInterceptor()` functions (q.v.); however, in this case the new handlers +// become the default for any subsequent interceptors created by the above functions. +// func Configure(options ...handlerOption) { for _, option := range options { option(interceptorStatusHandlerRegistry) } } +// +// Standard handler: IGNORE +// This does not report anything at all in the context. +// func IgnoreInterceptorStatusHandler(_ context.Context, _ *newrelic.Transaction, _ *status.Status) {} +// +// Standard handler: OK +// Reports only that the RPC call was successful by setting the web response header +// value. +// func OKInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { txn.SetWebResponse(nil).WriteHeader(int(codes.OK)) } +// +// Standard handler: ERROR +// Reports the transaction as an error, with the relevant error messages and +// contextual information gleaned from the error value received from the RPC call. +// func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { txn.SetWebResponse(nil).WriteHeader(int(s.Code())) // @@ -172,19 +196,37 @@ func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transactio }) } +// +// Standard handler: WARNING +// Reports the transaction's status with attributes containing information gleaned +// from the error value returned, but does not count this as an error. +// func WarningInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { txn.SetWebResponse(nil).WriteHeader(int(s.Code())) // TODO: just add some attributes about this (treat as WARN, not NoticeError()) } +// +// Standard handler: INFO +// Reports the transaction's status with attributes containing information gleaned +// from the error value returned, but does not count this as an error. +// func InfoInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { txn.SetWebResponse(nil).WriteHeader(int(s.Code())) // TODO: just add some attributes about this (treat as INFO, not NoticeError()) } +// +// Standard handler: DEFAULT +// The `DefaultInterceptorStatusHander` is used for any status code which is not +// explicitly assigned a handler. +// var DefaultInterceptorStatusHandler = InfoInterceptorStatusHandler -func reportInterceptorStatus(ctx context.Context, txn *newrelic.Transaction, handlers StatusHandlerMap, err error) { +// +// Common routine for reporting any kind of interceptor. +// +func reportInterceptorStatus(ctx context.Context, txn *newrelic.Transaction, handlers statusHandlerMap, err error) { grpcStatus := status.Convert(err) handler, ok := handlers[grpcStatus.Code()] if !ok { @@ -193,6 +235,47 @@ func reportInterceptorStatus(ctx context.Context, txn *newrelic.Transaction, han handler(ctx, txn, grpcStatus) } +// UnaryServerInterceptor instruments server unary RPCs. +// +// Use this function with grpc.UnaryInterceptor and a newrelic.Application to +// create a grpc.ServerOption to pass to grpc.NewServer. This interceptor +// records each unary call with a transaction. You must use both +// UnaryServerInterceptor and StreamServerInterceptor to instrument unary and +// streaming calls. +// +// Example: +// +// app, _ := newrelic.NewApplication( +// newrelic.ConfigAppName("gRPC Server"), +// newrelic.ConfigLicense(os.Getenv("NEW_RELIC_LICENSE_KEY")), +// newrelic.ConfigDebugLogger(os.Stdout), +// ) +// server := grpc.NewServer( +// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app)), +// grpc.StreamInterceptor(nrgrpc.StreamServerInterceptor(app)), +// ) +// +// These interceptors add the transaction to the call context so it may be +// accessed in your method handlers using newrelic.FromContext. +// +// The nrgrpc integration has a built-in set of handlers for each gRPC status +// code encountered. Serious errors are reported as error traces à la the +// `newrelic.NoticeError()` function, while the others are reported but not +// counted as errors. +// +// If you wish to change this behavior, you may do so at a global level for +// all intercepted functions by calling the `Configure()` function, passing +// any number of `WithStatusHandler(code, handler)` functions as parameters. +// +// You can specify a custom set of handlers with each interceptor creation by adding +// `WithStatusHandler()` calls at the end of the `StreamInterceptor()` call's parameter list, +// like so: +// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app, +// nrgrpc.WithStatusHandler(codes.OutOfRange, nrgrpc.WarningInterceptorStatusHandler), +// nrgrpc.WithStatusHandler(codes.Unimplemented, nrgrpc.InfoInterceptorStatusHandler))) +// In this case, those two handlers are used (along with the current defaults for the other status +// codes) only for that interceptor. +// func UnaryServerInterceptor(app *newrelic.Application, options ...handlerOption) grpc.UnaryServerInterceptor { if app == nil { return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { @@ -200,7 +283,7 @@ func UnaryServerInterceptor(app *newrelic.Application, options ...handlerOption) } } - localHandlerMap := make(StatusHandlerMap) + localHandlerMap := make(statusHandlerMap) for code, handler := range interceptorStatusHandlerRegistry { localHandlerMap[code] = handler } @@ -244,23 +327,7 @@ func newWrappedServerStream(stream grpc.ServerStream, txn *newrelic.Transaction) // UnaryServerInterceptor and StreamServerInterceptor to instrument unary and // streaming calls. // -// Example: -// -// app, _ := newrelic.NewApplication( -// newrelic.ConfigAppName("gRPC Server"), -// newrelic.ConfigLicense(os.Getenv("NEW_RELIC_LICENSE_KEY")), -// newrelic.ConfigDebugLogger(os.Stdout), -// ) -// server := grpc.NewServer( -// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app)), -// grpc.StreamInterceptor(nrgrpc.StreamServerInterceptor(app)), -// ) -// -// These interceptors add the transaction to the call context so it may be -// accessed in your method handlers using newrelic.FromContext. -// -// Full example: -// https://github.com/newrelic/go-agent/blob/master/v3/integrations/nrgrpc/example/server/server.go +// See the notes and examples for the `UnaryServerInterceptor()` function. // func StreamServerInterceptor(app *newrelic.Application, options ...handlerOption) grpc.StreamServerInterceptor { if app == nil { @@ -269,7 +336,7 @@ func StreamServerInterceptor(app *newrelic.Application, options ...handlerOption } } - localHandlerMap := make(StatusHandlerMap) + localHandlerMap := make(statusHandlerMap) for code, handler := range interceptorStatusHandlerRegistry { localHandlerMap[code] = handler } From b77ebd9c751fe467cf98538717db6dffc6231b01 Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Mon, 28 Jun 2021 12:09:29 -0700 Subject: [PATCH 03/11] first test with some actual attributes --- v3/integrations/nrgrpc/nrgrpc_server.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/v3/integrations/nrgrpc/nrgrpc_server.go b/v3/integrations/nrgrpc/nrgrpc_server.go index 9e92103d0..48c665c8a 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server.go +++ b/v3/integrations/nrgrpc/nrgrpc_server.go @@ -189,13 +189,18 @@ func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transactio // (it was txn.NoticeError(s.Err())) txn.NoticeError(&newrelic.Error{ Message: s.Err().Error(), - Class: "...", + Class: "GrpcStatus", Attributes: map[string]interface{}{ - "...": "...", + "GrpcStatusLevel": "error", + "GrpcStatusMessage": s.Message(), + "GrpcStatusCode": s.Code(), }, }) } +// examples +// Class "LoginError", Attr: "username": username + // // Standard handler: WARNING // Reports the transaction's status with attributes containing information gleaned @@ -203,6 +208,9 @@ func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transactio // func WarningInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { txn.SetWebResponse(nil).WriteHeader(int(s.Code())) + txn.AddAttribute("GrpcStatusLevel", "warning") + txn.AddAttribute("GrpcStatusMessage", s.Message()) + txn.AddAttribute("GrpcStatusCode", s.Code()) // TODO: just add some attributes about this (treat as WARN, not NoticeError()) } @@ -213,6 +221,9 @@ func WarningInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transact // func InfoInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { txn.SetWebResponse(nil).WriteHeader(int(s.Code())) + txn.AddAttribute("GrpcStatusLevel", "info") + txn.AddAttribute("GrpcStatusMessage", s.Message()) + txn.AddAttribute("GrpcStatusCode", s.Code()) // TODO: just add some attributes about this (treat as INFO, not NoticeError()) } From ecc3ec722c4fc865ba52f5c4cd8b1165bc20c34c Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Wed, 30 Jun 2021 13:13:05 -0700 Subject: [PATCH 04/11] more testing and refinement --- v3/integrations/nrgrpc/nrgrpc_server.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/v3/integrations/nrgrpc/nrgrpc_server.go b/v3/integrations/nrgrpc/nrgrpc_server.go index 48c665c8a..d753787b1 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server.go +++ b/v3/integrations/nrgrpc/nrgrpc_server.go @@ -34,6 +34,7 @@ package nrgrpc import ( "context" + "fmt" "net/http" "strings" @@ -187,6 +188,7 @@ func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transactio // // TODO: Figure out specifically how we want to set up the custom attributes for this // (it was txn.NoticeError(s.Err())) + fmt.Printf("**NoticeError %v %v %v**\n", s.Err().Error(), s.Message(), s.Code()) txn.NoticeError(&newrelic.Error{ Message: s.Err().Error(), Class: "GrpcStatus", From 98f82a3f51bc28009b7e243feb2ef72271c006bc Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Wed, 30 Jun 2021 15:17:33 -0700 Subject: [PATCH 05/11] errors working --- v3/integrations/nrgrpc/nrgrpc_server.go | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/v3/integrations/nrgrpc/nrgrpc_server.go b/v3/integrations/nrgrpc/nrgrpc_server.go index d753787b1..c9b72baaf 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server.go +++ b/v3/integrations/nrgrpc/nrgrpc_server.go @@ -34,7 +34,6 @@ package nrgrpc import ( "context" - "fmt" "net/http" "strings" @@ -185,19 +184,7 @@ func OKInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, // func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { txn.SetWebResponse(nil).WriteHeader(int(s.Code())) - // - // TODO: Figure out specifically how we want to set up the custom attributes for this - // (it was txn.NoticeError(s.Err())) - fmt.Printf("**NoticeError %v %v %v**\n", s.Err().Error(), s.Message(), s.Code()) - txn.NoticeError(&newrelic.Error{ - Message: s.Err().Error(), - Class: "GrpcStatus", - Attributes: map[string]interface{}{ - "GrpcStatusLevel": "error", - "GrpcStatusMessage": s.Message(), - "GrpcStatusCode": s.Code(), - }, - }) + txn.NoticeError(s.Err()) } // examples @@ -213,7 +200,6 @@ func WarningInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transact txn.AddAttribute("GrpcStatusLevel", "warning") txn.AddAttribute("GrpcStatusMessage", s.Message()) txn.AddAttribute("GrpcStatusCode", s.Code()) - // TODO: just add some attributes about this (treat as WARN, not NoticeError()) } // @@ -226,7 +212,6 @@ func InfoInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction txn.AddAttribute("GrpcStatusLevel", "info") txn.AddAttribute("GrpcStatusMessage", s.Message()) txn.AddAttribute("GrpcStatusCode", s.Code()) - // TODO: just add some attributes about this (treat as INFO, not NoticeError()) } // From bae9df32ead6b36e9afae4072d5dcfe3980030b5 Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Wed, 30 Jun 2021 15:39:50 -0700 Subject: [PATCH 06/11] added advice on usage of WaitForConnection() call --- v3/newrelic/application.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/v3/newrelic/application.go b/v3/newrelic/application.go index fb81eb793..865668d30 100644 --- a/v3/newrelic/application.go +++ b/v3/newrelic/application.go @@ -83,6 +83,15 @@ func (app *Application) RecordCustomMetric(name string, value float64) { // If Infinite Tracing is enabled, WaitForConnection will block until a // connection to the Trace Observer is made, a fatal error is reached, or the // timeout is hit. +// +// Note that in most cases, it is not necesary nor recommended to call +// WaitForConnection() at all, particularly for any but the most short-lived +// processes. It is better to simply start the application and allow the +// instrumentation code handle connections on its own, which it will do +// as needed in the background (and will continue attempting to connect +// if it wasn't immediately successful, all while allowing your application +// to proceed with its primary function). +// func (app *Application) WaitForConnection(timeout time.Duration) error { if nil == app { return nil From 8400fc0b105542607a55a2104f08d53168379d0f Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Wed, 30 Jun 2021 17:05:21 -0700 Subject: [PATCH 07/11] typo corrections --- v3/newrelic/application.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/v3/newrelic/application.go b/v3/newrelic/application.go index 865668d30..7a8db268f 100644 --- a/v3/newrelic/application.go +++ b/v3/newrelic/application.go @@ -85,9 +85,9 @@ func (app *Application) RecordCustomMetric(name string, value float64) { // timeout is hit. // // Note that in most cases, it is not necesary nor recommended to call -// WaitForConnection() at all, particularly for any but the most short-lived +// WaitForConnection() at all, particularly for any but the most trivial, short-lived // processes. It is better to simply start the application and allow the -// instrumentation code handle connections on its own, which it will do +// instrumentation code to handle its connections on its own, which it will do // as needed in the background (and will continue attempting to connect // if it wasn't immediately successful, all while allowing your application // to proceed with its primary function). From 094273df318536d9441d3e546475ce10207e4844 Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Sat, 3 Jul 2021 14:02:50 -0700 Subject: [PATCH 08/11] changed non-error handlers to report OK web responses --- v3/integrations/nrgrpc/nrgrpc_server.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/v3/integrations/nrgrpc/nrgrpc_server.go b/v3/integrations/nrgrpc/nrgrpc_server.go index c9b72baaf..6db33d2bd 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server.go +++ b/v3/integrations/nrgrpc/nrgrpc_server.go @@ -196,7 +196,7 @@ func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transactio // from the error value returned, but does not count this as an error. // func WarningInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { - txn.SetWebResponse(nil).WriteHeader(int(s.Code())) + txn.SetWebResponse(nil).WriteHeader(int(codes.OK)) txn.AddAttribute("GrpcStatusLevel", "warning") txn.AddAttribute("GrpcStatusMessage", s.Message()) txn.AddAttribute("GrpcStatusCode", s.Code()) @@ -208,7 +208,7 @@ func WarningInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transact // from the error value returned, but does not count this as an error. // func InfoInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { - txn.SetWebResponse(nil).WriteHeader(int(s.Code())) + txn.SetWebResponse(nil).WriteHeader(int(codes.OK)) txn.AddAttribute("GrpcStatusLevel", "info") txn.AddAttribute("GrpcStatusMessage", s.Message()) txn.AddAttribute("GrpcStatusCode", s.Code()) From 1ccdb9d0968b2d85baf121a9d10091f4dc448014 Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Thu, 8 Jul 2021 09:20:30 -0700 Subject: [PATCH 09/11] modified error reporting to avoid double reporting --- .../nrgrpc/example/server/server.go | 5 +- v3/integrations/nrgrpc/nrgrpc_server.go | 14 ++- v3/integrations/nrgrpc/nrgrpc_server_test.go | 86 +++++++------------ 3 files changed, 44 insertions(+), 61 deletions(-) diff --git a/v3/integrations/nrgrpc/example/server/server.go b/v3/integrations/nrgrpc/example/server/server.go index 469f3c80d..4a37dfcff 100644 --- a/v3/integrations/nrgrpc/example/server/server.go +++ b/v3/integrations/nrgrpc/example/server/server.go @@ -14,6 +14,8 @@ import ( sampleapp "github.com/newrelic/go-agent/v3/integrations/nrgrpc/example/sampleapp" newrelic "github.com/newrelic/go-agent/v3/newrelic" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // Server is a gRPC server. @@ -28,7 +30,8 @@ func processMessage(ctx context.Context, msg *sampleapp.Message) { // DoUnaryUnary is a unary request, unary response method. func (s *Server) DoUnaryUnary(ctx context.Context, msg *sampleapp.Message) (*sampleapp.Message, error) { processMessage(ctx, msg) - return &sampleapp.Message{Text: "Hello from DoUnaryUnary"}, nil + // return &sampleapp.Message{Text: "Hello from DoUnaryUnary"}, nil + return &sampleapp.Message{}, status.New(codes.DataLoss, "oooooops!").Err() } // DoUnaryStream is a unary request, stream response method. diff --git a/v3/integrations/nrgrpc/nrgrpc_server.go b/v3/integrations/nrgrpc/nrgrpc_server.go index 6db33d2bd..e77168060 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server.go +++ b/v3/integrations/nrgrpc/nrgrpc_server.go @@ -183,8 +183,14 @@ func OKInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, // contextual information gleaned from the error value received from the RPC call. // func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { - txn.SetWebResponse(nil).WriteHeader(int(s.Code())) - txn.NoticeError(s.Err()) + txn.SetWebResponse(nil).WriteHeader(int(codes.OK)) + txn.NoticeError(&newrelic.Error{ + Message: s.Message(), + Class: "gRPC Error: " + s.Code().String(), + }) + txn.AddAttribute("GrpcStatusLevel", "error") + txn.AddAttribute("GrpcStatusMessage", s.Message()) + txn.AddAttribute("GrpcStatusCode", s.Code().String()) } // examples @@ -199,7 +205,7 @@ func WarningInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transact txn.SetWebResponse(nil).WriteHeader(int(codes.OK)) txn.AddAttribute("GrpcStatusLevel", "warning") txn.AddAttribute("GrpcStatusMessage", s.Message()) - txn.AddAttribute("GrpcStatusCode", s.Code()) + txn.AddAttribute("GrpcStatusCode", s.Code().String()) } // @@ -211,7 +217,7 @@ func InfoInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction txn.SetWebResponse(nil).WriteHeader(int(codes.OK)) txn.AddAttribute("GrpcStatusLevel", "info") txn.AddAttribute("GrpcStatusMessage", s.Message()) - txn.AddAttribute("GrpcStatusCode", s.Code()) + txn.AddAttribute("GrpcStatusCode", s.Code().String()) } // diff --git a/v3/integrations/nrgrpc/nrgrpc_server_test.go b/v3/integrations/nrgrpc/nrgrpc_server_test.go index d06e77b06..d77004c0d 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server_test.go +++ b/v3/integrations/nrgrpc/nrgrpc_server_test.go @@ -181,10 +181,14 @@ func TestUnaryServerInterceptorError(t *testing.T) { "sampled": internal.MatchAnything, "traceId": internal.MatchAnything, }, - UserAttributes: map[string]interface{}{}, + UserAttributes: map[string]interface{}{ + "GrpcStatusMessage": "oooooops!", + "GrpcStatusCode": "DataLoss", + "GrpcStatusLevel": "error", + }, AgentAttributes: map[string]interface{}{ - "httpResponseCode": 15, - "http.statusCode": 15, + "httpResponseCode": 0, + "http.statusCode": 0, "request.headers.contentType": "application/grpc", "request.method": "TestApplication/DoUnaryUnaryError", "request.uri": "grpc://bufnet/TestApplication/DoUnaryUnaryError", @@ -192,8 +196,8 @@ func TestUnaryServerInterceptorError(t *testing.T) { }}) app.ExpectErrorEvents(t, []internal.WantEvent{{ Intrinsics: map[string]interface{}{ - "error.class": "15", - "error.message": "response code 15", + "error.class": "gRPC Error: DataLoss", + "error.message": "oooooops!", "guid": internal.MatchAnything, "priority": internal.MatchAnything, "sampled": internal.MatchAnything, @@ -202,36 +206,19 @@ func TestUnaryServerInterceptorError(t *testing.T) { "transactionName": "WebTransaction/Go/TestApplication/DoUnaryUnaryError", }, AgentAttributes: map[string]interface{}{ - "httpResponseCode": 15, - "http.statusCode": 15, + "httpResponseCode": 0, + "http.statusCode": 0, "request.headers.User-Agent": internal.MatchAnything, "request.headers.userAgent": internal.MatchAnything, "request.headers.contentType": "application/grpc", "request.method": "TestApplication/DoUnaryUnaryError", "request.uri": "grpc://bufnet/TestApplication/DoUnaryUnaryError", }, - UserAttributes: map[string]interface{}{}, - }, { - Intrinsics: map[string]interface{}{ - "error.class": internal.MatchAnything, - "error.message": "rpc error: code = DataLoss desc = oooooops!", - "guid": internal.MatchAnything, - "priority": internal.MatchAnything, - "sampled": internal.MatchAnything, - "spanId": internal.MatchAnything, - "traceId": internal.MatchAnything, - "transactionName": "WebTransaction/Go/TestApplication/DoUnaryUnaryError", - }, - AgentAttributes: map[string]interface{}{ - "httpResponseCode": 15, - "http.statusCode": 15, - "request.headers.User-Agent": internal.MatchAnything, - "request.headers.userAgent": internal.MatchAnything, - "request.headers.contentType": "application/grpc", - "request.method": "TestApplication/DoUnaryUnaryError", - "request.uri": "grpc://bufnet/TestApplication/DoUnaryUnaryError", + UserAttributes: map[string]interface{}{ + "GrpcStatusMessage": "oooooops!", + "GrpcStatusCode": "DataLoss", + "GrpcStatusLevel": "error", }, - UserAttributes: map[string]interface{}{}, }}) } @@ -604,10 +591,14 @@ func TestStreamServerInterceptorError(t *testing.T) { "sampled": internal.MatchAnything, "traceId": internal.MatchAnything, }, - UserAttributes: map[string]interface{}{}, + UserAttributes: map[string]interface{}{ + "GrpcStatusLevel": "error", + "GrpcStatusMessage": "oooooops!", + "GrpcStatusCode": "DataLoss", + }, AgentAttributes: map[string]interface{}{ - "httpResponseCode": 15, - "http.statusCode": 15, + "httpResponseCode": 0, + "http.statusCode": 0, "request.headers.contentType": "application/grpc", "request.method": "TestApplication/DoUnaryStreamError", "request.uri": "grpc://bufnet/TestApplication/DoUnaryStreamError", @@ -615,8 +606,8 @@ func TestStreamServerInterceptorError(t *testing.T) { }}) app.ExpectErrorEvents(t, []internal.WantEvent{{ Intrinsics: map[string]interface{}{ - "error.class": "15", - "error.message": "response code 15", + "error.class": "gRPC Error: DataLoss", + "error.message": "oooooops!", "guid": internal.MatchAnything, "priority": internal.MatchAnything, "sampled": internal.MatchAnything, @@ -625,36 +616,19 @@ func TestStreamServerInterceptorError(t *testing.T) { "transactionName": "WebTransaction/Go/TestApplication/DoUnaryStreamError", }, AgentAttributes: map[string]interface{}{ - "httpResponseCode": 15, - "http.statusCode": 15, + "httpResponseCode": 0, + "http.statusCode": 0, "request.headers.User-Agent": internal.MatchAnything, "request.headers.userAgent": internal.MatchAnything, "request.headers.contentType": "application/grpc", "request.method": "TestApplication/DoUnaryStreamError", "request.uri": "grpc://bufnet/TestApplication/DoUnaryStreamError", }, - UserAttributes: map[string]interface{}{}, - }, { - Intrinsics: map[string]interface{}{ - "error.class": internal.MatchAnything, - "error.message": "rpc error: code = DataLoss desc = oooooops!", - "guid": internal.MatchAnything, - "priority": internal.MatchAnything, - "sampled": internal.MatchAnything, - "spanId": internal.MatchAnything, - "traceId": internal.MatchAnything, - "transactionName": "WebTransaction/Go/TestApplication/DoUnaryStreamError", - }, - AgentAttributes: map[string]interface{}{ - "httpResponseCode": 15, - "http.statusCode": 15, - "request.headers.User-Agent": internal.MatchAnything, - "request.headers.userAgent": internal.MatchAnything, - "request.headers.contentType": "application/grpc", - "request.method": "TestApplication/DoUnaryStreamError", - "request.uri": "grpc://bufnet/TestApplication/DoUnaryStreamError", + UserAttributes: map[string]interface{}{ + "GrpcStatusLevel": "error", + "GrpcStatusMessage": "oooooops!", + "GrpcStatusCode": "DataLoss", }, - UserAttributes: map[string]interface{}{}, }}) } From 0efa74cda8ec9900188cabe0b122b0d08b6fd09b Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Thu, 8 Jul 2021 15:07:22 -0700 Subject: [PATCH 10/11] updated documentation --- v3/integrations/nrgrpc/nrgrpc_doc.go | 61 +++++++++++++++++++++---- v3/integrations/nrgrpc/nrgrpc_server.go | 57 +++++++++++------------ 2 files changed, 80 insertions(+), 38 deletions(-) diff --git a/v3/integrations/nrgrpc/nrgrpc_doc.go b/v3/integrations/nrgrpc/nrgrpc_doc.go index 075b4f610..c059d13fe 100644 --- a/v3/integrations/nrgrpc/nrgrpc_doc.go +++ b/v3/integrations/nrgrpc/nrgrpc_doc.go @@ -1,6 +1,7 @@ // Copyright 2020 New Relic Corporation. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +// // Package nrgrpc instruments https://github.com/grpc/grpc-go. // // This package can be used to instrument gRPC servers and gRPC clients. @@ -9,18 +10,58 @@ // // To instrument a gRPC server, use UnaryServerInterceptor and // StreamServerInterceptor with your newrelic.Application to create server -// interceptors to pass to grpc.NewServer. Example: +// interceptors to pass to grpc.NewServer. // +// The results of these calls are reported as errors or as informational +// messages (of levels OK, Info, Warning, or Error) based on the gRPC status +// code they return. // -// app, _ := newrelic.NewApplication( -// newrelic.ConfigAppName("gRPC Server"), -// newrelic.ConfigLicense(os.Getenv("NEW_RELIC_LICENSE_KEY")), -// newrelic.ConfigDebugLogger(os.Stdout), -// ) -// server := grpc.NewServer( -// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app)), -// grpc.StreamInterceptor(nrgrpc.StreamServerInterceptor(app)), -// ) +// In the simplest case, simply add interceptors as in the following example: +// +// app, _ := newrelic.NewApplication( +// newrelic.ConfigAppName("gRPC Server"), +// newrelic.ConfigLicense(os.Getenv("NEW_RELIC_LICENSE_KEY")), +// newrelic.ConfigDebugLogger(os.Stdout), +// ) +// server := grpc.NewServer( +// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app)), +// grpc.StreamInterceptor(nrgrpc.StreamServerInterceptor(app)), +// ) +// +// The disposition of each, in terms of how to report each of the various +// gRPC status codes, is determined by a built-in set of defaults: +// OK OK +// Info AlreadyExists, Canceled, InvalidArgument, NotFound, +// Unauthenticated +// Warning Aborted, DeadlineExceeded, FailedPrecondition, OutOfRange, +// PermissionDenied, ResourceExhausted, Unavailable +// Error DataLoss, Internal, Unknown, Unimplemented +// +// These +// may be overridden on a case-by-case basis using `WithStatusHandler()` +// options to each `UnaryServerInterceptor()` or `StreamServerInterceptor()` +// call, or globally via the `Configure()` function. +// +// For example, to report DeadlineExceeded as an error and NotFound +// as a warning, for the UnaryInterceptor only: +// server := grpc.NewServer( +// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app, +// nrgrpc.WithStatusHandler(codes.DeadlineExceeded, nrgrpc.ErrorInterceptorStatusHandler), +// nrgrpc.WithStatusHandler(codes.NotFound, nrgrpc.WarningInterceptorStatusHandler)), +// grpc.StreamInterceptor(nrgrpc.StreamServerInterceptor(app)), +// ) +// +// If you wanted to make those two changes to the overall default behavior, so they +// apply to all subsequently declared interceptors: +// nrgrpc.Configure( +// nrgrpc.WithStatusHandler(codes.DeadlineExceeded, nrgrpc.ErrorInterceptorStatusHandler), +// nrgrpc.WithStatusHandler(codes.NotFound, nrgrpc.WarningInterceptorStatusHandler), +// ) +// server := grpc.NewServer( +// grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app)), +// grpc.StreamInterceptor(nrgrpc.StreamServerInterceptor(app)), +// ) +// In this case the new behavior for those two status codes applies to both interceptors. // // These interceptors create transactions for inbound calls. The transaction is // added to the call context and can be accessed in your method handlers diff --git a/v3/integrations/nrgrpc/nrgrpc_server.go b/v3/integrations/nrgrpc/nrgrpc_server.go index e77168060..a9b1f6e0a 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server.go +++ b/v3/integrations/nrgrpc/nrgrpc_server.go @@ -3,7 +3,7 @@ // // This integration instruments grpc service calls via -// `UnaryServerInterceptor()` and `StreamServerInterceptor()` functions. +// UnaryServerInterceptor() and StreamServerInterceptor() functions. // // The results of these calls are reported as errors or as informational // messages (of levels OK, Info, Warning, or Error) based on the gRPC status @@ -23,13 +23,14 @@ // // The disposition of each, in terms of how to report each of the various // gRPC status codes, is determined by a built-in set of defaults. These -// may be overridden on a case-by-case basis using `WithStatusHandler()` -// options to each `UnaryServerInterceptor()` or `StreamServerInterceptor()` -// call, or globally via the `Configure()` function. +// may be overridden on a case-by-case basis using WithStatusHandler() +// options to each UnaryServerInterceptor() or StreamServerInterceptor() +// call, or globally via the Configure() function. // // Full example: // https://github.com/newrelic/go-agent/blob/master/v3/integrations/nrgrpc/example/server/server.go // + package nrgrpc import ( @@ -73,8 +74,8 @@ func startTransaction(ctx context.Context, app *newrelic.Application, fullMethod } // -// `ErrorHandler` is the type of a gRPC status handler function. -// Normally the supplied set of `ErrorHandler` functions will suffice, but +// ErrorHandler is the type of a gRPC status handler function. +// Normally the supplied set of ErrorHandler functions will suffice, but // a custom handler may be crafted by the user and installed as a handler // if needed. // @@ -113,35 +114,35 @@ var interceptorStatusHandlerRegistry = statusHandlerMap{ type handlerOption func(statusHandlerMap) // -// `WithStatusHandler()` indicates a handler function to be used to +// WithStatusHandler() indicates a handler function to be used to // report the indicated gRPC status. Zero or more of these may be -// given to the `Configure()`, `StreamServiceInterceptor()`, or -// `UnaryServiceInterceptor()` functions. +// given to the Configure(), StreamServiceInterceptor(), or +// UnaryServiceInterceptor() functions. // -// The `ErrorHandler` parameter is generally one of the provided standard +// The ErrorHandler parameter is generally one of the provided standard // reporting functions: -// `OKInterceptorStatusHandler` // report the operation as successful -// `ErrorInterceptorStatusHandler` // report the operation as an error -// `WarningInterceptorStatusHandler` // report the operation as a warning -// `InfoInterceptorStatusHandler` // report the operation as an informational message +// OKInterceptorStatusHandler // report the operation as successful +// ErrorInterceptorStatusHandler // report the operation as an error +// WarningInterceptorStatusHandler // report the operation as a warning +// InfoInterceptorStatusHandler // report the operation as an informational message // // The following reporting function should only be used if you know for sure // you want this. It does not report the error in any way at all, but completely // ignores it. -// `IgnoreInterceptorStatusHandler` // report the operation as successful +// IgnoreInterceptorStatusHandler // report the operation as successful // // Finally, if you have a custom reporting need that isn't covered by the standard // handler functions, you can create your own handler function as // func myHandler(ctx context.Context, txn *newrelic.Transaction, s *status.Status) { // ... // } -// Within the function, do whatever you need to do with the `txn` parameter to report the -// gRPC status passed as `s`. If needed, the context is also passed to your function. +// Within the function, do whatever you need to do with the txn parameter to report the +// gRPC status passed as s. If needed, the context is also passed to your function. // -// If you wish to use your custom handler for a code such as `codes.NotFound`, you would +// If you wish to use your custom handler for a code such as codes.NotFound, you would // include the parameter // WithStatusHandler(codes.NotFound, myHandler) -// to your `Configure()`, `StreamServiceInterceptor()`, or `UnaryServiceInterceptor()` function. +// to your Configure(), StreamServiceInterceptor(), or UnaryServiceInterceptor() function. // func WithStatusHandler(c codes.Code, h ErrorHandler) handlerOption { return func(m statusHandlerMap) { @@ -150,10 +151,10 @@ func WithStatusHandler(c codes.Code, h ErrorHandler) handlerOption { } // -// `Configure()` takes a list of `WithStatusHandler()` options and sets them +// Configure() takes a list of WithStatusHandler() options and sets them // as the new default handlers for the specified gRPC status codes, in the same -// way as if `WithStatusHandler()` were given to the `StreamServiceInterceptor()` -// or `UnaryServiceInterceptor()` functions (q.v.); however, in this case the new handlers +// way as if WithStatusHandler() were given to the StreamServiceInterceptor() +// or UnaryServiceInterceptor() functions (q.v.); however, in this case the new handlers // become the default for any subsequent interceptors created by the above functions. // func Configure(options ...handlerOption) { @@ -222,7 +223,7 @@ func InfoInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transaction // // Standard handler: DEFAULT -// The `DefaultInterceptorStatusHander` is used for any status code which is not +// The DefaultInterceptorStatusHander is used for any status code which is not // explicitly assigned a handler. // var DefaultInterceptorStatusHandler = InfoInterceptorStatusHandler @@ -264,15 +265,15 @@ func reportInterceptorStatus(ctx context.Context, txn *newrelic.Transaction, han // // The nrgrpc integration has a built-in set of handlers for each gRPC status // code encountered. Serious errors are reported as error traces à la the -// `newrelic.NoticeError()` function, while the others are reported but not +// newrelic.NoticeError() function, while the others are reported but not // counted as errors. // // If you wish to change this behavior, you may do so at a global level for -// all intercepted functions by calling the `Configure()` function, passing -// any number of `WithStatusHandler(code, handler)` functions as parameters. +// all intercepted functions by calling the Configure() function, passing +// any number of WithStatusHandler(code, handler) functions as parameters. // // You can specify a custom set of handlers with each interceptor creation by adding -// `WithStatusHandler()` calls at the end of the `StreamInterceptor()` call's parameter list, +// WithStatusHandler() calls at the end of the StreamInterceptor() call's parameter list, // like so: // grpc.UnaryInterceptor(nrgrpc.UnaryServerInterceptor(app, // nrgrpc.WithStatusHandler(codes.OutOfRange, nrgrpc.WarningInterceptorStatusHandler), @@ -331,7 +332,7 @@ func newWrappedServerStream(stream grpc.ServerStream, txn *newrelic.Transaction) // UnaryServerInterceptor and StreamServerInterceptor to instrument unary and // streaming calls. // -// See the notes and examples for the `UnaryServerInterceptor()` function. +// See the notes and examples for the UnaryServerInterceptor() function. // func StreamServerInterceptor(app *newrelic.Application, options ...handlerOption) grpc.StreamServerInterceptor { if app == nil { From b5d08f3536f7d07ee4c4e654e8fad68e6fa0f70a Mon Sep 17 00:00:00 2001 From: Steve Willoughby Date: Thu, 8 Jul 2021 16:24:19 -0700 Subject: [PATCH 11/11] removed test code --- v3/integrations/nrgrpc/example/server/server.go | 5 +---- v3/integrations/nrgrpc/nrgrpc_server.go | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/v3/integrations/nrgrpc/example/server/server.go b/v3/integrations/nrgrpc/example/server/server.go index 4a37dfcff..469f3c80d 100644 --- a/v3/integrations/nrgrpc/example/server/server.go +++ b/v3/integrations/nrgrpc/example/server/server.go @@ -14,8 +14,6 @@ import ( sampleapp "github.com/newrelic/go-agent/v3/integrations/nrgrpc/example/sampleapp" newrelic "github.com/newrelic/go-agent/v3/newrelic" "google.golang.org/grpc" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) // Server is a gRPC server. @@ -30,8 +28,7 @@ func processMessage(ctx context.Context, msg *sampleapp.Message) { // DoUnaryUnary is a unary request, unary response method. func (s *Server) DoUnaryUnary(ctx context.Context, msg *sampleapp.Message) (*sampleapp.Message, error) { processMessage(ctx, msg) - // return &sampleapp.Message{Text: "Hello from DoUnaryUnary"}, nil - return &sampleapp.Message{}, status.New(codes.DataLoss, "oooooops!").Err() + return &sampleapp.Message{Text: "Hello from DoUnaryUnary"}, nil } // DoUnaryStream is a unary request, stream response method. diff --git a/v3/integrations/nrgrpc/nrgrpc_server.go b/v3/integrations/nrgrpc/nrgrpc_server.go index a9b1f6e0a..afca8033b 100644 --- a/v3/integrations/nrgrpc/nrgrpc_server.go +++ b/v3/integrations/nrgrpc/nrgrpc_server.go @@ -187,7 +187,7 @@ func ErrorInterceptorStatusHandler(ctx context.Context, txn *newrelic.Transactio txn.SetWebResponse(nil).WriteHeader(int(codes.OK)) txn.NoticeError(&newrelic.Error{ Message: s.Message(), - Class: "gRPC Error: " + s.Code().String(), + Class: "gRPC Status: " + s.Code().String(), }) txn.AddAttribute("GrpcStatusLevel", "error") txn.AddAttribute("GrpcStatusMessage", s.Message())