feat(ratelimiter): expose GET /info endpoint - #591
Conversation
Signed-off-by: priyaselvaganesan <pselvaganesa@nvidia.com>
📝 WalkthroughWalkthroughThe ratelimiter management server now serves version metadata at ChangesRatelimiter management metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant newHealthServeMux
participant nvcfversion.Handler
participant rateLimiter
Client->>newHealthServeMux: GET /info
newHealthServeMux->>nvcfversion.Handler: Serve version metadata
nvcfversion.Handler-->>Client: JSON metadata response
Client->>newHealthServeMux: GET /health
newHealthServeMux->>rateLimiter: Health()
rateLimiter-->>Client: Health response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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
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
New Features
/infoendpoint that reports service version and build metadata, including the full Git commit identifier./infoacceptsGETrequests and returns structured JSON metadata.Bug Fixes
/infonow return HTTP 405 with anAllow: GETheader and an empty response body.