Skip to content

Conversation

@cloud-j-luna
Copy link
Member

@cloud-j-luna cloud-j-luna commented Oct 28, 2025

Fix health probe endpoints.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed a potential crash in load balancing when no servers are available.
    • Updated API endpoints to use standardized blockchain protocol paths for improved compatibility.
    • Simplified error reporting for status validation.
  • Chores

    • Updated dependency management for monitoring library.

@coderabbitai
Copy link

coderabbitai bot commented Oct 28, 2025

Walkthrough

The 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

Cohort / File(s) Summary
Dependency Management
go.mod
Promotes github.com/prometheus/client_model v0.6.1 from indirect to direct dependency
Proxy Balancer Safety
internal/proxy/balancer.go, internal/proxy/balancer_test.go
Adds nil-check guard in LatencyBased.NextServer() to prevent out-of-bounds access on empty server lists; adds test case TestLatencyBased_NextServer_EmptyServers to verify nil return for empty input
Seed Probes API Updates
internal/seed/probes.go
Updates REST endpoint paths from generic routes (/syncing, /blocks/latest) to Cosmos v1beta1 API paths (/cosmos/base/tendermint/v1beta1/syncing, /cosmos/base/tendermint/v1beta1/blocks/latest); removes error wrapping in status failure messages

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • internal/seed/probes.go: Verify that the Cosmos v1beta1 API paths are correct and compatible with the target Cosmos SDK version; confirm error message changes don't mask important debugging information

Poem

🐰 A hop through the code, so clean and so neat,
Dependencies shift, and the balancer's fleet,
With nil-checks in place, no crashes to greet,
Cosmos paths gleam in v1beta1 sweet! 🌟

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "fix: rest probe endpoint" is clearly and specifically related to the changes in the pull request. The title directly corresponds to the modifications in internal/seed/probes.go, where REST endpoint paths are updated from generic paths to cosmos-specific v1beta1 paths. The title is concise, non-vague, and aligns with the branch name "luna/fix-rest-probe," indicating this was the primary focus of the PR. While the changeset includes additional fixes (balancer empty server list handling and a dependency adjustment), the title appropriately highlights the main change without requiring exhaustive coverage of all modifications.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch luna/fix-rest-probe

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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 %w from 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 use errors.Is() and errors.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

📥 Commits

Reviewing files that changed from the base of the PR and between 486052c and da203dc.

📒 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/latest

These 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 NextServer returns nil when the latency-based load balancer has no available servers. This aligns with the nil-safety changes in internal/proxy/balancer.go and 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" at internal/metrics/metrics.go:10 is actively used in the file—specifically in return types ([]*dto.MetricFamily at line 26) and type instantiations (&dto.LabelPair at line 39). The transition to a direct dependency is justified.

@chainzero chainzero merged commit eed8c78 into main Oct 29, 2025
8 checks passed
@chainzero chainzero deleted the luna/fix-rest-probe branch October 29, 2025 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants