-
Notifications
You must be signed in to change notification settings - Fork 3
fix: rest probe endpoint #39
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
Conversation
WalkthroughThe pull request transitions the Prometheus client model from an indirect to direct dependency, adds a nil-check safeguard in the load balancer's NextServer method when handling empty server lists, introduces a corresponding test case, and updates REST endpoint paths in seed probes to use Cosmos-specific v1beta1 API routes while removing error wrapping from certain status checks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/seed/probes.go (1)
133-133: Consider preserving error wrapping for better error context.The removal of
%wfrom the error formatting prevents error unwrapping, which can complicate error handling and debugging in calling code. While the formatted message still provides useful information, wrapped errors enable callers to useerrors.Is()anderrors.As()for more sophisticated error handling.If there's no specific reason to avoid error wrapping, consider preserving it:
- return Status{}, fmt.Errorf("unexpected status from REST client [%d %s]", resp.StatusCode, resp.Status) + return Status{}, fmt.Errorf("unexpected status from REST client [%d %s]: %w", resp.StatusCode, resp.Status, err)Note: This would require capturing an appropriate error value to wrap, or maintaining the current behavior if the formatted message alone is sufficient for your use case.
Also applies to: 158-158
internal/proxy/balancer.go (1)
131-134: Redundant nil check, but acceptable defensive programming.The nil check at line 131 is redundant because line 116 already returns nil when
len(rr.servers) == 0. If execution reaches this fallback, the server list is guaranteed to be non-empty.However, this defensive check makes the code more explicit and protects against potential future refactoring that might alter the early-return logic. The performance impact is negligible.
If you prefer to remove the redundancy:
// Fallback, shouldn't be reached if rates are normalized - if len(rr.servers) > 0 { - return rr.servers[len(rr.servers)-1].Server - } - return nil + return rr.servers[len(rr.servers)-1].Server
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
go.mod(1 hunks)internal/proxy/balancer.go(1 hunks)internal/proxy/balancer_test.go(1 hunks)internal/seed/probes.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/seed/probes.go (1)
internal/seed/seed.go (1)
Status(27-42)
internal/proxy/balancer_test.go (2)
internal/proxy/balancer.go (1)
NewLatencyBased(96-101)internal/proxy/server.go (1)
Server(28-37)
internal/proxy/balancer.go (1)
internal/proxy/server.go (1)
Server(28-37)
🔇 Additional comments (3)
internal/seed/probes.go (1)
121-121: LGTM! Correct REST endpoint paths for Cosmos SDK v0.50.x.The updated endpoint paths correctly use the v1beta1 Tendermint REST API:
/cosmos/base/tendermint/v1beta1/syncing/cosmos/base/tendermint/v1beta1/blocks/latestThese paths are the standard Cosmos SDK REST endpoints for the version in use.
Also applies to: 146-146
internal/proxy/balancer_test.go (1)
82-91: LGTM! Good test coverage for empty server list edge case.The test correctly verifies that
NextServerreturnsnilwhen the latency-based load balancer has no available servers. This aligns with the nil-safety changes ininternal/proxy/balancer.goand follows the existing test patterns.go.mod (1)
9-9: Direct usage of prometheus/client_model confirmed.The import
dto "github.com/prometheus/client_model/go"atinternal/metrics/metrics.go:10is actively used in the file—specifically in return types ([]*dto.MetricFamilyat line 26) and type instantiations (&dto.LabelPairat line 39). The transition to a direct dependency is justified.
Fix health probe endpoints.
Summary by CodeRabbit
Bug Fixes
Chores