-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix issue 221: grpc error reporting #317
fix issue 221: grpc error reporting #317
Conversation
This includes module version number updates in the integration's |
v3/integrations/nrgrpc/go.mod
Outdated
// protobuf v1.3.0 is the earliest version using modules, we use v1.3.1 | ||
// because all dependencies were removed in this version. | ||
github.com/golang/protobuf v1.3.1 | ||
github.com/golang/protobuf v1.3.3 | ||
github.com/kisielk/gotool v1.0.0 // indirect | ||
github.com/newrelic/go-agent/v3 v3.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we don't have a "replace" directive in the go.mod file, the tests will break on customers' systems. Let's put the Go Agent requirements up to 3.12.0. We'll have better luck in the GitHub Actions CI with that, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'll make the change and clean up a couple of things and try the pull request again.
@@ -69,7 +69,7 @@ func startTransaction(ctx context.Context, app *newrelic.Application, fullMethod | |||
// https://github.com/newrelic/go-agent/blob/master/v3/integrations/nrgrpc/example/server/server.go | |||
// | |||
func UnaryServerInterceptor(app *newrelic.Application) grpc.UnaryServerInterceptor { | |||
if nil == app { | |||
if app == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the new non-Yoda conditions!
@@ -82,6 +82,9 @@ func UnaryServerInterceptor(app *newrelic.Application) grpc.UnaryServerIntercept | |||
ctx = newrelic.NewContext(ctx, txn) | |||
resp, err = handler(ctx, req) | |||
txn.SetWebResponse(nil).WriteHeader(int(status.Code(err))) | |||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the right thing to do! Do we have an example of this in the UI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll add it to the original issue's conversation.
// protobuf v1.3.0 is the earliest version using modules, we use v1.3.1 | ||
// because all dependencies were removed in this version. | ||
github.com/golang/protobuf v1.3.1 | ||
github.com/golang/protobuf v1.3.3 | ||
github.com/kisielk/gotool v1.0.0 // indirect | ||
github.com/newrelic/go-agent/v3 v3.0.0 | ||
// v1.15.0 is the earliest version of grpc using modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we should remove or change this comment.
We'll want a CHANGELOG.md entry with this, I think. |
ChangeLogFixes
|
Adds error reporting to the nrgrpc integration. An error value returned from an instrumented unary or stream server call now creates an error span in the current transaction which contains the error message. Previously it only set an error status code.