feat: improve interceptor observability, performance, and stability#19
feat: improve interceptor observability, performance, and stability#19
Conversation
- Fix TraceIdInterceptor dropping updated context (trace IDs were partially lost for downstream handlers). - Use NotifyAsync for bounded error notifications instead of unbounded fire-and-forget goroutines. - Add grpcMethod and duration tags to error notifications. - Add debug.Stack() to PanicRecoveryInterceptor for full panic traces. - Add gRPC status code to response time log when error is non-nil. - Add SetResponseTimeLogLevel for configurable log level (default Info). - Reduce FilterMethodsFunc allocations by hoisting ToLower before loop.
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughUpdated documentation generation tooling in the Makefile, enhanced package documentation for interceptors with initialization and concurrency requirements, added new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves gRPC interceptor observability and stability by fixing trace ID propagation, enriching error/latency logging/notifications, and adding guardrails to prevent unbounded notifier goroutine creation.
Changes:
- Fix
TraceIdInterceptorto propagate the updated context returned bynotifier.UpdateTraceId. - Replace fire-and-forget notifier goroutines with
notifier.NotifyAsyncand add richer tags (grpcMethod,duration); include panic stack traces in logs/notifications. - Enhance response time logging with
grpcCodeon errors and add a configurable response-time log level; reduce per-RPC allocations inFilterMethodsFunc.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| README.md | Updates generated docs and clarifies interceptor configuration constraints, but appears out of sync with the new exported API. |
| interceptors.go | Implements the interceptor behavior changes: trace propagation fix, async/bounded notifications, richer logging/notification context, and new response-time log-level configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Update dependencies to pick up NotifyAsync, bounded notifications, and log.AddToContext. Fix Makefile doc target to use @latest and output to README.md.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| responseTimeLogLevel loggers.Level = loggers.InfoLevel | ||
| ) | ||
|
|
||
| // SetResponseTimeLogLevel sets the log level for response time logging. | ||
| // Default is InfoLevel. Must be called during initialization, before the server starts. | ||
| func SetResponseTimeLogLevel(level loggers.Level) { | ||
| responseTimeLogLevel = level | ||
| } |
There was a problem hiding this comment.
SetResponseTimeLogLevel only affects the unary ResponseTimeLoggingInterceptor path; ResponseTimeLoggingStreamInterceptor still logs at Info unconditionally. If the goal is configurable log volume, consider applying responseTimeLogLevel (and optionally grpcCode) consistently to the stream response-time interceptor too.
- Add ctx context.Context param to SetResponseTimeLogLevel for API consistency with other config functions - Add "Not safe for concurrent use" to docstring - Apply responseTimeLogLevel and grpcCode to stream interceptor too - Remove duplicate package doc from documentations.go, regenerate README
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tream errors
- Move AddToLogContext("grpcMethod") before defer so the response time
log captures the method field from the context
- Add duration tag to ServerErrorStreamInterceptor for consistent
observability between unary and stream error notifications
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
notifier.UpdateTraceId, causing trace IDs to not propagate to log fields in downstream handlersgo func() { notifier.Notify(...) }()withnotifier.NotifyAsync()(bounded semaphore) to prevent goroutine explosion at scalegrpcMethodanddurationtags to error notifications for better debugging in Sentry/Rollbardebug.Stack()inPanicRecoveryInterceptorlog and notificationgrpcCodefield to response time log when error is non-nilSetResponseTimeLogLevel()(default Info) for services wanting to reduce log volumestrings.ToLowerbefore loop inFilterMethodsFunc— 1 alloc/RPC instead of 3Dependencies
go-coldbrew/errorswithNotifyAsync(this PR)Test plan
make test -race)golangci-lint run)errorsis published, updatego.modto reference new version🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
SetResponseTimeLogLevel()to configure response time logging verbosity.Improvements
Documentation