Introduce interceptor to log client errors #332
Conversation
|
Warning Rate limit exceeded@mkysel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe pull request introduces a new logging interceptor for gRPC services in the Changes
Assessment against linked issues
The changes comprehensively address the requirement to add middleware for logging client errors by implementing a robust logging interceptor that captures error details during gRPC calls. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/interceptors/client/logging.go (3)
18-26: Consider returning an error instead of panickingWhile validating the logger is good practice, consider returning an error instead of panicking for better error handling in production environments.
-func NewLoggingInterceptor(logger *zap.Logger) *LoggingInterceptor { +func NewLoggingInterceptor(logger *zap.Logger) (*LoggingInterceptor, error) { if logger == nil { - panic("logger is required") + return nil, fmt.Errorf("logger is required") } return &LoggingInterceptor{ logger: logger, - } + }, nil }
28-53: Consider enriching error logs with additional contextThe error logging is good but could be enhanced with:
- Request metadata/headers (sanitized)
- Error details from the status
- Trace ID for distributed tracing
if err != nil { st, _ := status.FromError(err) + details := st.Details() i.logger.Error( "Client Unary RPC Error", zap.String("method", info.FullMethod), zap.Duration("duration", duration), zap.Any("code", st.Code()), zap.String("message", st.Message()), + zap.Any("details", details), + zap.String("trace_id", trace.FromContext(ctx).SpanContext().TraceID.String()), ) }
55-80: Consider adding stream-specific context to logsFor stream RPCs, consider logging:
- Stream type (client/server/bidirectional)
- Stream state (active/closed)
if err != nil { st, _ := status.FromError(err) i.logger.Error( "Stream Client RPC Error", zap.String("method", info.FullMethod), zap.Duration("duration", duration), zap.Any("code", st.Code()), zap.String("message", st.Message()), + zap.Bool("client_stream", info.IsClientStream), + zap.Bool("server_stream", info.IsServerStream), ) }pkg/interceptors/client/logging_test.go (1)
21-27: Consider testing Context cancellationThe mock ServerStream implementation could be enhanced to test context cancellation scenarios.
type mockServerStream struct { grpc.ServerStream + ctx context.Context } func (m *mockServerStream) Context() context.Context { - return context.Background() + if m.ctx != nil { + return m.ctx + } + return context.Background() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/api/server.go(3 hunks)pkg/interceptors/client/logging.go(1 hunks)pkg/interceptors/client/logging_test.go(1 hunks)
🔇 Additional comments (2)
pkg/api/server.go (2)
65-66: 🛠️ Refactor suggestion
Verify interceptor initialization after constructor change
If the constructor is modified to return an error as suggested, this initialization needs to be updated:
- loggingInterceptor := client.NewLoggingInterceptor(log)
+ loggingInterceptor, err := client.NewLoggingInterceptor(log)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create logging interceptor: %w", err)
+ }Also applies to: 81-82
81-82: Consider interceptor ordering impact
The logging interceptor is added after the prometheus interceptor. Consider if this ordering affects the accuracy of duration measurements.
✅ Verification successful
Let me gather more context about the interceptor setup to understand the full chain and ordering.
Let me check the implementation of the prometheus and logging interceptors to understand their functionality better.
Let me check the implementation of the prometheus interceptor to understand how it handles metrics.
Interceptor ordering is correct for accurate metrics
The current implementation has prometheus interceptors registered first, followed by logging interceptors, which is the correct order. This ensures that prometheus metrics capture the full duration of request handling, including any time spent in the logging interceptor. If the order were reversed, the metrics would not include the logging overhead.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other interceptor orderings in the codebase
rg "ChainUnaryInterceptor|ChainStreamInterceptor" --type go
Length of output: 333
Script:
#!/bin/bash
# Get more context around the interceptor setup
rg "ChainUnaryInterceptor|ChainStreamInterceptor" -B 5 -A 5 --type go pkg/api/server.go
Length of output: 845
Script:
#!/bin/bash
# Look for prometheus and logging interceptor implementations
rg "type.*Interceptor struct" -B 5 -A 5 --type go
rg "func.*Unary.*Interceptor" -B 5 -A 5 --type go
rg "func.*Stream.*Interceptor" -B 5 -A 5 --type go
Length of output: 29292
Script:
#!/bin/bash
# Look for prometheus interceptor implementation
rg "prometheus.*Interceptor" -B 5 -A 10 --type go
Length of output: 949
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/interceptors/client/logging.go (2)
30-54: Consider extracting common logging logic into a helper function for maintainability.Both unary and stream interceptors contain similar error-logging logic, which can introduce duplication. Extracting the shared logic (e.g., status extraction, time measurement, structured log fields) into a helper method can reduce code duplication and improve maintainability.
Example approach (no guarantee of perfect fit, just illustrating the idea):
func (i *LoggingInterceptor) logError(method string, duration time.Duration, err error) { if err == nil { return } st, _ := status.FromError(err) i.logger.Error( "RPC Error", zap.String("method", method), zap.Duration("duration", duration), zap.Any("code", st.Code()), zap.String("message", st.Message()), ) } func (i *LoggingInterceptor) Unary() grpc.UnaryServerInterceptor { return func( ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler, ) (interface{}, error) { start := time.Now() resp, err := handler(ctx, req) duration := time.Since(start) - if err != nil { - st, _ := status.FromError(err) - i.logger.Error(...same fields...) - } + i.logError(info.FullMethod, duration, err) return resp, err } }
41-50: Include additional context if available.Currently, the log statement captures method, duration, error code, and message. In certain situations, additional context (e.g., request ID, user ID, or correlation ID) might be available via metadata or context. Integrating such fields can help in debugging complex client errors or tracing user-specific issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/api/server.go(3 hunks)pkg/interceptors/client/logging.go(1 hunks)pkg/interceptors/client/logging_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/api/server.go
- pkg/interceptors/client/logging_test.go
🔇 Additional comments (2)
pkg/interceptors/client/logging.go (2)
19-27: Great job checking for a nil logger!
The conditional check ensures that a valid logger is always provided, preventing potential runtime panics and enhancing robustness.
57-81: Stream interceptor implementation looks solid!
This code properly logs errors along with method name, duration, error code, and message. It mirrors the unary logic suitably and should provide consistent logs across different RPC types.
Here is an example of the logger:
Fixes #329
Summary by CodeRabbit