Skip to content

Add grpc interceptors #938

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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Add grpc interceptors #938

wants to merge 19 commits into from

Conversation

ribice
Copy link
Collaborator

@ribice ribice commented Dec 27, 2024

Resolves #240

Sample error logged to Sentry:

CleanShot 2024-12-27 at 09 52 53

Possible improvements / todos:

  • Create docs in sentry and sentry-docs
  • The options is a simple struct (as in other integrations). Could implement Functional Options Pattern for these.
  • More tests and examples?

@ribice ribice requested a review from cleptric December 27, 2024 08:55
@ribice ribice changed the title grpc interceptors Add grpc interceptors Dec 27, 2024
Copy link

codecov bot commented Dec 27, 2024

Codecov Report

Attention: Patch coverage is 95.85799% with 7 lines in your changes missing coverage. Please review.

Project coverage is 84.32%. Comparing base (6b014ea) to head (6b3eefb).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
grpc/server.go 94.30% 5 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
+ Coverage   84.00%   84.32%   +0.31%     
==========================================
  Files          50       52       +2     
  Lines        5171     5340     +169     
==========================================
+ Hits         4344     4503     +159     
- Misses        673      680       +7     
- Partials      154      157       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

grpc/server.go Outdated
Comment on lines 35 to 36
// CaptureRequestBody determines whether to capture and send request bodies to Sentry.
CaptureRequestBody bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be handled by the base SDK.

grpc/server.go Outdated
Comment on lines 38 to 39
// OperationName overrides the default operation name (grpc.server).
OperationName string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not expose this to users.

grpc/server.go Outdated

options := []sentry.SpanOption{
sentry.ContinueTrace(hub, sentryTraceHeader, sentryBaggageHeader),
sentry.WithOpName(opts.OperationName),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hard code this grpc.server.

examplepb.RegisterExampleServiceServer(server, &ExampleServiceServer{})

// Start the server
listener, err := net.Listen("tcp", grpcPort)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semgrep identified an issue in your code:
Detected a network listener listening on 0.0.0.0 or an empty string. This could unexpectedly expose the server publicly as it binds to all available interfaces. Instead, specify another IP address that is not 0.0.0.0 nor the empty string.

To resolve this comment:

✨ Commit Assistant Fix Suggestion
  1. Update the value of grpcPort so it is not just a port or set to 0.0.0.0.
    For example, if grpcPort is ":50051", change it to "127.0.0.1:50051" or another appropriate interface (like your private network IP address).
  2. If you need the server to be accessible only from the local machine, use "127.0.0.1:<port>" as the address when calling net.Listen.
  3. If you do need remote access, restrict the IP address as much as possible to only the needed network interface, rather than using 0.0.0.0 or a blank string.
  4. Example: listener, err := net.Listen("tcp", "127.0.0.1:50051")

When a server binds to 0.0.0.0 or just the port (like ":50051"), it listens on all interfaces, which could make your service accessible from unwanted sources. Use a specific IP to limit access.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by avoid-bind-to-all-interfaces.

You can view more details about this finding in the Semgrep AppSec Platform.

@stephanie-anderson stephanie-anderson added Feature Issue type and removed Type: Feature labels Apr 25, 2025
grpc/client.go Outdated
Comment on lines 20 to 21
// OperationName overrides the default operation name (grpc.client).
OperationName string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this here as well.

grpc/server.go Outdated
Comment on lines 32 to 33
// ReportOn defines the conditions under which errors are reported to Sentry.
ReportOn func(error) bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this

grpc/server.go Outdated
Comment on lines 62 to 99
func reportErrorToSentry(hub *sentry.Hub, err error, methodName string, req any, md map[string]string) {
hub.WithScope(func(scope *sentry.Scope) {
scope.SetExtras(map[string]any{
"grpc.method": methodName,
"grpc.error": err.Error(),
})

if req != nil {
scope.SetExtra("request", req)
}

if len(md) > 0 {
scope.SetExtra("metadata", md)
}

defer hub.CaptureException(err)

statusErr, ok := status.FromError(err)
if !ok {
return
}

for _, detail := range statusErr.Details() {
debugInfo, ok := detail.(*errdetails.DebugInfo)
if !ok {
continue
}
hub.AddBreadcrumb(&sentry.Breadcrumb{
Type: "debug",
Category: "grpc.server",
Message: debugInfo.Detail,
Data: map[string]any{"stackTrace": strings.Join(debugInfo.StackEntries, "\n")},
Level: sentry.LevelError,
Timestamp: time.Now(),
}, nil)
}
})
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reported errors here add zero value. Let's remove this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC interceptors
4 participants