-
Notifications
You must be signed in to change notification settings - Fork 211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add grpc unary server interceptor #312
Open
shouichi
wants to merge
3
commits into
getsentry:master
Choose a base branch
from
shouichi:grpc
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+476
−1
Open
Changes from 2 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| // Package sentrygrpc provides Sentry integration for servers and clients based | ||
| // on the google.golang.org/grpc package. | ||
| package sentrygrpc |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| package sentrygrpc | ||
|
|
||
| import ( | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| type options struct { | ||
| repanic bool | ||
| reportOn ReportOn | ||
| } | ||
|
|
||
| func buildOptions(ff ...Option) options { | ||
| opts := options{ | ||
| reportOn: ReportAlways, | ||
| } | ||
|
|
||
| for _, f := range ff { | ||
| f(&opts) | ||
| } | ||
|
|
||
| return opts | ||
| } | ||
|
|
||
| // Option configures reporting behavior. | ||
| type Option func(*options) | ||
|
|
||
| // WithRepanic configures whether to panic again after recovering from | ||
| // a panic. Use this option if you have other panic handlers. | ||
| func WithRepanic(b bool) Option { | ||
| return func(o *options) { | ||
| o.repanic = b | ||
| } | ||
| } | ||
|
|
||
| // WithReportOn configures whether to report on errors. | ||
| func WithReportOn(r ReportOn) Option { | ||
| return func(o *options) { | ||
| o.reportOn = r | ||
| } | ||
| } | ||
|
|
||
| // ReportOn decides error should be reported to sentry. | ||
| type ReportOn func(error) bool | ||
|
|
||
| // ReportAlways returns true if err is non-nil. | ||
| func ReportAlways(err error) bool { | ||
| return err != nil | ||
| } | ||
|
|
||
| // ReportOnCodes returns true if error code matches on of the given codes. | ||
| func ReportOnCodes(cc ...codes.Code) ReportOn { | ||
| return func(err error) bool { | ||
| c := status.Code(err) | ||
| for i := range cc { | ||
| if c == cc[i] { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
| } | ||
shouichi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| package sentrygrpc | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "github.com/getsentry/sentry-go" | ||
| "google.golang.org/grpc" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| // UnaryServerInterceptor is a grpc interceptor that reports errors and panics | ||
| // to sentry. It also sets *sentry.Hub to context. | ||
| func UnaryServerInterceptor(options ...Option) grpc.UnaryServerInterceptor { | ||
| opts := buildOptions(options...) | ||
|
|
||
| return func( | ||
| ctx context.Context, | ||
| req interface{}, | ||
| info *grpc.UnaryServerInfo, | ||
| handler grpc.UnaryHandler, | ||
| ) (resp interface{}, err error) { | ||
| hub := sentry.GetHubFromContext(ctx) | ||
| if hub == nil { | ||
| hub = sentry.CurrentHub().Clone() | ||
| ctx = sentry.SetHubOnContext(ctx, hub) | ||
| } | ||
|
|
||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| hub.RecoverWithContext(ctx, r) | ||
|
|
||
| if opts.repanic { | ||
| panic(r) | ||
| } | ||
|
|
||
| err = status.Errorf(codes.Internal, "%s", r) | ||
rhcarvalho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| }() | ||
|
|
||
| resp, err = handler(ctx, req) | ||
|
|
||
| if opts.reportOn(err) { | ||
| hub.CaptureException(err) | ||
| } | ||
|
|
||
| return resp, err | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 wonder if we should separate options for server/client and unary/stream. Probably they won't diverge too much?
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.
Good point,
perhaps what would give us more flexibility isLater down the road it could becomeIt would allow us to share options between servers and clients, unary and stream.
Though I'm not sure that's actually a good idea. One downside is discoverability and documentation. If server/client/etc options have the same type, all of them will be grouped together in godoc. If an option doesn't make sense to a certain type (client/server/unary/stream), that would also require documentation to explain instead of being impossible to compile because of incompatible types.
I think the options we have right now are "interceptor options", and should probably be the same for other types of interceptors? Are there other existing concepts we'd expand into?
If there is only reason to be one type of option, for interceptors, then we can keep the name simple,
Option.Sorry, busy day, didn't have much time to think this through but wanted to share and continue the conversation.
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 not really sure but both of the following changes shouldn't break user code because they don't directly use
Option. So can we start simple?type Option func(*options)totype Option interface {}.OptiontoInterceptorOption.No worries! We don't need to rush. I (we?) just want to agree on a good design ;)