feat: add Executor hook for pluggable resilience#45
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 57 seconds. ⌛ 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. 📝 WalkthroughWalkthroughIntroduces a pluggable Executor type and per-call/default executor selection, adds ExecutorClientInterceptor and SetDefaultExecutor, generalizes exclusion options and per-call executor controls, updates DefaultClientInterceptors to prefer a configured executor over Hystrix, adds failsafe examples and tests, and marks Hystrix APIs deprecated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Interceptor as ExecutorClientInterceptor
participant Executor as UserExecutor
participant Invoker as gRPCInvoker
Client->>Interceptor: outbound RPC (ctx, method, opts)
Interceptor->>Interceptor: resolve executor (per-call > default > none)
alt executor configured
Interceptor->>Executor: exec(ctx, method, wrapper_fn)
Executor->>Invoker: wrapper_fn -> call invoker
Invoker-->>Executor: invoker returns err
Executor-->>Interceptor: executor returns executorErr
Interceptor->>Interceptor: apply excluded errors/codes (executor sees nil if excluded)
Interceptor-->>Client: return invokerErr if present else executorErr
else no executor
Interceptor->>Invoker: call invoker directly
Invoker-->>Interceptor: return err
Interceptor-->>Client: return err
end
alt panic in executor
Executor-->>Interceptor: panic
Interceptor->>Interceptor: recover, log/notify, wrap as error
Interceptor-->>Client: return wrapped panic error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
…etries, etc.) Adds a library-agnostic Executor type that wraps RPC invocations with user-provided resilience logic. This replaces the tightly-coupled hystrix-go dependency with a simple function hook — users bring their own resilience library (failsafe-go recommended). New API: - Executor type: func(ctx, method, fn) error - SetDefaultExecutor: init-time global executor - WithExecutor / WithoutExecutor: per-call options - WithExcludedErrors / WithExcludedCodes: library-agnostic names - ExecutorClientInterceptor: replaces HystrixClientInterceptor when executor is set DefaultClientInterceptors branches automatically: executor set → new path, no executor → hystrix fallback (zero behavior change for existing users). All With*Hystrix* functions and HystrixClientInterceptor are deprecated with migration guidance pointing to the new API. Includes separate examples/ Go module with failsafe-go integration examples (no failsafe-go dependency on interceptors itself).
There was a problem hiding this comment.
Pull request overview
Introduces a library-agnostic Executor hook to wrap outbound unary gRPC RPCs with pluggable resilience logic (retries/circuit-breaking/bulkheading), while preserving existing Hystrix behavior as a fallback and providing a deprecation/migration path.
Changes:
- Adds
Executortype +SetDefaultExecutor/WithExecutor/WithoutExecutoroptions, and implementsExecutorClientInterceptor. - Updates default client interceptor chain to branch between executor vs Hystrix based on whether a default executor is configured.
- Adds an
examples/Go module with failsafe-go integration examples, updates generated docs, and excludesexamples/from README generation.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| options.go | Defines Executor type, adds executor call options, and deprecates Hystrix-specific options in favor of library-agnostic ones. |
| client.go | Adds ExecutorClientInterceptor and switches default interceptor chain to executor-vs-hystrix branching. |
| config.go | Adds defaultExecutor to config and exposes SetDefaultExecutor. |
| interceptors_test.go | Adds unit tests for executor behavior and default chain branching. |
| README.md | Regenerates docs to include the new executor API and updated references. |
| Makefile | Excludes examples/ from gomarkdoc generation. |
| examples/go.mod | Adds a standalone examples module (with local replace) for failsafe-go integration demos. |
| examples/go.sum | Tracks example module dependency checksums. |
| examples/failsafe_test.go | Adds runnable Example* functions demonstrating failsafe-go integrations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ankurs
force-pushed
the
feat/executor-interceptor
branch
from
e0b50ed to
49a558a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client.go (1)
104-113: Consider documenting the invoker-error-wins precedence.When the executor wraps
fn's error (e.g., failsafe-go retry policies that report"exceeded max retries"), the caller will actually receive the original invoker error, becauseExecutorClientInterceptorreturnsinvokerErrahead ofexecutorErrwhenever the invoker ran. This is a deliberate and useful choice (preserves the underlying gRPC status), but it's non-obvious for users coming from raw failsafe-go where the policy error would be returned. Worth a sentence in the doc comment so retry/fallback users aren't surprised.📝 Suggested doc addition
// Excluded errors and codes (set via [WithExcludedErrors] / [WithExcludedCodes]) // are reported as nil to the executor, preventing them from tripping circuit // breakers or retry logic. The original error is still returned to the caller. +// +// If the invoker returned an error, that error is returned to the caller even +// if the executor transforms it (e.g., wraps it in a retry-exhausted error). +// The executor's return value is only surfaced when the invoker was not called +// or returned nil (e.g., when a circuit breaker is open). func ExecutorClientInterceptor(defaultOpts ...grpc.CallOption) grpc.UnaryClientInterceptor {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client.go` around lines 104 - 113, Update the doc comment for ExecutorClientInterceptor to explicitly state the precedence rule: when the RPC invoker runs and returns an error, that invoker error (invokerErr) is returned to the caller even if the Executor returns an error (executorErr) from its resilience policies (e.g., retries/fallbacks), so users of Executor, SetDefaultExecutor, WithExecutor, WithExcludedErrors and WithExcludedCodes should expect the original gRPC/invoker error to win over the executor policy error.examples/failsafe_test.go (1)
91-116: These two examples don't actually demonstrate usage.
ExampleWithoutExecutorandExampleWithExecutorbuild the option and immediately discard it with_ =, so a reader learns only that the functions exist — not how to apply them to a call. Since these are doc examples, consider showing the intended call-site pattern (as a comment, to keep the// Output:deterministic without needing a live gRPC stub).📝 Suggested improvement
func ExampleWithoutExecutor() { - _ = interceptors.WithoutExecutor() - fmt.Println("executor disabled for this call") - // Output: executor disabled for this call + // Pass WithoutExecutor as a grpc.CallOption to skip the configured + // executor for a single RPC (e.g., health checks or loopback calls): + // + // resp, err := client.Check(ctx, req, interceptors.WithoutExecutor()) + _ = interceptors.WithoutExecutor() + fmt.Println("executor disabled for this call") + // Output: executor disabled for this call }Apply the same pattern to
ExampleWithExecutor— show aclient.Foo(ctx, req, interceptors.WithExecutor(customExec))call-site in the comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/failsafe_test.go` around lines 91 - 116, Both examples discard the constructed interceptor options (e.g., interceptors.WithoutExecutor() and interceptors.WithExecutor(...)) so they don't demonstrate how to apply them; update ExampleWithoutExecutor and ExampleWithExecutor to show the call-site pattern by replacing the `_ =` discard with a commented example call that applies the option (for example a comment like `// client.Foo(ctx, req, interceptors.WithoutExecutor())` and similarly `// client.Foo(ctx, req, interceptors.WithExecutor(func...))`) so readers see how to pass the option to a client RPC while keeping the Output deterministic; reference the existing functions ExampleWithoutExecutor, ExampleWithExecutor, interceptors.WithoutExecutor, interceptors.WithExecutor and failsafe.With when adding the commented call-site lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interceptors_test.go`:
- Around line 560-579: The test "panic recovery" in interceptors_test.go
currently uses the tautological assertion errors.Is(err, err); update the
assertion to verify the panic was wrapped by checking the error string (e.g.,
use strings.Contains(err.Error(), "test panic") or "panic") instead of
errors.Is(err, err), and add "strings" to the imports; locate the test function
and the ExecutorClientInterceptor() invocation and replace the final errors.Is
check with a strings.Contains-based assertion that fails if the panic text is
not present.
---
Nitpick comments:
In `@client.go`:
- Around line 104-113: Update the doc comment for ExecutorClientInterceptor to
explicitly state the precedence rule: when the RPC invoker runs and returns an
error, that invoker error (invokerErr) is returned to the caller even if the
Executor returns an error (executorErr) from its resilience policies (e.g.,
retries/fallbacks), so users of Executor, SetDefaultExecutor, WithExecutor,
WithExcludedErrors and WithExcludedCodes should expect the original gRPC/invoker
error to win over the executor policy error.
In `@examples/failsafe_test.go`:
- Around line 91-116: Both examples discard the constructed interceptor options
(e.g., interceptors.WithoutExecutor() and interceptors.WithExecutor(...)) so
they don't demonstrate how to apply them; update ExampleWithoutExecutor and
ExampleWithExecutor to show the call-site pattern by replacing the `_ =` discard
with a commented example call that applies the option (for example a comment
like `// client.Foo(ctx, req, interceptors.WithoutExecutor())` and similarly `//
client.Foo(ctx, req, interceptors.WithExecutor(func...))`) so readers see how to
pass the option to a client RPC while keeping the Output deterministic;
reference the existing functions ExampleWithoutExecutor, ExampleWithExecutor,
interceptors.WithoutExecutor, interceptors.WithExecutor and failsafe.With when
adding the commented call-site lines.
🪄 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: b5da8fce-46a6-46ea-a8b0-c0619ae80a90
⛔ Files ignored due to path filters (1)
examples/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
MakefileREADME.mdclient.goconfig.goexamples/failsafe_test.goexamples/go.modinterceptors_test.gooptions.go
…heck in panic recovery test
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
options.go (1)
114-135: Nit: parameter nameserrorsandcodesshadow the stdliberrorspackage and the importedcodespackage.Inside these functions the bodies only use the parameters as slices, so there's no bug today. However, any future edit that tries to call
codes.Code(...)orerrors.Is(...)inside these closures would fail to compile or behave unexpectedly. Consider renaming toerrs/codeList(or similar) to avoid the shadowing trap. Same applies to the deprecated delegates on lines 78 and 86.Proposed rename
-func WithExcludedErrors(errors ...error) clientOption { +func WithExcludedErrors(errs ...error) clientOption { return &optionCarrier{ processor: func(co *clientOptions) { - co.excludedErrors = append(co.excludedErrors, errors...) + co.excludedErrors = append(co.excludedErrors, errs...) }, } } @@ -func WithExcludedCodes(codes ...codes.Code) clientOption { +func WithExcludedCodes(codeList ...codes.Code) clientOption { return &optionCarrier{ processor: func(co *clientOptions) { - co.excludedCodes = append(co.excludedCodes, codes...) + co.excludedCodes = append(co.excludedCodes, codeList...) }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@options.go` around lines 114 - 135, Rename the shadowing parameters to avoid clobbering the imported packages: change the variadic parameter name in WithExcludedErrors from errors to errs and in WithExcludedCodes from codes to codeList, and update their optionCarrier processor closures to append errs and codeList respectively to clientOptions.excludedErrors and clientOptions.excludedCodes; also make the same parameter-renaming change for the deprecated delegate functions referenced in the file so they no longer shadow the stdlib errors package or the imported codes package.config.go (1)
209-216: LGTM — follows the established init-only setter convention.
SetDefaultExecutormirrors the otherSet*helpers (documented as init-time, not concurrency-safe), and the doc correctly explains the interaction withExecutorClientInterceptor/HystrixClientInterceptorselection inDefaultClientInterceptors(client.go:21-48).One optional consideration: passing
nilhere will silently leave the chain on the hystrix path, which is the intended fallback but may surprise callers who expectSetDefaultExecutor(nil)to explicitly clear a previously-set executor. A brief note in the doc (or a guard) could prevent confusion — non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.go` around lines 209 - 216, Update SetDefaultExecutor's documentation to explicitly state the behavior when nil is passed: calling SetDefaultExecutor(nil) will clear any previously-set executor (defaultConfig.defaultExecutor) and cause DefaultClientInterceptors to fall back to HystrixClientInterceptor rather than enabling ExecutorClientInterceptor; alternatively, if you prefer to forbid nil, add a guard in SetDefaultExecutor that rejects nil (e.g., panic or log+no-op) and document that choice, referencing SetDefaultExecutor, defaultConfig.defaultExecutor, ExecutorClientInterceptor, HystrixClientInterceptor, and DefaultClientInterceptors so readers can find the related logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@options.go`:
- Around line 52-62: Add a migration/deprecation note to the docs explaining
that WithoutHystrix() now mirrors WithoutExecutor() by setting hasExecutor =
true and executor = nil (the same behavior as the option implemented in
optionCarrier.processor), which disables any default executor configured via
SetDefaultExecutor and results in passthrough behavior when both used; update
README/migration guide to mention this semantic change, show the recommended
replacement (use WithoutExecutor) and warn users that mixing legacy
WithoutHystrix with SetDefaultExecutor will silently disable the default
executor.
---
Nitpick comments:
In `@config.go`:
- Around line 209-216: Update SetDefaultExecutor's documentation to explicitly
state the behavior when nil is passed: calling SetDefaultExecutor(nil) will
clear any previously-set executor (defaultConfig.defaultExecutor) and cause
DefaultClientInterceptors to fall back to HystrixClientInterceptor rather than
enabling ExecutorClientInterceptor; alternatively, if you prefer to forbid nil,
add a guard in SetDefaultExecutor that rejects nil (e.g., panic or log+no-op)
and document that choice, referencing SetDefaultExecutor,
defaultConfig.defaultExecutor, ExecutorClientInterceptor,
HystrixClientInterceptor, and DefaultClientInterceptors so readers can find the
related logic.
In `@options.go`:
- Around line 114-135: Rename the shadowing parameters to avoid clobbering the
imported packages: change the variadic parameter name in WithExcludedErrors from
errors to errs and in WithExcludedCodes from codes to codeList, and update their
optionCarrier processor closures to append errs and codeList respectively to
clientOptions.excludedErrors and clientOptions.excludedCodes; also make the same
parameter-renaming change for the deprecated delegate functions referenced in
the file so they no longer shadow the stdlib errors package or the imported
codes package.
🪄 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: 97f3dd8f-27fa-4446-b058-437744603dcf
⛔ Files ignored due to path filters (1)
examples/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
MakefileREADME.mdclient.goconfig.goexamples/failsafe_test.goexamples/go.modinterceptors_test.gooptions.go
✅ Files skipped from review due to trivial changes (2)
- Makefile
- examples/go.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/failsafe_test.go
- interceptors_test.go
- README.md
- client.go
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
💡 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 8 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client.go (1)
201-221:⚠️ Potential issue | 🟠 MajorApply the same req/reply sanitization to
HystrixClientInterceptor.The PR's sanitization fix ("removed request/reply from panic error and logs to avoid leaking sensitive data") was applied only to
ExecutorClientInterceptor(lines 145–146), butHystrixClientInterceptorstill embedsreqandreplyin both the wrapped error (line 204) and thelog.Errorarguments (line 205). SinceDefaultClientInterceptorskeeps routing throughHystrixClientInterceptorwhenever no executor is configured (i.e., the zero-migration default until v1), the PII/secret-leak risk the fix targets remains fully active on the fallback path. Applying the same redaction here keeps both paths consistent and preserves the intended privacy guarantee throughout the deprecation window.🛡️ Proposed sanitization for the Hystrix fallback path
var invokerErr error hystrixErr := hystrix.Do(options.hystrixName, func() (err error) { defer func() { if r := recover(); r != nil { - err = errors.Wrap(fmt.Errorf("panic inside hystrix method: %s, req: %v, reply: %v", method, req, reply), "Hystrix") - log.Error(ctx, "panic", r, "method", method, "req", req, "reply", reply) + err = errors.Wrap(fmt.Errorf("panic inside hystrix method: %s", method), "Hystrix") + log.Error(ctx, "panic", r, "method", method) } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client.go` around lines 201 - 221, HystrixClientInterceptor still includes raw req/reply in the panic-wrapped error and log; replicate the ExecutorClientInterceptor sanitization by removing request/reply from both the fmt.Errorf and log.Error calls inside the hystrix.Do panic handler. In the hystrix.Do block (the closure passed to hystrix.Do where invokerErr is set and the deferred recover runs), change the wrapped error construction to omit req/reply (e.g., "panic inside hystrix method: %s" with method only) and remove req/reply from the log.Error call arguments (keep context, "panic", r, "method", method), preserving notifier.NotifyOnPanic(newCtx, method) and all existing invoker/invokerErr handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client.go`:
- Around line 201-221: HystrixClientInterceptor still includes raw req/reply in
the panic-wrapped error and log; replicate the ExecutorClientInterceptor
sanitization by removing request/reply from both the fmt.Errorf and log.Error
calls inside the hystrix.Do panic handler. In the hystrix.Do block (the closure
passed to hystrix.Do where invokerErr is set and the deferred recover runs),
change the wrapped error construction to omit req/reply (e.g., "panic inside
hystrix method: %s" with method only) and remove req/reply from the log.Error
call arguments (keep context, "panic", r, "method", method), preserving
notifier.NotifyOnPanic(newCtx, method) and all existing invoker/invokerErr
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1795a04-50b1-471c-9ee3-6169cac1f5d7
📒 Files selected for processing (2)
README.mdclient.go
✅ Files skipped from review due to trivial changes (1)
- README.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Executortype (func(ctx, method, fn) error) that wraps RPC invocations with user-provided resilience logic (circuit breaking, retries, bulkheading, etc.)hystrix-godependency with a simple function hook — users bring their own resilience library (failsafe-go recommended)With*Hystrix*functions andHystrixClientInterceptorwith migration guidanceDefaultClientInterceptorsauto-branches: executor set → new path, no executor → hystrix fallback (zero behavior change for existing users)examples/Go module with 5 failsafe-go integration examples (no failsafe-go dependency on interceptors itself)New API
Executortypefunc(ctx context.Context, method string, fn func(ctx context.Context) error) errorSetDefaultExecutor(e)WithExecutor(e)WithoutExecutor()WithExcludedErrors(...)WithHystrixExcludedErrors)WithExcludedCodes(...)WithHystrixExcludedCodes)ExecutorClientInterceptor(...)HystrixClientInterceptorwhen executor is setMigration path
Test plan
make test— 1.3s, 0 failures)make lint— 0 issues, 0 vulnerabilitiesmake doc— README regenerated with new API docsmake buildpassesSummary by CodeRabbit
New Features
Documentation
Examples
Tests