refactor: consolidate globals + split interceptors.go (ROADMAP 4.6)#40
Merged
refactor: consolidate globals + split interceptors.go (ROADMAP 4.6)#40
Conversation
Replace 34+ scattered package-level vars with a single interceptorConfig struct (defaultConfig). Split 936-line interceptors.go into 8 feature files: - config.go: interceptorConfig struct, defaults, all Set*() functions - filter.go: FilterMethods, filterState, FilterMethodsFunc - server.go: all server interceptors (timeout, logging, tracing, etc.) - client.go: all client interceptors (hystrix, newrelic, retry) - chain.go: interceptor chain helpers - http.go: DoHTTPtoGRPC, NRHttpTracer - metrics.go: Prometheus metrics setup - ratelimit.go: rate limiting interceptor Zero public API changes. All Set*() functions now assign to defaultConfig fields. resetGlobals() test helper simplified.
Contributor
📝 WalkthroughWalkthroughThe PR splits a large interceptors file into focused modules, adds configurable global interceptor state and builders, implements server/client interceptors (proto-validate, Hystrix, New Relic, rate-limiting, panic recovery), introduces interceptor chaining, metrics registration, HTTP→gRPC helper, and updates tests and README anchors. Changes
Sequence Diagram(s)mermaid Client->>InterceptorChain: invoke RPC Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the interceptors package to centralize configuration state and break up the previous monolithic interceptors.go into smaller feature-focused files, while aiming to preserve the existing public API and behavior.
Changes:
- Consolidated package-level configuration into a single
interceptorConfig(defaultConfig) and updatedSet*functions and interceptors to use it. - Split server/client/filter/http/metrics/ratelimit/chain logic into dedicated files.
- Simplified the test helper
resetGlobals()and removed the emptydocumentations.go.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server.go | Server-side interceptor implementations (timeout, logging, NR, panic recovery, protovalidate, etc.) moved here and wired to defaultConfig. |
| client.go | Client-side interceptors (hystrix, retry, NR, metrics) moved here and wired to defaultConfig. |
| config.go | New consolidated config struct + defaults + all Set* configuration functions. |
| filter.go | Filtering state/caching and FilterMethods logic split into its own file. |
| http.go | DoHTTPtoGRPC and NRHttpTracer moved here with cached chain handling. |
| metrics.go | Prometheus metrics singleton setup extracted here. |
| ratelimit.go | Rate limiting singleton setup extracted here (token bucket + custom limiter support). |
| chain.go | Shared interceptor chaining helpers extracted here. |
| interceptors.go | Reduced to package docs + version compatibility constant/assertion. |
| interceptors_test.go | Updated tests/benchmarks to reference defaultConfig and simplified resetGlobals(). |
| documentations.go | Removed empty file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@filter.go`:
- Around line 16-20: The cache invalidation only checks slice length or element
0, so direct mutations to other elements of the exported FilterMethods slice
leave stale entries; update the changed() logic used for filter caching (the
function named changed() that compares previous vs current filter method state)
to do a full element-wise comparison (or use reflect.DeepEqual) of the entire
FilterMethods slice snapshot instead of only length/element 0, ensuring any
in-place change to any index invalidates the cache; locate usages of
FilterMethods and changed() (and the analogous comparisons around the other
filter vars mentioned) and replace the partial check with a full-slice
comparison to restore compatibility for callers that mutate the exported slice.
In `@http.go`:
- Around line 66-77: The filter is currently only applied in the fallback
http.HandlerFunc and uses context.Background(); evaluate
defaultConfig.filterFunc once using the request context
(defaultConfig.filterFunc(r.Context(), r.URL.Path)) before choosing the tracing
path so filtered routes are not traced when pattern != "". Update the logic
around pattern and
newrelic.WrapHandleFunc/newrelic.RequestWithTransactionContext to first call the
filter with the incoming request context and only create/wrap a New Relic
transaction (txn via app.StartTransaction, txn.SetWebResponse,
txn.SetWebRequestHTTP, newrelic.RequestWithTransactionContext) when that filter
returns true; ensure both the newrelic.WrapHandleFunc branch and the
http.HandlerFunc fallback respect the same precomputed filter result.
In `@metrics.go`:
- Around line 20-31: In registerCollector, do not call prometheus.Unregister on
a previously registered collector; instead, when prometheus.Register(c) returns
a prometheus.AlreadyRegisteredError (use stdError.As to extract the error into a
prometheus.AlreadyRegisteredError variable), reuse the existing collector via
the error's ExistingCollector field (assign that existing collector back to the
package variable that would hold the new collector, e.g., srvMetrics or
cltMetrics) and return; remove the Unregister call and the second Register
attempt so you don't mutate the global registry or lose the already-registered
collector.
In `@server.go`:
- Around line 229-233: The handler is still receiving the original
grpc.ServerStream so downstream code never sees the new context from
SetTraceIdWithValue; wrap the incoming stream with a small struct that embeds
grpc.ServerStream and overrides Context() to return the new ctx (e.g. add type
serverStreamWithContext struct { grpc.ServerStream; ctx context.Context } with
func (s *serverStreamWithContext) Context() context.Context { return s.ctx })
and pass an instance of that wrapper (constructed with the original stream and
the ctx returned by notifier.SetTraceIdWithValue) into handler(srv,
wrappedStream) instead of the original stream.
- Around line 34-39: The current fallback to protovalidate.GlobalValidator hides
configuration errors when protovalidate.New(defaultConfig.protoValidateOpts...)
fails; instead, propagate the construction error and fail fast so custom options
from SetProtoValidateOptions() are not silently discarded. Replace the
assignment to protoValidatorVal = protovalidate.GlobalValidator and the silent
return with behavior that surfaces the error (e.g., return the error from the
enclosing init/setup function or log.Fatal/panic) so callers can detect and fix
the bad configuration; keep references to defaultConfig.protoValidateOpts,
protovalidate.New, protoValidatorVal and SetProtoValidateOptions to locate and
update the failing branch.
- Around line 102-117: DefaultStreamInterceptors() is missing a stream panic
recovery equivalent; implement a PanicRecoveryStreamInterceptor() as a
grpc.StreamServerInterceptor that mirrors the behavior of
PanicRecoveryInterceptor() for unary calls (wrap the stream handler in
defer/recover, log/notify the panic, convert to an appropriate grpc error and
prevent the goroutine from crashing), then add PanicRecoveryStreamInterceptor()
into DefaultStreamInterceptors() after ServerErrorStreamInterceptor() so
streaming handlers are protected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b09cf604-e556-41af-b431-a66fbd57abbf
📒 Files selected for processing (12)
README.mdchain.goclient.goconfig.godocumentations.gofilter.gohttp.gointerceptors.gointerceptors_test.gometrics.goratelimit.goserver.go
💤 Files with no reviewable changes (2)
- documentations.go
- interceptors.go
- Add PanicRecoveryStreamInterceptor to default stream chain — stream handler panics were previously uncaught, crashing the server - Fix ServerErrorStreamInterceptor: wrap stream with updated context from SetTraceIdWithValue so downstream handlers see the trace ID - Add wrappedStream helper for context override - Regenerate README Follow-up issues created for remaining pre-existing items: - #41 FilterMethods stale cache - #42 NRHttpTracer filter bypass - #43 registerCollector pattern - #44 protovalidate silent fallback
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eamInterceptor - PanicRecoveryStreamInterceptor: no-panic, string panic, error panic - ServerErrorStreamInterceptor: context wrapping, error propagation - DefaultStreamInterceptors: verify panic recovery is in the chain
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
interceptors_test.go (1)
1521-1535: Add an empty-slice guard before indexing default stream interceptors.On Line 1525,
ints[len(ints)-1]can panic if defaults are ever disabled/empty; a guard makes the failure clearer and test behavior more robust.Suggested patch
func TestDefaultStreamInterceptors_IncludesPanicRecovery(t *testing.T) { resetGlobals() ints := DefaultStreamInterceptors() + if len(ints) == 0 { + t.Fatal("expected at least one stream interceptor") + } // Verify the chain handles panics by running a panicking handler chain := ints[len(ints)-1] // PanicRecoveryStreamInterceptor is last stream := &mockServerStream{ctx: context.Background()} info := &grpc.StreamServerInfo{FullMethod: "/test.Svc/Stream"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interceptors_test.go` around lines 1521 - 1535, The test TestDefaultStreamInterceptors_IncludesPanicRecovery assumes DefaultStreamInterceptors() returns a non-empty slice and directly indexes ints[len(ints)-1], which will panic if defaults are disabled; add an empty-slice guard after calling DefaultStreamInterceptors() (check len(ints) == 0) and call t.Fatal or t.Fatalf with a clear message if it's empty, otherwise proceed to set chain := ints[len(ints)-1] and run the panic handler—this makes the intent explicit and prevents a confusing index panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@interceptors_test.go`:
- Around line 1521-1535: The test
TestDefaultStreamInterceptors_IncludesPanicRecovery assumes
DefaultStreamInterceptors() returns a non-empty slice and directly indexes
ints[len(ints)-1], which will panic if defaults are disabled; add an empty-slice
guard after calling DefaultStreamInterceptors() (check len(ints) == 0) and call
t.Fatal or t.Fatalf with a clear message if it's empty, otherwise proceed to set
chain := ints[len(ints)-1] and run the panic handler—this makes the intent
explicit and prevents a confusing index panic.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
interceptorConfigstructinterceptors.gomonolith into 8 feature filesnewDefaultConfig()as single source of truth for defaultsPanicRecoveryStreamInterceptorto default stream chainServerErrorStreamInterceptorcontext wrappingTest plan
make build+make test(race) +make lint— all passFollow-up issues: #41, #42, #43, #44