feat(ratelimiter): expose GET /info endpoint - #591
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ratelimiter management server now serves ChangesRatelimiter management metadata
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/invocation-plane-services/ratelimiter/cmd/info_test.go (1)
44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify metadata from the stamped binary.
These assertions accept
"unknown"values. The tests can pass if thego_binarylinker definitions are removed or use incorrect keys. Add a stamped-binary test that verifies the injected service, version, and commit values, or confirm that the repository’sgo_testrule can apply equivalentx_defs.As per coding guidelines, “Code changes must include tests.” Based on learnings, use the repository-native test runner for build and test rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/invocation-plane-services/ratelimiter/cmd/info_test.go` around lines 44 - 50, Strengthen the metadata test around the stamped binary response so it verifies the expected injected service, version, and commit values rather than merely checking non-empty fields or allowing "unknown". Use the repository-native build/test runner and either add a stamped-binary test or configure the go_test rule with equivalent x_defs, ensuring the assertions detect missing or incorrect linker keys.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/invocation-plane-services/ratelimiter/cmd/main.go`:
- Around line 143-156: Wrap the management mux returned by newHealthServeMux
before passing it to http.ListenAndServe using the established HTTP telemetry
middleware. Configure stable service.operation names for /health and /info, and
enable the Prometheus integration to export failed-request status attributes
while preserving the existing handlers and responses.
---
Nitpick comments:
In `@src/invocation-plane-services/ratelimiter/cmd/info_test.go`:
- Around line 44-50: Strengthen the metadata test around the stamped binary
response so it verifies the expected injected service, version, and commit
values rather than merely checking non-empty fields or allowing "unknown". Use
the repository-native build/test runner and either add a stamped-binary test or
configure the go_test rule with equivalent x_defs, ensuring the assertions
detect missing or incorrect linker keys.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8927bec0-9b1a-48fb-aa1e-9e1bef6582c1
⛔ Files ignored due to path filters (1)
src/invocation-plane-services/ratelimiter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
src/invocation-plane-services/ratelimiter/cmd/BUILD.bazelsrc/invocation-plane-services/ratelimiter/cmd/info_test.gosrc/invocation-plane-services/ratelimiter/cmd/main.gosrc/invocation-plane-services/ratelimiter/go.modsrc/invocation-plane-services/ratelimiter/tools/workspace_status.sh
Signed-off-by: priyaselvaganesan <pselvaganesa@nvidia.com>
a54c2fa to
36253c1
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
TL;DR
Adds
GET /infoto the ratelimiter management HTTP server (port 8080, alongside/health), serving service name, version, and commit SHA via go-lib's sharednvcfversion.Handler().Additional Details
Service,Version, andGitHashx_defsinjected at build time (--stamp).GitHashuses{STABLE_GIT_COMMIT_FULL}, socommitis the full 40-char SHA. Non-GET returns405(Allow: GET).newHealthServeMuxso the routes are unit-testable./healthbehavior is unchanged.go.modgo-lib pin to the merged version that providesHandler()(feat(go-lib/version): add Handler and HandlerFor http.Handler #270). Per Go MVS this also movesgo.opentelemetry.io/otelto v1.44 and relatedgolang.org/xdeps forward, as go-lib requires.Changes:
cmd/main.go: extractsnewHealthServeMux, registersnvcfversion.Handler()atGET /infocmd/BUILD.bazel: addsService/Version/GitHashx_defs on the binary and the go-libpkg/versiondepcmd/info_test.go: covers/info200 shape and non-GET 405tools/workspace_status.sh: emitsSTABLE_GIT_COMMIT_FULLgo.mod/go.sum: pin the merged go-libFor the Reviewer
GET /infoendpoint #275 (same pattern)For QA
Built with
bazel build //cmd:image.tar --stampand deployed to a local cluster, then curled the endpoint:In a local build
versionis the tag ormr-<sha>at HEAD. In a CI release build it is the service release semver.Issues
Relates to #315
Checklist
Summary by CodeRabbit
/infoendpoint that reports the service name, version, and Git commit metadata in JSON format./info; unsupported requests return HTTP 405 withAllow: GET./healthand/infoare served consistently.